Fix a potential use-after-free in StopInfoBreakpoint. #163471
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
StopInfoBreakpoint keeps a BreakpointLocationCollection for all the breakpoint locations at the BreakpointSite that was hit. It is also lives through the time a given thread is stopped, so there are plenty of opportunities for one of the owning breakpoints to get deleted. But BreakpointLocations don't keep their owner Breakpoints alive, so if the BreakpointLocationCollection can live past when some code gets a chance to delete an owner breakpoint, and then you ask that location for some breakpoint information, it will access freed memory. This wasn't a problem before PR llvm#158128 because the StopInfoBreakpoint just kept the BreakpointSite that was hit, and when you asked it questions, it relooked up that list. That was not great, however, because if you hit breakpoints 5 & 6, deleted 5 and then asked which breakpoints got hit, you would just get 6. For that and other reasons that PR changed to storing a BreakpointLocationCollection of the breakpoints that were hit. That's better from a UI perspective but caused this potential problem. I fix it by adding a variant of the BreakpointLocationCollection that also holds onto a shared pointer to the Breakpoints that own the locations that were hit, thus keeping them alive till the StopInfoBreakpoint goes away. This fixed the ASAN assertion. I also added a test that works harder to cause trouble by deleting breakpoints during a stop.
|
@llvm/pr-subscribers-lldb Author: None (jimingham) ChangesStopInfoBreakpoint keeps a BreakpointLocationCollection for all the breakpoint locations at the BreakpointSite that was hit. It is also lives through the time a given thread is stopped, so there are plenty of opportunities for one of the owning breakpoints to get deleted. But BreakpointLocations don't keep their owner Breakpoints alive, so if the BreakpointLocationCollection can live past when some code gets a chance to delete an owner breakpoint, and then you ask that location for some breakpoint information, it will access freed memory. This wasn't a problem before PR #158128 because the StopInfoBreakpoint just kept the BreakpointSite that was hit, and when you asked it questions, it relooked up that list. That was not great, however, because if you hit breakpoints 5 & 6, deleted 5 and then asked which breakpoints got hit, you would just get 6. For that and other reasons that PR changed to storing a BreakpointLocationCollection of the breakpoints that were hit. That's better from a UI perspective but caused this potential problem. I fix it by adding a variant of the BreakpointLocationCollection that also holds onto a shared pointer to the Breakpoints that own the locations that were hit, thus keeping them alive till the StopInfoBreakpoint goes away. This fixed the ASAN assertion. I also added a test that works harder to cause trouble by deleting breakpoints during a stop. Full diff: https://github.com/llvm/llvm-project/pull/163471.diff 6 Files Affected:
diff --git a/lldb/include/lldb/Breakpoint/BreakpointLocationCollection.h b/lldb/include/lldb/Breakpoint/BreakpointLocationCollection.h
index 1df4e074680f5..10a22476aba9b 100644
--- a/lldb/include/lldb/Breakpoint/BreakpointLocationCollection.h
+++ b/lldb/include/lldb/Breakpoint/BreakpointLocationCollection.h
@@ -9,6 +9,7 @@
#ifndef LLDB_BREAKPOINT_BREAKPOINTLOCATIONCOLLECTION_H
#define LLDB_BREAKPOINT_BREAKPOINTLOCATIONCOLLECTION_H
+#include <map>
#include <mutex>
#include <vector>
@@ -19,9 +20,17 @@ namespace lldb_private {
class BreakpointLocationCollection {
public:
- BreakpointLocationCollection();
-
- ~BreakpointLocationCollection();
+ /// Breakpoint locations don't keep their breakpoint owners alive, so neither
+ /// will a collection of breakpoint locations. However, if you need to
+ /// use this collection in a context where some of the breakpoints whose
+ /// locations are in the collection might get deleted during its lifespan,
+ /// then you need to make sure the breakpoints don't get deleted out from
+ /// under you. To do that, pass true for preserving, and so long as there is
+ /// a location for a given breakpoint in the collection, the breakpoint will
+ /// not get destroyed.
+ BreakpointLocationCollection(bool preserving = false);
+
+ virtual ~BreakpointLocationCollection();
BreakpointLocationCollection &operator=(const BreakpointLocationCollection &rhs);
@@ -30,7 +39,7 @@ class BreakpointLocationCollection {
/// \param[in] bp_loc_sp
/// Shared pointer to the breakpoint location that will get added
/// to the list.
- void Add(const lldb::BreakpointLocationSP &bp_loc_sp);
+ virtual void Add(const lldb::BreakpointLocationSP &bp_loc_sp);
/// Removes the breakpoint location given by \b breakID from this
/// list.
@@ -43,7 +52,7 @@ class BreakpointLocationCollection {
///
/// \result
/// \b true if the breakpoint was in the list.
- bool Remove(lldb::break_id_t break_id, lldb::break_id_t break_loc_id);
+ virtual bool Remove(lldb::break_id_t break_id, lldb::break_id_t break_loc_id);
/// Returns a shared pointer to the breakpoint location with id \a
/// breakID.
@@ -164,6 +173,14 @@ class BreakpointLocationCollection {
collection m_break_loc_collection;
mutable std::mutex m_collection_mutex;
+ /// These are used if we're preserving breakpoints in this list:
+ const bool m_preserving_bkpts = false;
+ struct RefCountedBPSP {
+ RefCountedBPSP(lldb::BreakpointSP in_bp_sp) : ref_cnt(1), bp_sp(in_bp_sp) {}
+ uint64_t ref_cnt;
+ lldb::BreakpointSP bp_sp;
+ };
+ std::map<lldb::break_id_t, RefCountedBPSP> m_preserved_bps;
public:
typedef llvm::iterator_range<collection::const_iterator>
@@ -172,7 +189,6 @@ class BreakpointLocationCollection {
return BreakpointLocationCollectionIterable(m_break_loc_collection);
}
};
-
} // namespace lldb_private
#endif // LLDB_BREAKPOINT_BREAKPOINTLOCATIONCOLLECTION_H
diff --git a/lldb/source/Breakpoint/BreakpointLocationCollection.cpp b/lldb/source/Breakpoint/BreakpointLocationCollection.cpp
index 1d052c5fc9bb6..f250091e46f71 100644
--- a/lldb/source/Breakpoint/BreakpointLocationCollection.cpp
+++ b/lldb/source/Breakpoint/BreakpointLocationCollection.cpp
@@ -17,7 +17,8 @@ using namespace lldb;
using namespace lldb_private;
// BreakpointLocationCollection constructor
-BreakpointLocationCollection::BreakpointLocationCollection() = default;
+BreakpointLocationCollection::BreakpointLocationCollection(bool preserving) :
+ m_preserving_bkpts(preserving) {}
// Destructor
BreakpointLocationCollection::~BreakpointLocationCollection() = default;
@@ -26,8 +27,18 @@ void BreakpointLocationCollection::Add(const BreakpointLocationSP &bp_loc) {
std::lock_guard<std::mutex> guard(m_collection_mutex);
BreakpointLocationSP old_bp_loc =
FindByIDPair(bp_loc->GetBreakpoint().GetID(), bp_loc->GetID());
- if (!old_bp_loc.get())
+ if (!old_bp_loc.get()) {
m_break_loc_collection.push_back(bp_loc);
+ if (m_preserving_bkpts) {
+ Breakpoint &bkpt = bp_loc->GetBreakpoint();
+ lldb::break_id_t bp_id = bkpt.GetID();
+ auto entry = m_preserved_bps.find(bp_id);
+ if (entry == m_preserved_bps.end())
+ m_preserved_bps.emplace(bp_id, RefCountedBPSP(bkpt.shared_from_this()));
+ else
+ entry->second.ref_cnt++;
+ }
+ }
}
bool BreakpointLocationCollection::Remove(lldb::break_id_t bp_id,
@@ -35,6 +46,13 @@ bool BreakpointLocationCollection::Remove(lldb::break_id_t bp_id,
std::lock_guard<std::mutex> guard(m_collection_mutex);
collection::iterator pos = GetIDPairIterator(bp_id, bp_loc_id); // Predicate
if (pos != m_break_loc_collection.end()) {
+ if (m_preserving_bkpts) {
+ auto entry = m_preserved_bps.find(bp_id);
+ assert(entry != m_preserved_bps.end()
+ && "Breakpoint added to base but not preserving map.");
+ if (--entry->second.ref_cnt == 0)
+ m_preserved_bps.erase(entry);
+ }
m_break_loc_collection.erase(pos);
return true;
}
diff --git a/lldb/source/Target/StopInfo.cpp b/lldb/source/Target/StopInfo.cpp
index 7fa1fc5d71f13..6d5572c62294c 100644
--- a/lldb/source/Target/StopInfo.cpp
+++ b/lldb/source/Target/StopInfo.cpp
@@ -87,11 +87,15 @@ bool StopInfo::HasTargetRunSinceMe() {
namespace lldb_private {
class StopInfoBreakpoint : public StopInfo {
public:
+ // We use a "breakpoint preserving BreakpointLocationCollection because we
+ // may need to hand out the "breakpoint hit" list as any point, potentially
+ // after the breakpoint has been deleted. But we still need to refer to them.
StopInfoBreakpoint(Thread &thread, break_id_t break_id)
: StopInfo(thread, break_id), m_should_stop(false),
m_should_stop_is_valid(false), m_should_perform_action(true),
m_address(LLDB_INVALID_ADDRESS), m_break_id(LLDB_INVALID_BREAK_ID),
- m_was_all_internal(false), m_was_one_shot(false) {
+ m_was_all_internal(false), m_was_one_shot(false),
+ m_async_stopped_locs(true) {
StoreBPInfo();
}
@@ -99,7 +103,8 @@ class StopInfoBreakpoint : public StopInfo {
: StopInfo(thread, break_id), m_should_stop(should_stop),
m_should_stop_is_valid(true), m_should_perform_action(true),
m_address(LLDB_INVALID_ADDRESS), m_break_id(LLDB_INVALID_BREAK_ID),
- m_was_all_internal(false), m_was_one_shot(false) {
+ m_was_all_internal(false), m_was_one_shot(false),
+ m_async_stopped_locs(true) {
StoreBPInfo();
}
@@ -699,6 +704,10 @@ class StopInfoBreakpoint : public StopInfo {
lldb::break_id_t m_break_id;
bool m_was_all_internal;
bool m_was_one_shot;
+ /// The StopInfoBreakpoint lives after the stop, and could get queried
+ /// at any time so we need to make sure that it keeps the breakpoints for
+ /// each of the locations it records alive while it is around. That's what
+ /// The BreakpointPreservingLocationCollection does.
BreakpointLocationCollection m_async_stopped_locs;
};
diff --git a/lldb/test/API/functionalities/breakpoint/callback_deletes_breakpoints/Makefile b/lldb/test/API/functionalities/breakpoint/callback_deletes_breakpoints/Makefile
new file mode 100644
index 0000000000000..695335e068c0c
--- /dev/null
+++ b/lldb/test/API/functionalities/breakpoint/callback_deletes_breakpoints/Makefile
@@ -0,0 +1,4 @@
+C_SOURCES := main.c
+CFLAGS_EXTRAS := -std=c99
+
+include Makefile.rules
diff --git a/lldb/test/API/functionalities/breakpoint/callback_deletes_breakpoints/TestCallbackDeletesBreakpoints.py b/lldb/test/API/functionalities/breakpoint/callback_deletes_breakpoints/TestCallbackDeletesBreakpoints.py
new file mode 100644
index 0000000000000..e8173dc000386
--- /dev/null
+++ b/lldb/test/API/functionalities/breakpoint/callback_deletes_breakpoints/TestCallbackDeletesBreakpoints.py
@@ -0,0 +1,65 @@
+"""
+Make sure that deleting breakpoints in another breakpoint
+callback doesn't cause problems.
+"""
+
+
+import lldb
+import lldbsuite.test.lldbutil as lldbutil
+from lldbsuite.test.lldbtest import *
+
+
+class TestBreakpointDeletionInCallback(TestBase):
+ NO_DEBUG_INFO_TESTCASE = True
+
+ def test_breakpoint_deletion_in_callback(self):
+ self.build()
+ self.main_source_file = lldb.SBFileSpec("main.c")
+ self.delete_others_test()
+
+ def delete_others_test(self):
+ """You might use the test implementation in several ways, say so here."""
+
+ # This function starts a process, "a.out" by default, sets a source
+ # breakpoint, runs to it, and returns the thread, process & target.
+ # It optionally takes an SBLaunchOption argument if you want to pass
+ # arguments or environment variables.
+ (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(
+ self, "Set a breakpoint here", self.main_source_file
+ )
+
+ # Now set a breakpoint on "I did something" several times
+ #
+ bkpt_numbers = []
+ for idx in range(0,5):
+ bkpt_numbers.append(lldbutil.run_break_set_by_source_regexp(self, "// Deletable location"))
+
+ # And add commands to the third one to delete two others:
+ deleter = target.FindBreakpointByID(bkpt_numbers[2])
+ self.assertTrue(deleter.IsValid(), "Deleter is a good breakpoint")
+ commands = lldb.SBStringList()
+ deleted_ids = [bkpt_numbers[0], bkpt_numbers[3]]
+ for idx in deleted_ids:
+ commands.AppendString(f"break delete {idx}")
+
+ deleter.SetCommandLineCommands(commands)
+
+ thread_list = lldbutil.continue_to_breakpoint(process, deleter)
+ self.assertEqual(len(thread_list), 1)
+ stop_data = thread.stop_reason_data
+ # There are 5 breakpoints so 10 break_id, break_loc_id.
+ self.assertEqual(len(stop_data), 10)
+ # We should have been able to get break ID's and locations for all the
+ # breakpoints that we originally hit, but some won't be around anymore:
+ for idx in range(0,5):
+ bkpt_id = stop_data[idx*2]
+ print(f"{idx}: {bkpt_id}")
+ self.assertIn(bkpt_id, bkpt_numbers, "Found breakpoints are right")
+ loc_id = stop_data[idx*2 + 1]
+ self.assertEqual(loc_id, 1, "All breakpoints have one location")
+ bkpt = target.FindBreakpointByID(bkpt_id)
+ if bkpt_id in deleted_ids:
+ # Looking these up should be an error:
+ self.assertFalse(bkpt.IsValid(), "Deleted breakpoints are deleted")
+ else:
+ self.assertTrue(bkpt.IsValid(), "The rest are still valid")
diff --git a/lldb/test/API/functionalities/breakpoint/callback_deletes_breakpoints/main.c b/lldb/test/API/functionalities/breakpoint/callback_deletes_breakpoints/main.c
new file mode 100644
index 0000000000000..279b359523cab
--- /dev/null
+++ b/lldb/test/API/functionalities/breakpoint/callback_deletes_breakpoints/main.c
@@ -0,0 +1,15 @@
+#include <stdio.h>
+
+int
+do_something(int input) {
+ return input % 5; // Deletable location
+}
+
+int
+main()
+{
+ printf ("Set a breakpoint here.\n");
+ do_something(100);
+ do_something(200);
+ return 0;
+}
|
|
✅ With the latest revision this PR passed the Python code formatter. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Remove the `virtual` marker I no longer need.
| const bool m_preserving_bkpts = false; | ||
| struct RefCountedBPSP { | ||
| RefCountedBPSP(lldb::BreakpointSP in_bp_sp) : ref_cnt(1), bp_sp(in_bp_sp) {} | ||
| uint64_t ref_cnt; |
There was a problem hiding this comment.
Why do we need our own refcount? The shared pointer already maintains a ref count. Why can't we use that? This seems like a recipe for leaks if we don't remember to keep the two in sync.
There was a problem hiding this comment.
The ref count was to handle the case where this BreakpointLocationCollection has two locations from the same breakpoint. If you remove one of the locations, you still nee to keep the breakpoint alive for the second location. The other way to do this would be to have the {breakpoint id, breakpoint location id} be the key and add BreakpointSP's for each location's breakpoint.
This way seemedd simpler.
There was a problem hiding this comment.
Off-line Jonas expressed a preference for holding a BreakpointSP per location to simplify the code. So I pushed a change to do that.
JDevlieghere
approved these changes
Oct 20, 2025
jimingham
added a commit
to jimingham/from-apple-llvm-project
that referenced
this pull request
Oct 21, 2025
StopInfoBreakpoint keeps a BreakpointLocationCollection for all the breakpoint locations at the BreakpointSite that was hit. It is also lives through the time a given thread is stopped, so there are plenty of opportunities for one of the owning breakpoints to get deleted. But BreakpointLocations don't keep their owner Breakpoints alive, so if the BreakpointLocationCollection can live past when some code gets a chance to delete an owner breakpoint, and then you ask that location for some breakpoint information, it will access freed memory. This wasn't a problem before PR llvm#158128 because the StopInfoBreakpoint just kept the BreakpointSite that was hit, and when you asked it questions, it relooked up that list. That was not great, however, because if you hit breakpoints 5 & 6, deleted 5 and then asked which breakpoints got hit, you would just get 6. For that and other reasons that PR changed to storing a BreakpointLocationCollection of the breakpoints that were hit. That's better from a UI perspective but caused this potential problem. I fix it by adding a variant of the BreakpointLocationCollection that also holds onto a shared pointer to the Breakpoints that own the locations that were hit, thus keeping them alive till the StopInfoBreakpoint goes away. This fixed the ASAN assertion. I also added a test that works harder to cause trouble by deleting breakpoints during a stop. (cherry picked from commit c9124a1)
jimingham
added a commit
to jimingham/from-apple-llvm-project
that referenced
this pull request
Oct 21, 2025
StopInfoBreakpoint keeps a BreakpointLocationCollection for all the breakpoint locations at the BreakpointSite that was hit. It is also lives through the time a given thread is stopped, so there are plenty of opportunities for one of the owning breakpoints to get deleted. But BreakpointLocations don't keep their owner Breakpoints alive, so if the BreakpointLocationCollection can live past when some code gets a chance to delete an owner breakpoint, and then you ask that location for some breakpoint information, it will access freed memory. This wasn't a problem before PR llvm#158128 because the StopInfoBreakpoint just kept the BreakpointSite that was hit, and when you asked it questions, it relooked up that list. That was not great, however, because if you hit breakpoints 5 & 6, deleted 5 and then asked which breakpoints got hit, you would just get 6. For that and other reasons that PR changed to storing a BreakpointLocationCollection of the breakpoints that were hit. That's better from a UI perspective but caused this potential problem. I fix it by adding a variant of the BreakpointLocationCollection that also holds onto a shared pointer to the Breakpoints that own the locations that were hit, thus keeping them alive till the StopInfoBreakpoint goes away. This fixed the ASAN assertion. I also added a test that works harder to cause trouble by deleting breakpoints during a stop. (cherry picked from commit c9124a1)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
StopInfoBreakpoint keeps a BreakpointLocationCollection for all the breakpoint locations at the BreakpointSite that was hit. It is also lives through the time a given thread is stopped, so there are plenty of opportunities for one of the owning breakpoints to get deleted.
But BreakpointLocations don't keep their owner Breakpoints alive, so if the BreakpointLocationCollection can live past when some code gets a chance to delete an owner breakpoint, and then you ask that location for some breakpoint information, it will access freed memory.
This wasn't a problem before PR #158128 because the StopInfoBreakpoint just kept the BreakpointSite that was hit, and when you asked it questions, it relooked up that list. That was not great, however, because if you hit breakpoints 5 & 6, deleted 5 and then asked which breakpoints got hit, you would just get 6. For that and other reasons that PR changed to storing a BreakpointLocationCollection of the breakpoints that were hit. That's better from a UI perspective but caused this potential problem.
I fix it by adding a variant of the BreakpointLocationCollection that also holds onto a shared pointer to the Breakpoints that own the locations that were hit, thus keeping them alive till the StopInfoBreakpoint goes away.
This fixed the ASAN assertion. I also added a test that works harder to cause trouble by deleting breakpoints during a stop.