From 10c5245383c230cd8f12d58fcb04da584d77c998 Mon Sep 17 00:00:00 2001 From: Augusto Noronha Date: Mon, 25 Aug 2025 17:16:13 -0700 Subject: [PATCH 1/5] [lldb] Fix use after free on ModuleList::RemoveSharedModuleIfOrphaned 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. --- lldb/source/Target/Target.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) 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(); From e87ab596d3909cb838025eb7a24381113b4eebc3 Mon Sep 17 00:00:00 2001 From: Augusto Noronha Date: Thu, 4 Sep 2025 13:12:09 -0700 Subject: [PATCH 2/5] Use weak_ptr instead --- lldb/include/lldb/Core/ModuleList.h | 2 +- lldb/source/Core/ModuleList.cpp | 11 ++++++++++- lldb/source/Target/Target.cpp | 7 ++----- 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/lldb/include/lldb/Core/ModuleList.h b/lldb/include/lldb/Core/ModuleList.h index 6ecdcf10fa85f..a6f0600ece876 100644 --- a/lldb/include/lldb/Core/ModuleList.h +++ b/lldb/include/lldb/Core/ModuleList.h @@ -489,7 +489,7 @@ class ModuleList { static size_t RemoveOrphanSharedModules(bool mandatory); - static bool RemoveSharedModuleIfOrphaned(const Module *module_ptr); + static bool RemoveSharedModuleIfOrphaned(const lldb::ModuleWP module_ptr); /// Applies 'callback' to each module in this ModuleList. /// If 'callback' returns false, iteration terminates. diff --git a/lldb/source/Core/ModuleList.cpp b/lldb/source/Core/ModuleList.cpp index bc63a41c90d17..9d17ebf5a2929 100644 --- a/lldb/source/Core/ModuleList.cpp +++ b/lldb/source/Core/ModuleList.cpp @@ -1278,7 +1278,16 @@ bool ModuleList::RemoveSharedModule(lldb::ModuleSP &module_sp) { return GetSharedModuleList().Remove(module_sp); } -bool ModuleList::RemoveSharedModuleIfOrphaned(const Module *module_ptr) { +bool ModuleList::RemoveSharedModuleIfOrphaned(const ModuleWP module_wp) { + // Get the module pointer if the shared pointer is still valid, + // but be careful to call RemoveIfOrphaned after the shared pointer + // is out of scope, otherwise the use count would be incremented by one and + // RemoveIfOrphaned would never identify the module as an orphan. + Module *module_ptr = nullptr; + if (ModuleSP module_sp = module_wp.lock()) + module_ptr = module_sp.get(); + else + return false; return GetSharedModuleList().RemoveIfOrphaned(module_ptr); } diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp index d5406d88ec80a..a8dd968466d9b 100644 --- a/lldb/source/Target/Target.cpp +++ b/lldb/source/Target/Target.cpp @@ -2564,12 +2564,9 @@ 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(); + auto old_module_wp = old_module_sp->weak_from_this(); old_module_sp.reset(); - // If the use count was one, this was not in the shared module list. - if (use_count > 1) - ModuleList::RemoveSharedModuleIfOrphaned(old_module_ptr); + ModuleList::RemoveSharedModuleIfOrphaned(old_module_wp); } } else module_sp.reset(); From 9aa099476e0c752c60cfa957fe4e8263d077260b Mon Sep 17 00:00:00 2001 From: Augusto Noronha Date: Thu, 4 Sep 2025 13:18:05 -0700 Subject: [PATCH 3/5] clang-format --- lldb/source/Target/Target.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp index a8dd968466d9b..214849b2cd502 100644 --- a/lldb/source/Target/Target.cpp +++ b/lldb/source/Target/Target.cpp @@ -2564,7 +2564,7 @@ ModuleSP Target::GetOrCreateModule(const ModuleSpec &orig_module_spec, m_images.Append(module_sp, notify); for (ModuleSP &old_module_sp : replaced_modules) { - auto old_module_wp = old_module_sp->weak_from_this(); + auto old_module_wp = old_module_sp->weak_from_this(); old_module_sp.reset(); ModuleList::RemoveSharedModuleIfOrphaned(old_module_wp); } From 89c8d996eef37e3fab4b3366ce5c4b1d66ace201 Mon Sep 17 00:00:00 2001 From: Augusto Noronha Date: Thu, 2 Oct 2025 16:01:49 -0700 Subject: [PATCH 4/5] Move weak pointer --- lldb/include/lldb/Core/ModuleList.h | 5 ++- lldb/source/Core/ModuleList.cpp | 57 +++++++++++++++++------------ 2 files changed, 37 insertions(+), 25 deletions(-) diff --git a/lldb/include/lldb/Core/ModuleList.h b/lldb/include/lldb/Core/ModuleList.h index a6f0600ece876..e71f3b2bad6b4 100644 --- a/lldb/include/lldb/Core/ModuleList.h +++ b/lldb/include/lldb/Core/ModuleList.h @@ -435,7 +435,7 @@ class ModuleList { size_t Remove(ModuleList &module_list); - bool RemoveIfOrphaned(const Module *module_ptr); + bool RemoveIfOrphaned(const lldb::ModuleWP module_ptr); size_t RemoveOrphans(bool mandatory); @@ -531,6 +531,9 @@ class ModuleList { Notifier *m_notifier = nullptr; + /// An orphaned module that lives only in the ModuleList has a count of 1. + static constexpr long kUseCountModuleListOrphaned = 1; + public: typedef LockingAdaptedIterable ModuleIterable; diff --git a/lldb/source/Core/ModuleList.cpp b/lldb/source/Core/ModuleList.cpp index 9d17ebf5a2929..5ff3d583a1c93 100644 --- a/lldb/source/Core/ModuleList.cpp +++ b/lldb/source/Core/ModuleList.cpp @@ -349,17 +349,20 @@ bool ModuleList::ReplaceModule(const lldb::ModuleSP &old_module_sp, return true; } -bool ModuleList::RemoveIfOrphaned(const Module *module_ptr) { - if (module_ptr) { +bool ModuleList::RemoveIfOrphaned(const ModuleWP module_wp) { + if (auto module_sp = module_wp.lock()) { std::lock_guard guard(m_modules_mutex); collection::iterator pos, end = m_modules.end(); for (pos = m_modules.begin(); pos != end; ++pos) { - if (pos->get() == module_ptr) { - if (pos->use_count() == 1) { + if (pos->get() == module_sp.get()) { + // Since module_sp increases the refcount by 1, the use count should be + // the regular use count + 1. + constexpr long kUseCountOrphaned = kUseCountModuleListOrphaned + 1; + if (pos->use_count() == kUseCountOrphaned) { pos = RemoveImpl(pos); return true; - } else - return false; + } + return false; } } } @@ -386,7 +389,7 @@ size_t ModuleList::RemoveOrphans(bool mandatory) { made_progress = false; collection::iterator pos = m_modules.begin(); while (pos != m_modules.end()) { - if (pos->use_count() == 1) { + if (pos->use_count() == kUseCountModuleListOrphaned) { pos = RemoveImpl(pos); ++remove_count; // We did make progress. @@ -832,7 +835,7 @@ class SharedModuleList { if (!module_sp) return false; std::lock_guard guard(GetMutex()); - RemoveFromMap(*module_sp.get()); + RemoveFromMap(module_sp); return m_list.Remove(module_sp, use_notifier); } @@ -843,10 +846,10 @@ class SharedModuleList { ReplaceEquivalentInMap(module_sp); } - bool RemoveIfOrphaned(const Module *module_ptr) { + bool RemoveIfOrphaned(const ModuleWP module_wp) { std::lock_guard guard(GetMutex()); - RemoveFromMap(*module_ptr, /*if_orphaned=*/true); - return m_list.RemoveIfOrphaned(module_ptr); + RemoveFromMap(module_wp, /*if_orphaned=*/true); + return m_list.RemoveIfOrphaned(module_wp); } std::recursive_mutex &GetMutex() const { return m_list.GetMutex(); } @@ -886,16 +889,22 @@ class SharedModuleList { m_name_to_modules[name].push_back(module_sp); } - void RemoveFromMap(const Module &module, bool if_orphaned = false) { - ConstString name = module.GetFileSpec().GetFilename(); - if (!m_name_to_modules.contains(name)) - return; - llvm::SmallVectorImpl &vec = m_name_to_modules[name]; - for (auto *it = vec.begin(); it != vec.end(); ++it) { - if (it->get() == &module) { - if (!if_orphaned || it->use_count() == kUseCountOrphaned) { - vec.erase(it); - break; + void RemoveFromMap(const ModuleWP module_wp, bool if_orphaned = false) { + if (auto module_sp = module_wp.lock()) { + ConstString name = module_sp->GetFileSpec().GetFilename(); + if (!m_name_to_modules.contains(name)) + return; + llvm::SmallVectorImpl &vec = m_name_to_modules[name]; + for (auto *it = vec.begin(); it != vec.end(); ++it) { + if (it->get() == module_sp.get()) { + // Since module_sp increases the refcount by 1, the use count should + // be the regular use count + 1. + constexpr long kUseCountOrphaned = + kUseCountSharedModuleListOrphaned + 1; + if (!if_orphaned || it->use_count() == kUseCountOrphaned) { + vec.erase(it); + break; + } } } } @@ -933,7 +942,7 @@ class SharedModuleList { // remove_if moves the elements that match the condition to the end of the // container, and returns an iterator to the first element that was moved. auto *to_remove_start = llvm::remove_if(vec, [](const ModuleSP &module) { - return module.use_count() == kUseCountOrphaned; + return module.use_count() == kUseCountSharedModuleListOrphaned; }); ModuleList to_remove; @@ -976,7 +985,7 @@ class SharedModuleList { llvm::DenseMap> m_name_to_modules; /// The use count of a module held only by m_list and m_name_to_modules. - static constexpr long kUseCountOrphaned = 2; + static constexpr long kUseCountSharedModuleListOrphaned = 2; }; struct SharedModuleListInfo { @@ -1288,7 +1297,7 @@ bool ModuleList::RemoveSharedModuleIfOrphaned(const ModuleWP module_wp) { module_ptr = module_sp.get(); else return false; - return GetSharedModuleList().RemoveIfOrphaned(module_ptr); + return GetSharedModuleList().RemoveIfOrphaned(module_wp); } bool ModuleList::LoadScriptingResourcesInTarget(Target *target, From 67d6b403f5e9fca4aa8525edd71658e5ace06996 Mon Sep 17 00:00:00 2001 From: Augusto Noronha Date: Thu, 2 Oct 2025 16:34:09 -0700 Subject: [PATCH 5/5] remove leftover code --- lldb/source/Core/ModuleList.cpp | 9 --------- 1 file changed, 9 deletions(-) diff --git a/lldb/source/Core/ModuleList.cpp b/lldb/source/Core/ModuleList.cpp index 5ff3d583a1c93..2ccebf3fabfc5 100644 --- a/lldb/source/Core/ModuleList.cpp +++ b/lldb/source/Core/ModuleList.cpp @@ -1288,15 +1288,6 @@ bool ModuleList::RemoveSharedModule(lldb::ModuleSP &module_sp) { } bool ModuleList::RemoveSharedModuleIfOrphaned(const ModuleWP module_wp) { - // Get the module pointer if the shared pointer is still valid, - // but be careful to call RemoveIfOrphaned after the shared pointer - // is out of scope, otherwise the use count would be incremented by one and - // RemoveIfOrphaned would never identify the module as an orphan. - Module *module_ptr = nullptr; - if (ModuleSP module_sp = module_wp.lock()) - module_ptr = module_sp.get(); - else - return false; return GetSharedModuleList().RemoveIfOrphaned(module_wp); }