[MESOS-6424] Possible nullptr dereference in flag loading

• Type: Bug
• Status: Resolved
• Priority: Major
• Resolution: Fixed
• Fix Version/s: 1.0.2, 1.1.0
• Component/s: stout
• Labels: coverity
• Target Version/s: 1.0.2, 1.1.0
• Story Points: 2
• Assignee: Benjamin Bannier
• Reporter: Benjamin Bannier
• Shepherd: Till Toenshoff
• Created: 20/Oct/16 08:16
• Updated: 26/Mar/18 08:14
• Resolved: 28/Oct/16 14:27

Description

Coverity reports the following:

/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     

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.