[
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 <[email protected]>
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)
