Check that the "StructuredData Plugin weak pointer" is good before trying to turn it into a shared pointer

[jingham]

Summary

Prevent a crash using SBStructuredData.GetDescription.

For some reason SBStructuredData requires a plugin for printing, and
doesn't have a default version that just prints the current contents
as JSON or something sensible?

I'm not sure what is supposed to happen here, but as the code stands
now, trying to call GetDescription on an SBStructuredData will crash
when the empty weak pointer is turned into a shared pointer.

This patch just adds a check that the weak pointer hasn't expired
before trying to access it. That prevents the crash.

This fix seems fairly obvious, but I put it up here in case somebody knows what is
actually supposed to happen here, the state of things doesn't make much sense to me.

[clayborg]

So StructuredData::Object does have a dump function:

void Object::Dump(lldb_private::Stream &s, bool pretty_print = true) const;

Not sure when that got added. So we actually don't even need the m_plugin_wp anymore if we switch to using that?

lldb/include/lldb/Core/StructuredDataImpl.h

71–84

All of this could become:

m_data_sp->Dump(stream);

And we can get rid of m_plugin_wp from this class?

[jingham]

I am happier removing things when I know what they were for??? Not really sure why this plugin architecture is there. My guess is it is part of Todd's os_log facility, but I'm not really sure.

[jingham]

But, sure, if nobody can come up with a use for this plugin dealie, I'll get rid of it. Otherwise I guess we should make a default plugin that calls "Dump"?

[clayborg]

The only place m_plugin_wp is used is for dumping, so I would vote to get rid of it. (At least on a non-swift LLVM checkout).

[jingham]

Jonas just revived the long Doc that Todd wrote about the Structured Data printing plugins. Seems like that is a useful thing, so I'm not in favor of getting rid of it just yet.

Seems the right thing to do is if the plugin doesn't exist, call Dump straight-away. I'll adjust the patch to do that as well.

[clayborg]

In D88266#2299110, @jingham wrote:

Jonas just revived the long Doc that Todd wrote about the Structured Data printing plugins. Seems like that is a useful thing, so I'm not in favor of getting rid of it just yet.

Seems the right thing to do is if the plugin doesn't exist, call Dump straight-away. I'll adjust the patch to do that as well.

Sounds good.

[jingham]

Check safely for an unset plugin, then try to get the plugin and if either of those fails, use the underlying ObjectSP's Dump method to dump the data.

Also added a test to make sure we get the data and don't crash.

[labath]

lldb/include/lldb/Core/StructuredDataImpl.h

72–74

This is racy. The right way to do that is to call m_plugin_wp.lock() and then check the validity of the returned shared_ptr.

[jingham]

lldb/include/lldb/Core/StructuredDataImpl.h

72–74

I tried that, but at least on the version of the STL on the latest macOS if the weak pointer has just been default initialized if you call lock on it it crashes. expired was the only call I could make that didn't crash.

[clayborg]

lldb/include/lldb/Core/StructuredDataImpl.h

72–74

I tried that, but at least on the version of the STL on the latest macOS if the weak pointer has just been default initialized if you call lock on it it crashes. expired was the only call I could make that didn't crash.

that seems like a pretty serious bug?! This should be fixed or any use of std::weak_ptr would crash?

[clayborg]

Jim: were you able to repro this with a simple a.out program? Is there something worse going on here like maybe incompatible libc++/libstdc++ implementations?

[jingham]

In D88266#2301934, @clayborg wrote:

Jim: were you able to repro this with a simple a.out program? Is there something worse going on here like maybe incompatible libc++/libstdc++ implementations?

The crash is pretty easy to repro. This is pretty much what the code in question was doing:

#include <memory>

class Foo {
public:
  int first = 10;
  int second = 20;
  std::weak_ptr<Foo> my_wp;

  Foo() : first(10), second(20), my_wp() {}
  virtual int doSomething() {
    return first * second;
  }
};

int
main()
{
  Foo my_foo;
  std::shared_ptr<Foo> my_sp(my_foo.my_wp);
  return my_sp.get() ? 0 : 1 ;
}

Compile and run that on at least on macOS and you get:

 > ./tryit
libc++abi.dylib: terminating with uncaught exception of type std::__1::bad_weak_ptr: bad_weak_ptr

And looking at the implementation, the SP constructor from a WP explicitly throws if the weak pointer's __cntrl is null. I'm not sure why that is desirable, but it's clearly intended.

However, looking at the code again, the weak_ptr::lock call really should succeed, just returning an empty shared pointer. And indeed, I can't reproduce that crash anymore. I must have had some other changes I had made in the course of investigation lying around that were confusing the issue.

I've resubmitted the patch with the same set of changes, but making the SP by calling lock on the WP rather than calling the SP from WP constructor.

lldb/include/lldb/Core/StructuredDataImpl.h

72–74

To be clear. I started looking at this because SBStructuredData.GetDescription was crashing at this line:

plugin_sp = lldb::StructuredDataPluginSP(m_plugin_wp);

trying to lock the wp, in the case where m_plugin_wp is default initialized. I also tried calling lock directly and unsurprisingly that also crashed.

72–74

What actually happens is that the lock call throws a "bad weak pointer" exception. So maybe the correct programming approach it to try the lock and catch the exception? But we build with rtti off and apparently w/o that you can't do try/catch (or at least that's what clang told me...)

[jingham]

Use std::weak_ptr::lock to make the shared pointer, rather than std::shared_ptr::shared_ptr(std::weak_ptr) (sort of), as the latter throws.

[labath]

In D88266#2302245, @jingham wrote:

Use std::weak_ptr::lock to make the shared pointer, rather than std::shared_ptr::shared_ptr(std::weak_ptr) (sort of), as the latter throws.

Yep, wp.lock() behaves differently from the shared_ptr constructor. :)

[jingham]

We should probably outlaw the shared pointer from weak pointer constructor in lldb. It looks like the only safe way to use it is in a try/catch block (or by preflighting the weak pointer with lock which renders it pretty much pointless.)

Jim

[clayborg]

Agreed, must call weak_ptr.lock().

[labath]

Sounds like a job for a clang-tidy check.

lldb/include/lldb/Core/StructuredDataImpl.h

 //===-- StructuredDataImpl.h ------------------------------------*- C++ -*-===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_CORE_STRUCTUREDDATAIMPL_H
 #define LLDB_CORE_STRUCTUREDDATAIMPL_H
 
 #include "lldb/Target/StructuredDataPlugin.h"
 #include "lldb/Utility/Event.h"
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/Stream.h"
 #include "lldb/Utility/StructuredData.h"
 #include "lldb/lldb-enumerations.h"
 #include "lldb/lldb-forward.h"
 #include "llvm/ADT/StringRef.h"
 
Status GetDescription(Stream &stream) const {
     Status error;
 
     if (!m_data_sp) {
       error.SetErrorString("Cannot pretty print structured data: "
                            "no data to print.");
       return error;
     }
 
-    // Grab the plugin.
-    auto plugin_sp = lldb::StructuredDataPluginSP(m_plugin_wp);
+    // Grab the plugin
+    lldb::StructuredDataPluginSP plugin_sp;
+    if (!m_plugin_wp.expired())
+      plugin_sp = lldb::StructuredDataPluginSP(m_plugin_wp);
+
+    // If there's no plugin, call underlying data's dump method:
     if (!plugin_sp) {
-      error.SetErrorString("Cannot pretty print structured data: "
-                           "plugin doesn't exist.");
+      if (!m_data_sp) {
+        error.SetErrorString("No data to describe.");
+        return error;
+      }
+      m_data_sp->Dump(stream, true);
       return error;
     }
-
     // Get the data's description.
     return plugin_sp->GetDescription(m_data_sp, stream);
   }
 
   StructuredData::ObjectSP GetObjectSP() { return m_data_sp; }
 
   void SetObjectSP(const StructuredData::ObjectSP &obj) { m_data_sp = obj; }
 

lldb/test/API/python_api/sbstructureddata/TestStructuredDataAPI.py

def structured_data_api_test(self):
         # values, strings and arrays
         error = example.SetFromJSON(s)
         if not error.Success():
             self.fail("FAILED:   " + error.GetCString())
 
         # Tests for invalid data type
         self.invalid_struct_test(example)
 
+        # Test that GetDescription works:
+        s.Clear()
+        error = example.GetDescription(s)
+        self.assertTrue(error.Success(), "GetDescription works")
+        if not "key_float" in s.GetData():
+            self.fail("FAILED: could not find key_float in description output")
+        
         dict_struct = lldb.SBStructuredData()
         dict_struct = example.GetValueForKey("key_dict")
 
         # Tests for dictionary data type
         self.dictionary_struct_test(example)
 
         # Tests for string data type
         self.string_struct_test(dict_struct)