From 64934a5206329f5cb0eef0aab01ad119350d0d78 Mon Sep 17 00:00:00 2001 From: Christophe Pettus Date: Wed, 29 Jul 2026 12:26:59 -0700 Subject: [PATCH] fix: compute file_size_bytes from the live stride, not PAGE_SIZE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Chisel::file_size_bytes` and `Stats.file_size_bytes` both multiplied `PageIo::page_count` by the hardcoded `PAGE_SIZE`. That count is in stride-units, and an encrypted database runs at `ENC_PAGE_SIZE` (8232) from birth, so every encrypted database under-reported its own on-disk size by 0.49% — 40 MB at one million pages. `open_existing` already got this right in its `FileSizeMismatch` arithmetic and even names the hazard in a comment; the two public accessors did not. Adds `TransactionManager::file_stride()` and multiplies by it at both sites, so the value now matches `stat(2)` for plaintext and encrypted databases alike — which is what `Stats.file_size_bytes` ("Raw size of the database file on disk") and README ("Physical size of the database file in bytes") have always promised. The new test asserts against `fs::metadata(path).len()` rather than a computed constant: PAGE_SIZE arithmetic can only agree with stat in the plaintext case, so the encrypted arm fails on the old code and the plaintext arm catches an over-correction in the other direction. Closes #94. --- src/lib.rs | 17 ++++++++------ src/stats.rs | 7 ++++-- src/transaction/stats.rs | 14 ++++++++++++ tests/encryption_roundtrip.rs | 43 +++++++++++++++++++++++++++++++++++ 4 files changed, 72 insertions(+), 9 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index bd6c496..5d9f99e 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -874,9 +874,11 @@ impl Chisel { /// Summary statistics derived by scanning the current handle table and /// querying the underlying file length. `file_size_bytes` is computed - /// from `page_count * PAGE_SIZE` rather than `stat(2)` so it reflects + /// from `page_count * stride` rather than `stat(2)` so it reflects /// the page-aligned view the engine has, not any trailing partial page - /// that might exist mid-extend. + /// that might exist mid-extend. `stride` is the on-disk unit size — + /// `PAGE_SIZE` for a plaintext database, `ENC_PAGE_SIZE` (8232) for an + /// encrypted one, whose per-page nonce and tag make every unit larger. /// /// # Errors /// Only on poisoning — a fatal `IoError` while scanning the handle table or @@ -904,7 +906,7 @@ impl Chisel { // behaviour is the right semantic here: "as big as a u64 // can represent" is closer to truth than "wrapped to a // small number". - file_size_bytes: page_count.saturating_mul(PAGE_SIZE as u64), + file_size_bytes: page_count.saturating_mul(self.txm.file_stride() as u64), spillway_logical_bytes: spillway_cap.map(|(logical, _)| logical), spillway_max_bytes: spillway_cap.map(|(_, max)| max), }) @@ -924,9 +926,10 @@ impl Chisel { } /// Page-aligned on-disk size of the database, computed as - /// `page_count × PAGE_SIZE`. Same number `stats().file_size_bytes` - /// returns, but without the handle-table scan that `stats()` does - /// to populate `handle_count`. + /// `page_count × stride` — where `stride` is `PAGE_SIZE` for a plaintext + /// database and `ENC_PAGE_SIZE` (8232) for an encrypted one. Same number + /// `stats().file_size_bytes` returns, but without the handle-table scan + /// that `stats()` does to populate `handle_count`. /// /// I53 (ISSUES.md, 2026-05-22): broken out for the bench harness, /// which calls this per measurement cell — `stats()` walks all @@ -940,7 +943,7 @@ impl Chisel { /// Only on poisoning (a fatal `IoError` reading the file length). pub fn file_size_bytes(&self) -> Result { let page_count = self.txm.file_page_count()?; - Ok(page_count.saturating_mul(PAGE_SIZE as u64)) + Ok(page_count.saturating_mul(self.txm.file_stride() as u64)) } /// Returns true if this database handle has been poisoned by a diff --git a/src/stats.rs b/src/stats.rs index c843e81..dbbbff4 100644 --- a/src/stats.rs +++ b/src/stats.rs @@ -20,8 +20,11 @@ pub struct Stats { pub handle_count: u64, /// Total allocated pages in the file, matching Superblock.total_pages. pub total_pages: u64, - /// Raw size of the database file on disk. May exceed - /// `total_pages * PAGE_SIZE` when a previous crash left orphan + /// Raw size of the database file on disk: `page_count × stride`, where + /// `stride` is `PAGE_SIZE` for a plaintext database and `ENC_PAGE_SIZE` + /// (8232) for an encrypted one — an encrypted page carries a 24-byte + /// nonce and a 16-byte tag on top of its 8192 plaintext bytes. May exceed + /// `total_pages * stride` when a previous crash left orphan /// pages in the file tail — the last-durable superblock's /// `total_pages` is authoritative, anything beyond it is dead /// weight that the next allocation will overwrite (see I4). diff --git a/src/transaction/stats.rs b/src/transaction/stats.rs index 16698c1..fcc6604 100644 --- a/src/transaction/stats.rs +++ b/src/transaction/stats.rs @@ -80,6 +80,20 @@ impl TransactionManager { self.poison_on_fatal(result) } + /// On-disk bytes per page-unit: `PAGE_SIZE` (8192) for a plaintext + /// database, `ENC_PAGE_SIZE` (8232) for an encrypted one. The counts + /// returned by `file_page_count` are in these units, so any caller + /// converting pages to bytes must multiply by THIS, not by the + /// hardcoded `PAGE_SIZE` — see `open_existing`'s FileSizeMismatch + /// arithmetic, which has always used the live stride. + /// + /// Infallible and poison-independent: the stride is fixed for the life + /// of the file and read from memory, so there is no I/O to fail and + /// nothing a fatal error could invalidate. + pub fn file_stride(&self) -> usize { + self.cache.borrow().io().stride() + } + // --- Selective defragmentation support (ISSUES.md R3 + I17) --- // // These methods expose just enough of the R1 live-slot tracking diff --git a/tests/encryption_roundtrip.rs b/tests/encryption_roundtrip.rs index e3e3d1d..72a43d1 100644 --- a/tests/encryption_roundtrip.rs +++ b/tests/encryption_roundtrip.rs @@ -118,3 +118,46 @@ fn in_memory_plaintext_roundtrip_baseline() { db.commit().expect("commit"); assert_eq!(db.read(h).expect("read"), b"plain"); } + +#[test] +fn file_size_bytes_matches_stat_for_both_strides() { + // CRYPTO-5: `file_size_bytes()` / `stats().file_size_bytes` used to + // multiply the page count by the hardcoded PAGE_SIZE. Page counts are in + // stride-units, and an encrypted DB's stride is ENC_PAGE_SIZE (8232), so + // every encrypted database under-reported its own size by 0.49%. + // + // Asserting against stat(2) rather than a hardcoded expected size is what + // makes this test fail on the bug: PAGE_SIZE arithmetic can only match + // stat for the plaintext case, so the encrypted arm is the real check and + // the plaintext arm guards against over-correcting in the other direction. + for (name, key) in [("plain.db", None), ("enc.db", Some(raw_key(0x5C)))] { + let dir = tempfile::tempdir().expect("tempdir"); + let path = dir.path().join(name); + + let mut options = Options::default(); + if let Some(k) = key { + options = options.encryption_key(k); + } + let mut db = Chisel::open(&path, options).expect("open"); + + // Force the file past the superblock region so a stride error is + // several pages' worth of bytes, not a rounding coincidence. + db.begin().expect("begin"); + for i in 0..64u32 { + db.allocate(&vec![i as u8; 4096]).expect("allocate"); + } + db.commit().expect("commit"); + + let on_disk = std::fs::metadata(&path).expect("stat").len(); + assert_eq!( + db.file_size_bytes().expect("file_size_bytes"), + on_disk, + "{name}: file_size_bytes() disagrees with stat(2)" + ); + assert_eq!( + db.stats().expect("stats").file_size_bytes, + on_disk, + "{name}: stats().file_size_bytes disagrees with stat(2)" + ); + } +}