pastebin: refuse a fractional burnAfterReads instead of silently flooring it, and guard FormsBridge's callbacks - #315
Merged
Conversation
…ilently `Reads` is `Quantity<Unit::count, 1>` — `Quantity` requires `DeclaredDecimals >= 1`, so a tenth is an ordinary value of the type and `2.5` travels the whole path intact: it survives the wire codec (a `Rational` serialises as its own num/den pair) and reaches `CreatePaste::validate()`, which checked content, syntax and the budget's sign but never its integrality. `paste_model.cpp`'s `countOf` then applied `math::floor` under a comment asserting that "`Reads` only ever carries whole numbers here" — a premise nothing enforced. Measured on the pre-fix tree: a create with `burnAfterReads = 2.5` succeeded, stored 2/1, and `GetPaste` reported the budget back as 2. The client was told a budget it never asked for had been accepted. This is the silent-data-loss class `kMaxSyntaxBytes` already exists to prevent, arriving through an unguarded door, so it gets the same answer: refuse the input rather than rewrite it. A rung that ever wants "2.5 means 2" must say so in `Reads`' declared precision, which is the only honest place for it — not in a conversion helper nobody reads. Three comments documented the constraint as already enforced and were all wrong; they now describe what the code actually does. `units.hpp` additionally corrects "the DTOs that use `Reads`" to the one DTO that actually accepts one from outside: every other `Reads` in the rung is minted by `readsOf` from a whole `int64_t`. No `formRules` entry accompanies this. `morph::forms`' comparison rules (`Greater`/`GreaterOrEqual`/`Less`/`LessOrEqual`) take two member pointers of the same action, never a field and a literal, so "burnAfterReads >= 1" has no spelling; `equals` takes a literal but expresses only equality, and neither `FieldMeta` nor the schema writer carries a `multipleOf`-style integrality annotation. Any client-side gate here would have to be a hand-written QML conditional, which `TESTING.md` presenter rule 6 forbids. Reported as a framework gap rather than worked around. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`FormsBridge::submitIfValid` handed the forms controller two lambdas capturing a bare `this`, and both emit `replyReceived`. They are attached to a `Completion`, which always resolves through the executor — never inline, not even under `LocalBackend` (docs/spec/core/completion.md) — so a reply in flight outlives the call that issued it. Both shells own their bridges by `unique_ptr` in `main()`, so the destroy-then-deliver window is real, and nothing closed it. The rung's two neighbours are already covered by mechanisms that do not reach this class: `PastePresenter` inherits `Presenter::track()`, whose `QPointer` re-check was added for a measured AddressSanitizer `stack-use-after-scope` (morph#137), and `PasteBridge` relays through Qt signal/slot connections, which Qt severs on destruction. `FormsBridge` goes through neither. So it takes the framework's general answer, which no rung had used yet: `morph::async::CallbackScope` as a last-declared member, with both arms wrapped in `_callbacks.guard(...)` (docs/spec/core/callback_scope.md — "Read this before attaching any callback that captures `this`"). `guard()` rather than `Completion`'s `then(scope, fn)` overload because the completion is created and attached one frame further in, inside `PasteFormsController`; what this function hands over is a pair of plain callables, and the gate belongs in the class that owns the captured `this`. Since this is the ladder's first use of the type, the header carries the full argument for the next rung to copy — including why the member must stay declared last. `tests/test_paste_qml_bridges.cpp` drives the window deterministically (`LocalSingleThread`, so the abandoned submit is strictly ordered before a live one) and asserts the observable half: the abandoned dispatch really did complete after its bridge was gone — its paste exists — which is what makes this a genuine use-after-free window rather than a call that never happened. Stated plainly, because it is easy to overclaim: this is a by-construction hazard closed pre-emptively. No use-after-free was reproduced here. The suppression itself has no observable signature outside a sanitized build, and the test says so rather than implying its pass proves the gate works. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nger in Three groups of claims in this rung's prose had gone stale, all in the same direction: they described the world as it was when the rung was written rather than as it is. 1. The fault-injection proxy. "Until the fault-injection proxy exists (rung 4)" — it shipped at rung 0-1 as `examples/common/testkit/fault_proxy.hpp`, `LADDER.md`'s queued-work list records it as *Shipped*, and `examples/kanban`'s offline suite already drives it. The same sentence also described this rung's test as a "double-execute with the same op id"; `CreatePaste` has no op-id field, and the test asserts the *opposite* of the bullet's requirement — two identical creates mint two pastes, deliberately, because there is no idempotency key to deduplicate on. Both the "Required tests" bullet and the deferred-work list now say what the rung does and why, and name the missing key rather than missing tooling as the blocker. 2. The case count. "33 cases" against a file that has 40. The count now carries the command that produces it, so the next drift is one grep away from being caught. 3. Dangling finding citations. `017`, `018`, `021`, `023` and `026` appeared in the README, both `main.cpp`s, the forms controller and the model suite. `docs/findings/` holds only `r5-001`-`r5-004` under FINDINGS.md's namespaced `<ns>-NNN-<kebab-slug>` scheme, so every one of those resolved to nothing. Each is replaced by a description of the gap plus a pointer to the code or spec that closes it — no finding file was invented to make an old number resolve, and the README records why the numbers are gone. Two of those citations propped up claims that were themselves wrong: both shells asserted that a `Remote` `AppContext` is unusable the line after its constructor returns and that an early registration "fails permanently with no retry". `Remote` mode sets `asyncRegistrationEnabled` and `registerModelAsync()` queues and retries such a registration (docs/spec/core/backend.md), so building handlers inside `onReady()` is now a readability choice, not a correctness requirement — which is exactly what `app_context.hpp`'s own readiness contract says. The WASM comment keeps the constraint that *is* still hard: `setConnectHandler`, never `waitForConnected()`. Also records `CallbackScope` under "morph subsystems exercised": this rung is its first consumer on the ladder. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Yaraslaut
added a commit
that referenced
this pull request
Aug 27, 2026
…capture in bookmarks Migrates all four remaining hand-rolled liveness-token sites (issue #304 section B2) to morph::async::CallbackScope, which replaces the per-class "token must stay last-declared" convention this framework primitive existed to retire. Correctness fix, not just cleanup: bookmarks::gui::FormsBridge::submitIfValid captured a bare `this` in both of BookmarkFormsController::submitIfValid's reply callbacks, with no guard at all. That path goes through neither Presenter::track()'s QPointer re-check nor Qt's signal/slot auto-disconnect -- a FormsBridge destroyed while a Completion is still in flight (an ordinary GUI case, e.g. process teardown with a submit outstanding) would run `emit` against freed storage. Confirmed by adding "A FormsBridge destroyed with a submit in flight has its reply suppressed, not delivered", mirroring pastebin's identical regression test (pastebin's own FormsBridge was fixed the same way in #315). Fixed by giving FormsBridge a last-declared CallbackScope and wrapping both callbacks in _callbacks.guard(...), exactly as pastebin's FormsBridge already does. polls::gui::PollBridge and kanban::gui::BoardBridge each hand-rolled a `std::shared_ptr<const void> _liveness` member with a `weak_ptr<const void>{_liveness}` + `.expired()` re-derivation at every one of their Completion/EventPoller/QNetworkReply callback sites (polls: 6 sites across 5 methods; kanban: 9 sites across 4 methods, including two probe-thread NetworkMonitor callbacks and a nested post()). Both were already correctly guarded -- this is pure adoption, not a bug fix -- and now use _callbacks.token()/.guard()/.then(scope, fn)/.onError(scope, fn) instead, removing every hand-written expired() check. ledger::gui::ReportJobPoller (not a QObject) held the same hand-rolled token; migrated to CallbackScope the same way, and a new regression test ("A ReportJobPoller destroyed with a dispatch in flight has its reply suppressed, not delivered") pins the destroy-mid-flight behavior post- migration, since it had no such coverage before. Net effect: ~50 lines of re-derived weak_ptr/expired() boilerplate removed across the four rungs, one confirmed use-after-free window closed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Yaraslaut
added a commit
that referenced
this pull request
Aug 27, 2026
…capture in bookmarks (#330) Migrates all four remaining hand-rolled liveness-token sites (issue #304 section B2) to morph::async::CallbackScope, which replaces the per-class "token must stay last-declared" convention this framework primitive existed to retire. Correctness fix, not just cleanup: bookmarks::gui::FormsBridge::submitIfValid captured a bare `this` in both of BookmarkFormsController::submitIfValid's reply callbacks, with no guard at all. That path goes through neither Presenter::track()'s QPointer re-check nor Qt's signal/slot auto-disconnect -- a FormsBridge destroyed while a Completion is still in flight (an ordinary GUI case, e.g. process teardown with a submit outstanding) would run `emit` against freed storage. Confirmed by adding "A FormsBridge destroyed with a submit in flight has its reply suppressed, not delivered", mirroring pastebin's identical regression test (pastebin's own FormsBridge was fixed the same way in #315). Fixed by giving FormsBridge a last-declared CallbackScope and wrapping both callbacks in _callbacks.guard(...), exactly as pastebin's FormsBridge already does. polls::gui::PollBridge and kanban::gui::BoardBridge each hand-rolled a `std::shared_ptr<const void> _liveness` member with a `weak_ptr<const void>{_liveness}` + `.expired()` re-derivation at every one of their Completion/EventPoller/QNetworkReply callback sites (polls: 6 sites across 5 methods; kanban: 9 sites across 4 methods, including two probe-thread NetworkMonitor callbacks and a nested post()). Both were already correctly guarded -- this is pure adoption, not a bug fix -- and now use _callbacks.token()/.guard()/.then(scope, fn)/.onError(scope, fn) instead, removing every hand-written expired() check. ledger::gui::ReportJobPoller (not a QObject) held the same hand-rolled token; migrated to CallbackScope the same way, and a new regression test ("A ReportJobPoller destroyed with a dispatch in flight has its reply suppressed, not delivered") pins the destroy-mid-flight behavior post- migration, since it had no such coverage before. Net effect: ~50 lines of re-derived weak_ptr/expired() boilerplate removed across the four rungs, one confirmed use-after-free window closed. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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.
The defect
ReadsisQuantity<Unit::count, 1>— one declared decimal place, so2.5is exactly representable and survives the wire codec.CreatePaste::validate()checked empty/zero/negative but never integrality, andcountOf()then floored under a comment asserting "Readsonly ever carries whole numbers here" — an unenforced premise.Pinned before fixing:
and the data loss itself, with a throwaway case that becomes unreachable after the fix:
Accepted, floored to 2, reported back as 2 — the same silent-data-loss class the rung fought hard over for
syntax, arriving through an unguarded door. Three separate places documented the constraint as enforced; none enforced it. All three now describe what the code does.Why no
formRulesentrymorph::formscannot express this.Greater/GreaterOrEqual/Less/LessOrEqualall take two member pointers of the same action (V A::* lhs, V A::* rhs) — there is no field-vs-literal form, so>= 1has no right-hand operand.equalstakes a literal but is equality-only.FieldMetahas nominimum/multipleOf, andx-exactMinimumonly annotates bounds glaze already stamped.UnitTraits::boundsis per-unit, so it would also constrainPasteView::readCount, which legitimately starts at 0.The remaining option is a hand-written QML conditional, which
TESTING.mdpresenter rule 6 forbids. Filed as #310 rather than bodged.The ladder's first
CallbackScopeadoptionFormsBridge::submitIfValidpassed[this, ...]lambdas as both completion arms with no guard of any kind. They resolve through the executor, so they can outlive the bridge — andFormsBridgegoes through neither of the two mechanisms that protect its siblings (Presenter::track'sQPointer, added for a real ASanstack-use-after-scope; and Qt signal/slot auto-disconnect).Now a
morph::async::CallbackScope _callbacksas the last-declared member, both arms wrapped in_callbacks.guard(...).guard()rather thanCompletion::then(scope, fn)because the completion is created a frame further in. No rung had adoptedCallbackScopebefore this, so the header carries the full argument — including why the member must stay last — as the reference for the other rungs.No use-after-free was reproduced. The ASan build never reached test execution (see #311); the hazard is closed by construction, and the commit says so rather than claiming a demonstrated fix. The new test drives the window under
LocalSingleThreadand asserts the observable half: the abandoned dispatch completed after its bridge was gone.Documentation truth pass
CreatePastehas no op-id field and the test asserts the opposite. The bullet now says the rung ships the inverse of that requirement, deliberately.33→40, with thegrepthat produces it.017,018,021,023,026) repointed to real code/spec locations. No finding file was invented; the README records why the numbers are gone.main.cpps asserted aRemoteAppContextis unusable after construction and that early registration "fails permanently with no retry".registerModelAsync()queues and retries. Corrected, keeping the constraint that is still hard.Verification
ladder_pastebin_tests: 853 assertions / 56 cases, all passing.clang-format --dry-run -Werrorclean on every changed file.Not run: the WASM client (no Emscripten toolchain, as the rung's DoD records), the Doxygen target, and any sanitizer configuration — see #311.
Part of #304 (§A4, §B2).
🤖 Generated with Claude Code