[Lldb-commits] [lldb] [lldb] Fix use after free on ModuleList::RemoveSharedModuleIfOrphaned (PR #155331)
Augusto Noronha via lldb-commits
lldb-commits at lists.llvm.org
Thu Oct 2 16:34:49 PDT 2025
https://github.com/augusto2112 updated https://github.com/llvm/llvm-project/pull/155331
>From 10c5245383c230cd8f12d58fcb04da584d77c998 Mon Sep 17 00:00:00 2001
From: Augusto Noronha <anoronha at apple.com>
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 <anoronha at apple.com>
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 <anoronha at apple.com>
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 <anoronha at apple.com>
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<std::recursive_mutex, collection>
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<std::recursive_mutex> 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<std::recursive_mutex> 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<std::recursive_mutex> 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<ModuleSP> &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<ModuleSP> &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<ConstString, llvm::SmallVector<ModuleSP, 1>> 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 <anoronha at apple.com>
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);
}
More information about the lldb-commits
mailing list