Page MenuHomePhabricator

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

Authored by jingham on Sep 24 2020, 3:30 PM.

Details

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.

Diff Detail

Event Timeline

jingham created this revision.Sep 24 2020, 3:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 24 2020, 3:30 PM
jingham requested review of this revision.Sep 24 2020, 3:30 PM

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?

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.

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"?

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).

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.

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 updated this revision to Diff 294844.Sep 28 2020, 5:48 PM

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.

clayborg accepted this revision.Sep 28 2020, 6:16 PM
This revision is now accepted and ready to land.Sep 28 2020, 6:16 PM
labath added inline comments.Sep 29 2020, 4:59 AM
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 added inline comments.Sep 29 2020, 10:49 AM
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 added inline comments.Sep 29 2020, 2:49 PM
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?

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?

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 updated this revision to Diff 295156.Sep 29 2020, 5:57 PM

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.

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. :)

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

This revision was landed with ongoing or failed builds.Sep 30 2020, 11:49 AM
This revision was automatically updated to reflect the committed changes.

Agreed, must call weak_ptr.lock().

labath added a comment.Oct 1 2020, 1:03 AM

Sounds like a job for a clang-tidy check.