[ https://issues.apache.org/jira/browse/MESOS-6424?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15615532#comment-15615532 ]
Till Toenshoff commented on MESOS-6424: --------------------------------------- {noformat} commit 189ac949784bfebfb14c99c76dc75d2bf924a412 Author: Benjamin Bannier <benjamin.bann...@mesosphere.io> Date: Fri Oct 28 13:39:02 2016 +0200 Fixed incorrect check in dynamic_cast. We checked here whether the passed pointer is not null, while we in fact wanted to confirm that the performed dynamic_cast succeeded (like is already done e.g., in the definitions of flags.stringify or flags.validate). This change makes us check the result of the dynamic_cast like originally intended. Review: https://reviews.apache.org/r/53055/ {noformat} > Possible nullptr dereference in flag loading > -------------------------------------------- > > Key: MESOS-6424 > URL: https://issues.apache.org/jira/browse/MESOS-6424 > Project: Mesos > Issue Type: Bug > Components: stout > Reporter: Benjamin Bannier > Assignee: Benjamin Bannier > Labels: coverity > > Coverity reports the following: > {code} > /3rdparty/stout/include/stout/flags/flags.hpp: 375 in > flags::FlagsBase::add<mesos::internal::logger::rotate::Flags, > std::basic_string<char, std::char_traits<char>, std::allocator<char>>, char > [10], mesos::internal::logger::rotate::Flags::Flags()::[lambda(const > std::basic_string<char, std::char_traits<char>, std::allocator<char>>&) > (instance 1)]>(T2 T1::*, const flags::Name &, const Option<flags::Name> &, > const std::basic_string<char, std::char_traits<char>, std::allocator<char>>&, > const T3 *, T4)::[lambda(flags::FlagsBase*, const std::basic_string<char, > std::char_traits<char>, std::allocator<char>>&) (instance 1)]::operator > ()(flags::FlagsBase*, const std::basic_string<char, std::char_traits<char>, > std::allocator<char>>&) const() > 369 Flags* flags = dynamic_cast<Flags*>(base); > 370 if (base != nullptr) { > 371 // NOTE: 'fetch' "retrieves" the value if necessary and then > 372 // invokes 'parse'. See 'fetch' for more details. > 373 Try<T1> t = fetch<T1>(value); > 374 if (t.isSome()) { > CID 1374083: (FORWARD_NULL) > Dereferencing null pointer "flags". > 375 flags->*t1 = t.get(); > 376 } else { > 377 return Error("Failed to load value '" + value + "': " + > t.error()); > 378 } > 379 } > 380 > ** CID 1374082: Null pointer dereferences (FORWARD_NULL) > /3rdparty/stout/include/stout/flags/flags.hpp: 375 in > flags::FlagsBase::add<mesos::internal::logger::LoggerFlags, Bytes, Megabytes, > Option<Error> (*)(const Bytes &)>(T2 T1::*, const flags::Name &, > Option<flags::Name>&, const std::basic_string<char, std::char_traits<char>, > std::allocator<char>>&, const T3 *, T4)::[lambda(flags::FlagsBase*, const > std::basic_string<char, std::char_traits<char>, std::allocator<char>>&) > (instance 1)]::operator ()(flags::FlagsBase*, const std::basic_string<char, > std::char_traits<char>, std::allocator<char>>&) const() > ________________________________________________________________________________________________________ > *** CID 1374082: Null pointer dereferences (FORWARD_NULL) > /3rdparty/stout/include/stout/flags/flags.hpp: 375 in > flags::FlagsBase::add<mesos::internal::logger::LoggerFlags, Bytes, Megabytes, > Option<Error> (*)(const Bytes &)>(T2 T1::*, const flags::Name &, > Option<flags::Name>&, const std::basic_string<char, std::char_traits<char>, > std::allocator<char>>&, const T3 *, T4)::[lambda(flags::FlagsBase*, const > std::basic_string<char, std::char_traits<char>, std::allocator<char>>&) > (instance 1)]::operator ()(flags::FlagsBase*, const std::basic_string<char, > std::char_traits<char>, std::allocator<char>>&) const() > 369 Flags* flags = dynamic_cast<Flags*>(base); > 370 if (base != nullptr) { > 371 // NOTE: 'fetch' "retrieves" the value if necessary and then > 372 // invokes 'parse'. See 'fetch' for more details. > 373 Try<T1> t = fetch<T1>(value); > 374 if (t.isSome()) { > CID 1374082: Null pointer dereferences (FORWARD_NULL) > Dereferencing null pointer "flags". > 375 flags->*t1 = t.get(); > 376 } else { > 377 return Error("Failed to load value '" + value + "': " + > t.error()); > 378 } > 379 } > 380 > {code} > The {{dynamic_cast}} is needed here if the derived {{Flags}} class got > intentionally sliced (e.g., to a {{FlagsBase}}). Since the base class of the > hierarchy ({{FlagsBase}}) stores the flags they would not be sliced away; the > {{dynamic_cast}} here effectively filters out all flags still valid for the > {{Flags}} used when the {{Flag}} was {{add}}'ed. > It seems the intention here was to confirm that the {{dynamic_cast}} to > {{Flags*}} succeeded like is done e.g., in {{flags.stringify}} and > {{flags.validate}} just below. > AFAICT this code has existed since 2013, but was only reported by coverity > recently. -- This message was sent by Atlassian JIRA (v6.3.4#6332)