diff --git a/src/error.rs b/src/error.rs index bde271b..8a314b0 100644 --- a/src/error.rs +++ b/src/error.rs @@ -176,6 +176,23 @@ pub enum ChiselError { stored: u32, max: u32, }, + // Raised when a commit (or a crypto-header rewrite) would push + // `txn_counter` past `MAX_TXN_COUNTER`, the bound `Superblock::validate` + // enforces on the read side. + // + // Fatal, and refused BEFORE any commit I/O, because the alternative is + // writing a superblock this same binary would refuse to read: the database + // would either lose an acknowledged commit on the next open (silent + // fallback to an older valid slot) or become permanently unopenable once + // every slot has been written past the bound. + // + // Legitimately unreachable — a file that opens has 2^32 increments of + // headroom, and a reopen re-validates. It IS reachable adversarially, on + // the first commit after opening a file forged at exactly MAX_TXN_COUNTER, + // which is precisely the case this exists to make clean. + TxnCounterExhausted { + current: u64, + }, // Operational — the caller supplied the wrong key material or none, or // asked an unencrypted-only build to open an encrypted DB. The on-disk @@ -241,6 +258,7 @@ impl ChiselError { | ChiselError::InvalidPageId { .. } | ChiselError::UnsupportedPageSize { .. } | ChiselError::InvalidFreemapDepth { .. } + | ChiselError::TxnCounterExhausted { .. } | ChiselError::DecryptionFailed { .. } ) } @@ -331,6 +349,10 @@ impl fmt::Display for ChiselError { f, "page size mismatch: file was written with {stored}-byte pages, this build uses {compiled}-byte pages" ), + ChiselError::TxnCounterExhausted { current } => write!( + f, + "txn_counter {current} is at the maximum this format supports; the database can accept no further commits" + ), ChiselError::InvalidFreemapDepth { stored, max } => write!( f, "superblock declares freemap depth {stored}, which exceeds the maximum {max} (corrupt or forged superblock)" @@ -594,6 +616,7 @@ mod tests { | ChiselError::InvalidPageId { .. } | ChiselError::UnsupportedPageSize { .. } | ChiselError::InvalidFreemapDepth { .. } + | ChiselError::TxnCounterExhausted { .. } | ChiselError::DecryptionFailed { .. } => true, } } @@ -640,6 +663,7 @@ mod tests { compiled: 0, }, ChiselError::InvalidFreemapDepth { stored: 0, max: 0 }, + ChiselError::TxnCounterExhausted { current: 0 }, ChiselError::NoEncryptionKey, ChiselError::InvalidEncryptionKey, ChiselError::EncryptionNotSupported, @@ -654,13 +678,15 @@ mod tests { "is_fatal() disagrees with the documented Fatal/Operational block for {e:?}" ); } - // Tripwire: exactly 11 variants are fatal today. If this count moves, the + // Tripwire: exactly 12 variants are fatal today. If this count moves, the // Fatal/Operational split changed — confirm that was intentional (it is a // breaking change for callers doing error-class matching, per the header). - // Last moved 10 -> 11 by InvalidFreemapDepth, which is fatal because a - // superblock field outside its representable range cannot be resolved by - // retrying: every sibling slot carries the same value. - assert_eq!(all.iter().filter(|e| e.is_fatal()).count(), 11); + // Last moved 11 -> 12 by TxnCounterExhausted (a database that cannot + // accept another commit is terminal for the handle); before that, + // 10 -> 11 by InvalidFreemapDepth, fatal because a superblock field + // outside its representable range cannot be resolved by retrying — + // every sibling slot carries the same value. + assert_eq!(all.iter().filter(|e| e.is_fatal()).count(), 12); } // Phase 4: the three operational encryption errors are recoverable (the diff --git a/src/recovery_tests.rs b/src/recovery_tests.rs index 8d4b311..11704b1 100644 --- a/src/recovery_tests.rs +++ b/src/recovery_tests.rs @@ -2179,3 +2179,73 @@ fn a_forged_txn_counter_in_one_slot_loses_to_its_healthy_sibling() { db.allocate(b"and it still commits").unwrap(); db.commit().unwrap(); } + +#[test] +fn a_database_forged_at_the_boundary_refuses_to_commit_rather_than_writing_past_it() { + // The read-side bound alone is only half an invariant, and the half it + // leaves open is worse than the hole it closed. + // + // MAX_TXN_COUNTER itself is VALID — the check is `>`, not `>=` — so a file + // forged at exactly the boundary opens cleanly, with `diagnose` silent. + // Without a matching write-side check, the commits that follow are + // acknowledged and fsynced at MAX+1, MAX+2 … producing superblocks this + // same binary refuses to read. One commit past the bound is silently + // discarded on the next open (selection falls back to an older valid slot); + // once every slot has been written past it, the database is permanently + // unopenable. Both outcomes are strictly worse than the original forged- + // u64::MAX panic, which at least failed loudly and immediately. + // + // So: refuse the commit, before any I/O, and leave the last durable state + // exactly as it was. + let file = NamedTempFile::new().unwrap(); + let path = file.path().to_owned(); + let handle; + { + let mut db = Chisel::open(&path, Default::default()).unwrap(); + db.begin().unwrap(); + handle = db.allocate(b"still here afterwards").unwrap(); + db.commit().unwrap(); + } + + // Forge only the WINNING slot. Forging both would tie their counters, and + // `max_by_key` returns the last maximum — so selection would pick the + // create-seed slot and the database would come back empty for reasons that + // have nothing to do with what this test is about. + 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, crate::superblock::MAX_TXN_COUNTER); + + // Opens: the boundary value is legal. + let mut db = Chisel::open(&path, Default::default()) + .expect("MAX_TXN_COUNTER is inside the accepted range and must open"); + + db.begin().unwrap(); + db.allocate(b"this commit cannot be represented").unwrap(); + match db.commit() { + Err(ChiselError::TxnCounterExhausted { current }) => { + assert_eq!(current, crate::superblock::MAX_TXN_COUNTER); + } + Ok(()) => panic!("committing past MAX_TXN_COUNTER must be refused, not persisted"), + Err(other) => panic!("expected TxnCounterExhausted, got {other:?}"), + } + drop(db); + + // The refusal wrote nothing: the database still opens and still holds the + // last durable state. This is the assertion that actually distinguishes + // "refused" from "wrote a superblock nobody can read". + let db = Chisel::open(&path, Default::default()) + .expect("a refused commit must leave the file exactly as it was"); + assert_eq!(db.read(handle).unwrap(), b"still here afterwards"); +} diff --git a/src/superblock/mod.rs b/src/superblock/mod.rs index 8bc90a4..383871f 100644 --- a/src/superblock/mod.rs +++ b/src/superblock/mod.rs @@ -76,6 +76,29 @@ pub const DEFAULT_SUPERBLOCK_COUNT: u32 = 2; pub const TXN_COUNTER_RESERVE: u64 = 1 << 32; pub const MAX_TXN_COUNTER: u64 = u64::MAX - TXN_COUNTER_RESERVE; +/// The counter the next superblock write should carry, or a typed error if +/// bumping would leave the reserved band. +/// +/// This exists because the read-side bound in `validate` is only half an +/// invariant. Bounding what we ACCEPT without bounding what we WRITE lets the +/// engine persist a superblock it will later refuse to read: forge a slot at +/// exactly `MAX_TXN_COUNTER` (which validates, and opens cleanly), and the next +/// commits are acknowledged and fsynced at MAX+1, MAX+2 — after which every +/// slot fails `validate` and the database is permanently unopenable. One commit +/// past the bound is quieter and worse: the reopen silently falls back to an +/// older valid slot, discarding an acknowledged commit. +/// +/// So the rule is: never write a superblock we would not read back. Callers +/// must consult this BEFORE doing any commit I/O, so a refusal costs nothing +/// and leaves the last durable state untouched. +pub fn next_txn_counter(current: u64) -> Result { + let next = current.saturating_add(1); + if next > MAX_TXN_COUNTER { + return Err(crate::error::ChiselError::TxnCounterExhausted { current }); + } + Ok(next) +} + // 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 @@ -805,6 +828,48 @@ mod tests { .copy_from_slice(&99u32.to_le_bytes()); page::stamp_checksum(&mut bad_count); assert_eq!(validate(&bad_count), Err(SuperblockDefect::BadCount(99))); + + // The bound is `>`, so MAX_TXN_COUNTER itself must be accepted. Pinning + // both sides matters here: a database forged at exactly the boundary is + // the case that opens cleanly and then exercises the write-side guard. + let mut at_bound = good; + at_bound[8..16].copy_from_slice(&MAX_TXN_COUNTER.to_le_bytes()); + page::stamp_checksum(&mut at_bound); + assert_eq!(validate(&at_bound), Ok(())); + + let mut over_bound = good; + over_bound[8..16].copy_from_slice(&(MAX_TXN_COUNTER + 1).to_le_bytes()); + page::stamp_checksum(&mut over_bound); + assert_eq!( + validate(&over_bound), + Err(SuperblockDefect::BadTxnCounter(MAX_TXN_COUNTER + 1)) + ); + } + + // The write side of the same invariant. Bounding what we ACCEPT without + // bounding what we WRITE lets the engine persist superblocks it will refuse + // to read back, which is worse than refusing the commit: one commit past the + // bound is silently discarded on the next open (fallback to an older valid + // slot), and two make the file permanently unopenable. + #[test] + fn next_txn_counter_refuses_to_leave_the_readable_range() { + assert_eq!(next_txn_counter(0).unwrap(), 1); + assert_eq!( + next_txn_counter(MAX_TXN_COUNTER - 1).unwrap(), + MAX_TXN_COUNTER + ); + assert!(matches!( + next_txn_counter(MAX_TXN_COUNTER), + Err(crate::error::ChiselError::TxnCounterExhausted { + current + }) if current == MAX_TXN_COUNTER + )); + // Saturating, not wrapping: u64::MAX must not roll over to 0 and be + // mistaken for a fresh database by "highest counter wins". + assert!(matches!( + next_txn_counter(u64::MAX), + Err(crate::error::ChiselError::TxnCounterExhausted { .. }) + )); } #[test] @@ -1206,7 +1271,11 @@ mod tests { proptest::proptest! { #[test] fn prop_serialize_deserialize_roundtrip( - txn_counter in 0u64..u64::MAX, + // Same reasoning as superblock_count below: values above + // MAX_TXN_COUNTER make deserialize() return None by design, so + // sampling the full u64 range would fail the round-trip property + // roughly once in 2^32 draws — a landmine rather than a signal. + txn_counter in 0u64..=MAX_TXN_COUNTER, root_handle_table_page in 0u64..u64::MAX, root_freemap_page in 0u64..u64::MAX, total_pages in 0u64..u64::MAX, diff --git a/src/transaction/commit.rs b/src/transaction/commit.rs index 9034bf3..723d448 100644 --- a/src/transaction/commit.rs +++ b/src/transaction/commit.rs @@ -32,6 +32,15 @@ pub(super) struct CommitCtx<'a> { } pub(super) fn run_commit(ctx: &mut CommitCtx<'_>) -> Result<()> { + // Refuse a commit that could not be read back BEFORE doing any work. The + // counter this commit will stamp is decided here, at the top, so exhausting + // the range costs no I/O and leaves the last durable state exactly as it + // was. See `superblock::next_txn_counter` for why writing past the bound is + // worse than refusing: it produces superblocks this binary rejects on the + // next open, silently discarding an acknowledged commit or bricking the file + // outright once every slot has been written past it. + let next_counter = crate::superblock::next_txn_counter(*ctx.txn_counter)?; + // I27: flatten every still-active savepoint's `freed_pages` // back into `txn_freed_pages` before persist_freemap consumes // it. savepoint_inner moves `txn_freed_pages` INTO the @@ -123,14 +132,15 @@ pub(super) fn run_commit(ctx: &mut CommitCtx<'_>) -> Result<()> { // 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) - .expect("txn_counter overflowed u64 (2^64 commits) — unreachable"); + // TXN_COUNTER_RESERVE (2^32) increments of headroom. + // + // `next_txn_counter` was computed at the TOP of this function, before any + // I/O, so the refusal case costs nothing and leaves the last durable state + // untouched. Bounding only what we ACCEPT and not what we WRITE would let a + // file forged at exactly MAX_TXN_COUNTER (which validates, and opens + // cleanly) take acknowledged, fsynced commits at MAX+1, MAX+2 — superblocks + // this same binary then refuses to read. There is no panic left here at all. + *ctx.txn_counter = next_counter; let total_pages = cache.file_page_count()?; let sb = Superblock { magic: page::MAGIC, diff --git a/src/transaction/keys.rs b/src/transaction/keys.rs index 2b3b790..0c5b333 100644 --- a/src/transaction/keys.rs +++ b/src/transaction/keys.rs @@ -61,17 +61,18 @@ impl TransactionManager { // future changes. cache.flush()?; - // I119: use checked_add, not `+= 1`. A wrapped counter corrupts - // Superblock::select's "highest counter wins" rule on recovery. + // I119: a wrapped counter corrupts Superblock::select's "highest counter + // wins" rule on recovery. This used to be `checked_add(1).expect(...)` + // justified by a "structurally unreachable" claim that was wrong — the + // counter is read from the file at open, so a forged value made it + // reachable. See the matching site in commit.rs for the full story. // - // 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) - .expect("txn_counter overflowed u64 (2^64 commits) — unreachable"); + // Bounded rather than panicking now, and bounded on the WRITE side so + // this never persists a header the next open would reject. Note this + // rewrite is metadata-only but still consumes a counter, so key rotation + // draws from the same headroom pool as commit. + let next_counter = crate::superblock::next_txn_counter(self.txn_counter)?; + self.txn_counter = next_counter; let total_pages = cache.file_page_count()?; let r = &self.committed_roots;