@llvm/pr-subscribers-lldb

Author: Augusto Noronha (augusto2112)

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();

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

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?

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.

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

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.