Fix offset overflow and TOC size accounting for 0.5.3 - #69
Merged
Conversation
added 15 commits
September 1, 2026 00:55
seek_to_page_start_relatively multiplied index * PAGE_SIZE in u32, so any page starting beyond 4 GiB had its offset wrapped back into the head of the file, making batch persist and batch parse read and write the wrong pages on large files. Route both seek helpers through a shared page_start_offset helper that does the arithmetic in u64, like the absolute seek_to_page_start already did.
update_at (both the file-level helper and DataPage::update_at) and DataPage::get_at summed link.offset + link.length in u32 before comparing against the page bound. A link whose sum wraps past 4 GiB passed the check with a range far outside the page: the in-memory variants then panicked on the slice, and the file variant wrote outside the page slot. Do the addition in wider arithmetic so such links are rejected with the existing bounds error.
remove_without_record credited estimated_size with one PageId per call to pre-pay for the empty_pages entry that only remove() actually pushes, so every call that did not go through remove() left the page accounting one PageId too large. Move the empty-page accounting into remove(), next to the push it pays for.
insert measured a record as (key, PageId)::aligned_size, which rounds to 8 bytes for 8-aligned keys, while remove_without_record subtracted the 4-aligned align(key + PageId) instead. For key types like (u64, Link) every insert/remove cycle therefore leaked 4 bytes of estimated_size. Route both through a shared record_size helper that mirrors the tuple formula.
BTreeMap::insert over an existing key replaces the PageId in place, but insert still added a full record size to estimated_size, so re-pointing a key inflated the page accounting by one record every time. Only grow estimated_size when the map actually gained a record.
update_key removed the old record and inserted the re-keyed one without touching estimated_size, so key growth through updates silently invalidated the page's own size accounting and consumers later overflowed the fixed page slot on persist. Subtract the old record size, add the new one, and treat an update onto an already existing key as an in-place replacement. update_key now requires T: SizeMeasurable; every key type used with the page already satisfies it (insert and remove required it before).
A table-of-contents page could always report success for a record whose serialized form cannot fit the page slot, so consumers persisted over-budget segments. Add capacity-checked variants that reject any mutation pushing the page's serialized form past INNER_PAGE_SIZE, leaving the page untouched and handing the key back in a typed TableOfContentsOverflowError; fits_empty_page distinguishes a record that can be relocated to a fresh page from one that can never fit and must be rejected upstream.
persist_page_in_place wrote the serialized page with no check against the slot size, so an over-budget page spilled its tail into the neighboring page's slot and corrupted it. Fail the persist with a typed PageOverflowError naming the page before anything is written, so a consumer bug surfaces on its own page instead of corrupting the neighbor. Covers persist_page and persist_pages_batch, which both write through this path.
The workflow still targeted main after the default branch moved to master, so pushes and pull requests stopped running CI entirely.
rkyv pads archived compounds out to their widest member alignment, which is 16 for u128 and i128, but the tuple, IndexValue and table-of-contents record size models clamped their rounding to 8. Every archived record with a u128-family key was under-counted by 8 bytes, so estimated_size drifted below the real serialized size and the capacity-checked insertion kept accepting records until the real archive was hundreds of bytes past the page slot (the reviewed reproduction: 344 bytes over, 8 bytes times 43 records). Propagate the real member alignment through tuple align()/aligned_size (via a shared align_to helper), IndexValue and the table-of-contents record size. The String model was measured against rkyv across lengths 1..48 and only ever over-estimates (it rounds each record up to 4 bytes), so the capacity check is safe on string keys; a property-style churn test now pins both directions: serialized <= estimated always, with slack below 4 bytes per record. For exotic composite keys (for example (u128, u64)) the old model also sized IndexPage value slots smaller than their real archived form, so any such persisted index layout was already corrupt; the corrected sizes supersede them.
from_bytes deserialized the stored estimated_size field as-is, so pages written by 0.5.2 (whose accounting leaked on removals, replacing inserts and key updates) carried their wrong accounting across the upgrade and misled the new capacity checks. Derive the accounting from the parsed records and empty-page list instead; the field is still persisted for format compatibility but no longer trusted on load. The Persistable impl for TableOfContentsPage now additionally requires T: SizeMeasurable, which every persistable key type already satisfies (insert and remove required it before).
The audit of file write paths that take an offset within a page found four unbounded ones: IndexPage::persist_value and remove_value seek to a computed slot offset and write, so a bad value index writes into the neighboring page; the IndexPageUtility default persist writes the utility bytes with no check against the slot; and the unsized index page's tail-first persist_value subtracts its offset from the page end, so an oversized offset writes into this page's header or the previous page (and its u32 offset addition could also wrap, the same class as the earlier seek fix - it now sums in u64). Each path now rejects an out-of-slot write with the typed PageOverflowError before writing anything.
The capacity-checked mutators walked the BTreeMap repeatedly: try_insert did contains_key plus a delegated insert (and recomputed the record size), and try_update_key did two contains_key walks before update_key's remove and insert. Rebuild them on the entry API with the size arithmetic computed up front: - insert and try_insert: one traversal (entry), was two to three - update_key: two traversals (remove old, entry new), the minimum for two distinct keys - try_update_key: two traversals on the accept and missing-key paths (remove_entry old, entry new); a rejected update restores the removed record with a third The overflow check is pure arithmetic on precomputed sizes and runs before the map gains any record; no path clones a key or serializes the page. insert and try_insert drop their now-unneeded T: Clone bound (a loosening; no caller breaks).
tokio's File buffers writes through its blocking pool, so a metadata() taken right after a successful write raced it and sometimes saw a stale length, making the write-bound tests flaky. Flush before every length snapshot the assertions compare against.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the data_bucket findings from the 2026-08-31 WorkTable audit, plus three same-family accounting bugs found during the sweep. Targets an 0.5.3 release (version bumped; publish is manual).
Fixes
seek_to_page_start_relativelymultipliedindex * PAGE_SIZEin u32, so batch persist/parse past 4 GiB wrote wrapped offsets onto the file head. All page offsets now route through one u64page_start_offset; the sweep also widened theoffset + lengthbound checks inupdate_at/get_atthat a wrapping link could slip past. Regression tests at the exact boundary indices plus a sparse-file round-trip with a page at ~4 GiB.TableOfContentsPage::update_keyleftestimated_sizeunaccounted (and re-keying onto an existing key double-counted). Accounting is now exact across grow/shrink/replace, verified by a churn test asserting serialized size equals estimated size at every step. Three same-family bugs fixed alongside:remove_without_recordover-counting one PageId, insert/remove using different alignment formulas (leaking 4 bytes per cycle on 8-aligned keys), and insert-replacing-a-key adding a full record.try_insertandtry_update_keyreject any mutation that would push the page past its slot, leaving the page untouched and returning a typedTableOfContentsOverflowError(withfits_empty_page()so callers can distinguish relocate-to-fresh-page from can-never-fit). No silent truncation is possible through these paths.PageOverflowErrorbefore writing a byte (persist_page_in_place, covering both single and batch persists) instead of overwriting the neighboring page.API notes
Additive except one bound:
update_keynow requiresT: SizeMeasurable(every usable key type already satisfies it). New exports: the two error types,EMPTY_TABLE_OF_CONTENTS_PAGE_SIZE, and thetry_*methods. Behavioral change by design: over-budget persists error instead of corrupting.Consumer follow-up
WorkTable's TableOfContents wrapper carries the oversized-entry fallback the audit attributed to this crate; it should adopt
try_insert/try_update_keyand the typed errors when it bumps to 0.5.3 so the guarantee reaches the observed defect.Verification
64/64 tests green, clippy
--all-targets --all-features -D warningsclean, fmt clean,cargo publish --dry-runpackages and verifies as 0.5.3.