From cd9eb5e3f1e0bd9b215e1911ff1f2da9b3e27de8 Mon Sep 17 00:00:00 2001 From: Christophe Pettus Date: Wed, 1 Jul 2026 18:23:40 -0700 Subject: [PATCH] =?UTF-8?q?fix:=20four=20BUG-severity=20findings=20from=20?= =?UTF-8?q?the=202026-07-02=20deep=20review=20(I143=E2=80=93I146)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I143 — TOCTOU create-vs-open race (src/lib.rs): the create_new vs open_existing decision was made from an unlocked path.exists() stat taken BEFORE the flock. A process racing open() could stat an empty file while another creates + commits + releases the lock, then run create_new over the committed data. Decide from io.page_count() (cached from the post-lock file length) instead; the pre-lock stat is kept only for the create_if_missing gate, which must stay pre-lock so a refused open never materializes a file. I144 — unvalidated crypto-header stride (src/transaction/recovery.rs): the plaintext stride (guarded only by the forgeable XXH3 checksum) went verbatim into set_stride, which computes file_len / stride. A forged stride=0 was a division-by-zero panic (violating poison-not-panic); a huge value drove multi-GiB allocations. Require stride == ENC_PAGE_SIZE before set_stride; mismatch returns CorruptSuperblock. Regression test forges the on-disk stride to 0, re-stamps the checksum, and asserts the reopen errors, not panics. I145 — reclaim_freemap_orphans not poison-wrapped (src/transaction/freemap.rs): a fatal error mid-sweep returned un-poisoned, and reclaim_orphans writes the partially-advanced freemap root back into current_roots even on its error path. Wrap the sweep in poison_on_fatal like every other TM entry point. I146 — README contradicted the code (README.md): the format-compatibility section said the newer-minor write-refusal gate was "not yet wired up"; it shipped as I29 and THEORY.md documents it as live. Corrected to match. Full test suite (incl. the new forge test), clippy --workspace -D warnings, and fmt --check all clean. --- README.md | 2 +- src/lib.rs | 11 ++++++++++- src/recovery_tests.rs | 29 ++++++++++++++++++++++++++++- src/transaction/freemap.rs | 24 +++++++++++++++++------- src/transaction/recovery.rs | 12 ++++++++++++ 5 files changed, 68 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index 0a51b1d..eb22e0b 100644 --- a/README.md +++ b/README.md @@ -360,7 +360,7 @@ Encryption introduces the second major: an encrypted database is stamped MAJOR = **Page level** — each non-superblock page carries a one-byte `page_format_version` in its header, letting individual page layouts evolve within a major without a file-wide format bump. The post-1.0 upgrade story is lazy migration: on read, the page-type module dispatches on its page's declared version; on write, it always produces the current version; cold pages stay in the old layout until an opt-in `db.upgrade()` sweep rewrites them. An additional 8 bytes are reserved in every non-superblock page header for future common-header fields. -Write safety across minors is a narrower guarantee: a binary at MINOR = *m* opening a file at MINOR = *m' > m* cannot safely commit without risking overwriting fields it doesn't know about. The open gate is MAJOR-only by design, so minor variants coexist — same-major files of any minor open successfully, and the chunk-tags MINOR = 1 variant is the first such case. The write-refusal arm (refuse writes when file MINOR > binary MINOR, leaving the newer-minor file read-only) is not yet wired up; it lands with the first post-1.0 minor bump that makes the direction observable. The post-1.0 cross-minor read-compatibility guarantee is absolute; write-compatibility requires binary MINOR ≥ file MINOR. +Write safety across minors is a narrower guarantee: a binary at MINOR = *m* opening a file at MINOR = *m' > m* cannot safely commit without risking overwriting fields it doesn't know about. The open gate is MAJOR-only by design, so minor variants coexist — same-major files of any minor open successfully, and the chunk-tags MINOR = 1 variant is the first such case. The write-refusal arm (refuse writes when file MINOR > binary MINOR) is implemented (I29): opening a newer-minor file forces the handle read-only, so any mutation returns `ReadOnlyMode` rather than risking a write that clobbers fields the binary doesn't know about. The post-1.0 cross-minor read-compatibility guarantee is absolute; write-compatibility requires binary MINOR ≥ file MINOR. ### Pre-1.0 caveat diff --git a/src/lib.rs b/src/lib.rs index f6bcb0e..13d86bb 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -373,6 +373,15 @@ impl Chisel { } let io = PageIo::open(path, options.read_only)?; + // I143: decide create-vs-open from the file length observed AFTER the + // flock is held (page_count() returns the count cached from the post-lock + // length), NOT from the pre-lock `file_exists` stat. The pre-lock stat + // races a concurrent creator: another process can create + commit + + // release the lock between our stat and our lock, and the stale boolean + // would then run create_new over its just-committed data. `file_exists` + // stays above only for the create_if_missing gate, which must remain + // pre-lock so a refused open never materializes an empty file. + let existed = io.page_count()? > 0; let cache = PageCache::new( io, options.cache_max_bytes, @@ -381,7 +390,7 @@ impl Chisel { SpillwayLocation::Path(path.to_path_buf()), ); - let txm = if file_exists { + let txm = if existed { // Existing database: N is discovered from the on-disk // superblock. options.superblock_count is ignored here. TransactionManager::open_existing(cache, options.encryption_key.clone())? diff --git a/src/recovery_tests.rs b/src/recovery_tests.rs index e15e17a..4602160 100644 --- a/src/recovery_tests.rs +++ b/src/recovery_tests.rs @@ -9,10 +9,11 @@ use crate::page::{self, PageType, PAGE_SIZE}; use crate::superblock::Superblock; -use crate::{Chisel, ChiselError, Options}; +use crate::{Chisel, ChiselError, Key, Options}; use std::fs; use std::io::{Read as _, Seek, SeekFrom, Write}; use tempfile::{NamedTempFile, TempDir}; +use zeroize::Zeroizing; /// Scan the file for the first page whose type tag matches `want` and /// return its page id. Panics if no such page exists; test helper only. @@ -1623,3 +1624,29 @@ fn test_open_rejects_page_size_mismatch() { Ok(_) => panic!("Chisel::open accepted a file with a mismatched page_size"), } } + +// I144 regression: a forged crypto-header stride must fail the open with a typed +// CorruptSuperblock, never a division-by-zero panic in set_stride (the stride is +// plaintext, guarded only by the forgeable XXH3 checksum). Page 0 is always read +// first to learn the stride, so we scribble byte 325 (the u32 stride field at +// CRYPTO_HEADER_OFFSET+1) to 0 and re-stamp the checksum so deserialize accepts it. +#[test] +fn corrupt_crypto_header_stride_errors_not_panic() { + let dir = TempDir::new().unwrap(); + let path = dir.path().join("enc.db"); + let opts = || Options::default().encryption_key(Key::Raw(Zeroizing::new(vec![0x11u8; 32]))); + { + let mut db = Chisel::open(&path, opts()).unwrap(); + db.begin().unwrap(); + db.allocate(b"payload").unwrap(); + db.commit().unwrap(); + } + rewrite_page_with_valid_checksum(&path, 0, |buf| { + buf[325..329].copy_from_slice(&0u32.to_le_bytes()); + }); + match Chisel::open(&path, opts()) { + Err(ChiselError::CorruptSuperblock { .. }) => {} + Err(other) => panic!("expected CorruptSuperblock for a forged stride, got {other:?}"), + Ok(_) => panic!("forged-stride open unexpectedly succeeded (should have failed)"), + } +} diff --git a/src/transaction/freemap.rs b/src/transaction/freemap.rs index eebbe2b..54bba32 100644 --- a/src/transaction/freemap.rs +++ b/src/transaction/freemap.rs @@ -663,12 +663,22 @@ impl TransactionManager { pub(crate) fn reclaim_freemap_orphans(&mut self) -> Result { let savepoint_active = !self.savepoints.is_empty(); let superblock_count = self.superblock_count; - let mut cache = self.cache.borrow_mut(); - self.freemap.reclaim_orphans( - &mut cache, - &mut self.current_roots, - savepoint_active, - superblock_count, - ) + // I145: wrap the sweep in poison_on_fatal like every other TM entry point. + // A fatal error mid-sweep (an IoError/CorruptPage from a page read or a + // live-page cache.get) otherwise returns un-poisoned, and reclaim_orphans + // writes the partially-advanced freemap root back into current_roots even + // on its error path — leaving a usable manager holding an indeterminate + // freemap. The inner borrow is scoped so the cache is released before + // poison_on_fatal takes &self. + let result = { + let mut cache = self.cache.borrow_mut(); + self.freemap.reclaim_orphans( + &mut cache, + &mut self.current_roots, + savepoint_active, + superblock_count, + ) + }; + self.poison_on_fatal(result) } } diff --git a/src/transaction/recovery.rs b/src/transaction/recovery.rs index 42d43c3..81c3cec 100644 --- a/src/transaction/recovery.rs +++ b/src/transaction/recovery.rs @@ -255,6 +255,18 @@ impl TransactionManager { if let Some(stride) = Superblock::deserialize(&page0).and_then(|sb| sb.encryption.map(|h| h.stride as usize)) { + // I144: the stride comes from the plaintext crypto-header, guarded + // only by the forgeable (non-cryptographic) XXH3 page checksum — the + // same trust boundary decrypt_body already bounds-checks ct_len at. + // ENC_PAGE_SIZE is the only value ever written; reject anything else + // BEFORE set_stride, which divides the file length by stride (a forged + // 0 is a division-by-zero panic; a huge value drives multi-GiB read + // allocations). Fail as CorruptSuperblock, honoring poison-not-panic. + if stride != crate::crypto::ENC_PAGE_SIZE { + return Err(ChiselError::CorruptSuperblock { + defects: Vec::new(), + }); + } cache.io_mut().set_stride(stride); } // With an intact page 0 the stride is already correct here, so this