[lldb] Fix use after free on ModuleList::RemoveSharedModuleIfOrphaned

[augusto2112]

This fixes a potential use after free where
ModuleList::RemoveSharedModuleIfOrphaned ->
SharedModuleList::RemoveIfOrphaned -> SharedModuleList::RemoveFromMap would potentially dereference a freed pointer. This fixes it by not calling ModuleList::RemoveSharedModuleIfOrphaned at all if the pointer was just freed.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Augusto Noronha (augusto2112)

Changes

This fixes a potential use after free where
ModuleList::RemoveSharedModuleIfOrphaned ->
SharedModuleList::RemoveIfOrphaned -> SharedModuleList::RemoveFromMap would potentially dereference a freed pointer. This fixes it by not calling ModuleList::RemoveSharedModuleIfOrphaned at all if the pointer was just freed.

---

Full diff: https://github.com/llvm/llvm-project/pull/155331.diff

1 Files Affected:

• (modified) lldb/source/Target/Target.cpp (+4-1)

diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
index fa98c24606492..d5406d88ec80a 100644
--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -2564,9 +2564,12 @@ ModuleSP Target::GetOrCreateModule(const ModuleSpec &orig_module_spec,
           m_images.Append(module_sp, notify);
 
         for (ModuleSP &old_module_sp : replaced_modules) {
+          auto use_count = old_module_sp.use_count();
           Module *old_module_ptr = old_module_sp.get();
           old_module_sp.reset();
-          ModuleList::RemoveSharedModuleIfOrphaned(old_module_ptr);
+          // If the use count was one, this was not in the shared module list.
+          if (use_count > 1)
+            ModuleList::RemoveSharedModuleIfOrphaned(old_module_ptr);
         }
       } else
         module_sp.reset();

[augusto2112]

@JDevlieghere @felipepiovezan with the changes I made to SharedModuleList, I accidentally introduced a use after free, since the caller of ModuleList::RemoveSharedModuleIfOrphaned might potentially call it with an already freed pointer. This used to not be a problem because ModuleList::RemoveIfOrphaned would just compare the pointer with what was on the list, and not actually dereference it. I feel like the solution here is to simply not pass in an already potentially freed pointer. Let me know what you think.

[JDevlieghere]

Why does RemoveSharedModuleIfOrphaned take a raw pointer and not the ModuleSP? If it did, then we could just check if the SP is valid (like Remove does) and avoid yet another place where we check the use count.

[JDevlieghere]

I realize that this isn't enough to address the issue. But I think this shows what a bad idea it is to mix raw pointers, shared pointer references and references.

[augusto2112]

If RemoveSharedModuleIfOrphaned takes a shared pointer we have to account for the caller also having a shared pointer to decide whether the module is an orphan or not, which would probably be more confusing.

[JDevlieghere]

I'm confused where the "free" in this "use-after-free" is actually occurring. RemoveIfOrphaned takes a raw pointer, checks the pointer and removes it from the map before actually removing it from the module list. So for this to cause a crash, the module already has to be pointing to invalid memory. Is the problem the fact that the loop is iterating over modules while holding a a reference to the SP and not bumping the use count?

[augusto2112]

I'm confused where the "free" in this "use-after-free" is actually occurring. RemoveIfOrphaned takes a raw pointer, checks the pointer and removes it from the map before actually removing it from the module list. So for this to cause a crash, the module already has to be pointing to invalid memory. Is the problem the fact that the loop is iterating over modules while holding a a reference to the SP and not bumping the use count?

The caller of ModuleList::RemoveSharedModuleIfOrphaned first gets the raw pointer, then resets the shared pointer, then calls ModuleList::RemoveSharedModuleIfOrphaned. If that shared pointer was the only one to that memory address, the underlying pointer is now invalid.

Old code:

        for (ModuleSP &old_module_sp : replaced_modules) {
          auto use_count = old_module_sp.use_count();
          Module *old_module_ptr = old_module_sp.get();
          old_module_sp.reset();
          ModuleList::RemoveSharedModuleIfOrphaned(old_module_ptr);

SharedModuleList::RemoveFromMap will then try to read the filespec from that invalid pointer, which is the use after free.

[augusto2112]

@JDevlieghere @felipepiovezan any suggestions on this, or can I merge it as is?

The other way to do this would be to allow already freed pointers, document it, and be careful to never dereference it. That feels weird and wasteful though.

I guess a third possibility is add a new function which takes a reference to the shared pointer (haha), have it check the count, free the pointer from shared pointer and remove the orphans if the reference count > 1.

[JDevlieghere]

Can we solve this by handing down a weak pointer to the module? That way we can check if the ref count has hit zero.

[augusto2112]

@JDevlieghere I updated it to use a weak pointer instead.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[JDevlieghere]

We discussed this offline today. I should've been less terse in explaining my idea, which is to use weak pointers all the way down, lock it just before we're going to remove it to create a critical section, account for the increase in refcount, and then release the pointer. Augusto seems on board and was going to give that a go.

[augusto2112]

@JDevlieghere sorry for the delay, got sidetracked with other work.

[JDevlieghere]

LGTM

[JDevlieghere]

Both RemoveFromMap and RemoveIfOrphaned lock the WP before removing it, which may beg the question why not do it here. The answer is because you call these functions from elsewhere so we don't want to account for that in the ref-count and complicate things more, but might be worth a comment for the future.

Also it's safe, in the sense that the ref-count can't have gone to zero between these two calls, because both check that the count is at least one more than when passed it, which means that neither of these functions by themselves can have reduce the count to zero and get the pointer deallocated.