fix: bound txn_counter at the trust boundary (#108)

.github/workflows/ci.yml

@@ -3,8 +3,14 @@ name: CI
 on:
   push:
     branches: [main]
+  # Deliberately NOT filtered to `branches: [main]`. That filter matches the
+  # PR's BASE, so a stacked PR — one based on its predecessor rather than on
+  # main, which is how a chain of dependent fixes stays individually
+  # reviewable — got no CI at all. The gap was silent: the PR simply showed no
+  # checks, which reads like "nothing to run" rather than "never triggered",
+  # and the work only met CI after being retargeted to main at merge time,
+  # i.e. after review had already happened against an unverified diff.
   pull_request:
-    branches: [main]
 
 env:
   CARGO_TERM_COLOR: always

src/recovery_tests.rs

@@ -2060,3 +2060,122 @@ fn a_forged_overflow_length_cannot_run_the_delete_path_out_of_memory() {
         Err(other) => panic!("expected CorruptPage, got {other:?}"),
     }
 }
+
+// Helper for the two txn_counter tests below. Patches bytes 8..16 of one
+// superblock slot and re-stamps the XXH3 checksum, which is exactly what an
+// attacker with byte-level file access does — the checksum is non-cryptographic
+// and publicly recomputable. Going through `Superblock::serialize` would not
+// work here: `deserialize` now rejects the forged value, so the round-trip
+// cannot produce the bytes under test.
+#[cfg(test)]
+fn forge_txn_counter(path: &std::path::Path, slot: u64, value: u64) {
+    let mut f = fs::OpenOptions::new()
+        .read(true)
+        .write(true)
+        .open(path)
+        .unwrap();
+    let mut buf = [0u8; PAGE_SIZE];
+    f.seek(SeekFrom::Start(slot * PAGE_SIZE as u64)).unwrap();
+    f.read_exact(&mut buf).unwrap();
+    buf[8..16].copy_from_slice(&value.to_le_bytes());
+    page::stamp_checksum(&mut buf);
+    f.seek(SeekFrom::Start(slot * PAGE_SIZE as u64)).unwrap();
+    f.write_all(&buf).unwrap();
+    f.sync_all().unwrap();
+}
+
+#[test]
+fn a_forged_txn_counter_in_every_slot_is_refused_at_open() {
+    // SECURITY-SWEEP-6. `open_existing` adopted `txn_counter` verbatim from the
+    // superblock, and `deserialize` read it with no bound. commit.rs then did
+    // `txn_counter.checked_add(1).expect("...unreachable")` on the strength of
+    // a comment arguing overflow needs 2^64 commits — an argument that only
+    // holds if the counter can ONLY come from this binary's own increments.
+    // It cannot: it comes off disk. A forged u64::MAX panicked on the first
+    // commit after open, bypassing the poison-and-reopen contract entirely and
+    // reaching Python as a PanicException rather than a mapped error class.
+    let file = NamedTempFile::new().unwrap();
+    let path = file.path().to_owned();
+    {
+        let mut db = Chisel::open(&path, Default::default()).unwrap();
+        db.begin().unwrap();
+        db.allocate(b"payload").unwrap();
+        db.commit().unwrap();
+    }
+
+    for slot in 0..2u64 {
+        forge_txn_counter(&path, slot, u64::MAX);
+    }
+
+    // Every slot is bad, so selection has nothing to fall back to and the open
+    // fails with a per-slot diagnosis rather than reaching a commit at all.
+    match Chisel::open(&path, Default::default()) {
+        Err(ChiselError::CorruptSuperblock { defects }) => {
+            assert!(
+                defects
+                    .iter()
+                    .any(|d| matches!(d.defect, crate::superblock::SuperblockDefect::BadTxnCounter(n) if n == u64::MAX)),
+                "the diagnosis must name the offending field, got {defects:?}"
+            );
+        }
+        Ok(_) => panic!("a txn_counter of u64::MAX in every slot must not open"),
+        Err(other) => panic!("expected CorruptSuperblock, got {other:?}"),
+    }
+}
+
+#[test]
+fn a_forged_txn_counter_in_one_slot_loses_to_its_healthy_sibling() {
+    // The reason this bound lives in `validate` (the shared torn-slot rule)
+    // rather than at the open-time gate next to page_size/freemap_depth: the
+    // counter is PER-SLOT. Shadow paging's whole point is that a damaged slot
+    // loses to a good one, so a forged counter must be discarded the same way a
+    // bad checksum is — not fail the entire open. Without that, the forged slot
+    // would win selection outright, since selection is "highest counter wins".
+    let file = NamedTempFile::new().unwrap();
+    let path = file.path().to_owned();
+    let first;
+    {
+        let mut db = Chisel::open(&path, Default::default()).unwrap();
+        db.begin().unwrap();
+        first = db.allocate(b"from the first commit").unwrap();
+        db.commit().unwrap();
+        // A second commit so BOTH slots hold a real state; the newest one is
+        // the forgery target below.
+        db.begin().unwrap();
+        db.allocate(b"from the second commit").unwrap();
+        db.commit().unwrap();
+    }
+
+    // Forge whichever slot currently wins selection. u64::MAX would outrank
+    // every sibling, so without the bound this slot is guaranteed to be chosen
+    // and its adopted counter would then panic the next commit.
+    let newest = {
+        let mut f = fs::File::open(&path).unwrap();
+        let mut best = (0u64, 0u64);
+        for slot in 0..2u64 {
+            let mut buf = [0u8; PAGE_SIZE];
+            f.seek(SeekFrom::Start(slot * PAGE_SIZE as u64)).unwrap();
+            f.read_exact(&mut buf).unwrap();
+            let sb = Superblock::deserialize(&buf).expect("slot must be valid");
+            if sb.txn_counter >= best.1 {
+                best = (slot, sb.txn_counter);
+            }
+        }
+        best.0
+    };
+    forge_txn_counter(&path, newest, u64::MAX);
+
+    let mut db = Chisel::open(&path, Default::default())
+        .expect("a healthy sibling slot must still open the database");
+    // Discarding the newest slot rewinds to the previous commit — that is the
+    // correct and unavoidable consequence of "a damaged slot loses to a good
+    // one", since the good one is by definition older. What matters is that the
+    // open SUCCEEDS from the sibling instead of failing outright or adopting
+    // the forged counter.
+    assert_eq!(db.read(first).unwrap(), b"from the first commit");
+    // And the engine is fully usable: the counter it adopted came from the
+    // sibling, so the commit path has its full headroom and does not panic.
+    db.begin().unwrap();
+    db.allocate(b"and it still commits").unwrap();
+    db.commit().unwrap();
+}

src/superblock/mod.rs

@@ -55,6 +55,27 @@ pub const MIN_SUPERBLOCKS: u32 = 2;
 pub const MAX_SUPERBLOCKS: u32 = 16;
 pub const DEFAULT_SUPERBLOCK_COUNT: u32 = 2;
 
+// Headroom the commit path is guaranteed at open, reserved off the top of
+// `txn_counter`'s range. `validate` rejects any slot claiming a counter above
+// `MAX_TXN_COUNTER`, so a file that opens is always at least this far from
+// overflowing.
+//
+// This exists because `commit` and `rewrite_crypto_header` both do
+// `txn_counter.checked_add(1).expect(...)`, justified by a comment calling
+// overflow "structurally unreachable". That argument assumed the counter could
+// only ever be produced by this binary's own increments. It is in fact read
+// from a file the threat model says an attacker controls, so a forged
+// `txn_counter = u64::MAX` made the "unreachable" panic reachable from the
+// first commit after open — bypassing the poison-and-reopen contract and, in
+// the PyO3 binding, surfacing as a PanicException rather than a mapped error.
+//
+// 2^32 is chosen to be unreachable from the other direction too: exhausting it
+// requires 4.3 billion commits within a SINGLE session (a reopen re-validates),
+// at three fsyncs each. A database that legitimately climbed past
+// MAX_TXN_COUNTER would be refused, but reaching it needs ~1.8e19 commits.
+pub const TXN_COUNTER_RESERVE: u64 = 1 << 32;
+pub const MAX_TXN_COUNTER: u64 = u64::MAX - TXN_COUNTER_RESERVE;
+
 // Byte offset of the superblock_count field within the serialized
 // superblock page. Placed AFTER the named-root table (which ends at
 // NAMED_ROOTS_END = 308). Deserialization rejects any value outside
@@ -140,6 +161,13 @@ pub enum SuperblockDefect {
     BadChecksum,
     BadMagic,
     BadCount(u32), // the out-of-range superblock_count value
+    // The slot's txn_counter leaves less than TXN_COUNTER_RESERVE of headroom
+    // before u64 overflow. Treated as a torn-slot rule rather than an open-time
+    // gate precisely because the counter is PER-SLOT: a forged slot should lose
+    // to a healthy sibling under the normal "highest counter wins" selection,
+    // which is exactly what returning a defect here achieves. Only if EVERY
+    // slot is bad does this surface, and then `diagnose` names the slot.
+    BadTxnCounter(u64),
     // The file is too short to contain this slot at all. Distinct from
     // BadMagic: there are no bytes to have magic, so the slot was never
     // read. Raised by `Chisel::open` for a file that has content but is
@@ -155,6 +183,9 @@ impl fmt::Display for SuperblockDefect {
             SuperblockDefect::BadChecksum => write!(f, "bad checksum"),
             SuperblockDefect::BadMagic => write!(f, "bad magic"),
             SuperblockDefect::BadCount(n) => write!(f, "bad superblock_count {n}"),
+            SuperblockDefect::BadTxnCounter(n) => {
+                write!(f, "txn_counter {n} is too close to u64 overflow")
+            }
             SuperblockDefect::TooShort => write!(f, "file too short to contain this slot"),
         }
     }
@@ -259,6 +290,10 @@ fn validate(buf: &[u8; PAGE_SIZE]) -> Result<(), SuperblockDefect> {
     if !(MIN_SUPERBLOCKS..=MAX_SUPERBLOCKS).contains(&count) {
         return Err(SuperblockDefect::BadCount(count));
     }
+    let txn_counter = u64::from_le_bytes(buf[8..16].try_into().unwrap());
+    if txn_counter > MAX_TXN_COUNTER {
+        return Err(SuperblockDefect::BadTxnCounter(txn_counter));
+    }
     Ok(())
 }
 

src/transaction/commit.rs

@@ -110,10 +110,23 @@ pub(super) fn run_commit(ctx: &mut CommitCtx<'_>) -> Result<()> {
     //
     // I119 (ISSUES.md, 2026-06-21): checked, not `+= 1`. A wrapped counter
     // would corrupt `Superblock::select`'s "highest counter wins" (release
-    // wrap to 0) — far worse than the loud, controlled panic here. Overflow
-    // needs 2^64 commits, so it is structurally unreachable; a dedicated
-    // fatal error variant for an impossible event would be speculative
-    // public surface, so the `expect` on the invariant is proportionate.
+    // wrap to 0) — far worse than the loud, controlled panic here.
+    //
+    // The original rationale for the `expect` was "overflow needs 2^64 commits,
+    // so it is structurally unreachable". That was WRONG, and worth spelling out
+    // because it is the sentence a future maintainer would trust: the counter is
+    // not only produced by this binary's increments, it is also READ FROM THE
+    // FILE at open (`recovery.rs`, `txn_counter: sb.txn_counter`). A superblock
+    // forged with `txn_counter = u64::MAX` and a recomputed XXH3 checksum made
+    // the panic fire on the first commit after open.
+    //
+    // What makes it unreachable now is an actual invariant rather than an
+    // argument about counting: `Superblock::validate` rejects any slot whose
+    // counter exceeds MAX_TXN_COUNTER, so a file that opened at all has at least
+    // TXN_COUNTER_RESERVE (2^32) increments of headroom. Exhausting that takes
+    // 4.3 billion commits in one session, since a reopen re-validates.
+    // The `expect` therefore stays: it now documents a guarded invariant rather
+    // than asserting an unguarded hope.
     *ctx.txn_counter = ctx
         .txn_counter
         .checked_add(1)

src/transaction/keys.rs

@@ -63,6 +63,11 @@ impl TransactionManager {
 
         // I119: use checked_add, not `+= 1`. A wrapped counter corrupts
         // Superblock::select's "highest counter wins" rule on recovery.
+        //
+        // Reachable from a forged superblock until `Superblock::validate` grew
+        // its MAX_TXN_COUNTER bound; see the long note at the matching site in
+        // commit.rs for why the old "structurally unreachable" claim was wrong
+        // and what actually guarantees the headroom now.
         self.txn_counter = self
             .txn_counter
             .checked_add(1)