Skip to content

Refcount DebugAdapter to enforce cross-thread lifetime guarantees #1092

@xusheng6

Description

@xusheng6

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:

  1. 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.
  2. 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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions