[lldb] Ensure SBStructuredData::m_impl_up is always non-null

[kastiglione]

Summary

By ensuring that SBStructuredData::m_impl_up always has a value, it prevents crashes that can happen when the SBStructuredDataImpl is null.

This fixes a case that caused SBStructuredData::operator= to crash. This happened when rhs had a null SBStructuredDataImpl.

[shafik]

LGTM, it seems like there a few places where we are not consistently checking m_impl_up is valid and other places we are using the conditional operator to check if it is set or not.

[kastiglione]

I should expand this to fix other unchecked dereferences.

[kastiglione]

Ensure m_impl_up is always non-null

[kastiglione]

I changed direction, to ensure the impl pointer is always non-null.

lldb/source/API/SBStructuredData.cpp

43

This is the key change, all else is cleanup.

[teemperor]

LGTM. I'm going to be that guy and ask if you could test this (there is an API unittest directory with an example test, so copying that and just exercising the nullptr SBStructuredData is good enough).

[augusto2112]

LGTM!

[kastiglione]

Add unittest

lldb/source/API/SBStructuredData.cpp

 #pragma mark--
 #pragma mark SBStructuredData
 
 SBStructuredData::SBStructuredData() : m_impl_up(new StructuredDataImpl()) {
   LLDB_RECORD_CONSTRUCTOR_NO_ARGS(SBStructuredData);
 }
 
 SBStructuredData::SBStructuredData(const lldb::SBStructuredData &rhs)
-    : m_impl_up(new StructuredDataImpl(*rhs.m_impl_up.get())) {
+    : m_impl_up(new StructuredDataImpl(*rhs.m_impl_up)) {
   LLDB_RECORD_CONSTRUCTOR(SBStructuredData, (const lldb::SBStructuredData &),
                           rhs);
 }
 
 SBStructuredData::SBStructuredData(const lldb::EventSP &event_sp)
     : m_impl_up(new StructuredDataImpl(event_sp)) {
   LLDB_RECORD_CONSTRUCTOR(SBStructuredData, (const lldb::EventSP &), event_sp);
 }
 
 SBStructuredData::SBStructuredData(lldb_private::StructuredDataImpl *impl)
-    : m_impl_up(impl) {
+    : m_impl_up(impl ? impl : new StructuredDataImpl()) {
   LLDB_RECORD_CONSTRUCTOR(SBStructuredData,
                           (lldb_private::StructuredDataImpl *), impl);
 }
 
 SBStructuredData::~SBStructuredData() = default;
 
 SBStructuredData &SBStructuredData::
 operator=(const lldb::SBStructuredData &rhs) {
lldb::SBError SBStructuredData::GetDescription(lldb::SBStream &stream) const {
   sb_error.SetError(error);
   return LLDB_RECORD_RESULT(sb_error);
 }
 
 StructuredDataType SBStructuredData::GetType() const {
   LLDB_RECORD_METHOD_CONST_NO_ARGS(lldb::StructuredDataType, SBStructuredData,
                                    GetType);
 
-  return (m_impl_up ? m_impl_up->GetType() : eStructuredDataTypeInvalid);
+  return m_impl_up->GetType();
 }
 
 size_t SBStructuredData::GetSize() const {
   LLDB_RECORD_METHOD_CONST_NO_ARGS(size_t, SBStructuredData, GetSize);
 
-  return (m_impl_up ? m_impl_up->GetSize() : 0);
+  return m_impl_up->GetSize();
 }
 
 bool SBStructuredData::GetKeys(lldb::SBStringList &keys) const {
   LLDB_RECORD_METHOD_CONST(bool, SBStructuredData, GetKeys,
                            (lldb::SBStringList &), keys);
 
-  if (!m_impl_up)
-    return false;
-
   if (GetType() != eStructuredDataTypeDictionary)
     return false;
 
   StructuredData::ObjectSP obj_sp = m_impl_up->GetObjectSP();
   if (!obj_sp)
     return false;
 
   StructuredData::Dictionary *dict = obj_sp->GetAsDictionary();
bool SBStructuredData::GetKeys(lldb::SBStringList &keys) const {
   });
   return true;
 }
 
 lldb::SBStructuredData SBStructuredData::GetValueForKey(const char *key) const {
   LLDB_RECORD_METHOD_CONST(lldb::SBStructuredData, SBStructuredData,
                            GetValueForKey, (const char *), key);
 
-  if (!m_impl_up)
-    return LLDB_RECORD_RESULT(SBStructuredData());
-
   SBStructuredData result;
   result.m_impl_up->SetObjectSP(m_impl_up->GetValueForKey(key));
   return LLDB_RECORD_RESULT(result);
 }
 
 lldb::SBStructuredData SBStructuredData::GetItemAtIndex(size_t idx) const {
   LLDB_RECORD_METHOD_CONST(lldb::SBStructuredData, SBStructuredData,
                            GetItemAtIndex, (size_t), idx);
 
-  if (!m_impl_up)
-    return LLDB_RECORD_RESULT(SBStructuredData());
-
   SBStructuredData result;
   result.m_impl_up->SetObjectSP(m_impl_up->GetItemAtIndex(idx));
   return LLDB_RECORD_RESULT(result);
 }
 
 uint64_t SBStructuredData::GetIntegerValue(uint64_t fail_value) const {
   LLDB_RECORD_METHOD_CONST(uint64_t, SBStructuredData, GetIntegerValue,
                            (uint64_t), fail_value);
 
-  return (m_impl_up ? m_impl_up->GetIntegerValue(fail_value) : fail_value);
+  return m_impl_up->GetIntegerValue(fail_value);
 }
 
 double SBStructuredData::GetFloatValue(double fail_value) const {
   LLDB_RECORD_METHOD_CONST(double, SBStructuredData, GetFloatValue, (double),
                            fail_value);
 
-  return (m_impl_up ? m_impl_up->GetFloatValue(fail_value) : fail_value);
+  return m_impl_up->GetFloatValue(fail_value);
 }
 
 bool SBStructuredData::GetBooleanValue(bool fail_value) const {
   LLDB_RECORD_METHOD_CONST(bool, SBStructuredData, GetBooleanValue, (bool),
                            fail_value);
 
-  return (m_impl_up ? m_impl_up->GetBooleanValue(fail_value) : fail_value);
+  return m_impl_up->GetBooleanValue(fail_value);
 }
 
 size_t SBStructuredData::GetStringValue(char *dst, size_t dst_len) const {
   LLDB_RECORD_CHAR_PTR_METHOD_CONST(size_t, SBStructuredData, GetStringValue,
                                     (char *, size_t), dst, "", dst_len);
 
-  return (m_impl_up ? m_impl_up->GetStringValue(dst, dst_len) : 0);
+  return m_impl_up->GetStringValue(dst, dst_len);
 }
 
 namespace lldb_private {
 namespace repro {
 
 template <> void RegisterMethods<SBStructuredData>(Registry &R) {
   LLDB_REGISTER_CONSTRUCTOR(SBStructuredData, ());
   LLDB_REGISTER_CONSTRUCTOR(SBStructuredData, (const lldb::SBStructuredData &));

lldb/unittests/API/CMakeLists.txt

 add_lldb_unittest(APITests
   SBCommandInterpreterTest.cpp
+  SBStructuredDataTest.cpp
 
   LINK_LIBS
     liblldb
   )
 
 if(Python3_RPATH)
   set_property(TARGET APITests APPEND PROPERTY BUILD_RPATH "${Python3_RPATH}")
 endif()

lldb/unittests/API/SBStructuredDataTest.cpp

+//===-- SBStructuredDataTest.cpp ------------------------===----------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===/
+
+#include "gtest/gtest.h"
+
+#include "lldb/API/SBStringList.h"
+#include "lldb/API/SBStructuredData.h"
+
+#include <cstring>
+#include <string>
+
+using namespace lldb;
+
+class SBStructuredDataTest : public testing::Test {};
+
+TEST_F(SBStructuredDataTest, NullImpl) {
+  SBStructuredData data(nullptr);
+  EXPECT_EQ(data.GetType(), eStructuredDataTypeInvalid);
+  EXPECT_EQ(data.GetSize(), 0ul);
+  SBStringList keys;
+  EXPECT_FALSE(data.GetKeys(keys));
+  EXPECT_EQ(data.GetValueForKey("key").GetType(), eStructuredDataTypeInvalid);
+  EXPECT_EQ(data.GetItemAtIndex(0).GetType(), eStructuredDataTypeInvalid);
+  EXPECT_EQ(data.GetIntegerValue(UINT64_MAX), UINT64_MAX);
+  EXPECT_EQ(data.GetFloatValue(DBL_MAX), DBL_MAX);
+  EXPECT_TRUE(data.GetBooleanValue(true));
+  EXPECT_FALSE(data.GetBooleanValue(false));
+  char dst[1];
+  EXPECT_EQ(data.GetStringValue(dst, sizeof(dst)), 0ul);
+}