Follow-up to #1090 (worker-queue refactor + state-on-worker).
What's the current situation
DebugAdapter is a plain heap-allocated object held as a raw DebugAdapter* in DebuggerController::m_adapter (and a redundant copy in DebuggerState::m_adapter). It's created on the worker thread in CreateDebugAdapter, destroyed inside ~DebuggerState (which runs inside ~DebuggerController), and never refcounted.
All access to m_adapter from the worker thread is safe — single writer, single reader. The interesting case is cross-thread reads: RequestInterrupt (called from Pause/Restart/Quit/Detach) reads m_adapter from the caller's thread to invoke BreakInto() out-of-band. Today, two race classes exist on this path:
- Pointer race — the worker writes
m_adapter in CreateDebugAdapter while another thread reads it. Strictly UB under C++20's memory model with a plain pointer, but only fires in the unlikely window where Pause is clicked during Launch/Attach/Connect.
- Object-lifetime race — the pointer's target object gets destroyed between the read and the call. This is the meaningful failure mode (use-after-free); the pointer race is downstream of it.
How safety is provided today
By convention, not by the type:
DebugAdapter is destroyed only inside ~DebuggerState, which runs only inside ~DebuggerController.
~DebuggerController runs only when the last DbgRef<DebuggerController> drops.
- Cross-thread callers (UI handlers, C API callers, plugins) hold a
DbgRef on the controller during their call to Pause/Restart/Quit/Detach.
- The worker thread is joined as the first step of
~DebuggerController, so no in-flight worker op can race with adapter destruction.
This chain works, but it's a guarantee a reader has to reconstruct from scattered invariants. Several recent Sentry reports (BINARYNINJA-1A, -5Z, -60, etc.) hit related lifetime concerns and were ultimately patched by adding DbgRef<DebuggerController> captures at lambda sites. The same pattern applied to adapters would be a structural fix rather than a per-callsite one.
Proposed fix
Make DebugAdapter inherit DbgRefCountObject so it can be held via DbgRef<DebugAdapter>. Convert m_adapter field type from DebugAdapter* to DbgRef<DebugAdapter>. Cross-thread call sites snapshot a DbgRef for the duration of the call:
void DebuggerController::RequestInterrupt() {
m_userRequestedBreak = true;
DbgRef<DebugAdapter> adapter = m_adapter; // bumps refcount, atomic snapshot
if (adapter)
adapter->BreakInto();
// local ref drops here; adapter can be destroyed safely now
}
This collapses both race classes into one structural solution:
- The
DbgRef copy is atomic (the existing DbgRef ctor uses the refcount infra, which is already thread-safe).
- The pointee can't be destroyed while any thread holds a
DbgRef to it.
- The "by convention" lifetime guarantee becomes type-enforced — any cross-thread caller who forgets the snapshot is now an obvious bug (raw pointer in a
DbgRef-typed field would just refuse to compile in many places).
Estimated scope
DebugAdapter inherits DbgRefCountObject and gains IMPLEMENT_DEBUGGER_API_OBJECT setup (already has IMPLEMENT_DEBUGGER_API_OBJECT(BNDebugAdapter) -- needs to extend to refcount machinery).
DebuggerController::m_adapter and DebuggerState::m_adapter change from DebugAdapter* to DbgRef<DebugAdapter>. The redundant duplication between controller and state is probably worth resolving in the same pass.
- All ~50 internal
m_adapter->... callsites compile unchanged (DbgRef::operator-> is the natural pointer-arrow semantic). The change is mostly transparent for intra-worker code.
- Cross-thread callers (
RequestInterrupt, the SubmitAndWait timeout-break path) snapshot a DbgRef local before the null check and call.
CreateDebugAdapter's assignment (m_adapter = type->Create(...)) just stores into the DbgRef; the existing leak risk where overwriting m_adapter skipped delete on the prior pointer (when the adapter type didn't change, no overwrite happens; when it did, the old pointer was likely leaked) is fixed for free -- assigning to a DbgRef releases the previous reference.
Net: roughly +30 / -20 lines in the controller + state + adapter, plus minor adapter-base-class changes. Should be a small focused PR.
Out of scope for this issue
- Refcounting other internal objects (DebuggerState, DebuggerModules, etc.). Those have well-defined single-owner lifetimes and don't have the cross-thread access pattern.
- Adapter-internal thread lifecycle (EngineLoop, RSP loops, etc.). Tracked separately in the worker-queue follow-ups.
Follow-up to #1090 (worker-queue refactor + state-on-worker).
What's the current situation
DebugAdapteris a plain heap-allocated object held as a rawDebugAdapter*inDebuggerController::m_adapter(and a redundant copy inDebuggerState::m_adapter). It's created on the worker thread inCreateDebugAdapter, destroyed inside~DebuggerState(which runs inside~DebuggerController), and never refcounted.All access to
m_adapterfrom the worker thread is safe — single writer, single reader. The interesting case is cross-thread reads:RequestInterrupt(called fromPause/Restart/Quit/Detach) readsm_adapterfrom the caller's thread to invokeBreakInto()out-of-band. Today, two race classes exist on this path:m_adapterinCreateDebugAdapterwhile another thread reads it. Strictly UB under C++20's memory model with a plain pointer, but only fires in the unlikely window where Pause is clicked during Launch/Attach/Connect.How safety is provided today
By convention, not by the type:
DebugAdapteris destroyed only inside~DebuggerState, which runs only inside~DebuggerController.~DebuggerControllerruns only when the lastDbgRef<DebuggerController>drops.DbgRefon the controller during their call toPause/Restart/Quit/Detach.~DebuggerController, so no in-flight worker op can race with adapter destruction.This chain works, but it's a guarantee a reader has to reconstruct from scattered invariants. Several recent Sentry reports (BINARYNINJA-1A, -5Z, -60, etc.) hit related lifetime concerns and were ultimately patched by adding
DbgRef<DebuggerController>captures at lambda sites. The same pattern applied to adapters would be a structural fix rather than a per-callsite one.Proposed fix
Make
DebugAdapterinheritDbgRefCountObjectso it can be held viaDbgRef<DebugAdapter>. Convertm_adapterfield type fromDebugAdapter*toDbgRef<DebugAdapter>. Cross-thread call sites snapshot aDbgReffor the duration of the call:This collapses both race classes into one structural solution:
DbgRefcopy is atomic (the existingDbgRefctor uses the refcount infra, which is already thread-safe).DbgRefto it.DbgRef-typed field would just refuse to compile in many places).Estimated scope
DebugAdapterinheritsDbgRefCountObjectand gainsIMPLEMENT_DEBUGGER_API_OBJECTsetup (already hasIMPLEMENT_DEBUGGER_API_OBJECT(BNDebugAdapter)-- needs to extend to refcount machinery).DebuggerController::m_adapterandDebuggerState::m_adapterchange fromDebugAdapter*toDbgRef<DebugAdapter>. The redundant duplication between controller and state is probably worth resolving in the same pass.m_adapter->...callsites compile unchanged (DbgRef::operator->is the natural pointer-arrow semantic). The change is mostly transparent for intra-worker code.RequestInterrupt, theSubmitAndWaittimeout-break path) snapshot aDbgReflocal before the null check and call.CreateDebugAdapter's assignment (m_adapter = type->Create(...)) just stores into theDbgRef; the existing leak risk where overwritingm_adapterskippeddeleteon the prior pointer (when the adapter type didn't change, no overwrite happens; when it did, the old pointer was likely leaked) is fixed for free -- assigning to aDbgRefreleases the previous reference.Net: roughly +30 / -20 lines in the controller + state + adapter, plus minor adapter-base-class changes. Should be a small focused PR.
Out of scope for this issue