From 9bf1c65d612977aa990cbab7a42e062cd836020a Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Wed, 9 Sep 2026 14:12:28 +0200 Subject: [PATCH 1/5] bridge: record what actually closes the *Async reply sites' UAF window (#489) Issue #489 named five liveness-check-then-touch-`this` windows of morph#486's shape. PR #491 fixed sites 1-3 and recorded why site 4 (`installReconnectHandler`) has no safe mechanical fix. This finishes the audit for the two things left over. Site 5 -- `switchBackend()` and `whenBound()`, which #489 explicitly did not audit ("treat the list as at least these, not exactly these") -- does not have the shape. Both are ordinary synchronous member functions called by the bridge's owner rather than liveness-gated callbacks: neither takes a `CallbackToken`, and `whenBound()`'s queued waiters capture only the `CompletionState` they resolve, never `this`. Recorded in the spec so the next audit does not redo it. The three `IBackend` `*Async` reply callbacks (`attachHandlerAsync`, `ensureBoundAsync`, `assignHandlerPrimary`) do still have it in the source: each tests `weakLiveness.active()` and then takes `_attachMtx` and calls `loadBackend()`. They are safe today, but for a reason that lived nowhere in the tree -- `QtWebSocketBackend`, the only backend overriding any of these hooks, 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 `~Bridge`. That made the safety an accident of one backend rather than a contract, and a future backend replying on its own transport thread would silently reopen morph#486's use-after-free. So state it as a contract instead: `IBackend::registerModelAsync`'s doc comment now carries the threading requirement for all four `*Async` hooks -- the callback's thread must not be able to run `~Bridge` concurrently -- with the other three pointing at it, and each of the three `Bridge` call sites naming the contract it relies on. `detail::BridgeLifetime` is not the answer here for the same reason it is not for site 4: gating would make `~Bridge` block behind a backend's registration path. Comments and documentation only; no behavioural change. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv --- docs/spec/concurrency_and_lifetimes.md | 36 +++++++++++++++++++++----- include/morph/core/backend.hpp | 33 ++++++++++++++++++++--- include/morph/core/bridge.hpp | 24 +++++++++++++++++ 3 files changed, 84 insertions(+), 9 deletions(-) diff --git a/docs/spec/concurrency_and_lifetimes.md b/docs/spec/concurrency_and_lifetimes.md index 1b40f424..8699e86b 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,29 @@ 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`, one per `IBackend` async hook. Each keeps its + `liveness()` 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 a + backend's registration path, the same objection that rules it 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/include/morph/core/backend.hpp b/include/morph/core/backend.hpp index 2cafa8c4..d97d15a8 100644 --- a/include/morph/core/backend.hpp +++ b/include/morph/core/backend.hpp @@ -130,6 +130,28 @@ 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.** + /// Every `Bridge` continuation behind these hooks — `registerHandlerImpl`, + /// `ensureBoundAsync`, `attachHandlerAsync` and `assignHandlerPrimary` + /// in `core/bridge.hpp` — tests `CallbackToken::active()` and then + /// dereferences `this`. Those are two steps, so a `~Bridge` that + /// completes between them is morph#486's use-after-free. Unlike + /// `~BridgeHandler`, these sites cannot close the window with + /// `detail::BridgeLifetime`: that gate makes `~Bridge` *block* for the + /// gated span, and these spans reach `loadBackend()` and a backend's + /// own registration path. What closes it 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 fires all four callbacks from + /// `onTextMessage` on that same thread, so check and use cannot + /// straddle a 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 +191,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 +290,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 +367,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..11133d75 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. What closes the window here is not a gate but + // `IBackend::registerModelAsync`'s threading contract: a backend + // overriding the `*Async` hooks must deliver their callbacks on a + // thread that cannot run `~Bridge` concurrently, which + // `QtWebSocketBackend` (the only such backend) satisfies by being + // Qt-event-loop-thread-only. Gating instead would make `~Bridge` + // block behind `loadBackend()`'s backend. 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. What closes the window here is not a gate but + // `IBackend::registerModelAsync`'s threading contract: a backend + // overriding the `*Async` hooks must deliver their callbacks on a + // thread that cannot run `~Bridge` concurrently, which + // `QtWebSocketBackend` (the only such backend) satisfies by being + // Qt-event-loop-thread-only. Gating instead would make `~Bridge` + // block behind `loadBackend()`'s backend. 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. What closes the window here is not a gate but + // `IBackend::registerModelAsync`'s threading contract: a backend + // overriding the `*Async` hooks must deliver their callbacks on a + // thread that cannot run `~Bridge` concurrently, which + // `QtWebSocketBackend` (the only such backend) satisfies by being + // Qt-event-loop-thread-only. Gating instead would make `~Bridge` + // block behind `loadBackend()`'s backend. See morph#489. if (!weakLiveness.active()) { return; // The Bridge is gone; do not touch `this`. } From 016d882256b70c862022f827ddd6a9131bc73f46 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Wed, 9 Sep 2026 15:54:37 +0200 Subject: [PATCH 2/5] Fix the two gates the previous commit broke, and the stale claim next door MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Header ↔ spec sync` requires an `include/morph/core/**` change to be matched under `docs/spec/core/**`; the previous commit updated only `concurrency_and_lifetimes.md`. The `*Async` threading contract belongs in `docs/spec/core/backend.md` anyway, next to the hooks it constrains, so it is now stated there in full rather than only in the header's doc comment. Writing it there surfaced a stale claim in the paragraph it replaces: backend.md said `assignPrimary` "has **no** async counterpart and is not covered here", which stopped being true when `assignPrimaryAsync` landed for issue #67 -- and the new contract covers "all four `*Async` hooks", so leaving the page asserting there are three would have made it contradict itself. Corrected with a short section for the hook that was missing. `Linux / clang-coverage` failed on `scripts/branch_partial_allowlist.json`: inserting comment lines moved three allowlisted branches, and the checker is deliberately strict about that (morph#349/#355) -- one entry had merely moved (bridge.hpp:1421 -> 1445) and two had become ambiguous, because their `source` text occurs twice in the file and the stale `line` no longer picked either (backend.hpp:741 -> 768, bridge.hpp:1539 -> 1563). Re-pinned to the first occurrence in each case, which is the one each entry's `reason` describes. Verified all 25 entries resolve: every `line` hint now lands on its own `source` text. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv --- docs/spec/core/backend.md | 44 +++++++++++++++++++++++++-- scripts/branch_partial_allowlist.json | 6 ++-- 2 files changed, 44 insertions(+), 6 deletions(-) diff --git a/docs/spec/core/backend.md b/docs/spec/core/backend.md index be4946c2..d57b7a20 100644 --- a/docs/spec/core/backend.md +++ b/docs/spec/core/backend.md @@ -253,9 +253,47 @@ 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. Each of the four continuations behind these +hooks — `registerHandlerImpl`, `ensureBoundAsync`, `attachHandlerAsync` and +`assignHandlerPrimary` in `core/bridge.hpp` — tests `CallbackToken::active()` +and then dereferences `this` (it 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. + +Unlike `~BridgeHandler`, these sites cannot close that window with +`detail::BridgeLifetime`: the gate makes `~Bridge` *block* for the gated span, +and these spans reach a backend's own registration path. What closes it 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 it fires all +four callbacks from `onTextMessage` on that same thread, so the check and the +use cannot straddle a 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/scripts/branch_partial_allowlist.json b/scripts/branch_partial_allowlist.json index 11cad65d..97423192 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": 768, "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." }, From 2506924ba41291017ca3f01503ccc96191d2efef Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Wed, 9 Sep 2026 16:31:33 +0200 Subject: [PATCH 3/5] Correct the threading-contract note: registerHandlerImpl is not one of them The note added two commits ago claimed all four `*Async` continuations "test `CallbackToken::active()` and then dereference `this`" and that none of them "can close the window with `detail::BridgeLifetime`". Both universals are false for `registerHandlerImpl`: PR #491 rewrote its callback to hold `detail::BridgeLifetime` across its whole touch of `this`, with no token check anywhere in it (bridge.hpp:1753-1780). So the note asserted the opposite of what the code does, at the first site it named, while the code's own comment there spells out the correct version. It is three sites, not four -- `attachHandlerAsync`, `ensureBoundAsync` and `assignHandlerPrimary` -- and the exception is now stated rather than absorbed. The reason those three cannot take the gate was also wrong. It said gating would block `~Bridge` "behind `loadBackend()`'s backend", but the span contains no backend call: only `_attachMtx`, a `shared_ptr` copy and field stores. The real objection is `_attachMtx` itself, which the synchronous `attachHandler` holds across a full `attachModel` round trip (bridge.hpp:502), unbounded on a wire backend. That is what would make a gated `~Bridge` wait on the network. Also collapses the three verbatim copies of the block in bridge.hpp to a four-line pointer each, leaving the canonical statement in `registerModelAsync`'s doc comment where the contract belongs; and re-pins the branch-coverage allowlist entry the added doc lines moved (backend.hpp 768 -> 773). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv --- docs/spec/concurrency_and_lifetimes.md | 36 ++++++++++++---------- docs/spec/core/backend.md | 34 ++++++++++++--------- include/morph/core/backend.hpp | 29 ++++++++++-------- include/morph/core/bridge.hpp | 42 +++++++++++++------------- scripts/branch_partial_allowlist.json | 2 +- warns.txt | 0 6 files changed, 79 insertions(+), 64 deletions(-) create mode 100644 warns.txt diff --git a/docs/spec/concurrency_and_lifetimes.md b/docs/spec/concurrency_and_lifetimes.md index 8699e86b..df7bd64e 100644 --- a/docs/spec/concurrency_and_lifetimes.md +++ b/docs/spec/concurrency_and_lifetimes.md @@ -292,22 +292,26 @@ that bounded wait into an unbounded one. Four dispositions, by site: 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`, one per `IBackend` async hook. Each keeps its - `liveness()` 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 a - backend's registration path, the same objection that rules it 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. + 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 diff --git a/docs/spec/core/backend.md b/docs/spec/core/backend.md index d57b7a20..3c7da43a 100644 --- a/docs/spec/core/backend.md +++ b/docs/spec/core/backend.md @@ -273,22 +273,28 @@ failure only. concurrently.** This is a contract on the backend, not an implementation detail of `Bridge`. -The reason is on `Bridge`'s side. Each of the four continuations behind these -hooks — `registerHandlerImpl`, `ensureBoundAsync`, `attachHandlerAsync` and -`assignHandlerPrimary` in `core/bridge.hpp` — tests `CallbackToken::active()` -and then dereferences `this` (it 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 +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. -Unlike `~BridgeHandler`, these sites cannot close that window with -`detail::BridgeLifetime`: the gate makes `~Bridge` *block* for the gated span, -and these spans reach a backend's own registration path. What closes it 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 it fires all -four callbacks from `onTextMessage` on that same thread, so the check and the -use cannot straddle a destructor. +`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 it fires all four callbacks from +`onTextMessage` on that same thread, so the check and the use cannot straddle a +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 diff --git a/include/morph/core/backend.hpp b/include/morph/core/backend.hpp index d97d15a8..452f3f19 100644 --- a/include/morph/core/backend.hpp +++ b/include/morph/core/backend.hpp @@ -132,18 +132,23 @@ struct IBackend { /// /// @note **Threading contract, shared by all four `*Async` hooks: the /// callback's thread must not be able to run `~Bridge` concurrently.** - /// Every `Bridge` continuation behind these hooks — `registerHandlerImpl`, - /// `ensureBoundAsync`, `attachHandlerAsync` and `assignHandlerPrimary` - /// in `core/bridge.hpp` — tests `CallbackToken::active()` and then - /// dereferences `this`. Those are two steps, so a `~Bridge` that - /// completes between them is morph#486's use-after-free. Unlike - /// `~BridgeHandler`, these sites cannot close the window with - /// `detail::BridgeLifetime`: that gate makes `~Bridge` *block* for the - /// gated span, and these spans reach `loadBackend()` and a backend's - /// own registration path. What closes it 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 + /// 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 fires all four callbacks from /// `onTextMessage` on that same thread, so check and use cannot /// straddle a destructor. **A backend that delivers these callbacks on diff --git a/include/morph/core/bridge.hpp b/include/morph/core/bridge.hpp index 11133d75..0182aead 100644 --- a/include/morph/core/bridge.hpp +++ b/include/morph/core/bridge.hpp @@ -591,13 +591,13 @@ class Bridge { return; // Completed inline: the dispatching frame will finish this. } // This check and the `this` touch below it are two steps -- the - // morph#486 shape. What closes the window here is not a gate but - // `IBackend::registerModelAsync`'s threading contract: a backend - // overriding the `*Async` hooks must deliver their callbacks on a - // thread that cannot run `~Bridge` concurrently, which - // `QtWebSocketBackend` (the only such backend) satisfies by being - // Qt-event-loop-thread-only. Gating instead would make `~Bridge` - // block behind `loadBackend()`'s backend. See morph#489. + // 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. } @@ -748,13 +748,13 @@ class Bridge { return; // Completed inline: the dispatching frame will finish this. } // This check and the `this` touch below it are two steps -- the - // morph#486 shape. What closes the window here is not a gate but - // `IBackend::registerModelAsync`'s threading contract: a backend - // overriding the `*Async` hooks must deliver their callbacks on a - // thread that cannot run `~Bridge` concurrently, which - // `QtWebSocketBackend` (the only such backend) satisfies by being - // Qt-event-loop-thread-only. Gating instead would make `~Bridge` - // block behind `loadBackend()`'s backend. See morph#489. + // 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. } @@ -892,13 +892,13 @@ class Bridge { ::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. What closes the window here is not a gate but - // `IBackend::registerModelAsync`'s threading contract: a backend - // overriding the `*Async` hooks must deliver their callbacks on a - // thread that cannot run `~Bridge` concurrently, which - // `QtWebSocketBackend` (the only such backend) satisfies by being - // Qt-event-loop-thread-only. Gating instead would make `~Bridge` - // block behind `loadBackend()`'s backend. See morph#489. + // 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 97423192..fd346eee 100644 --- a/scripts/branch_partial_allowlist.json +++ b/scripts/branch_partial_allowlist.json @@ -92,7 +92,7 @@ }, { "file": "include/morph/core/backend.hpp", - "line": 768, + "line": 773, "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." }, diff --git a/warns.txt b/warns.txt new file mode 100644 index 00000000..e69de29b From 7fa039896a40906fbe35a63f1a691482c0cdf15c Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Wed, 9 Sep 2026 18:27:02 +0200 Subject: [PATCH 4/5] Say what QtWebSocketBackend actually does, and drop a stray file The threading-contract note claimed `QtWebSocketBackend` "fires all four callbacks from `onTextMessage` on that same thread". That is true of the reply-driven path and not of two others, both of which I had not checked: - A dispatch on a disconnected socket invokes `onError("disconnected")` inline before returning `true` (qt_websocket_backend.cpp:238, :275, :370), and `assignPrimaryAsync` invokes `onRegistered(mid)` inline for its documented no-op case (:366). No reply, no `onTextMessage`. - `cancelPending` (:452) fires `onError` for every pending registration, queued registration and pending assign, and it is reached from `~Bridge` (bridge.hpp:423) and `Bridge::switchBackend` (:1343). The contract's *conclusion* survives all three -- an inline callback runs inside the caller's own frame, which `detail::parkIfInFrame` exists to handle, and `cancelPending` runs from `~Bridge` itself rather than concurrently with it -- but the sentence stating the mechanism was wrong, and this note is load-bearing precisely because backends are expected to reason from it. Both the header and docs/spec/core/backend.md now say which paths do what. Also removes `warns.txt`, an empty file committed by accident. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv --- docs/spec/core/backend.md | 7 +++++-- include/morph/core/backend.hpp | 10 +++++++--- warns.txt | 0 3 files changed, 12 insertions(+), 5 deletions(-) delete mode 100644 warns.txt diff --git a/docs/spec/core/backend.md b/docs/spec/core/backend.md index 3c7da43a..e919d797 100644 --- a/docs/spec/core/backend.md +++ b/docs/spec/core/backend.md @@ -292,9 +292,12 @@ gated span, and each span acquires `_attachMtx` — which the synchronous 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 it fires all four callbacks from +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. +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 diff --git a/include/morph/core/backend.hpp b/include/morph/core/backend.hpp index 452f3f19..9185208d 100644 --- a/include/morph/core/backend.hpp +++ b/include/morph/core/backend.hpp @@ -149,9 +149,13 @@ struct IBackend { /// `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 fires all four callbacks from - /// `onTextMessage` on that same thread, so check and use cannot - /// straddle a destructor. **A backend that delivers these callbacks on + /// (`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 diff --git a/warns.txt b/warns.txt deleted file mode 100644 index e69de29b..00000000 From 58e0a8211237663efb05cde9c4e50f542df5435c Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Wed, 9 Sep 2026 18:27:44 +0200 Subject: [PATCH 5/5] Re-pin the branch-coverage allowlist entry the previous commit moved backend.hpp's added doc lines shifted the `if (iter != _models.end())` entry from 773 to 777. The checker is strict about a stale `line` for good reason (morph#349/#355): the same source text occurs twice in the file, so a stale hint resolves ambiguously rather than failing loudly. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv --- scripts/branch_partial_allowlist.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/branch_partial_allowlist.json b/scripts/branch_partial_allowlist.json index fd346eee..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": 773, + "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." },