[lldb] Fix crash in BreakpointSite::BumpHitCounts

[JDevlieghere]

Fix crash in BreakpointSite::BumpHitCounts due to missing synchronization. When bumping the hit count, we were correctly acquiring the constituents mutex, but didn't protect the breakpoint location collection. This PR fixes the issue by making the iterator returned by the collection a LockingAdaptedIterable.

rdar://163760832

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Jonas Devlieghere (JDevlieghere)

Changes

Fix crash in BreakpointSite::BumpHitCounts due to missing synchronization. When bumping the hit count, we were correctly acquiring the constituents mutex, but didn't protect the breakpoint location collection. This PR fixes the issue by making the iterator returned by the collection a LockingAdaptedIterable.

rdar://163760832

---

Full diff: https://github.com/llvm/llvm-project/pull/166876.diff

1 Files Affected:

• (modified) lldb/include/lldb/Breakpoint/BreakpointLocationCollection.h (+3-2)

diff --git a/lldb/include/lldb/Breakpoint/BreakpointLocationCollection.h b/lldb/include/lldb/Breakpoint/BreakpointLocationCollection.h
index 124cb55eaf723..372bd0c51fe20 100644
--- a/lldb/include/lldb/Breakpoint/BreakpointLocationCollection.h
+++ b/lldb/include/lldb/Breakpoint/BreakpointLocationCollection.h
@@ -179,10 +179,11 @@ class BreakpointLocationCollection {
       m_preserved_bps;
 
 public:
-  typedef llvm::iterator_range<collection::const_iterator>
+  typedef LockingAdaptedIterable<std::mutex, collection>
       BreakpointLocationCollectionIterable;
   BreakpointLocationCollectionIterable BreakpointLocations() {
-    return BreakpointLocationCollectionIterable(m_break_loc_collection);
+    return BreakpointLocationCollectionIterable(m_break_loc_collection,
+                                                m_collection_mutex);
   }
 };
 } // namespace lldb_private

[jimingham]

Since the mutex is not recursive, that means while iterating over the locations in a collection, you can't recursively iterate over the same collection. For that to be a problem, you'd have to go from one of the locations you were iterating over in the Collection and find your way back to the same collection.

The Collections aren't how the Breakpoints store their locations. So you wouldn't be able to re-lookup your current collection from the Breakpoint. The other fairly public BreakpointLocationCollection is the "owners" BreakpointLocationCollection held by a BreakpointSite. Fortunately, the BreakpointSite never hands out its BreakpointLocationCollection, it just uses it internally. Something like BreakpointLocationCollection::ShouldStop does enough work that in the course of that someone might want to look up the Site and do IsBreakpointAtThisSite.

We're saved from that danger because BreakpointSite::ShouldStop always makes a copy of the BreakpointLocationCollection to iterate over, so that would be a different lock. This all feels a little too hand-balanced to me, and hard to ensure it won't cause deadlocks.

But I can't think of a great solution to this risk short of making this a recursive mutex, which always feels like giving up...

[JDevlieghere]

It sounds like you're on the fence. If you're looking for a tie-breaker... I hate recursive mutexes as much as anyone else, but it's nice that you don't have to think about it and it takes away a potential foot gun down the road.

[jimingham]

LGTM