diff --git a/docs/spec/concurrency_and_lifetimes.md b/docs/spec/concurrency_and_lifetimes.md index 1b40f424..df7bd64e 100644 --- a/docs/spec/concurrency_and_lifetimes.md +++ b/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 diff --git a/docs/spec/core/backend.md b/docs/spec/core/backend.md index be4946c2..e919d797 100644 --- a/docs/spec/core/backend.md +++ b/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 diff --git a/include/morph/core/backend.hpp b/include/morph/core/backend.hpp index 2cafa8c4..9185208d 100644 --- a/include/morph/core/backend.hpp +++ b/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. diff --git a/include/morph/core/bridge.hpp b/include/morph/core/bridge.hpp index fe0e340d..0182aead 100644 --- a/include/morph/core/bridge.hpp +++ b/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`. } diff --git a/scripts/branch_partial_allowlist.json b/scripts/branch_partial_allowlist.json index 11cad65d..3a4dee42 100644 --- a/scripts/branch_partial_allowlist.json +++ b/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." },