Fix a potential use-after-free in StopInfoBreakpoint.

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,7 +20,15 @@ namespace lldb_private {
 
 class BreakpointLocationCollection {
 public:
-  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);
 
   ~BreakpointLocationCollection();
 
@@ -164,6 +173,10 @@ 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;
+  std::map<std::pair<lldb::break_id_t, lldb::break_id_t>, lldb::BreakpointSP>
+      m_preserved_bps;
 
 public:
   typedef llvm::iterator_range<collection::const_iterator>
@@ -172,7 +185,6 @@ class BreakpointLocationCollection {
     return BreakpointLocationCollectionIterable(m_break_loc_collection);
   }
 };
-
 } // namespace lldb_private
 
 #endif // LLDB_BREAKPOINT_BREAKPOINTLOCATIONCOLLECTION_H

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,15 +27,35 @@ 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) {
+      lldb::break_id_t bp_loc_id = bp_loc->GetID();
+      Breakpoint &bkpt = bp_loc->GetBreakpoint();
+      lldb::break_id_t bp_id = bkpt.GetID();
+      std::pair<lldb::break_id_t, lldb::break_id_t> key =
+          std::make_pair(bp_id, bp_loc_id);
+      auto entry = m_preserved_bps.find(key);
+      if (entry == m_preserved_bps.end())
+        m_preserved_bps.emplace(key, bkpt.shared_from_this());
+    }
+  }
 }
 
 bool BreakpointLocationCollection::Remove(lldb::break_id_t bp_id,
                                           lldb::break_id_t bp_loc_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) {
+      std::pair<lldb::break_id_t, lldb::break_id_t> key =
+          std::make_pair(bp_id, bp_loc_id);
+      auto entry = m_preserved_bps.find(key);
+      if (entry == m_preserved_bps.end())
+        assert(0 && "Breakpoint added to collection but not preserving map.");
+      else
+        m_preserved_bps.erase(entry);
+    }
     m_break_loc_collection.erase(pos);
     return true;
   }

lldb/source/Target/StopInfo.cpp

@@ -87,19 +87,24 @@ 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();
   }
 
   StopInfoBreakpoint(Thread &thread, break_id_t break_id, bool should_stop)
       : 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;
 };
 

lldb/test/API/functionalities/breakpoint/callback_deletes_breakpoints/Makefile

@@ -0,0 +1,4 @@
+C_SOURCES := main.c
+CFLAGS_EXTRAS := -std=c99
+
+include Makefile.rules

lldb/test/API/functionalities/breakpoint/callback_deletes_breakpoints/TestCallbackDeletesBreakpoints.py

@@ -0,0 +1,67 @@
+"""
+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")

lldb/test/API/functionalities/breakpoint/callback_deletes_breakpoints/main.c

@@ -0,0 +1,12 @@
+#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;
+}