diff --git a/src/lib.rs b/src/lib.rs index 13d86bb..8484566 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -329,13 +329,18 @@ 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. @@ -343,7 +348,9 @@ impl Chisel { /// # 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 @@ -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 @@ -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, diff --git a/src/page_io.rs b/src/page_io.rs index bdb4650..618f6c9 100644 --- a/src/page_io.rs +++ b/src/page_io.rs @@ -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 { + 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. diff --git a/src/superblock/mod.rs b/src/superblock/mod.rs index b1b3362..15045db 100644 --- a/src/superblock/mod.rs +++ b/src/superblock/mod.rs @@ -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 { @@ -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"), } } } diff --git a/tests/api_edge_cases.rs b/tests/api_edge_cases.rs index 19e4380..4a97289 100644 --- a/tests/api_edge_cases.rs +++ b/tests/api_edge_cases.rs @@ -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); +}