diff --git a/crates/levcs-store/src/engine.rs b/crates/levcs-store/src/engine.rs index 66a78ff..6afe065 100644 --- a/crates/levcs-store/src/engine.rs +++ b/crates/levcs-store/src/engine.rs @@ -827,7 +827,9 @@ fn classify_root(layout: &RootLayout) -> Result { // disqualify the root, and a wildcard here is the one edit that would // silently stop it doing so. The type is part of the name's meaning — // a symlink called `FORMAT.tmp` is not residue, it is an instruction to - // write outside the root, and `segment::write_fenced` would follow it. + // write outside the root. `segment::write_fenced` refuses one directly + // now as well; this list is why the root is disqualified rather than + // initialized-then-refused, which is a different and better answer. if !matches!( (name.to_str(), kind.is_file(), kind.is_dir()), (Some("FORMAT.tmp"), true, _) @@ -1150,13 +1152,15 @@ fn build_root_under_lock( /// # Standing disclosure /// /// This began as a second copy of `segment::write_fenced`, which is private to -/// A1's file, and the duplication was filed as an interface request. The two -/// are no longer the same function: this one cannot truncate and cannot follow -/// a symlink, and `segment::write_fenced` still can. The request therefore -/// changes shape rather than going away — if one of them is hoisted, it must be -/// this one, and `FORMAT.tmp` should be opened through it. That is an interface -/// request against a frozen file and is recorded in this deliverable's report, -/// not made here. +/// A1's file, and the duplication was filed as an interface request. Neither can +/// follow a symlink any more — contract review 2026-07-29-B routed +/// `write_fenced` through the same funnel — so what remains is a real difference +/// in refusal rule rather than in safety: this one refuses an existing regular +/// file, because a marker that already exists is somebody's data and adopting it +/// would be a decision about content; `write_fenced` adopts and empties one, +/// because a `.tmp` name is residue from an interrupted attempt at that exact +/// write and reusing it is how the retry works. Both are correct for their name. +/// Any hoisting therefore has to keep two rules, not pick one. fn install_initializing_marker( tmp: &std::path::Path, bytes: &[u8], @@ -3783,18 +3787,18 @@ mod tests { /// The same hazard at the other two names this file opens or ignores. /// /// `segment::lock_root` opens `LOCK` and `segment::write_fenced` opens - /// `FORMAT.tmp` with `create` + `truncate`. Both are in A1's frozen file, so - /// neither open was changed by this deliverable; what it changed is that a - /// root carrying a symlink at either name never reaches them, because the - /// classification that authorizes the write refuses it first. + /// `FORMAT.tmp`. Both are in A1's frozen file, so neither open was changed by + /// this deliverable; what it changed is that a root carrying a symlink at + /// either name never reaches them, because the classification that authorizes + /// the write refuses it first. /// - /// `lock_root` has since been amended by the lead (contract review - /// 2026-07-29-A) to open through the no-follow funnel, because - /// `RecoverySession::open` reaches it without any classification at all — - /// this test therefore no longer carries the `LOCK` guarantee alone, and the - /// assertion below it in `segment.rs` is the load-bearing one. `FORMAT.tmp` - /// is still a follow-through open for any caller that reaches - /// `initialize_root` by another route, and remains an open interface request. + /// Both have since been amended by the lead — `lock_root` in contract review + /// 2026-07-29-A, `write_fenced` in 2026-07-29-B — because + /// `RecoverySession::open`, `drive.rs` and `store-bench` all reach them with + /// no classification at all. This test therefore no longer carries either + /// guarantee on its own; it asserts that the classifier refuses first, which + /// is still the answer an operator wants, and the assertions in `segment.rs` + /// are what hold the opens themselves. #[test] fn a_symlink_at_lock_or_format_tmp_is_refused_before_anything_opens_it() { let serial = writer_serial(); diff --git a/crates/levcs-store/src/segment.rs b/crates/levcs-store/src/segment.rs index 9cd8212..43d482d 100644 --- a/crates/levcs-store/src/segment.rs +++ b/crates/levcs-store/src/segment.rs @@ -117,6 +117,53 @@ pub fn segment_filename(generation: u64, first: u64, last: u64) -> String { // Root initialization // --------------------------------------------------------------------------- +/// `quarantine/`, `staging/`, `shards/` — the store-invented entries directly +/// beneath a root, in the order [`planned_directories`] lists them. `opened[0]` +/// and `opened[2]` in [`initialize_root`] are `quarantine/` and `shards/`. +const STORE_INVENTED_ROOT_ENTRIES: usize = 3; + +/// `/` plus `active`, `segments`, `indexes`, `checkpoints`, `manifests`. +const DIRS_PER_SHARD: usize = 6; + +/// Every directory [`initialize_root`] invents beneath the root, parents first. +/// +/// Separated from the creation loop because the refusal rule needs the whole set +/// before any of it is created, and because the fencing below indexes into it +/// positionally. +fn planned_directories(layout: &RootLayout, shard_count: u16) -> Vec { + let mut planned = + Vec::with_capacity(STORE_INVENTED_ROOT_ENTRIES + shard_count as usize * DIRS_PER_SHARD); + planned.push(layout.quarantine_dir()); + planned.push(layout.staging_dir()); + planned.push(layout.shards_dir()); + debug_assert_eq!(planned.len(), STORE_INVENTED_ROOT_ENTRIES); + for shard in 0..shard_count { + let paths = layout.shard(shard); + let before = planned.len(); + planned.push(paths.dir.clone()); + planned.push(paths.active()); + planned.push(paths.segments()); + planned.push(paths.indexes()); + planned.push(paths.checkpoints()); + planned.push(paths.manifests()); + debug_assert_eq!(planned.len() - before, DIRS_PER_SHARD); + } + planned +} + +/// A name the store must own as a directory, occupied by something else. +/// +/// [`StoreError::UnrecognizedLayout`] rather than an `Io`: the store is being +/// asked to build its tree through an object it did not create, which is the +/// same judgement as any other unrecognized occupant of a root, and the same +/// answer — refuse, modify nothing. +fn not_a_directory(path: &Path) -> StoreError { + StoreError::UnrecognizedLayout(format!( + "{} is not a directory; refusing to build the store tree through it", + path.display() + )) +} + /// Create the v2 tree and write `FORMAT`, fsyncing every new directory and the /// root's parent (scope 3.1 startup state 1). /// @@ -136,25 +183,49 @@ pub fn initialize_root( "shard_count must be nonzero".into(), )); } + // The root itself, and everything above it, is the caller's path. Resolving + // it is explicitly out of scope (§3.1): an operator who configures a root + // behind a symlink has said where the store goes. std::fs::create_dir_all(&layout.root)?; - std::fs::create_dir_all(layout.quarantine_dir())?; - std::fs::create_dir_all(layout.staging_dir())?; - std::fs::create_dir_all(layout.shards_dir())?; - for shard in 0..shard_count { - let paths = layout.shard(shard); - for dir in [ - paths.dir.clone(), - paths.active(), - paths.segments(), - paths.indexes(), - paths.checkpoints(), - paths.manifests(), - ] { - std::fs::create_dir_all(&dir)?; - sys::fsync_dir(&dir, counters)?; + // Everything below is a name *the store invents*, and none of it may be + // reached through a link. Two passes, because a refusal must not leave the + // tree half-extended: pass one classifies every planned directory and holds + // a descriptor onto each that already exists, pass two creates the rest. + // Ordered parents-before-children so pass two never creates through a name + // pass one did not see. + let planned = planned_directories(layout, shard_count); + let mut found = Vec::with_capacity(planned.len()); + for directory in &planned { + match sys::open_directory_nofollow(directory)? { + sys::NamedDirectory::Opened(handle) => found.push(Some(handle)), + sys::NamedDirectory::Absent => found.push(None), + sys::NamedDirectory::NotDirectory => return Err(not_a_directory(directory)), } - sys::fsync_dir(&paths.dir, counters)?; + } + let mut opened = Vec::with_capacity(planned.len()); + for (directory, existing) in planned.iter().zip(found) { + opened.push(match existing { + Some(handle) => handle, + None => sys::create_directory_nofollow(directory)? + .ok_or_else(|| not_a_directory(directory))?, + }); + } + + // Fenced through the descriptors just established, not by reopening their + // names: a second lookup would hand the fence to whatever the name resolves + // to now rather than to the directory that was validated. + // + // The shape of this sequence is load-bearing and unchanged — six directories + // per shard plus the shard directory again, which `engine.rs` asserts as an + // exact `fsync_dir` count. + for shard in 0..shard_count as usize { + let shard_directories = + &opened[STORE_INVENTED_ROOT_ENTRIES + shard * DIRS_PER_SHARD..][..DIRS_PER_SHARD]; + for directory in shard_directories { + sys::fsync_dir_fd(directory, counters)?; + } + sys::fsync_dir_fd(&shard_directories[0], counters)?; } let marker = crate::format::FormatMarker { @@ -169,8 +240,8 @@ pub fn initialize_root( write_fenced(&tmp, &bytes, counters)?; sys::rename_noreplace(&tmp, &layout.format_path())?; - sys::fsync_dir(&layout.shards_dir(), counters)?; - sys::fsync_dir(&layout.quarantine_dir(), counters)?; + sys::fsync_dir_fd(&opened[2], counters)?; + sys::fsync_dir_fd(&opened[0], counters)?; sys::fsync_dir(&layout.root, counters)?; if let Some(parent) = layout.root.parent() { sys::fsync_dir(parent, counters)?; @@ -179,8 +250,38 @@ pub fn initialize_root( } /// Read and validate `FORMAT`. +/// +/// Three answers, kept distinct because callers act on them differently: +/// +/// - **Absent** — `Io(NotFound)`, exactly as `File::open` reported it. An absent +/// `FORMAT` is startup state 1 or 3, not a refusal, and `classify_root` and +/// `drive.rs` both read it that way. +/// - **Not a regular file** — [`StoreError::UnrecognizedLayout`]. A symlink here +/// is the read-only member of the redirection family: following it would let a +/// `FORMAT` outside the root decide this root's `shard_count` and `root_uuid`, +/// and every file in the tree is then validated against a marker the store +/// never wrote. Nothing is destroyed, which is why it was ranked lowest of the +/// four, and it is still an authority the operator did not grant. +/// - **A regular file that does not decode** — unchanged. `FormatMismatch` or a +/// decode error, which is a corrupt marker and a different finding from a +/// redirected one. pub fn read_format(layout: &RootLayout) -> Result { - let file = File::open(layout.format_path())?; + let path = layout.format_path(); + let file = match sys::open_regular_nofollow_classified(&path)? { + sys::NamedRegularFile::Opened(file) => file, + sys::NamedRegularFile::Absent => { + return Err(StoreError::Io(Arc::new(std::io::Error::new( + std::io::ErrorKind::NotFound, + format!("{} not found", path.display()), + )))) + } + sys::NamedRegularFile::NotRegular => { + return Err(StoreError::UnrecognizedLayout(format!( + "{} is not a regular file; refusing to read a format marker through it", + path.display() + ))) + } + }; let mut bytes = [0u8; FORMAT_MARKER_LEN]; sys::pread_exact(&file, 0, &mut bytes)?; Ok(crate::format::FormatMarker::decode(&bytes)?) @@ -296,16 +397,34 @@ pub fn lock_root(layout: &RootLayout) -> Result { /// Write a file and fence it. The name is not yet durable; the caller renames /// and syncs the directory. +/// +/// # Why the open is not `create(true).truncate(true)` +/// +/// Every caller passes a `.tmp` name inside the root — `FORMAT.tmp`, +/// `.manifest.tmp`, `CURRENT.tmp` — and an `O_TRUNC` open destroys +/// whatever the name holds before anything can look at it. Through a symlink it +/// destroys a file *outside* the root: measured at `INITIALIZING.tmp` earlier in +/// this wave, a 4096-byte file went to 27 bytes with the link removed and the +/// store reporting success. These names are as reachable to an operator as that +/// one was. +/// +/// So the open and the truncation are separated: no flag combination truncates +/// only regular files, which is why the type check needs the descriptor first. +/// An existing regular file is **kept and emptied**, not refused — it is residue +/// from an interrupted attempt at this exact write, and reusing the name is how +/// the retry works. Anything else at the name is +/// [`StoreError::UnrecognizedLayout`]. fn write_fenced( path: &Path, bytes: &[u8], counters: &DurabilityCounters, ) -> Result<(), StoreError> { - let mut file = File::options() - .create(true) - .write(true) - .truncate(true) - .open(path)?; + let Some(mut file) = sys::open_or_create_regular_truncated_nofollow(path, counters)? else { + return Err(StoreError::UnrecognizedLayout(format!( + "{} is not a regular file; refusing to write a store metadata file through it", + path.display() + ))); + }; // Through the funnel, so the counters see every durable byte. sys::write_vectored_all(&mut file, &[IoSlice::new(bytes)], counters)?; sys::fdatasync(&file, counters)?; @@ -1337,3 +1456,395 @@ mod root_lock_tests { ); } } + +// --------------------------------------------------------------------------- +// Path-redirection tests (contract review 2026-07-29-B) +// --------------------------------------------------------------------------- + +/// The three remaining redirection hazards, each exercised at the `segment` +/// entry point that carries it. +/// +/// Deliberately **not** through `StoreEngine::open`. Its classifier refuses a +/// redirected root before these are reached, so a test that went in that way +/// would pass whether or not the protection here exists — and `RecoverySession`, +/// `drive.rs` and `store-bench` all arrive here without it. Coverage of the +/// caller is not coverage of the callee. +#[cfg(test)] +mod path_redirection_tests { + use super::*; + use std::os::unix::fs::symlink; + + fn counters() -> DurabilityCounters { + DurabilityCounters::default() + } + + /// Every name in a tree with its bytes, for "unchanged" assertions. + fn tree_image(root: &Path) -> Vec<(PathBuf, Option>)> { + let mut out = Vec::new(); + let mut stack = vec![root.to_path_buf()]; + while let Some(current) = stack.pop() { + let Ok(entries) = std::fs::read_dir(¤t) else { + continue; + }; + for entry in entries.flatten() { + let path = entry.path(); + let relative = path + .strip_prefix(root) + .expect("inside the tree") + .to_path_buf(); + let kind = entry.file_type().expect("file type"); + if kind.is_dir() { + stack.push(path); + out.push((relative, None)); + } else { + out.push((relative, std::fs::read(&path).ok())); + } + } + } + out.sort(); + out + } + + fn mkfifo_at(path: &Path) { + let name = std::ffi::CString::new(path.as_os_str().as_encoded_bytes()) + .expect("a path with no interior NUL"); + // SAFETY: `name` is a valid NUL-terminated path for the call's duration. + assert_eq!( + unsafe { libc::mkfifo(name.as_ptr(), 0o644) }, + 0, + "mkfifo: {}", + std::io::Error::last_os_error() + ); + } + + /// Run `body` on another thread and fail if it does not finish. + /// + /// A blocking `open(2)` is not a slow refusal, it is an unbounded startup + /// hang: one `mkfifo` in a configured root, reachable by any operator. The + /// only assertion that distinguishes the two is a deadline. + fn must_not_block( + what: &str, + body: impl FnOnce() -> T + Send + 'static, + ) -> T { + let (sender, receiver) = std::sync::mpsc::channel(); + std::thread::spawn(move || { + let _ = sender.send(body()); + }); + match receiver.recv_timeout(std::time::Duration::from_secs(10)) { + Ok(value) => value, + Err(_) => panic!( + "{what} did not return within ten seconds: the open blocked. A fifo at a store \ + metadata name must be refused, not waited on." + ), + } + } + + // ----------------------------------------------------------------------- + // write_fenced + // ----------------------------------------------------------------------- + + /// A live link at a temporary name must not reach its target at all. + #[test] + fn a_linked_temporary_name_cannot_alter_the_file_it_points_at() { + let outside = tempfile::tempdir().expect("tempdir"); + let victim = outside.path().join("ledger"); + std::fs::write(&victim, b"balances\n").expect("the file outside the root"); + let root = tempfile::tempdir().expect("temp root"); + let tmp = root.path().join("FORMAT.tmp"); + symlink(&victim, &tmp).expect("plant the link"); + + let outcome = write_fenced(&tmp, b"a store metadata marker", &counters()); + + // The damage first, so a regression reports what was destroyed rather + // than which error type came back. + assert_eq!( + std::fs::read(&victim).expect("the target still exists"), + b"balances\n", + "a fenced write through a symlink truncated and rewrote a file outside the root" + ); + assert!( + std::fs::symlink_metadata(&tmp).is_ok(), + "the link itself must be left alone for the operator to find" + ); + match outcome { + Err(StoreError::UnrecognizedLayout(reason)) => { + assert!(reason.contains("FORMAT.tmp"), "{reason}") + } + other => panic!("expected UnrecognizedLayout, got {other:?}"), + } + } + + /// The dangling form, which creates rather than destroys. + #[test] + fn a_dangling_link_at_a_temporary_name_creates_nothing() { + let outside = tempfile::tempdir().expect("tempdir"); + let absent = outside.path().join("not-there"); + let root = tempfile::tempdir().expect("temp root"); + let tmp = root.path().join("FORMAT.tmp"); + symlink(&absent, &tmp).expect("plant the link"); + + let outcome = write_fenced(&tmp, b"a store metadata marker", &counters()); + + assert!( + !absent.exists(), + "a fenced write through a dangling symlink created a file outside the root" + ); + match outcome { + Err(StoreError::UnrecognizedLayout(_)) => {} + other => panic!("expected UnrecognizedLayout, got {other:?}"), + } + } + + /// Every non-regular occupant, and the one occupant that is legitimate. + #[test] + fn a_non_regular_temporary_name_is_refused_and_a_regular_one_is_retried() { + let mut wrong = Vec::new(); + for (label, occupy) in [ + ( + "directory", + (|path: &Path| std::fs::create_dir(path).expect("mkdir")) as fn(&Path), + ), + ("unix socket", |path: &Path| { + std::os::unix::net::UnixListener::bind(path).expect("bind"); + }), + ("fifo", mkfifo_at), + ] { + let root = tempfile::tempdir().expect("temp root"); + let tmp = root.path().join("FORMAT.tmp"); + occupy(&tmp); + let attempt = tmp.clone(); + let outcome = must_not_block(label, move || { + write_fenced( + &attempt, + b"a store metadata marker", + &DurabilityCounters::default(), + ) + }); + match outcome { + Err(StoreError::UnrecognizedLayout(_)) => {} + other => wrong.push(format!("{label}: {other:?}")), + } + } + assert!( + wrong.is_empty(), + "a non-regular temporary name must be UnrecognizedLayout, not written through and \ + not an untyped errno: {wrong:?}" + ); + + // Residue from an interrupted attempt at this same write. Retrying is the + // point of a `.tmp` name, so it is adopted — and emptied, which the + // stale tail proves: it is longer than the new contents, so a missing + // truncation leaves it visible. + let root = tempfile::tempdir().expect("temp root"); + let tmp = root.path().join("FORMAT.tmp"); + std::fs::write( + &tmp, + b"residue from an interrupted attempt, longer than what follows", + ) + .expect("stale residue"); + write_fenced(&tmp, b"the retry", &counters()).expect("an existing regular temp is retried"); + assert_eq!( + std::fs::read(&tmp).expect("read the temp"), + b"the retry", + "a retried fenced write must leave exactly the new bytes" + ); + } + + // ----------------------------------------------------------------------- + // read_format + // ----------------------------------------------------------------------- + + /// A valid marker at the far end of a link is still not this root's marker. + /// + /// The target holds a real, decodable `FORMAT` — built by `initialize_root`, + /// not hand-rolled — so the refusal cannot be mistaken for the bytes being + /// rejected. Read as a regular file the same bytes are accepted, which the + /// second half asserts: without it this test would pass against an + /// implementation that had simply broken `read_format`. + #[test] + fn a_linked_format_marker_is_never_read_even_when_it_is_valid() { + let donor = tempfile::tempdir().expect("temp root"); + let donor_layout = RootLayout::new(donor.path()); + initialize_root(&donor_layout, 1, [0x5A; 16], 1, &counters()).expect("a real root"); + let valid = std::fs::read(donor_layout.format_path()).expect("real FORMAT bytes"); + + let outside = tempfile::tempdir().expect("tempdir"); + let target = outside.path().join("someone-elses-FORMAT"); + std::fs::write(&target, &valid).expect("a valid marker outside the root"); + + let root = tempfile::tempdir().expect("temp root"); + let layout = RootLayout::new(root.path()); + symlink(&target, layout.format_path()).expect("plant the link"); + match read_format(&layout) { + Err(StoreError::UnrecognizedLayout(reason)) => { + assert!(reason.contains("FORMAT"), "{reason}") + } + Ok(marker) => panic!( + "a FORMAT symlink was followed: this root's shard_count and root_uuid would come \ + from a marker outside it ({marker:?}), and every file in the tree would then be \ + validated against a marker the store never wrote" + ), + Err(other) => panic!("expected UnrecognizedLayout, got {other:?}"), + } + + // The same bytes, as a regular file, are read. + std::fs::remove_file(layout.format_path()).expect("remove the link"); + std::fs::write(layout.format_path(), &valid).expect("the same bytes, directly"); + read_format(&layout).expect("a regular FORMAT holding valid bytes must still be read"); + } + + /// The other two answers, which are not refusals and must not become ones. + #[test] + fn an_absent_format_is_not_found_and_a_corrupt_one_still_fails_to_decode() { + let root = tempfile::tempdir().expect("temp root"); + let layout = RootLayout::new(root.path()); + match read_format(&layout) { + Err(StoreError::Io(error)) => assert_eq!( + error.kind(), + std::io::ErrorKind::NotFound, + "an absent FORMAT is startup state 1 or 3 and must keep reporting NotFound" + ), + other => panic!("expected Io(NotFound), got {other:?}"), + } + + std::fs::write(layout.format_path(), [0xAB; FORMAT_MARKER_LEN]).expect("garbage"); + match read_format(&layout) { + Err(StoreError::UnrecognizedLayout(reason)) => panic!( + "a corrupt *regular* marker is a different finding from a redirected one and \ + must keep its decode error: {reason}" + ), + Err(_) => {} + Ok(marker) => panic!("garbage decoded as a marker: {marker:?}"), + } + } + + /// A fifo at `FORMAT` is the read-side hang. `open(2)` read-only on one + /// blocks until a writer arrives, so this is the case `O_NONBLOCK` in the + /// funnel exists for. + #[test] + fn a_fifo_at_format_is_refused_rather_than_waited_on() { + let root = tempfile::tempdir().expect("temp root"); + let layout = RootLayout::new(root.path()); + mkfifo_at(&layout.format_path()); + let probed = RootLayout::new(root.path().to_path_buf()); + match must_not_block("read_format on a fifo", move || read_format(&probed)) { + Err(StoreError::UnrecognizedLayout(_)) => {} + other => panic!("expected UnrecognizedLayout, got {other:?}"), + } + } + + // ----------------------------------------------------------------------- + // initialize_root + // ----------------------------------------------------------------------- + + /// A link where the store expects to invent a directory. + /// + /// Both positions: `shards/` itself, and one subdirectory of one shard. In + /// each case `create_dir_all` would have followed the link and built the + /// store's tree at the far end of it. + #[test] + fn a_linked_directory_name_creates_nothing_outside_the_root() { + for position in ["shards", "shards/00/segments"] { + let outside = tempfile::tempdir().expect("tempdir"); + let elsewhere = outside.path().join("elsewhere"); + std::fs::create_dir(&elsewhere).expect("a directory outside the root"); + + let root = tempfile::tempdir().expect("temp root"); + let layout = RootLayout::new(root.path()); + let planted = root.path().join(position); + if let Some(parent) = planted.parent() { + std::fs::create_dir_all(parent).expect("the parent the link sits in"); + } + symlink(&elsewhere, &planted).expect("plant the link"); + + let outcome = initialize_root(&layout, 2, [0x11; 16], 1, &counters()); + + // The damage first: a regression here is the tree being built + // somewhere it must never be, not the error type. + assert_eq!( + tree_image(&elsewhere), + Vec::new(), + "initialize_root built part of the store tree outside the root, through {position}" + ); + match outcome { + Err(StoreError::UnrecognizedLayout(reason)) => { + assert!( + reason.contains(position.rsplit('/').next().expect("name")), + "{reason}" + ) + } + other => { + panic!("expected UnrecognizedLayout for a link at {position}, got {other:?}") + } + } + } + } + + /// The preflight, which is what stops a refusal from half-extending a tree. + /// + /// The link sits at `shards/00/segments`, and the two names created *before* + /// it in creation order are `shards/00` (which exists already) and + /// `shards/00/active` (which does not). A single pass that created as it went + /// would leave `active` behind; the assertion is that it does not exist, and + /// that the second shard was never begun. + #[test] + fn a_refusal_does_not_extend_the_tree_it_refused() { + let outside = tempfile::tempdir().expect("tempdir"); + let elsewhere = outside.path().join("elsewhere"); + std::fs::create_dir(&elsewhere).expect("a directory outside the root"); + + let root = tempfile::tempdir().expect("temp root"); + let layout = RootLayout::new(root.path()); + // A partially built tree, as an interrupted initialization leaves. + std::fs::create_dir_all(layout.quarantine_dir()).expect("quarantine"); + std::fs::write(layout.quarantine_dir().join("keep"), b"forensics\n").expect("residue"); + std::fs::create_dir_all(layout.shard(0).dir).expect("the first shard directory"); + symlink(&elsewhere, layout.shard(0).segments()).expect("plant the link"); + + let before = tree_image(root.path()); + match initialize_root(&layout, 2, [0x22; 16], 1, &counters()) { + Err(StoreError::UnrecognizedLayout(_)) => {} + other => panic!("expected UnrecognizedLayout, got {other:?}"), + } + + assert!( + !layout.shard(0).active().exists(), + "the refusal created shards/00/active on its way to the link: every existing entry \ + must be classified before any missing one is created" + ); + assert!( + !layout.shard(1).dir.exists(), + "the refusal began a second shard" + ); + assert_eq!( + tree_image(root.path()), + before, + "a refused initialization must leave the tree it found byte-identical" + ); + } + + /// A non-directory occupant, and the legitimate case beside it: an existing + /// partial tree is adopted rather than refused, because that is what resuming + /// an interrupted initialization requires. + #[test] + fn a_non_directory_occupant_is_refused_and_a_real_partial_tree_is_adopted() { + let root = tempfile::tempdir().expect("temp root"); + let layout = RootLayout::new(root.path()); + std::fs::create_dir_all(&layout.root).expect("root"); + std::fs::write(layout.shards_dir(), b"not a directory\n").expect("occupy shards"); + match initialize_root(&layout, 1, [0x33; 16], 1, &counters()) { + Err(StoreError::UnrecognizedLayout(_)) => {} + other => panic!("expected UnrecognizedLayout for a file at shards/, got {other:?}"), + } + + let resumable = tempfile::tempdir().expect("temp root"); + let layout = RootLayout::new(resumable.path()); + std::fs::create_dir_all(layout.shard(0).active()).expect("a partial tree"); + std::fs::create_dir_all(layout.staging_dir()).expect("staging"); + let marker = initialize_root(&layout, 1, [0x44; 16], 1, &counters()) + .expect("an existing partial tree is adopted, not refused"); + assert_eq!(marker.shard_count, 1); + assert!(layout.shard(0).manifests().is_dir()); + read_format(&layout).expect("the marker is installed over an adopted tree"); + } +} diff --git a/crates/levcs-store/src/sys.rs b/crates/levcs-store/src/sys.rs index 41e4d0e..ef83db2 100644 --- a/crates/levcs-store/src/sys.rs +++ b/crates/levcs-store/src/sys.rs @@ -283,6 +283,12 @@ pub(crate) fn fdatasync(file: &File, counters: &DurabilityCounters) -> io::Resul /// Make a directory entry durable. Required after every create, rename, and /// unlink that recovery depends on. +/// +/// Resolves the name, so it is for directories whose path the store already +/// trusts. A caller that has just *established* what a name is holds a +/// descriptor onto that object and should fence it with [`fsync_dir_fd`] +/// instead — re-resolving the name would hand the fence back to whatever the +/// name resolves to now, which is not necessarily what was validated. pub(crate) fn fsync_dir(path: &Path, counters: &DurabilityCounters) -> io::Result<()> { if take_fault_if(|f| matches!(f, Fault::DirSyncEio)).is_some() { counters.fsync_dir.fetch_add(1, Relaxed); @@ -292,6 +298,19 @@ pub(crate) fn fsync_dir(path: &Path, counters: &DurabilityCounters) -> io::Resul File::open(path)?.sync_all() } +/// [`fsync_dir`] on a directory descriptor the caller already holds. +/// +/// Same counter and same injected fault, so the crash matrix reaches this seam +/// exactly as it reaches the by-name one. +pub(crate) fn fsync_dir_fd(directory: &File, counters: &DurabilityCounters) -> io::Result<()> { + if take_fault_if(|f| matches!(f, Fault::DirSyncEio)).is_some() { + counters.fsync_dir.fetch_add(1, Relaxed); + return Err(eio("directory fsync")); + } + counters.fsync_dir.fetch_add(1, Relaxed); + directory.sync_all() +} + // --------------------------------------------------------------------------- // Naming // --------------------------------------------------------------------------- @@ -370,6 +389,29 @@ pub(crate) fn unlink(path: &Path) -> io::Result<()> { /// reached by `mkfifo`, and one this function exists to be immune to because /// its whole job is to look at names it does not trust. pub(crate) fn open_regular_nofollow(path: &Path) -> io::Result> { + Ok(match open_regular_nofollow_classified(path)? { + NamedRegularFile::Opened(file) => Some(file), + NamedRegularFile::Absent | NamedRegularFile::NotRegular => None, + }) +} + +/// What a name held, for the callers that must tell "absent" from "occupied by +/// something else". +/// +/// [`open_regular_nofollow`] collapses the two, which is right for a caller +/// asking "are the bytes at this name mine" — every non-regular answer means no. +/// It is wrong for `segment::read_format`, where an absent `FORMAT` is an +/// ordinary startup state that must keep reporting `NotFound`, and a `FORMAT` +/// that is a symlink is a refusal. Two questions, so two answers. +pub(crate) enum NamedRegularFile { + Opened(File), + Absent, + /// A symlink, a directory, a fifo, a socket, or a device. + NotRegular, +} + +/// [`open_regular_nofollow`], keeping absence and wrong-type apart. +pub(crate) fn open_regular_nofollow_classified(path: &Path) -> io::Result { use rustix::fs::{FileType, Mode, OFlags}; let fd = match rustix::fs::open( path, @@ -377,20 +419,29 @@ pub(crate) fn open_regular_nofollow(path: &Path) -> io::Result> { Mode::empty(), ) { Ok(fd) => fd, - Err(rustix::io::Errno::NOENT) | Err(rustix::io::Errno::NOTDIR) => return Ok(None), + Err(rustix::io::Errno::NOENT) => return Ok(NamedRegularFile::Absent), + // A non-directory component *within* the path, which is not this name + // being absent — the store cannot create through it either. + Err(rustix::io::Errno::NOTDIR) => return Ok(NamedRegularFile::NotRegular), // `O_NOFOLLOW` on a symlink is `ELOOP` on Linux and `EMLINK` on some // BSDs. Both mean "the final component is a symlink", which is exactly // the answer this function is being asked for. - Err(rustix::io::Errno::LOOP) | Err(rustix::io::Errno::MLINK) => return Ok(None), + Err(rustix::io::Errno::LOOP) | Err(rustix::io::Errno::MLINK) => { + return Ok(NamedRegularFile::NotRegular) + } + // A socket refuses `open(2)` outright; see + // [`open_or_create_regular_nofollow`] for why this is mapped rather than + // returned as an `Io`. + Err(rustix::io::Errno::NXIO) => return Ok(NamedRegularFile::NotRegular), Err(e) => return Err(io::Error::from_raw_os_error(e.raw_os_error())), }; let file = File::from(fd); let stat = rustix::fs::fstat(&file).map_err(|e| io::Error::from_raw_os_error(e.raw_os_error()))?; if FileType::from_raw_mode(stat.st_mode) == FileType::RegularFile { - Ok(Some(file)) + Ok(NamedRegularFile::Opened(file)) } else { - Ok(None) + Ok(NamedRegularFile::NotRegular) } } @@ -465,6 +516,14 @@ pub(crate) fn create_new_nofollow(path: &Path) -> io::Result> { /// warrants: reaching it requires the privilege to `mknod` inside a configured /// store root. Recorded rather than silently accepted. pub(crate) fn open_or_create_regular_nofollow(path: &Path) -> io::Result> { + Ok(open_or_create_regular_nofollow_sized(path)?.map(|(file, _)| file)) +} + +/// [`open_or_create_regular_nofollow`], also reporting the length it found. +/// +/// The length is what lets [`open_or_create_regular_truncated_nofollow`] leave +/// the durability counters alone when there was nothing to truncate. +fn open_or_create_regular_nofollow_sized(path: &Path) -> io::Result> { use rustix::fs::{FileType, Mode, OFlags}; let fd = match rustix::fs::open( path, @@ -490,12 +549,104 @@ pub(crate) fn open_or_create_regular_nofollow(path: &Path) -> io::Result.manifest.tmp`, `CURRENT.tmp`. The operation these replace is +/// `File::options().create(true).write(true).truncate(true)`, whose `O_TRUNC` +/// destroys whatever the name held **before** anything can look at it, and +/// through a symlink destroys it outside the root entirely. Splitting the open +/// from the truncation is what makes the type check possible at all: there is +/// no flag combination that truncates only regular files. +/// +/// An existing regular file here is legitimate — it is residue from an +/// interrupted attempt at exactly this write, and reusing the name is how a +/// retry works — so it is adopted and emptied rather than refused. +/// +/// The truncation goes through [`truncate`] and is therefore counted, because a +/// truncation is a durability-relevant mutation; it is skipped when the file is +/// already empty, which is every freshly created one, so the common path adds no +/// syscall and no counter movement. +pub(crate) fn open_or_create_regular_truncated_nofollow( + path: &Path, + counters: &DurabilityCounters, +) -> io::Result> { + let Some((file, length)) = open_or_create_regular_nofollow_sized(path)? else { + return Ok(None); + }; + if length != 0 { + truncate(&file, 0, counters)?; + } + Ok(Some(file)) +} + +/// What a name held, for the directories the store invents beneath a root. +pub(crate) enum NamedDirectory { + Opened(File), + Absent, + /// A symlink — including one that resolves to a perfectly good directory — + /// a regular file, or any other non-directory. + NotDirectory, +} + +/// Open `path` as a directory, refusing to traverse a symlink at the final +/// component. +/// +/// `O_DIRECTORY` is the type check here and it happens in the kernel, before the +/// descriptor exists: there is no window in which a non-directory is open. A +/// symlink *to* a directory is refused as firmly as a symlink to anything else, +/// which is the whole point — `create_dir_all` follows one and builds the store's +/// tree wherever it leads. +pub(crate) fn open_directory_nofollow(path: &Path) -> io::Result { + use rustix::fs::{Mode, OFlags}; + match rustix::fs::open( + path, + OFlags::RDONLY | OFlags::DIRECTORY | OFlags::NOFOLLOW | OFlags::CLOEXEC, + Mode::empty(), + ) { + Ok(fd) => Ok(NamedDirectory::Opened(File::from(fd))), + Err(rustix::io::Errno::NOENT) => Ok(NamedDirectory::Absent), + // `ENOTDIR` is the name holding a non-directory, or a non-directory + // component within the path; `ELOOP`/`EMLINK` is a symlink at the final + // component. The store may build through none of them. + Err(rustix::io::Errno::NOTDIR) + | Err(rustix::io::Errno::LOOP) + | Err(rustix::io::Errno::MLINK) => Ok(NamedDirectory::NotDirectory), + Err(e) => Err(io::Error::from_raw_os_error(e.raw_os_error())), + } +} + +/// Create `path` as a directory and open it, or report that the name is taken +/// by something that is not one. +/// +/// `mkdir(2)` never follows a symlink at the final component — a name occupied +/// by one fails `EEXIST` whether or not it resolves — so this cannot create a +/// directory outside the directory it names. `Ok(None)` is that `EEXIST` where +/// the occupant turns out not to be a directory; an occupant that *is* one is +/// adopted, because two callers racing to create the same tree is ordinary. +pub(crate) fn create_directory_nofollow(path: &Path) -> io::Result> { + use rustix::fs::Mode; + match rustix::fs::mkdir(path, Mode::from_raw_mode(0o755)) { + Ok(()) => {} + Err(rustix::io::Errno::EXIST) => {} + Err(e) => return Err(io::Error::from_raw_os_error(e.raw_os_error())), + } + Ok(match open_directory_nofollow(path)? { + NamedDirectory::Opened(file) => Some(file), + NamedDirectory::NotDirectory => None, + // Unlinked between the `mkdir` and the open. Nothing was created that + // survives, and the caller's tree is not what it asked for, so this is + // the same answer as a name it may not use. + NamedDirectory::Absent => None, + }) +} + /// Truncate a file to `len`. /// /// In the funnel because it changes what is durable. `journal.rs` was calling diff --git a/doc/instance-throughput-rewrite-plan.md b/doc/instance-throughput-rewrite-plan.md index 4defcc5..e1c7edd 100644 --- a/doc/instance-throughput-rewrite-plan.md +++ b/doc/instance-throughput-rewrite-plan.md @@ -1307,6 +1307,67 @@ with the charter's item 8 — assert against the path that runs — as its analo they mean here, and the socket case is now one of four occupants the test loop covers. Safety was never affected; the type of the refusal was. +##### Contract review 2026-07-29-B + +**The remaining three redirection hazards of 2026-07-28-D are closed.** One commit, one owner, one +invariant: no name the store *invents* beneath a root may be reached through a link or resolve to an +object of the wrong type. `sys.rs` gains the mechanics — the flags and the descriptor checks are the +safety property, so they belong in the funnel where the next author looking for how this crate opens +files will find them. + +Refusal semantics, decided per name rather than uniformly, because the three names mean different +things: + +- **`write_fenced`** (`FORMAT.tmp`, `.manifest.tmp`, `CURRENT.tmp`). An existing regular + file is **adopted and emptied**: it is residue from an interrupted attempt at this exact write, and + reusing the name is how a retry works. The open and the truncation had to be separated to make that + possible at all — no flag combination truncates only regular files, so the type check needs the + descriptor first, and `O_TRUNC` cannot be in the open. Any non-regular occupant is + `UnrecognizedLayout`. The truncation goes through the funnel's `truncate` and is counted; it is + skipped when the file is already empty, so the common create path moves no counter. +- **`read_format`**. Three answers kept apart, because callers act on them differently: absent stays + `Io(NotFound)` — startup states 1 and 3 depend on it — non-regular is `UnrecognizedLayout`, and a + corrupt *regular* marker keeps its existing decode error, which is a different finding from a + redirected one. +- **`initialize_root`**. The root and its ancestors stay the caller's path: an operator who configures + a root behind a symlink has said where the store goes, and resolving that is out of scope. Every + directory *beneath* it is store-invented — existing directories adopted, absent ones created, + symlinks and non-directory occupants `UnrecognizedLayout`. Two passes: every planned entry is + classified before any missing one is created, so a refusal cannot half-extend the tree it refused. + `O_DIRECTORY` is the type check and the kernel applies it before the descriptor exists, so there is + no window in which a non-directory is open; `mkdir(2)` never follows a final symlink, so the create + side needs no separate guard. + +**Directory fences now go to descriptors already validated** (`sys::fsync_dir_fd`) rather than +re-resolving the name, which would hand the fence to whatever the name resolves to *now* instead of to +what was checked. The fence sequence is otherwise byte-for-byte the same, deliberately: `engine.rs` +asserts `initialize_root`'s `fsync_dir` count exactly, and that assertion is load-bearing evidence +about initialization, not incidental. + +Each of the three protections was reverted independently and the witnesses observed, not predicted: + +- `write_fenced` — a 4096-byte file outside the root truncated and rewritten through a live link; a + file *created* outside the root through a dangling one; and a fifo at the name **blocked the open + for the full ten-second deadline**, an unbounded startup hang from one `mkfifo`. +- `read_format` — a foreign `FORMAT` read in full and its `shard_count` and `root_uuid` returned as + this root's, so every file in the tree would then be validated against a marker the store never + wrote; and the same ten-second hang at `FORMAT` on the read side. +- `initialize_root` — returned **`Ok(FormatMarker)`**: a successful initialization reporting a working + store, with the shard tree built outside the root through a link at `shards/`. The two-pass + preflight has its own witness, `shards/00/active` existing after a refusal that named + `shards/00/segments`. + +The witnesses sit on the `segment` entry points, not on `StoreEngine::open`. Its classifier refuses a +redirected root before any of these are reached, so a test entering that way passes whether or not the +protection exists — and `RecoverySession::open`, `drive.rs` and `store-bench` all arrive without it. +Coverage of the caller is not coverage of the callee. + +**Disclosed.** The device-node residual from 2026-07-29-A now applies to `FORMAT.tmp` as well: one +`O_NONBLOCK` open lands before the `fstat` refuses it. Unchanged in judgement — it needs `mknod` +privilege inside a configured store root. `rename_noreplace` needed no change: `renameat2` with +`RENAME_NOREPLACE` fails `EEXIST` on an occupied target name whether or not it is a link, so the +`FORMAT.tmp` → `FORMAT` install could never have followed one. + ##### Contract review 2026-07-28-C B4 re-pointed `store-bench` at a real `StoreEngine::submit` and found that four verification diff --git a/doc/phase1-storage-spine-scope.md b/doc/phase1-storage-spine-scope.md index 81c1349..fc43dea 100644 --- a/doc/phase1-storage-spine-scope.md +++ b/doc/phase1-storage-spine-scope.md @@ -415,6 +415,11 @@ long as the name keeps resolving to the object the holder locked. Two cases follow, and only one of them is a defect the store can close: +The same rule now holds for every other name the store invents beneath a root — `FORMAT`, the `.tmp` +names a fenced write installs from, and every directory in the tree (contract review 2026-07-29-B). +The root itself and its ancestors are excluded on purpose: an operator who configures a root behind a +symlink has said where the store goes. + 1. **The name already resolves elsewhere when a process arrives.** A symlink, a fifo, a directory, a socket, or a device at `LOCK` — left by an operator, a restored backup, a symlink farm, or a previous tenant of the directory. A follow-through open takes the