From 0ffe3bcc450a6e0b5a7613ee7da089f9ef65b7c5 Mon Sep 17 00:00:00 2001 From: Christophe Pettus Date: Wed, 29 Jul 2026 14:27:37 -0700 Subject: [PATCH] docs(comments): correct three comments that state the opposite of the code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CRYPTO-2. `sb_identity_aad`'s doc claims it "binds the sealed body AND EACH KEY-SLOT'S DEK WRAP to this superblock's plaintext identity". It binds only the body: the function is passed to `seal_body`/`open_body` and nowhere else, while every DEK wrap authenticates against `KeySlot::aad()` — state, kdf_id, Argon2 params, salt, wrap_nonce, none of which carries superblock identity or generation. This is the most consequential of the three, because it asserts a security property whose ABSENCE is the actual weakness: a maintainer who believes key slots are already pinned to the superblock generation will not add the binding when it is needed. The doc now states the scope explicitly and says that binding wraps to the generation means extending the wrap AAD, not reusing this one. PAGE-IO-1. page_cache.rs's module header asserts "`next_page_id` is a monotonic allocator ... Rollback does NOT rewind it (see the note in `discard`)". `truncate` rewinds it — its own doc says "This is the only path that legitimately rewinds `next_page_id`" — and `truncate` is exactly what `rollback` and `rollback_to` call. The `discard` the header points at is `#[allow(dead_code)]` with no production caller, so the reader is directed at the one path that is NOT the rollback path. The consequence is an aliasing trap: allocate page 100, roll back to a committed total of 90, allocate again, and `new_page` returns 100 for different content. Anyone trusting the header would skip cache/spillway/freemap invalidation for the reissued range — precisely the bug class the header claims is impossible. The header now says monotonic within a transaction, rewound by `truncate` across one, and spells out the invalidation obligation. The old rationale ("two concurrent savepoint rollbacks could hand the same ID to two different allocations") never applied either: the engine is deliberately single-threaded and single-client. It is dropped rather than reworded. A unit test pins the behaviour — `truncate` rewinds the allocator and the next `new_page` reissues the id. Verified it fails without the rewind (left: 6, right: 3) rather than passing vacuously. PAGE-IO-4. `DataPage::insert` says "the transaction layer calls PageCache::new_page() for every insert rather than scanning existing pages for free slots ... this function itself is correct; it's just underutilized". R1 packing replaced that: `transaction::packing` keeps an insert cursor and calls `DataPage::insert` on it for every value, allocating a new page only when the cursor fills or a savepoint disables packing. Calling the slot-directory append path "underutilized" invites deleting machinery that now runs for every non-first insert in a transaction. Closes #101. --- src/data_page.rs | 15 ++++++++---- src/page_cache.rs | 56 ++++++++++++++++++++++++++++++++++++------- src/superblock/mod.rs | 17 +++++++++---- 3 files changed, 71 insertions(+), 17 deletions(-) diff --git a/src/data_page.rs b/src/data_page.rs index 7ff46c7..4a4d04c 100644 --- a/src/data_page.rs +++ b/src/data_page.rs @@ -159,10 +159,17 @@ impl DataPage { // The returned slot index is always the pre-insertion slot_count, making // indices monotonically increasing and stable for the page's lifetime. // - // Note (v1 simplification per ARCHITECTURE.md): the transaction layer calls - // PageCache::new_page() for every insert rather than scanning existing - // pages for free slots. Intentional, not a bug — this function itself is - // correct; it's just underutilized. + // The multi-slot machinery here is load-bearing, not spare capacity. Under + // R1 packing the transaction layer keeps an insert cursor — a data page + // allocated earlier in the SAME transaction that still has room — and every + // value is appended to it through this function. `PageCache::new_page` is + // called only when the cursor fills (this returns None) or when packing is + // disabled because a savepoint is active. See `transaction::packing`. + // + // This comment previously said the opposite: that the transaction layer + // allocated a fresh page per insert and that this function was "correct, + // just underutilized". Do not simplify the slot-directory append path on + // that basis — it now runs for every non-first insert in a transaction. pub fn insert(buf: &mut [u8; PAGE_SIZE], value: &[u8]) -> Option { let (free_start, free_end, slot_count) = Self::validate_header(buf)?; let needed = SLOT_ENTRY_SIZE + value.len(); diff --git a/src/page_cache.rs b/src/page_cache.rs index 01f7f97..188134d 100644 --- a/src/page_cache.rs +++ b/src/page_cache.rs @@ -21,11 +21,19 @@ // - `new_page()` allocates a FRESH page_id past the current EOF. It never // overwrites a live page. This is what makes copy-on-write safe: the old // committed page remains untouched on disk until the superblock swap. -// - `next_page_id` is a monotonic allocator. It is seeded from the file's -// page count at open time, and is bumped on every `new_page()`. Rollback -// does NOT rewind it (see the note in `discard`); orphaned page IDs are -// acceptable because they are reclaimed by the freemap after commit or -// simply re-truncated. +// - `next_page_id` is a monotonic allocator WITHIN a transaction: seeded from +// the file's page count at open, bumped on every `new_page()`, and never +// rewound while a transaction is in flight. It IS rewound across a +// transaction boundary — `truncate()` sets it back to the truncation point +// (see its doc), and both `rollback` and `rollback_to` call `truncate` with +// the last-committed page count. +// +// So a page id handed out before a rollback is NOT burned: allocate page +// 100, roll back to a committed total of 90, allocate again, and `new_page` +// returns 100 a second time. Any new allocation path must therefore treat a +// post-rollback id as potentially aliasing a pre-rollback one and invalidate +// cache / spillway / freemap state for the reissued range, exactly as +// `truncate` already does. // - The cache is a STRICT bound with sidecar overflow. `load_page` evicts // before insertion; `new_page` evicts after insertion. When every page // in the cache is dirty, `maybe_evict` spills the LRU-tail dirty page @@ -595,10 +603,13 @@ impl PageCache { /// committed pages — there is nothing to "undo" on disk, only cached /// garbage to throw away. /// - /// Note: `next_page_id` is deliberately NOT rewound. If rollback freed - /// IDs back to the allocator, two concurrent savepoint rollbacks could - /// hand the same ID to two different allocations. Leaving `next_page_id` - /// monotonic sacrifices a tiny amount of address space for correctness. + /// Note: this per-page path does not touch `next_page_id`, but that is a + /// property of `discard` alone — it is not the rollback contract. The + /// production rollback goes through `truncate`, which DOES rewind the + /// allocator; see the module header. (The rationale previously given here + /// — that rewinding would let "two concurrent savepoint rollbacks" hand + /// the same id to two allocations — never applied: the engine is + /// deliberately single-threaded and single-client.) /// /// `#[allow(dead_code)]`: the original rollback path called this /// per-page. Post-I3 (watermark rollback) the production path uses @@ -1680,6 +1691,33 @@ mod tests { assert_eq!(cache.spillway.as_ref().unwrap().slot_count(), 0); } + #[test] + /// PAGE-IO-1: the module header used to assert that rollback never + /// rewinds `next_page_id`, so a page id handed out before a rollback could + /// never be reissued. `truncate` — which is what `rollback` and + /// `rollback_to` call — does rewind it, and the very next `new_page` + /// hands the same id out again. Pinned here because the false version + /// invited callers to skip invalidation for a reissued range. + fn truncate_rewinds_the_allocator_so_ids_are_reissued() { + let (_dir, mut cache) = fresh_cache(16); + for _ in 0..6 { + cache.new_page().unwrap(); + } + assert_eq!(cache.next_page_id(), 6); + + // What rollback does: truncate back to the last committed page count. + cache.truncate(3).unwrap(); + assert_eq!( + cache.next_page_id(), + 3, + "truncate must rewind the allocator, not leave it monotonic" + ); + + // And the ids really do come back around. + assert_eq!(cache.new_page().unwrap(), 3); + assert_eq!(cache.new_page().unwrap(), 4); + } + #[test] fn truncate_drops_spillway_entries_above_watermark() { let max_pages = 2; diff --git a/src/superblock/mod.rs b/src/superblock/mod.rs index 15045db..1e448be 100644 --- a/src/superblock/mod.rs +++ b/src/superblock/mod.rs @@ -329,10 +329,19 @@ impl Superblock { buf } - /// Build the AAD that binds the sealed body and each key-slot's DEK wrap to - /// this superblock's plaintext identity. The four bootstrap fields that stay - /// cleartext in both encrypted and plaintext DBs are included; this prevents - /// transplanting a sealed body from a different DB or a different txn_counter. + /// Build the AAD that binds THE SEALED BODY to this superblock's plaintext + /// identity. The four bootstrap fields that stay cleartext in both + /// encrypted and plaintext DBs are included; this prevents transplanting a + /// sealed body from a different DB or a different txn_counter. + /// + /// Scope note: this covers the body and nothing else. The key-slot DEK + /// wraps do NOT use it — `wrap_dek`/`unwrap_dek` authenticate against + /// `KeySlot::aad()`, which is slot-local (state, kdf_id, Argon2 params, + /// salt, wrap_nonce) and carries no superblock identity or generation. + /// A key slot is therefore not cryptographically pinned to the superblock + /// generation it was written in. Do not read this function's existence as + /// evidence that it is; binding wraps to the superblock generation would + /// mean extending the wrap AAD, not reusing this one. /// /// These four MUST stay cleartext even in an encrypted DB precisely because /// they are the AAD: slot selection (`max_by_key` on `txn_counter`) and this