fix: refuse to create a database over an existing sub-page file - #127
Merged
Conversation
`Chisel::open` decided "does this file already exist?" two different ways,
and they disagreed for every file length in 1..8191:
* the `create_if_missing` gate used `metadata(path).len() > 0`
* the create-vs-open decision used `io.page_count()? > 0`, which is
`len / stride` and therefore floors any sub-page file to zero
A file in that range passed the first test (so the gate let the call
through even with `create_if_missing: false`) and failed the second (so
`create_new` ran over it). `PageIo::open` uses `.create(true)
.truncate(false)`, so the file was adopted and its contents replaced with
superblock slots. A 71-byte text file came back as a 16384-byte database
with the original bytes gone.
Both halves are fixed by deciding after the lock, where the true length is
available: if the post-lock page count is zero, distinguish an empty file
(a legitimate create target) from a short non-empty one (somebody else's
data) and refuse the latter with CorruptSuperblock. The empty-file branch
also re-checks `create_if_missing`, which closes the narrow race where the
file is removed between the pre-lock stat and the lock.
Adds `PageIo::byte_len()` for the post-lock length, and a
`SuperblockDefect::TooShort` variant so the error says why. The enum is
already `#[non_exhaustive]`, so the new variant is additive downstream.
Zero-length files still go through the create path unchanged; a regression
test pins that boundary so this fix cannot tighten into refusing them.
🚦 Bench results: PR vs main
Per-scenario detail (4 metrics × cells)document-store
mutation-log
ycsb-a
ycsb-b
|
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
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 issue behind draft advisory GHSA-w3wp-8244-2jcf.
The bug
Chisel::openanswered "does this file already exist?" two different ways, and they disagree for every file length in1..8191:create_if_missinggate (src/lib.rs:366)metadata(path).len() > 0src/lib.rs:384)io.page_count()? > 0=len / stridecreate_newrunsPageIo::openuses.create(true).truncate(false), so the existing file is adopted andcreate_newwrites superblock slots over it. Two failures at once:create_if_missing: falsecreated a database, and an arbitrary user file under 8 KB was destroyed.Reproduced against this tree from a downstream crate:
Where it came from
This is a regression from
04534c0(the I143 fix). Before that commit a single predicate,file_exists, drove both decisions. I143 correctly moved the create-vs-open decision to a post-lockio.page_count()? > 0to close a create-vs-open lock race, but left thecreate_if_missinggate on the pre-lock length. The race fix was right; it just needed both call sites.The fix
Decide after the lock, where the true length is available. If the post-lock page count is zero, distinguish the two cases the old code conflated:
creat(2)and the first superblock write, or a baretouch). Unchanged behaviour, except that it now re-checkscreate_if_missing, closing the narrow race where the file is removed between the pre-lock stat and the lock.CorruptSuperblock, regardless ofcreate_if_missing.The pre-lock gate stays pre-lock, per I143's reasoning, so a refused open still does not materialize an empty file in the common case.
Supporting changes:
PageIo::byte_len()—page_count()islen / strideand cannot distinguish "empty" from "has bytes but not a whole page". Costs a seek, so it is documented as open-path only.SuperblockDefect::TooShort— so the error says why rather than overloadingBadMagic(there are no bytes to have magic). The enum is already#[non_exhaustive], so this is additive for downstream matches.Tests
Three added to
tests/api_edge_cases.rs, written before the fix and confirmed failing for the right reason (bothopen()calls returnedOk):open_refuses_to_create_over_a_sub_page_file— asserts the error and that the original bytes are untouchedopen_with_create_if_missing_false_does_not_create_over_a_sub_page_fileopen_still_creates_over_a_zero_length_file— pins the boundary so this fix cannot tighten into refusing empty files (this one passed before the fix, as it should)cargo test684 passed / 0 failed ·cargo clippy --workspace -- -D warningsclean ·cargo fmt --checkclean.Note for reviewers
Branched from
mainrather than the local development lineage, which shares no ancestry with it. The six files involved are byte-identical across the two, so the change applies cleanly either way — but it will need porting to the dev lineage separately.Found by the clean-slate deep review of 2026-07-29 (finding
PUBLIC-API-1).