fix: bound txn_counter at the trust boundary (#108) - #151
Merged
Conversation
…cked PRs
SECURITY-SWEEP-6. `open_existing` adopted `txn_counter` verbatim from the
superblock and `deserialize` read it with no bound, while commit.rs did
txn_counter.checked_add(1).expect("...(2^64 commits) — unreachable")
The comment justifying that `expect` over a typed error argued overflow is
"structurally unreachable", which assumed the counter could only ever be
produced by this binary's own increments. It cannot: it is read from a file
the threat model says the attacker controls. A superblock forged with
txn_counter = u64::MAX, valid magic, an in-range superblock_count and a
recomputed XXH3 checksum opens fine, and the first commit after it panics —
bypassing the documented poison-and-reopen contract, and surfacing in the
PyO3 binding as a PanicException rather than a mapped error class. The new
test reproduces exactly that: with the guard removed it panics at commit.rs's
`expect`, not at the assertion.
The bound goes in `Superblock::validate` — the shared torn-slot rule — rather
than beside the page_size / freemap_depth gates in open_existing, because the
counter is PER-SLOT. Selection is "highest counter wins", so a forged slot
would otherwise win outright; returning a defect makes it lose to a healthy
sibling exactly as a bad checksum does, which is what shadow paging is for.
Only when every slot is bad does this surface, and then `diagnose` names the
offending field instead of reporting a bare "no valid superblock".
MAX_TXN_COUNTER reserves 2^32 off the top. That makes the `expect` rest on a
guarded invariant rather than an argument about counting: a file that opened
at all has 4.3 billion increments of headroom, and a reopen re-validates. The
rationale comments at both bump sites are corrected to say so — they are the
sentences a future maintainer would otherwise trust.
Also fixes CI, which was not running on any of this. The workflow filtered
`pull_request: branches: [main]`, and that filter matches the PR's BASE — so
a stacked PR, based on its predecessor to stay individually reviewable, got
no checks at all. The failure was silent: GitHub shows no checks, which reads
as "nothing to run" rather than "never triggered", and the work only met CI
after being retargeted to main at merge time — after review had happened
against an unverified diff.
Note that SUPERBLOCK-RECOVERY-4, the other finding on #108, already landed on
main as PR #127 (sub-page files are refused rather than created over).
Xof
changed the base branch from
docs/publish-review-adr-and-retire-issues
to
main
August 5, 2026 03:41
Xof
added a commit
that referenced
this pull request
Aug 5, 2026
fix: bound txn_counter on the write side too (review follow-up to #151)
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
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.
Closes #108. Stacked on #150.
SECURITY-SWEEP-6
open_existingadoptedtxn_counterverbatim from the superblock anddeserializeread it with no bound, whilecommit.rsdid:The comment justifying that
expectover a typed error argued overflow is "structurally unreachable". That assumed the counter could only ever be produced by this binary's own increments. It cannot — it is read from a file the threat model says the attacker controls. A superblock forged withtxn_counter = u64::MAX, valid magic, an in-rangesuperblock_countand a recomputed XXH3 checksum opens fine, and the first commit after it panics: bypassing the documented poison-and-reopen contract, and surfacing in the PyO3 binding as aPanicExceptionrather than a mapped error class.The regression test demonstrates precisely this — with the guard removed it panics at
commit.rs:133(theexpectitself), not at the assertion.Why the bound lives in
validate()Not beside the
page_size/freemap_depthgates inopen_existing, because the counter is per-slot. Selection is "highest counter wins", so a forged slot would win outright. Returning aSuperblockDefectinstead makes it lose to a healthy sibling exactly as a bad checksum does — which is what shadow paging is for. Only when every slot is bad does this surface, and thendiagnosenames the offending field rather than reporting a bare "no valid superblock".Both behaviours are pinned by tests: all-slots-forged →
CorruptSuperblockcarryingBadTxnCounter; one-slot-forged → opens from the sibling and still commits.MAX_TXN_COUNTERreserves 2³² off the top, so theexpectnow rests on a guarded invariant rather than an argument about counting: a file that opened has 4.3 billion increments of headroom, and a reopen re-validates. The rationale comments at both bump sites (commit.rs,keys.rs) are corrected — they are the sentences a future maintainer would otherwise trust.Also: CI was not running on any of this
The workflow filtered
pull_request: branches: [main], and that filter matches the PR's base. A stacked PR — based on its predecessor so each stays individually reviewable — got no checks at all. The failure was silent: GitHub shows "no checks", which reads as nothing to run rather than never triggered, so the work only met CI after being retargeted tomainat merge time, i.e. after review had already happened against an unverified diff. #149 got CI only because it happens to targetmaindirectly.On the other #108 finding
SUPERBLOCK-RECOVERY-4 (sub-page files silently overwritten) already landed on
mainas #127 — verified against the current tree, no further work needed.Verification
714 tests pass, clippy and fmt clean. Both new tests verified non-vacuous by removing the guard.