bridge: record what actually closes the *Async reply sites' UAF window (#489)

docs/spec/concurrency_and_lifetimes.md

@@ -255,12 +255,13 @@ No framework path does this; a caller that arranges it is outside the contract.
 ### The same check-then-call shape, elsewhere in `Bridge` — issue #489
 
 `~BridgeHandler` was one check-then-call site of this shape; issue #489 named
-four more inside `Bridge` itself, each gating a callback on `liveness()` (or an
-equivalent snapshot) and then touching `this`. Not all of them can take
-`BridgeLifetime`'s gate the way `~BridgeHandler` does — the gate makes `~Bridge`
-block for as long as the gated span takes, and a span that can call into
-consumer-supplied code or a backend's blocking registration path turns that
-bounded wait into an unbounded one. Three dispositions, by site:
+four more inside `Bridge` itself, and a follow-up audit of that issue found three
+further ones on the `IBackend` `*Async` reply path — each gating a callback on
+`liveness()` (or an equivalent snapshot) and then touching `this`. Not all of
+them can take `BridgeLifetime`'s gate the way `~BridgeHandler` does — the gate
+makes `~Bridge` block for as long as the gated span takes, and a span that can
+call into consumer-supplied code or a backend's blocking registration path turns
+that bounded wait into an unbounded one. Four dispositions, by site:
 
 - **`executeVia()`'s `.then`/`.onError` continuations.** `_pendingCalls` and
   `_subscriptions` are now heap-allocated (`shared_ptr`, like `BridgeLifetime`
@@ -290,6 +291,33 @@ bounded wait into an unbounded one. Three dispositions, by site:
   loop pumps the very deferred-delete event that could run the destructor —
   that is a self-deadlock, not a slow teardown. No safe mechanical fix is known
   for this site; it remains open, tracked as the residual scope of issue #489.
+- **The `*Async` reply callbacks** — `attachHandlerAsync`, `ensureBoundAsync`
+  and `assignHandlerPrimary`, three of the four `IBackend` async hooks. (The
+  fourth, `registerHandlerImpl`, is covered by the `BridgeLifetime` bullet
+  above and is not one of these.) Each of the three keeps a
+  `CallbackToken::active()` check and then takes `_attachMtx` and calls
+  `loadBackend()`, so the two-step shape is present in the source. What closes
+  the window is not a gate but a contract on the backend:
+  `IBackend::registerModelAsync`'s doc comment now states that a backend
+  overriding any `*Async` hook must deliver its callbacks on a thread from
+  which `~Bridge` cannot run concurrently. `QtWebSocketBackend` — the only
+  backend in the tree that overrides them — satisfies this by construction
+  rather than by care: it must itself be used from the Qt event loop thread,
+  and fires all four callbacks from `onTextMessage` on that same thread, so the
+  check and the use cannot straddle a destructor. Gating these instead would
+  make `~Bridge` block behind `_attachMtx`, which the synchronous
+  `attachHandler` holds across a full `attachModel` round trip — the same shape
+  of objection that rules a gate out for the reconnect handler. **The safety
+  here is therefore conditional on a documented contract, not on `Bridge`
+  alone**: a future backend delivering these replies on its own transport
+  thread would reopen morph#486's use-after-free, and that is a contract break
+  rather than a latent race to be rediscovered.
+
+`switchBackend()` and `whenBound()` were audited for the same shape and do not
+have it. Both are ordinary synchronous member functions called by the bridge's
+owner, not liveness-gated callbacks: neither takes a `CallbackToken`, and
+`whenBound()`'s queued waiters capture only the `CompletionState` they resolve,
+never `this`.
 
 ### `RemoteServer` must be `make_shared` and outlive its transports
 

docs/spec/core/backend.md

@@ -253,9 +253,56 @@ this pair's contract does not forbid it on the success path either.
 (they defer the outcome out of the dispatch frame rather than acting on it
 under `_attachMtx`), so an inline completion is legal, not merely tolerated.
 
-`assignPrimary` — the *promote* half of a result-keyed action — has **no**
-async counterpart and is not covered here: it is still synchronous on every
-backend, so a result-keyed creating action still blocks at that step.
+### Promotion — `assignPrimaryAsync`
+
+`assignPrimary` — the *promote* half of a result-keyed action — has the same
+optional non-blocking counterpart, `assignPrimaryAsync`, preferred by
+`Bridge::assignHandlerPrimary` and falling back to the synchronous
+`assignPrimary` when a backend returns `false`. Its `onRegistered` echoes the
+`ModelId` back for symmetry with `registerModelAsync`'s callback shape, and
+fires for the no-op cases `assignPrimary` documents (empty primary, dead `mid`,
+key already taken, `mid` already keyed differently) — those are not backend
+failures, so they resolve `onRegistered` exactly as the synchronous path
+returns normally for them. `onError` is for a genuine backend or transport
+failure only.
+
+### Threading contract — the callback's delivery thread
+
+**All four `*Async` hooks share one requirement: a backend must not deliver
+`onRegistered`/`onError` on a thread from which `~Bridge` can run
+concurrently.** This is a contract on the backend, not an implementation detail
+of `Bridge`.
+
+The reason is on `Bridge`'s side. Three of the four continuations behind these
+hooks — `ensureBoundAsync`, `attachHandlerAsync` and `assignHandlerPrimary` in
+`core/bridge.hpp` — test `CallbackToken::active()` and then dereference `this`
+(each takes `_attachMtx` and calls `loadBackend()`). Those are two steps, so a
+`~Bridge` completing between them is the use-after-free of issue #486 — the same
+check-then-act shape
+[concurrency_and_lifetimes.md](../concurrency_and_lifetimes.md) describes.
+
+`registerHandlerImpl`'s callback is the exception and does **not** rely on this
+contract: it holds `detail::BridgeLifetime` across its whole touch of `this`
+(`_mtx`, `loadBackend()`), which is safe there because nothing inside that span
+calls into consumer code or a blocking backend path.
+
+The other three cannot take that same gate. It makes `~Bridge` *block* for the
+gated span, and each span acquires `_attachMtx` — which the synchronous
+`Bridge::attachHandler` holds across a full `attachModel` round trip, unbounded
+on a wire backend. What closes the window instead is the delivery thread.
+`QtWebSocketBackend` — the only backend in the tree overriding any of the four —
+satisfies the contract by construction rather than by care: it must itself be
+used from the Qt event loop thread, and every *reply-driven* callback fires from
+`onTextMessage` on that same thread, so the check and the use cannot straddle a
+destructor. Its two non-reply paths do not weaken this — a disconnected or no-op
+dispatch invokes the callback inline, inside the caller's own frame (which
+`Bridge::detail::parkIfInFrame` exists to handle), and `cancelPending` fires the
+remainder from `~Bridge` itself, which is not a *concurrent* destructor.
+
+A backend that replies on its own transport thread therefore reopens #486's
+use-after-free. That is a **contract break**, diagnosable from this page and
+from `IBackend::registerModelAsync`'s doc comment — not a latent race to be
+rediscovered by a sanitizer.
 
 ## Error types
 

include/morph/core/backend.hpp

@@ -130,6 +130,37 @@ struct IBackend {
     /// falls back to `registerModelWithContext` in that case, so a backend
     /// with no override behaves synchronously.
     ///
+    /// @note **Threading contract, shared by all four `*Async` hooks: the
+    ///       callback's thread must not be able to run `~Bridge` concurrently.**
+    ///       Three of the four `Bridge` continuations behind these hooks —
+    ///       `attachHandlerAsync`, `ensureBoundAsync` and `assignHandlerPrimary`
+    ///       in `core/bridge.hpp` — test `CallbackToken::active()` and then
+    ///       dereference `this`. Those are two steps, so a `~Bridge` that
+    ///       completes between them is morph#486's use-after-free.
+    ///       (`registerHandlerImpl`'s callback is the exception: it holds
+    ///       `detail::BridgeLifetime` across its whole touch of `this`, so it
+    ///       does not depend on this contract.)
+    ///
+    ///       Those three cannot take that same gate. It makes `~Bridge` *block*
+    ///       for the gated span, and each span acquires `_attachMtx` — which
+    ///       the synchronous `Bridge::attachHandler` holds across a full
+    ///       `attachModel` round trip, unbounded on a wire backend. What closes
+    ///       the window instead is delivery on the thread that owns the
+    ///       `Bridge`. `QtWebSocketBackend` — the only backend in the tree
+    ///       overriding any of these — satisfies that by construction: it must
+    ///       itself be used from the Qt event loop thread
+    ///       (`qt/qt_websocket_backend.hpp`), and every *reply-driven* callback
+    ///       fires from `onTextMessage` on that same thread, so check and use
+    ///       cannot straddle a destructor. Its two non-reply paths do not weaken
+    ///       this: a disconnected or no-op dispatch invokes the callback inline,
+    ///       still inside the caller's own frame (which `detail::parkIfInFrame`
+    ///       exists to handle), and `cancelPending` fires the remainder from
+    ///       `~Bridge` itself — which is not a *concurrent* destructor. **A backend that delivers these callbacks on
+    ///       a thread the `Bridge`'s owner does not control breaks this contract
+    ///       and reopens that use-after-free** — it is a contract break, not a
+    ///       latent race to be discovered. See morph#489 and
+    ///       docs/spec/concurrency_and_lifetimes.md.
+    ///
     /// @note Scope: only `Bridge::registerHandler()`'s plain (non-shared)
     ///       registration path — a `BridgeHandler`'s initial construction —
     ///       uses this. Shared/keyed registration has its own opt-in async
@@ -169,7 +200,9 @@ struct IBackend {
     /// returns `true` immediately, then invokes exactly one of
     /// @p onRegistered / @p onError once the reply arrives, on the backend's
     /// own thread (unless the backend is destroyed first, in which case
-    /// neither fires).
+    /// neither fires) — subject to `registerModelAsync`'s threading contract,
+    /// which applies here unchanged: that thread must not be able to run
+    /// `~Bridge` concurrently.
     ///
     /// The default implementation offers no async path and returns `false`
     /// without calling either callback — the caller (`Bridge::ensureBoundAsync`)
@@ -266,7 +299,8 @@ struct IBackend {
     ///
     /// Same rationale and shape as `registerModelSharedAsync` immediately
     /// above (itself mirroring `registerModelAsync`) — see that doc comment
-    /// for the full opt-in/fallback contract.
+    /// for the full opt-in/fallback contract, and `registerModelAsync`'s for
+    /// the threading contract the callback's delivery thread must satisfy.
     ///
     /// @note Unlike the synchronous `attachModel` default above, this method
     ///       does *not* release @p current itself: an overriding backend is
@@ -342,7 +376,9 @@ struct IBackend {
     /// request and return `true` immediately, then invoke exactly one of
     /// @p onRegistered / @p onError once the reply arrives, on the backend's
     /// own thread (unless the backend is destroyed first, in which case
-    /// neither fires). `Bridge::assignHandlerPrimary` prefers this path when
+    /// neither fires) — subject to `registerModelAsync`'s threading contract,
+    /// which applies here unchanged: that thread must not be able to run
+    /// `~Bridge` concurrently. `Bridge::assignHandlerPrimary` prefers this path when
     /// it is available and falls back to the synchronous `assignPrimary`
     /// otherwise, so every backend that has not opted in (every backend as of
     /// this writing, other than `QtWebSocketBackend`) is unaffected.

include/morph/core/bridge.hpp

@@ -590,6 +590,14 @@ class Bridge {
                     if (detail::parkIfInFrame(*handoff, true, newId, nullptr)) {
                         return;  // Completed inline: the dispatching frame will finish this.
                     }
+                    // This check and the `this` touch below it are two steps -- the
+                    // morph#486 shape. Closed not by a gate but by
+                    // `IBackend::registerModelAsync`'s threading contract (see its
+                    // doc comment): an overriding backend must deliver `*Async`
+                    // replies on a thread that cannot run `~Bridge` concurrently.
+                    // Gating instead would block `~Bridge` behind `_attachMtx`,
+                    // which `attachHandler` holds across a full `attachModel` round
+                    // trip. See morph#489.
                     if (!weakLiveness.active()) {
                         return;  // The Bridge is gone; publishing this id would be pointless.
                     }
@@ -739,6 +747,14 @@ class Bridge {
                     if (detail::parkIfInFrame(*handoff, true, newId, nullptr)) {
                         return;  // Completed inline: the dispatching frame will finish this.
                     }
+                    // This check and the `this` touch below it are two steps -- the
+                    // morph#486 shape. Closed not by a gate but by
+                    // `IBackend::registerModelAsync`'s threading contract (see its
+                    // doc comment): an overriding backend must deliver `*Async`
+                    // replies on a thread that cannot run `~Bridge` concurrently.
+                    // Gating instead would block `~Bridge` behind `_attachMtx`,
+                    // which `attachHandler` holds across a full `attachModel` round
+                    // trip. See morph#489.
                     if (!weakLiveness.active()) {
                         return;  // The Bridge is gone; publishing this id would be pointless.
                     }
@@ -875,6 +891,14 @@ class Bridge {
         bool const started = backend->assignPrimaryAsync(
             ::morph::exec::detail::ModelId{raw}, binding->typeId, primary,
             [this, weakLiveness, weakBackend, weakBinding, primary](::morph::exec::detail::ModelId) {
+                // This check and the `this` touch below it are two steps -- the
+                // morph#486 shape. Closed not by a gate but by
+                // `IBackend::registerModelAsync`'s threading contract (see its
+                // doc comment): an overriding backend must deliver `*Async`
+                // replies on a thread that cannot run `~Bridge` concurrently.
+                // Gating instead would block `~Bridge` behind `_attachMtx`,
+                // which `attachHandler` holds across a full `attachModel` round
+                // trip. See morph#489.
                 if (!weakLiveness.active()) {
                     return;  // The Bridge is gone; do not touch `this`.
                 }

scripts/branch_partial_allowlist.json

@@ -92,7 +92,7 @@
     },
     {
       "file": "include/morph/core/backend.hpp",
-      "line": 741,
+      "line": 777,
       "source": "if (iter != _models.end()) {",
       "reason": "Unreachable by construction given the `_changeAware`/`_models` invariant (core audit finding BK2). Every model id is inserted into `_changeAware` (when change-aware) in the same `_regMtx`-held critical section that inserts it into `_models` (`createAndTrack`, this file: `if (holder->isBackendChangeAware()) { _changeAware.insert(mid); } _models[mid] = std::move(holder);`), and both are erased together at the single erasure site (`deregisterModel`: `_models.erase(mid); _changeAware.erase(mid);`, also under `_regMtx`). `notifyBackendChanged()` (this function) holds the same `_regMtx` while iterating `_changeAware` and looking each id up in `_models` at this line, so every id it walks is guaranteed still present in `_models` -- the \"not found\" arm cannot occur without a code change that breaks this lockstep bookkeeping."
     },
@@ -110,13 +110,13 @@
     },
     {
       "file": "include/morph/core/bridge.hpp",
-      "line": 1421,
+      "line": 1445,
       "source": "if (_executeDeadline.count() > 0 && _timeoutScheduler) {",
       "reason": "Unreachable by construction (core audit finding B6). `setExecuteDeadline` (this file) is the only writer of both `_executeDeadline` and `_timeoutScheduler`, and always creates `_timeoutScheduler` in the same call that sets `_executeDeadline` positive (`_executeDeadline = deadline; if (_executeDeadline.count() > 0 && !_timeoutScheduler) { _timeoutScheduler = std::make_shared<...>(); }`, both under `_executeDeadlineMtx`); nothing anywhere resets `_timeoutScheduler` back to null -- the class's own doc comment on `setExecuteDeadline` says so explicitly (\"setting the deadline back to 0 stops new calls from arming it but does not tear the thread down\"). So `_executeDeadline > 0 && !_timeoutScheduler` cannot happen at this line once any positive deadline has ever been set."
     },
     {
       "file": "include/morph/core/bridge.hpp",
-      "line": 1539,
+      "line": 1563,
       "source": "if (deadlineHandle && schedulerRef) {",
       "reason": "Unreachable by construction, same joint-assignment shape as B6 above (core audit finding B11, reclassified (a)->(b) on review). `deadlineHandle` and `schedulerRef` are assigned together, a few lines above this one in `executeVia`, only inside `if (_executeDeadline.count() > 0 && _timeoutScheduler) { schedulerRef = _timeoutScheduler; ... }` (see the bridge.hpp:1421 entry above) -- there is no path that sets `deadlineHandle` without also having set `schedulerRef` from the same non-null `_timeoutScheduler` in the same conditional. So `schedulerRef` null while `deadlineHandle` is non-null cannot occur; the only theoretically-open arm this compound condition has is structurally impossible."
     },