bridge: record what actually closes the *Async reply sites' UAF window (#489) - #492
Merged
Conversation
#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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv
… door `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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv
…f 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Finishes the audit issue #489 asked for, for the two items PR #491 left.
Site 5 does not have the shape
#489 listed
switchBackend()/whenBound()as unaudited ("treat the list as at least these, not exactly these"). Audited: neither is a liveness-gated callback. Both are ordinary synchronous member functions called by the bridge's owner, neither takes aCallbackToken, andwhenBound()'s queued waiters capture only theCompletionStatethey resolve, neverthis. Recorded in the spec so this is not re-derived.The three
*Asyncreply sites are safe by a contract that was never written downattachHandlerAsync,ensureBoundAsyncandassignHandlerPrimaryeach testweakLiveness.active()and then take_attachMtxand callloadBackend()— morph#486's two-step shape, in the source.They do not fault, and the reason is real but was an accident of one backend rather than a contract:
QtWebSocketBackendis the only backend in the tree overriding any*Asynchook, its header requires it be used from the Qt event loop thread (qt_websocket_backend.hpp:93), and every reply-driven callback fires fromonTextMessageon that thread. Check and use therefore cannot straddle a~Bridge.Nothing said so. A future backend delivering these replies on its own transport thread would reopen the use-after-free with no test and no comment objecting. So this states it as a contract on
IBackend::registerModelAsync, referenced from the other three hooks and from each call site.detail::BridgeLifetimeis not the fix here: gating would block~Bridgebehind_attachMtx, which the synchronousattachHandlerholds across a fullattachModelround trip — unbounded on a wire backend.Two corrections made during review, worth calling out
The first version of this PR got two things wrong, both caught before merge:
registerHandlerImplamong the token-guarded sites and said none of them could useBridgeLifetime. Both are false for that site — PR Batch: forms boundaries, bridge lifetime sites 1-3, replay-ledger promotion, scheduled mutation gate #491 rewrote its callback to hold the gate across its whole touch ofthis, with no token check anywhere. It is three sites, not four, and the exception is now stated.QtWebSocketBackend"fires all four callbacks fromonTextMessage". True of the reply path only: a disconnected or no-op dispatch invokes the callback inline on the caller's thread (qt_websocket_backend.cpp:238,:275,:366,:370), andcancelPending(:452) fires the rest from~Bridgeitself. The contract's conclusion survives all three — an inline callback runs inside the caller's own frame, whichdetail::parkIfInFrameexists to handle, andcancelPendingruns from~Bridgerather than concurrently with it — but the stated mechanism was wrong, and this note is load-bearing precisely because backends reason from it.Scope
Comments and documentation only — no behavioural change. Verified: edited headers pass
clang++ -std=c++23 -fsyntax-onlyand-Wdocumentation -Werror;scripts/check_spec_citations.shclean;scripts/branch_partial_allowlist.jsonre-pinned where doc lines moved, with every entry confirmed to land on its ownsourcetext.Leaves #489 open for site 4 (
installReconnectHandler), which keeps its documented no-safe-fix status. Site-related defect found while doing this and filed rather than folded in: #505.🤖 Generated with Claude Code
https://claude.ai/code/session_01SxkvtHvan2fWKDDaBpqkgv