This is an archive of the discontinued LLVM Phabricator instance.

Make CompilerType safe [Was: Add a check for TypeSystem use-after-free problems]
ClosedPublic

Authored by aprantl on Oct 24 2022, 4:54 PM.

Details

Summary

Make CompilerType safe

When a process gets restarted TypeSystem objects associated with it
may get deleted, and any CompilerType objects holding on to a
reference to that type system are a use-after-free in waiting. Because
of the SBAPI, we don't have tight control over where CompilerTypes go
and when they are used. This is particularly a problem in the Swift
plugin, where the scratch TypeSystem can be restarted while the
process is still running. The Swift plugin has a lock to prevent
abuse, but where there's a lock there can be bugs.

This patch changes CompilerType to store a std::weak_ptr<TypeSystem>.
Most of the std::weak_ptr<TypeSystem>* uglyness is hidden by
introducing a wrapper class CompilerType::WrappedTypeSystem that has a
dyn_cast_or_null() method. The only sites that need to know about the
weak pointer implementation detail are the ones that deal with
creating TypeSystems.

rdar://101505232

Diff Detail

Event Timeline

aprantl created this revision.Oct 24 2022, 4:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 24 2022, 4:54 PM
aprantl requested review of this revision.Oct 24 2022, 4:54 PM

There are *so* many reasons that I love this patch.

First and foremost: I am a professor of computer science at University of Cincinnati and teaching Programming Languages this semester and I am about to teach *this very technique* for protecting against use-after-free errors. Having seen this patch I now have a great example of where it is used in real life! So, thank you!

Second, I have left some comments but please take what I said with a grain of salt. I hope that it is helpful and not a waste of time.

Thank you!
Will

lldb/source/Symbol/CompilerType.cpp
33

I am sorry if this is obvious, but is CompilerType used in a multithreaded environment? So, is there a possibility that we could pass the check on line 32 but become invalid by the use of m_type_system here and fall victim to an attempt (in Lookup, perhaps?) to dereference a NULL pointer? Again, I am sorry if that is a stupid question!

34

I know that this is a nit, but should we write TypeSystem?

lldb/source/Symbol/TypeSystem.cpp
22

I spent *way* too long researching to make sure that this initialization is thread safe and wanted to drop a link here in case anyone is wondering the same thing:

C++ standard: stmt.dcl para 3, sentence 3

aprantl added a project: Restricted Project.
aprantl added inline comments.Oct 25 2022, 8:24 PM
lldb/source/Symbol/CompilerType.cpp
33

Yes, CompilerType could be used on any thread and this will not catch a use-after-free if the TypeSystem is deleted between the IsValid check and its use.

A solution would be to store a TypeSystemSP/WP in CompilerType, but I'm expecting this to be rather expensive. The intention of this patch is as a bug-finding tool. It's not intended to make holding on to CompilerType objects safe. That said, I'm open to explore ideas for how to efficiently make this safe in a future patch.

The Halloween theme is nice, but I think it'd be more traditional if we kept a list of active instances, instead of inactive ones. That said, a part of me dies every time I see a class keeping a list of its instances.

I don't know if you've seen it, but I think the Module::m_old_symfiles member exists for the very same reason, and it could be why this does not happen so often outside of swift (which, I believe, stores most of its types in the target type system instead of the module systems).

lldb/source/Symbol/CompilerType.cpp
33

Well.. if you're only interested in SB uses, then I'd say that the SBType object should hold an SP/WP on the type system or some parent of it.

aprantl updated this revision to Diff 472194.Oct 31 2022, 5:59 PM
aprantl retitled this revision from Add a check for TypeSystem use-after-free problems to Make CompilerType safe [Was: Add a check for TypeSystem use-after-free problems].

All right! After @hawkinsw raised the question whether my previous patch didn't still allow for race conditions and some off-line encouragement from @JDevlieghere, I decided to give it a try and properly design away the issue.

This new patch turns the type system stored by CompilerType into weak pointer. Weak pointers use up two pointers as storage so to avoid blowing up the memory footprint, each TypeSystem is now required to have a canonical weak pointer that is never freed. This is implemented as a vector of unique pointers to weak pointers in TypeSystemMap.

This makes all calls to CompilerType completely safe, and CompilerType::GetTypeSystem now returns a nullable shared pointer to the type system. Calls into CompilerType become slower by one pointer lookup and one atomic increment operation, which will likely not be measurable.

Code that call GetTypeSystem() can remain almost unchanged, it just needs to store the shared pointer in a local variable first.

Code that creates a new ad-hoc TypeSystem gets slightly more complex, and I admit I was surprised at how many places in LLDB actually do this.

Please take another look, I think this is much better!

labath added a comment.Nov 2 2022, 7:56 AM

Well... like they say, there's no software engineering problem that can't be solved by adding an extra layer of indirection.

In this case, that's "adding an extra layer and leaking it", so I'm wondering if we can drop the second part. I'm going to assume that we want to preserve sizeof(ptr<TypeSystem>), even though I'm not entirely convinced that this would cause a significant increase in our memory footprint.

An ideal solution for this would be a IntrusiveRefCntPtr, but with weak pointer support. That doesn't exist right now, but since we're already adding a layer of indirection, I think that layer could be used to introduce weak semantics. Instead of this "canonical" TypeSystemWP *, we could pass around a IntrusiveRefCntPtr<TypeSystemHolder>, where TypeSystemHolder is essentially a TypeSystemWP wrapped in a ThreadSafeRefCountedBase. I think this doesn't increase the number of dereferences (it's still takes two derefs to get from this funky representation to an actual type system), but it has the advantage that the type system holder will go away when/if all references to it are gone. Another advantage (I hope) could be that we don't see this funky double deref (TypeSystemWP *) pattern everywhere, as it's helpfully hidden in the holder class.

aprantl updated this revision to Diff 472785.Nov 2 2022, 4:09 PM

I have an updated patch here that I started working on before I saw Pavel's comment.

This version removes a lot of the std::weak_ptr<TypeSystem>* uglyness by introducing a wrapper class CompilerType::ManagedTypeSystem that has a dyn_cast_or_null() method and hides the fact that we store weak pointers in CompilerType. Now the only sites that need to know about the weak pointer implementation detail are the ones that deal with creating TypeSystems.

Meanwhile I did some unscientific measurements of attaching LLDB to LLDB and running a few expressions and any difference that an extra pointer in CompilerType makes is smaller than the difference between subsequent runs.

Based on that I'm going to rework this now to just store a WP in CompilerType directly, while still returning a wrapper object with a convenient dyn_cast method. That should be the best of both worlds.

aprantl updated this revision to Diff 473079.Nov 3 2022, 4:30 PM

Here is the greatly simplified version of the patch that just stores a std::weak_ptr<TypeSystem> in CompilerType.
As mentioned earlier the memory increase is entirely within thew noise.

I like the new approach, it's much easier to follow. I also like the wrapper class to abstract over the common operations. It just seems like there are some remnants of the canonical wp approach that we no longer need.

lldb/include/lldb/Symbol/CompilerType.h
51

nit: lldb::TypeSystemSP m_typesystem_sp = {}; or even lldb::TypeSystemSP m_typesystem_sp

58–60

If it's a dyn_cast_or_null why not do the m_typesystem_sp check, won't dyn_cast_or_null repeat that for you anyway?

lldb/include/lldb/Symbol/TypeSystem.h
88–91

Now that the weak pointer is no longer "canonical", there's no reason that this should be a reference anymore, right?

517–522

Do we still need this? I assume we can now just return the weak pointer directly (through shared_from_this)?

550–553

No longer necessary?

aprantl updated this revision to Diff 473825.Nov 7 2022, 4:23 PM

Address feedback from @JDevlieghere !

JDevlieghere accepted this revision.Nov 11 2022, 1:35 PM

LGTM modulo a few small nits

lldb/include/lldb/Symbol/CompilerType.h
60

Nit: static_cast<bool>(m_typesystem_sp)

lldb/source/API/SBModule.cpp
457

no else after return

502–504

if (CompilerType compiler_type = ts->GetBuiltinTypeByName(name))

This revision is now accepted and ready to land.Nov 11 2022, 1:35 PM
aprantl updated this revision to Diff 474879.Nov 11 2022, 3:58 PM

Address nits.

labath added inline comments.Nov 15 2022, 12:52 AM
lldb/include/lldb/Symbol/CompilerType.h
58

Maybe do something like this instead:

template <class TypeSystemType> std::shared_ptr<TypeSystemType> dyn_cast_or_null() {
  if (llvm::isa<TypeSystemType>(m_typesystem_sp.get()))
    return std::shared_ptr<TypeSystemType>(m_typesystem_sp, llvm::cast<TypeSystemType>(m_typesystem_sp.get()));
  return nullptr;
}

That said, llvm::dyn_cast supports custom casts for smart pointers (it already has them for std::unique_ptr), so it should be possible to completely avoid this wrapper class by implementing this logic inside llvm.
Although, then we'd have to answer awkward questions like "why are you using a shared_ptr in the first place".

60

add explicit

lldb/include/lldb/Symbol/TypeSystem.h
517–520

Can this be replaced by inheriting from std::enable_shared_from_this?

lldb/source/Core/DumpDataExtractor.cpp
322

why?

aprantl updated this revision to Diff 475642.Nov 15 2022, 5:16 PM
aprantl edited the summary of this revision. (Show Details)

Address (excellent) feedback from @labath!

lldb/include/lldb/Symbol/CompilerType.h
58

I think that's a good idea. Based on the assumption that we are going to rework the ownership model in LLDB to move away from shared pointers towards contexts at some point in the future, I'm not going to extend casting support in Support now.

labath accepted this revision.Nov 16 2022, 5:08 AM

Some more comments inline, but I think this is essentially as good as we can make it.

lldb/include/lldb/Symbol/Type.h
300

Maybe the wrapper type should just be its own top-level entity? Or TypeSystem::SPWrapper?

lldb/source/Plugins/Platform/FreeBSD/PlatformFreeBSD.cpp
193–194

Remove m_type_system, change m_type_system_sp to std::shared_ptr<TypeSystemClang>

lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp
316–321

Same here (but also remove the _wp variable).

lldb/source/Plugins/Platform/NetBSD/PlatformNetBSD.cpp
211

and here

lldb/source/Symbol/TypeSystem.cpp
276–278

With the current implementation, I don't think these can ever fire.

lldb/source/Target/Target.cpp
2461

Maybe this error needs some updating? (also, missing spaces -- the language will get glued to the error msg)

lldb/unittests/TestingSupport/Symbol/ClangTestUtils.h
48

Can this be replaced by a shared_ptr<TypeSystemClang> now?

aprantl added inline comments.Nov 16 2022, 3:10 PM
lldb/include/lldb/Symbol/Type.h
300

I tried it and then immediately undid it because TypeSystem.h already depends on CompilerType.h.

aprantl updated this revision to Diff 475935.Nov 16 2022, 3:15 PM
This revision was landed with ongoing or failed builds.Nov 16 2022, 3:52 PM
This revision was automatically updated to reflect the committed changes.

Looks like this broke the windows lldb bot: https://lab.llvm.org/buildbot/#/builders/83/builds/26042

I'm trying to fix this blindly. Should hopefully work after a few iterations.

I think it's fixed now.