[LLDB][NFC] Fix valobj_sp null pointer checks in lldb/source/Plugins/Language/

[xgupta]

Summary

The issue is that the SyntheticChildrenFrontEnd constructor is being
called with the dereferenced valobj_sp pointer, before the valobj_sp
pointer is verified to be non-null inside the body of the constructor function.

To fix the issue, we have moved the call to the SyntheticChildrenFrontEnd
constructor to after the null check for valobj_sp in the constructor
function body, so that it will only be called if valobj_sp is non-null.

Found by PVS-Studio - https://pvs-studio.com/en/blog/posts/cpp/1003/, N19-N36.

[Michael137]

Looks correct to me, given we seem to allow valobj_sp to be null. Though we seem to be doing it in a lot of other places too around this file. So could fix those all at once?

[xgupta]

In D142341#4072810, @Michael137 wrote:

Looks correct to me, given we seem to allow valobj_sp to be null. Though we seem to be doing it in a lot of other places too around this file. So could fix those all at once?

Yeah I am doing that, will update in 30 minutes or so.

[xgupta]

address comments

[Michael137]

lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp

225

this won't initialise the parent constructor though will it? Just creates a temporary and immediately destructs it? You might need to put it back into the initialiser list and use a ternary

[Michael137]

lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp

225

Oh but SyntheticChildrenFrontEnd seems to always take a reference. The interfaces seem to be a little mismatched

[Michael137]

lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp

225

Perhaps the null-check in the constructor is just redundant and should actually be an assert.

@jingham might know more about the assumptions here

[xgupta]

gentle ping for review.

[xgupta]

gentle ping for review.

[kastiglione]

lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp

225

@xgupta reiterating Michael's point, I think this change results in mis-construction. Have you run the test suite, I would expect some failures with this change.

I agree that this should be an assert. The other option would be to change the constructor's signature, using a lldb::ValueObject & instead of lldb::ValueObjectSP, which puts the onus on callers to handle any null-ness. Are we aware of any callers passing null?

[kastiglione]

lldb/source/Plugins/Language/ObjC/NSSet.cpp

673–675

by the way, these changes are missing {} around the if body.

[Michael137]

lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp

225

FYI, from offline conversation with Jim:

This is one of those things where you really should have a general guard against trying to create a FrontEnd with a null ValueObjectSP, since this isn't going to do any good.  But then you worry "what if one leaks through by accident" so you put some fallback code when that happens.  We were really insistent that "if we make a programming error in viewing a variable we'd really like to limit the damage and not crash..." So we didn't go straight to asserts in all these places the way clang, for instance, does.  Taking down a whole debug session because one variable was bizarre (maybe the DWARF was bad in a way that confused us...) is not acceptable, there's too much state in the investigations the debugger supports so we were often over cautious about these sort of checks.

The better solution would be to go to all the places that produce synthetic children and make sure that they error out early when told to format empty SP's.

Not sure how doable that is.  For most cases, the Synthetic Child Providers come from type matches, and an empty ValueObjectSP doesn't have a type, so it would never make a match.  But we do have a few "try it against anything" formatters now, things like the C++ std::function matchers, and the python functional matchers, so there are places where we might end up screwing this up.

[xgupta]

I am sorry for taking time, but I could not understand the right issue. Hence I am abandoning it.

lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp

  }
  __value_ = {
  first = 0
  second = { std::string }
  */
 
 lldb_private::formatters::LibCxxMapIteratorSyntheticFrontEnd::
     LibCxxMapIteratorSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp)
-    : SyntheticChildrenFrontEnd(*valobj_sp), m_pair_ptr(), m_pair_sp() {
+    : m_pair_ptr(), m_pair_sp() {
   if (valobj_sp)
+    SyntheticChildrenFrontEnd(*valobj_sp);
     Update();
 }
 
 bool lldb_private::formatters::LibCxxMapIteratorSyntheticFrontEnd::Update() {
   m_pair_sp.reset();
   m_pair_ptr = nullptr;
 
   ValueObjectSP valobj_sp = m_backend.GetSP();

 SyntheticChildrenFrontEnd *
 lldb_private::formatters::LibCxxMapIteratorSyntheticFrontEndCreator(
     CXXSyntheticChildren *, lldb::ValueObjectSP valobj_sp) {
   return (valobj_sp ? new LibCxxMapIteratorSyntheticFrontEnd(valobj_sp)
                     : nullptr);
 }
 
 lldb_private::formatters::LibCxxUnorderedMapIteratorSyntheticFrontEnd::
-    LibCxxUnorderedMapIteratorSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp)
-    : SyntheticChildrenFrontEnd(*valobj_sp) {
+    LibCxxUnorderedMapIteratorSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp) {
   if (valobj_sp)
+    SyntheticChildrenFrontEnd(*valobj_sp);
     Update();
 }
 
 bool lldb_private::formatters::LibCxxUnorderedMapIteratorSyntheticFrontEnd::
     Update() {
   m_pair_sp.reset();
   m_iter_ptr = nullptr;
 
lldb_private::formatters::LibCxxVectorIteratorSyntheticFrontEndCreator(
     CXXSyntheticChildren *, lldb::ValueObjectSP valobj_sp) {
   return (valobj_sp ? new VectorIteratorSyntheticFrontEnd(
                           valobj_sp, {ConstString("__i_"), ConstString("__i")})
                     : nullptr);
 }
 
 lldb_private::formatters::LibcxxSharedPtrSyntheticFrontEnd::
     LibcxxSharedPtrSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp)
-    : SyntheticChildrenFrontEnd(*valobj_sp), m_cntrl(nullptr) {
+    : m_cntrl(nullptr) {
   if (valobj_sp)
+    SyntheticChildrenFrontEnd(*valobj_sp);
     Update();
 }
 
 size_t lldb_private::formatters::LibcxxSharedPtrSyntheticFrontEnd::
     CalculateNumChildren() {
   return (m_cntrl ? 1 : 0);
 }
 

 SyntheticChildrenFrontEnd *
 lldb_private::formatters::LibcxxSharedPtrSyntheticFrontEndCreator(
     CXXSyntheticChildren *, lldb::ValueObjectSP valobj_sp) {
   return (valobj_sp ? new LibcxxSharedPtrSyntheticFrontEnd(valobj_sp)
                     : nullptr);
 }
 
 lldb_private::formatters::LibcxxUniquePtrSyntheticFrontEnd::
-    LibcxxUniquePtrSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp)
-    : SyntheticChildrenFrontEnd(*valobj_sp) {
+    LibcxxUniquePtrSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp) {
   if (valobj_sp)
+    SyntheticChildrenFrontEnd(*valobj_sp);
     Update();
 }
 
 lldb_private::formatters::LibcxxUniquePtrSyntheticFrontEnd::
     ~LibcxxUniquePtrSyntheticFrontEnd() = default;
 
 SyntheticChildrenFrontEnd *
 lldb_private::formatters::LibcxxUniquePtrSyntheticFrontEndCreator(

lldb/source/Plugins/Language/CPlusPlus/LibCxxInitializerList.cpp

private:
   uint32_t m_element_size = 0;
   size_t m_num_elements = 0;
 };
 } // namespace formatters
 } // namespace lldb_private
 
 lldb_private::formatters::LibcxxInitializerListSyntheticFrontEnd::
     LibcxxInitializerListSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp)
-    : SyntheticChildrenFrontEnd(*valobj_sp), m_element_type() {
+    : m_element_type() {
   if (valobj_sp)
+    SyntheticChildrenFrontEnd(*valobj_sp);
     Update();
 }
 
 lldb_private::formatters::LibcxxInitializerListSyntheticFrontEnd::
     ~LibcxxInitializerListSyntheticFrontEnd() {
   // this needs to stay around because it's a child object who will follow its
   // parent's life cycle
   // delete m_start;

lldb/source/Plugins/Language/CPlusPlus/LibCxxList.cpp

if (!impl_sp)
     return false;
   impl_sp = GetValueOfLibCXXCompressedPair(*impl_sp);
   if (!impl_sp)
     return false;
   m_head = impl_sp->GetChildMemberWithName(ConstString("__next_"), true).get();
   return false;
 }
 
-ListFrontEnd::ListFrontEnd(lldb::ValueObjectSP valobj_sp)
-    : AbstractListFrontEnd(*valobj_sp) {
+ListFrontEnd::ListFrontEnd(lldb::ValueObjectSP valobj_sp) {
   if (valobj_sp)
+    AbstractListFrontEnd(*valobj_sp);
     Update();
 }
 
 size_t ListFrontEnd::CalculateNumChildren() {
   if (m_count != UINT32_MAX)
     return m_count;
   if (!m_head || !m_tail || m_node_address == 0)
     return 0;

lldb/source/Plugins/Language/CPlusPlus/LibCxxMap.cpp

private:
   size_t m_count = UINT32_MAX;
   std::map<size_t, MapIterator> m_iterators;
 };
 } // namespace formatters
 } // namespace lldb_private
 
 lldb_private::formatters::LibcxxStdMapSyntheticFrontEnd::
     LibcxxStdMapSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp)
-    : SyntheticChildrenFrontEnd(*valobj_sp), m_element_type(), m_iterators() {
+    : m_element_type(), m_iterators() {
   if (valobj_sp)
+    SyntheticChildrenFrontEnd(*valobj_sp);
     Update();
 }
 
 size_t lldb_private::formatters::LibcxxStdMapSyntheticFrontEnd::
     CalculateNumChildren() {
   static ConstString g_pair3_("__pair3_");
   static ConstString g_first_("__first_");
   static ConstString g_value_("__value_");

lldb/source/Plugins/Language/CPlusPlus/LibCxxSpan.cpp

 private:
   ValueObject *m_start = nullptr; ///< First element of span. Held, not owned.
   CompilerType m_element_type{};  ///< Type of span elements.
   size_t m_num_elements = 0;      ///< Number of elements in span.
   uint32_t m_element_size = 0;    ///< Size in bytes of each span element.
 };
 
 lldb_private::formatters::LibcxxStdSpanSyntheticFrontEnd::
-    LibcxxStdSpanSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp)
-    : SyntheticChildrenFrontEnd(*valobj_sp) {
+    LibcxxStdSpanSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp) {
   if (valobj_sp)
+    SyntheticChildrenFrontEnd(*valobj_sp);
     Update();
 }
 
 size_t lldb_private::formatters::LibcxxStdSpanSyntheticFrontEnd::
     CalculateNumChildren() {
   return m_num_elements;
 }
 

lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp

private:
   ValueObject *m_next_element = nullptr;
   std::vector<std::pair<ValueObject *, uint64_t>> m_elements_cache;
 };
 } // namespace formatters
 } // namespace lldb_private
 
 lldb_private::formatters::LibcxxStdUnorderedMapSyntheticFrontEnd::
     LibcxxStdUnorderedMapSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp)
-    : SyntheticChildrenFrontEnd(*valobj_sp), m_element_type(),
-      m_elements_cache() {
+    : m_element_type(), m_elements_cache() {
   if (valobj_sp)
+    SyntheticChildrenFrontEnd(*valobj_sp);
     Update();
 }
 
 size_t lldb_private::formatters::LibcxxStdUnorderedMapSyntheticFrontEnd::
     CalculateNumChildren() {
   return m_num_elements;
 }
 

lldb/source/Plugins/Language/CPlusPlus/LibCxxVector.cpp

private:
   std::map<size_t, lldb::ValueObjectSP> m_children;
 };
 
 } // namespace formatters
 } // namespace lldb_private
 
 lldb_private::formatters::LibcxxStdVectorSyntheticFrontEnd::
     LibcxxStdVectorSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp)
-    : SyntheticChildrenFrontEnd(*valobj_sp), m_element_type() {
+    : m_element_type() {
   if (valobj_sp)
+    SyntheticChildrenFrontEnd(*valobj_sp);
     Update();
 }
 
 lldb_private::formatters::LibcxxStdVectorSyntheticFrontEnd::
     ~LibcxxStdVectorSyntheticFrontEnd() {
   // these need to stay around because they are child objects who will follow
   // their parent's life cycle
   // delete m_start;
size_t lldb_private::formatters::LibcxxStdVectorSyntheticFrontEnd::
     GetIndexOfChildWithName(ConstString name) {
   if (!m_start || !m_finish)
     return UINT32_MAX;
   return ExtractIndexFromString(name.GetCString());
 }
 
 lldb_private::formatters::LibcxxVectorBoolSyntheticFrontEnd::
     LibcxxVectorBoolSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp)
-    : SyntheticChildrenFrontEnd(*valobj_sp), m_bool_type(), m_exe_ctx_ref(),
-      m_children() {
+    : m_bool_type(), m_exe_ctx_ref(), m_children() {
   if (valobj_sp) {
+    SyntheticChildrenFrontEnd(*valobj_sp);
     Update();
     m_bool_type =
         valobj_sp->GetCompilerType().GetBasicTypeFromAST(lldb::eBasicTypeBool);
   }
 }
 
 size_t lldb_private::formatters::LibcxxVectorBoolSyntheticFrontEnd::
     CalculateNumChildren() {

lldb/source/Plugins/Language/CPlusPlus/LibStdcpp.cpp

public:
 
   size_t GetIndexOfChildWithName(ConstString name) override;
 };
 
 } // end of anonymous namespace
 
 LibstdcppMapIteratorSyntheticFrontEnd::LibstdcppMapIteratorSyntheticFrontEnd(
     lldb::ValueObjectSP valobj_sp)
-    : SyntheticChildrenFrontEnd(*valobj_sp), m_exe_ctx_ref(), m_pair_type(),
+    : m_exe_ctx_ref(), m_pair_type(),
       m_pair_sp() {
   if (valobj_sp)
+    SyntheticChildrenFrontEnd(*valobj_sp);
     Update();
 }
 
 bool LibstdcppMapIteratorSyntheticFrontEnd::Update() {
   ValueObjectSP valobj_sp = m_backend.GetSP();
   if (!valobj_sp)
     return false;
 
lldb_private::formatters::LibStdcppVectorIteratorSyntheticFrontEndCreator(
   return (valobj_sp ? new VectorIteratorSyntheticFrontEnd(
                           valobj_sp, {ConstString("_M_current")})
                     : nullptr);
 }
 
 lldb_private::formatters::VectorIteratorSyntheticFrontEnd::
     VectorIteratorSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp,
                                     llvm::ArrayRef<ConstString> item_names)
-    : SyntheticChildrenFrontEnd(*valobj_sp), m_exe_ctx_ref(),
-      m_item_names(item_names), m_item_sp() {
+    : m_exe_ctx_ref(), m_item_names(item_names), m_item_sp() {
   if (valobj_sp)
+    SyntheticChildrenFrontEnd(*valobj_sp);
     Update();
 }
 
 bool VectorIteratorSyntheticFrontEnd::Update() {
   m_item_sp.reset();
 
   ValueObjectSP valobj_sp = m_backend.GetSP();
   if (!valobj_sp)
if (addr_of_string != LLDB_INVALID_ADDRESS) {
     case eAddressTypeFile:
       break;
     }
   }
   return false;
 }
 
 LibStdcppSharedPtrSyntheticFrontEnd::LibStdcppSharedPtrSyntheticFrontEnd(
-    lldb::ValueObjectSP valobj_sp)
-    : SyntheticChildrenFrontEnd(*valobj_sp) {
+    lldb::ValueObjectSP valobj_sp) {
   if (valobj_sp)
+    SyntheticChildrenFrontEnd(*valobj_sp);
     Update();
 }
 
 size_t LibStdcppSharedPtrSyntheticFrontEnd::CalculateNumChildren() { return 1; }
 
 lldb::ValueObjectSP
 LibStdcppSharedPtrSyntheticFrontEnd::GetChildAtIndex(size_t idx) {
   ValueObjectSP valobj_sp = m_backend.GetSP();

lldb/source/Plugins/Language/ObjC/NSArray.cpp

bool lldb_private::formatters::NSArraySummaryProvider(
 
   stream.Printf("%s%" PRIu64 " %s%s%s", prefix.c_str(), value, "element",
                 value == 1 ? "" : "s", suffix.c_str());
   return true;
 }
 
 lldb_private::formatters::NSArrayMSyntheticFrontEndBase::
     NSArrayMSyntheticFrontEndBase(lldb::ValueObjectSP valobj_sp)
-    : SyntheticChildrenFrontEnd(*valobj_sp), m_exe_ctx_ref(), m_id_type() {
+    : m_exe_ctx_ref(), m_id_type() {
   if (valobj_sp) {
+    SyntheticChildrenFrontEnd(*valobj_sp);
     TypeSystemClangSP scratch_ts_sp = ScratchTypeSystemClang::GetForTarget(
         *valobj_sp->GetExecutionContextRef().GetTargetSP());
     if (scratch_ts_sp)
       m_id_type = CompilerType(
           scratch_ts_sp->weak_from_this(),
           scratch_ts_sp->getASTContext().ObjCBuiltinIdTy.getAsOpaquePtr());
     if (valobj_sp->GetProcessSP())
       m_ptr_size = valobj_sp->GetProcessSP()->GetAddressByteSize();
lldb_private::formatters::
   if (!m_data_32 && !m_data_64)
     return 0;
   return m_data_32 ? m_data_32->_size : m_data_64->_size;
 }
 
 template <typename D32, typename D64, bool Inline>
 lldb_private::formatters::GenericNSArrayISyntheticFrontEnd<D32, D64, Inline>::
     GenericNSArrayISyntheticFrontEnd(lldb::ValueObjectSP valobj_sp)
-    : SyntheticChildrenFrontEnd(*valobj_sp), m_exe_ctx_ref(),
-      m_data_32(nullptr), m_data_64(nullptr) {
+    : m_exe_ctx_ref(), m_data_32(nullptr), m_data_64(nullptr) {
   if (valobj_sp) {
+    SyntheticChildrenFrontEnd(*valobj_sp);
     CompilerType type = valobj_sp->GetCompilerType();
     if (type) {
       TypeSystemClangSP scratch_ts_sp = ScratchTypeSystemClang::GetForTarget(
           *valobj_sp->GetExecutionContextRef().GetTargetSP());
       if (scratch_ts_sp)
         m_id_type = scratch_ts_sp->GetType(
             scratch_ts_sp->getASTContext().ObjCBuiltinIdTy);
     }

lldb/source/Plugins/Language/ObjC/NSSet.cpp

if (class_name == g_SetI || class_name == g_OrderedSetI) {
     if (iter != end)
       return iter->second(synth, valobj_sp);
     return nullptr;
   }
 }
 
 lldb_private::formatters::NSSetISyntheticFrontEnd::NSSetISyntheticFrontEnd(
     lldb::ValueObjectSP valobj_sp)
-    : SyntheticChildrenFrontEnd(*valobj_sp), m_exe_ctx_ref() {
+    : m_exe_ctx_ref() {
   if (valobj_sp)
+    SyntheticChildrenFrontEnd(*valobj_sp);
     Update();
 }
 
 lldb_private::formatters::NSSetISyntheticFrontEnd::~NSSetISyntheticFrontEnd() {
   delete m_data_32;
   m_data_32 = nullptr;
   delete m_data_64;
   m_data_64 = nullptr;
lldb_private::formatters::NSCFSetSyntheticFrontEnd::GetChildAtIndex(
   }
 
   return set_item.valobj_sp;
 }
 
 template <typename D32, typename D64>
 lldb_private::formatters::GenericNSSetMSyntheticFrontEnd<
     D32, D64>::GenericNSSetMSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp)
-    : SyntheticChildrenFrontEnd(*valobj_sp), m_exe_ctx_ref(),
-      m_data_32(nullptr), m_data_64(nullptr) {
+    : m_exe_ctx_ref(), m_data_32(nullptr), m_data_64(nullptr) {
   if (valobj_sp)
+    SyntheticChildrenFrontEnd(*valobj_sp);
     Update();
 }
 
 template <typename D32, typename D64>
 lldb_private::formatters::
   GenericNSSetMSyntheticFrontEnd<D32, D64>::~GenericNSSetMSyntheticFrontEnd<D32, D64>() {
   delete m_data_32;
   m_data_32 = nullptr;
   delete m_data_64;