fix: bound txn_counter on the write side too (review follow-up to #151) - #153
Merged
Conversation
Adversarial review of #151 found that bounding what we ACCEPT without bounding what we WRITE is worse than not bounding at all, and reproduced it. MAX_TXN_COUNTER itself is valid — the check is `>`, not `>=` — so a file forged at exactly the boundary opens cleanly, with `diagnose` silent. Neither increment site checked the value it was about to persist, so the commits that followed were acknowledged and fsynced at MAX+1, MAX+2: superblocks this same binary then refuses to read. Two outcomes, both bad: * one commit past the bound — the next open silently falls back to an older valid slot, discarding an acknowledged commit; * every slot past the bound — the database is permanently unopenable. Both are strictly worse than the forged-u64::MAX panic #151 set out to fix, which at least failed loudly on the first commit rather than a session later. The engine must never write a superblock it would refuse to read. `superblock::next_txn_counter` is now the single place that decides the next counter, and both `run_commit` and `rewrite_crypto_header_inner` consult it. `run_commit` calls it at the TOP, before any I/O, so a refusal costs nothing and leaves the last durable state byte-identical — which is what the new end-to-end test actually asserts, by reopening and reading the pre-forge value back. Key rotation draws from the same headroom pool, so it is bounded by the same helper. This also removes the `expect` entirely rather than merely re-justifying it, which is the outcome I119 wanted and could not have at the time: there is now a typed fatal error (`TxnCounterExhausted`) for a condition that is unreachable legitimately and reachable adversarially on the first commit. Two more from the same review: * The round-trip proptest still sampled `txn_counter in 0u64..u64::MAX` while deserialize now rejects the top 2^32 — a test that fails roughly once in 2^32 draws is a landmine, not a signal. Constrained to the validated range, exactly as `superblock_count` already was, and for the reason its comment already gives. * `validate_classifies_each_defect` gained a BadTxnCounter case and, more importantly, pins BOTH sides of the boundary. MAX being accepted is what makes the write-side guard reachable at all, so it is the assertion that keeps these two halves honest about each other. The review also found `next_handle` carrying this defect verbatim — adopted unvalidated at recovery.rs:502, then `+= 1` unchecked at staging.rs:325, with no bound anywhere. Filed as #152 rather than widened into this change.
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.
Stacked on #151. Together they close #108.
An adversarial review of #151 found that bounding what we accept without bounding what we write is worse than not bounding at all, and reproduced it end to end.
The hole #151 left
MAX_TXN_COUNTERitself is valid — the check is>, not>=— so a file forged at exactly the boundary opens cleanly, withdiagnosesilent. Neither increment site checked the value it was about to persist, so the commits that follow are acknowledged and fsynced at MAX+1, MAX+2: superblocks this same binary then refuses to read.Both are strictly worse than the forged-
u64::MAXpanic #151 set out to fix, which at least failed loudly on the first commit rather than a session later.The fix
superblock::next_txn_counteris now the single place that decides the next counter; bothrun_commitandrewrite_crypto_header_innerconsult it.run_commitcalls it at the top, before any I/O, so a refusal costs nothing and leaves the last durable state byte-identical — which is what the new end-to-end test asserts, by reopening and reading the pre-forge value back.This also removes the
expectentirely rather than merely re-justifying it — the outcome I119 wanted and could not have at the time. There is now a typed fatalTxnCounterExhaustedfor a condition that is unreachable legitimately and reachable adversarially on the first commit.Two more from the same review
txn_counter in 0u64..u64::MAXwhiledeserializenow rejects the top 2³². A test that fails roughly once in 2³² draws is noise, not signal. Constrained to the validated range — exactly assuperblock_countalready was, for the reason its own comment gives.validate_classifies_each_defectgained aBadTxnCountercase and now pins both sides of the boundary. MAX being accepted is what makes the write-side guard reachable at all, so that assertion is what keeps these two halves honest about each other.Filed separately
The review found
next_handlecarrying this defect verbatim — adopted unvalidated atrecovery.rs:502, then+= 1unchecked atstaging.rs:325, with no bound anywhere. On a plaintext DB a forgedu64::MAXpanics in debug or wraps to the reserved0sentinel in release. Filed as #152 rather than widened into this change.Verification
716 tests pass, clippy and fmt clean. The new end-to-end test verified non-vacuous by disabling the guard.