Fix a potential use-after-free in StopInfoBreakpoint.

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.

Author: jimingham

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

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,32 @@ 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,
                                           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) {
+      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;
   }

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,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")

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;
+}