Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 41 additions & 3 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -329,21 +329,28 @@ pub struct Chisel {
impl Chisel {
/// Open or create a Chisel database at `path`.
///
/// The "exists" check deliberately treats a zero-length file as
/// The "exists" check deliberately treats a **zero-length** file as
/// nonexistent: a freshly-created-but-unwritten file (e.g. from a crash
/// between `creat(2)` and the first superblock write, or from a user
/// `touch`) has no valid superblock and must go through the
/// `create_new` path. Without this, `open_existing` would try to parse
/// an empty file and fail with a corruption error.
///
/// A file that is non-empty but shorter than one page is *not* treated
/// as nonexistent. It cannot be a valid database, but it is somebody's
/// data, so `open` refuses with `CorruptSuperblock` rather than creating
/// over it — regardless of `create_if_missing`.
///
/// Acquires an exclusive `flock` on the file before any parsing, so a
/// second concurrent `open()` on the same path fails fast with
/// `LockFailed` rather than racing on the superblock.
///
/// # Errors
/// `InvalidSuperblockCount` (the `superblock_count` option is out of
/// range), `FileNotFound` (no file at `path` and `create_if_missing` is
/// false), or `LockFailed` (another handle holds the exclusive flock).
/// false), `CorruptSuperblock` (the file has content but is shorter than
/// one page, so it cannot be a database and must not be created over),
/// or `LockFailed` (another handle holds the exclusive flock).
/// For an encrypted database: `NoEncryptionKey` (file is encrypted but
/// no `encryption_key` given), `InvalidEncryptionKey` (key unwraps no
/// key slot), or `EncryptionNotSupported` (key given for a plaintext
Expand Down Expand Up @@ -372,7 +379,7 @@ impl Chisel {
return Err(ChiselError::FileNotFound);
}

let io = PageIo::open(path, options.read_only)?;
let mut 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
Expand All @@ -382,6 +389,37 @@ impl Chisel {
// 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;
if !existed {
// `existed` is `len / stride > 0`, so it is false for BOTH an empty
// file and a file that has content but is shorter than one page.
// Those two need opposite answers, and only a post-lock length can
// tell them apart — the `file_exists` stat above is length-based
// but must stay pre-lock, so it cannot serve here.
//
// Creating over a short non-empty file destroys its contents
// irrecoverably, and nothing shorter than a page can be a valid
// database, so refuse. A mistyped path that lands on a small user
// file is the motivating case.
let len = io.byte_len()?;
if len > 0 {
return Err(ChiselError::CorruptSuperblock {
defects: vec![SlotDefect {
slot: 0,
defect: SuperblockDefect::TooShort,
}],
});
}
// Empty file. The pre-lock gate already refused this when
// `create_if_missing` is false, so reaching here with it false
// means the file was removed between that stat and our lock —
// honour the option rather than creating anyway. (Our own
// `.create(true)` has materialized an empty file by now; that is
// the pre-existing cost of deciding after the lock, and it is
// strictly better than adopting a file we were told not to create.)
if !options.create_if_missing {
return Err(ChiselError::FileNotFound);
}
}
let cache = PageCache::new(
io,
options.cache_max_bytes,
Expand Down
15 changes: 15 additions & 0 deletions src/page_io.rs
Original file line number Diff line number Diff line change
Expand Up @@ -522,6 +522,21 @@ impl PageIo {
Ok(self.cached_page_count.get())
}

/// Byte length of the backing store, measured now.
///
/// `page_count()` is `len / stride`, so it floors every file shorter than
/// one stride-unit to zero and cannot distinguish "empty" from "has bytes
/// but not a whole page". `Chisel::open` needs exactly that distinction:
/// an empty file is a legitimate create target, a short non-empty one is
/// somebody else's data. Unlike `page_count()` this costs a seek, so it is
/// for the open path only — not for anything on a hot path.
pub fn byte_len(&mut self) -> Result<u64> {
match &mut self.backing {
Backing::File { file } => Ok(file.seek(SeekFrom::End(0))?),
Backing::Memory { bytes } => Ok(bytes.len() as u64),
}
}

/// Truncate (or extend) the file to exactly `n` stride-units (pages).
///
/// File length is `n * stride` bytes. Used by defrag/truncate paths.
Expand Down
8 changes: 8 additions & 0 deletions src/superblock/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,13 @@ pub enum SuperblockDefect {
BadChecksum,
BadMagic,
BadCount(u32), // the out-of-range superblock_count value
// The file is too short to contain this slot at all. Distinct from
// BadMagic: there are no bytes to have magic, so the slot was never
// read. Raised by `Chisel::open` for a file that has content but is
// shorter than one page — `page_count()` floors such a file to zero
// pages, and without a distinct defect the create path could not tell
// it apart from an absent file.
TooShort,
}

impl fmt::Display for SuperblockDefect {
Expand All @@ -148,6 +155,7 @@ impl fmt::Display for SuperblockDefect {
SuperblockDefect::BadChecksum => write!(f, "bad checksum"),
SuperblockDefect::BadMagic => write!(f, "bad magic"),
SuperblockDefect::BadCount(n) => write!(f, "bad superblock_count {n}"),
SuperblockDefect::TooShort => write!(f, "file too short to contain this slot"),
}
}
}
Expand Down
69 changes: 69 additions & 0 deletions tests/api_edge_cases.rs
Original file line number Diff line number Diff line change
Expand Up @@ -536,3 +536,72 @@ dual_backing_test!(
test_file_size_bytes_matches_stats,
test_file_size_bytes_matches_stats_body
);

// --- Refusing to create a database over a file that already has content ---
//
// `PageIo::open` uses `.create(true).truncate(false)`, so `Chisel::open` will
// happily adopt whatever file is at `path`. The create-vs-open decision is made
// on the post-lock page count, which is `len / stride` and therefore 0 for ANY
// file shorter than one page. Nothing below a page is a valid database, so the
// only safe answer for a short-but-non-empty file is to refuse: creating over
// it destroys the user's bytes irrecoverably.

/// A file too short to hold a superblock must not be adopted and overwritten.
#[test]
fn open_refuses_to_create_over_a_sub_page_file() {
let tmp = NamedTempFile::new().unwrap();
let original: &[u8] = b"IMPORTANT USER DATA - not a chisel database\n";
std::fs::write(tmp.path(), original).unwrap();

let err = Chisel::open(tmp.path(), Options::default())
.err()
.expect("opening a non-empty sub-page file must not succeed");
assert!(
matches!(err, ChiselError::CorruptSuperblock { .. }),
"expected CorruptSuperblock for a short non-database file, got {err:?}"
);
assert_eq!(
std::fs::read(tmp.path()).unwrap(),
original,
"the existing file's contents must be left untouched"
);
}

/// The same file with `create_if_missing(false)`: the option must be honoured.
/// Before the fix this returned `Ok` — the length-based pre-lock gate saw a
/// non-empty file and let the call through, then the page-count-based decision
/// treated it as absent and created over it.
#[test]
fn open_with_create_if_missing_false_does_not_create_over_a_sub_page_file() {
let tmp = NamedTempFile::new().unwrap();
let original: &[u8] = b"IMPORTANT USER DATA - not a chisel database\n";
std::fs::write(tmp.path(), original).unwrap();

let err = Chisel::open(tmp.path(), Options::default().create_if_missing(false))
.err()
.expect("create_if_missing(false) must never create a database");
assert!(
matches!(err, ChiselError::CorruptSuperblock { .. }),
"expected CorruptSuperblock, got {err:?}"
);
assert_eq!(
std::fs::read(tmp.path()).unwrap(),
original,
"the existing file's contents must be left untouched"
);
}

/// A zero-length file is still legitimately "nonexistent" — a crash between
/// `creat(2)` and the first superblock write, or a bare `touch`, must keep
/// going through the create path. This pins the boundary so the fix above
/// cannot regress it into refusing empty files.
#[test]
fn open_still_creates_over_a_zero_length_file() {
let tmp = NamedTempFile::new().unwrap();
assert_eq!(std::fs::metadata(tmp.path()).unwrap().len(), 0);

let db = Chisel::open(tmp.path(), Options::default())
.expect("a zero-length file must go through the create path");
drop(db);
assert!(std::fs::metadata(tmp.path()).unwrap().len() > 0);
}