diff --git a/crates/levcs-store/src/engine.rs b/crates/levcs-store/src/engine.rs index d2b2c00..66a78ff 100644 --- a/crates/levcs-store/src/engine.rs +++ b/crates/levcs-store/src/engine.rs @@ -653,7 +653,7 @@ fn initializing_marker_is_ours(layout: &RootLayout) -> Result /// failing is a refusal and never a rebuild. /// - **`LOCK`, present as a regular file.** Ignored. It is not store data and /// it is not evidence that anything has been decided: `LOCK` is a zero-length -/// file that [`segment::lock_root`] creates with `create(true)` as a side +/// file that [`segment::lock_root`] creates if absent, as a side /// effect of *asking* whether the root is busy, so any process that merely /// probed the root leaves one. Treating it as state 4 would permanently wedge /// a root that has no bytes to lose. @@ -3373,8 +3373,8 @@ mod tests { /// State 1, and the deliberate decision inside it: a directory holding /// only `LOCK` is empty. /// - /// This is not a hypothetical. `segment::lock_root` opens `LOCK` with - /// `create(true)`, so an initialization that dies between taking the lock + /// This is not a hypothetical. `segment::lock_root` creates `LOCK` if it is + /// absent, so an initialization that dies between taking the lock /// and the `FORMAT` rename leaves precisely this, and so does any process /// that merely asked whether the root was busy. Classifying it as /// unrecognized would wedge a root with no bytes to lose. @@ -3782,14 +3782,19 @@ mod tests { /// The same hazard at the other two names this file opens or ignores. /// - /// `segment::lock_root` opens `LOCK` with `create(true).write(true)` and - /// `segment::write_fenced` opens `FORMAT.tmp` with `create` + `truncate`. - /// Both are in A1's frozen file, so neither open is changed here; what is - /// changed is that a root carrying a symlink at either name never reaches - /// them, because the classification that authorizes the write now refuses - /// it. The residual interface request is recorded in this deliverable's - /// report: those two opens are still follow-through opens for any caller - /// that reaches them by another route, and state 2's `lock_root` does. + /// `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. + /// + /// `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. #[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 857d2c2..d057aeb 100644 --- a/crates/levcs-store/src/segment.rs +++ b/crates/levcs-store/src/segment.rs @@ -252,13 +252,41 @@ impl Drop for RootLock { /// /// The returned [`RootLock`] releases explicitly when dropped; see its /// documentation for why holding a bare `File` was not equivalent. +/// +/// # Why the open goes through the no-follow funnel +/// +/// Contract review 2026-07-29-A. This used to be +/// `File::options().create(true).read(true).write(true).open(..)`, which +/// traverses a symlink at `LOCK` — and `flock` locks the inode the descriptor +/// reached, not the name that was asked for. A link at `LOCK` therefore put the +/// root lock on a foreign inode while leaving the root itself unlocked, so a +/// second process arriving at that same root took the lock as well and two +/// owners each believed they held it exclusively. Scope 3.1 exclusion is the +/// property the whole engine's single-writer reasoning rests on, so this is a +/// safety defect rather than a data hazard: nothing is destroyed, and everything +/// downstream is permitted to race. +/// +/// `classify_root` refuses a non-regular `LOCK` on the `StoreEngine::open` path, +/// but that is not where the guarantee can live. `RecoverySession::open` and +/// `drive.rs` reach here directly, without any classification, and the check has +/// to hold for them too. It is also strictly ordered: the type is established +/// from the descriptor *before* `flock` is attempted, so a refused name is never +/// locked even momentarily. +/// +/// A `LOCK` that is not a regular file is [`StoreError::UnrecognizedLayout`] and +/// not [`StoreError::AlreadyLocked`]: the root is not busy, it is malformed, and +/// the two call for opposite operator responses. pub fn lock_root(layout: &RootLayout) -> Result { - let file = File::options() - .create(true) - .read(true) - .write(true) - .truncate(false) - .open(layout.lock_path())?; + let path = layout.lock_path(); + let file = match sys::open_or_create_regular_nofollow(&path)? { + Some(file) => file, + None => { + return Err(StoreError::UnrecognizedLayout(format!( + "{} is not a regular file; refusing to take the root lock on it", + path.display() + ))); + } + }; if sys::try_lock_exclusive(&file)? { Ok(RootLock { file }) } else { @@ -1017,6 +1045,130 @@ impl SegmentCache { mod root_lock_tests { use super::*; + /// The exclusion defect a follow-through open at `LOCK` produced. + /// + /// `flock` locks the inode the descriptor reached. So with a symlink at + /// `LOCK`, the second caller's lock lands on a foreign inode and the root's + /// own lock file is left unlocked — and *both* callers hold what each + /// believes is exclusive ownership of one root. That is the scope 3.1 + /// property every single-writer argument above this layer depends on. + /// + /// The first lock is taken before the link is planted, and is still held + /// when the second is attempted, so the arrangement is a state and not a + /// race. Against the previous open this test fails by taking the second + /// lock successfully. + #[test] + fn a_symlink_at_lock_cannot_produce_a_second_owner_of_one_root() { + let dir = tempfile::tempdir().expect("temp root"); + let layout = RootLayout::new(dir.path()); + let held = lock_root(&layout).expect("the first and only owner"); + + // The name is redirected at a file that exists and is unlocked, which is + // what makes the second `flock` succeed rather than fail for an + // unrelated reason. + let outside = tempfile::tempdir().expect("a directory outside the root"); + let foreign = outside.path().join("elsewhere"); + std::fs::write(&foreign, b"not the store's lock\n").expect("the foreign file"); + std::fs::remove_file(layout.lock_path()).expect("unlink the real LOCK"); + std::os::unix::fs::symlink(&foreign, layout.lock_path()).expect("plant the link"); + + match lock_root(&layout) { + Err(StoreError::UnrecognizedLayout(reason)) => { + assert!(reason.contains("LOCK"), "{reason}"); + } + Ok(_) => panic!( + "a second caller took the root lock while the first still held it, because \ + the symlink at LOCK sent its flock to a foreign inode. Two processes now \ + own one root and scope 3.1 exclusion no longer holds." + ), + Err(other) => panic!("expected UnrecognizedLayout, got {other:?}"), + } + drop(held); + } + + /// The same open, in its destructive form: a *dangling* link at `LOCK` and + /// `create(true)` brings a file into being outside the root. + #[test] + fn a_symlink_at_lock_creates_no_file_outside_the_root() { + let dir = tempfile::tempdir().expect("temp root"); + let layout = RootLayout::new(dir.path()); + let outside = tempfile::tempdir().expect("a directory outside the root"); + let absent = outside.path().join("not-there"); + std::os::unix::fs::symlink(&absent, layout.lock_path()).expect("plant the link"); + + // The outside file is checked before the refusal is classified, so a + // regression reports the damage rather than the error type. + let outcome = lock_root(&layout); + assert!( + !absent.exists(), + "taking the root lock created a file outside the root through a dangling symlink" + ); + match outcome { + Err(StoreError::UnrecognizedLayout(_)) => {} + other => panic!("expected UnrecognizedLayout, got {other:?}"), + } + } + + /// Every other non-regular occupant of the name, including the two that + /// reach different arms of the funnel: a directory refuses `O_RDWR` before + /// any type check runs, a fifo opens and is refused by the `fstat`. + /// + /// The fifo is the `O_NONBLOCK` case. A read-write open of a fifo does not + /// block on Linux, so what this asserts is the refusal, not the absence of a + /// hang; the flag carries the intent for the device nodes that would block. + #[test] + fn a_non_regular_file_at_lock_is_refused_rather_than_locked() { + // Both arms always run and both are reported: with one panicking early, + // the first failure would hide whatever the second did. + let mut wrong = Vec::new(); + for (label, occupy) in [ + ( + "directory", + (|path: &Path| std::fs::create_dir(path).expect("mkdir")) as fn(&Path), + ), + ("fifo", |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 lifetime + // of the call. + let made = unsafe { libc::mkfifo(name.as_ptr(), 0o644) }; + assert_eq!(made, 0, "mkfifo: {}", std::io::Error::last_os_error()); + }), + ] { + let dir = tempfile::tempdir().expect("temp root"); + let layout = RootLayout::new(dir.path()); + occupy(&layout.lock_path()); + match lock_root(&layout) { + Err(StoreError::UnrecognizedLayout(_)) => {} + other => wrong.push(format!("{label} at LOCK: {other:?}")), + } + } + assert!( + wrong.is_empty(), + "a non-regular LOCK must be UnrecognizedLayout, not locked and not an untyped \ + errno: {wrong:?}" + ); + } + + /// A regression guard, not a defect proof: the previous open already passed + /// `truncate(false)`. It is here because the amended open is the one place + /// an `O_TRUNC` would be easy to add and impossible to notice — `LOCK` holds + /// no bytes the store reads, so nothing else would ever complain. + #[test] + fn taking_the_lock_does_not_rewrite_an_existing_lock_file() { + let dir = tempfile::tempdir().expect("temp root"); + let layout = RootLayout::new(dir.path()); + std::fs::write(layout.lock_path(), b"operator note\n").expect("pre-existing LOCK"); + + let held = lock_root(&layout).expect("adopt the existing lock file"); + assert_eq!( + std::fs::read(layout.lock_path()).expect("read LOCK"), + b"operator note\n", + "taking the root lock rewrote bytes it did not write" + ); + drop(held); + } + /// A second holder is refused, never queued (scope 3.1). #[test] fn a_second_lock_on_a_held_root_is_refused() { diff --git a/crates/levcs-store/src/sys.rs b/crates/levcs-store/src/sys.rs index e38ad86..925b5a7 100644 --- a/crates/levcs-store/src/sys.rs +++ b/crates/levcs-store/src/sys.rs @@ -340,12 +340,13 @@ pub(crate) fn unlink(path: &Path) -> io::Result<()> { // Opening a name this process does not yet own // --------------------------------------------------------------------------- // -// Both of these exist for startup state 1, where the store has to look at, and -// then write into, a directory it has not established is its own. Plain -// `File::open` and `File::options().create(true).truncate(true)` both traverse a -// symlink at the final component, so either one at a name an operator can -// occupy is a write to a path outside the root. `open`/`stat` are therefore not -// available on that path at all; these are. +// These exist for the paths where the store has to look at, and then write +// into, a directory it has not established is its own — startup state 1, and +// `lock_root` on every path. Plain `File::open` and +// `File::options().create(true)` both traverse a symlink at the final +// component, so either one at a name an operator can occupy is a write to a +// path outside the root. `open`/`stat` are therefore not available on those +// paths at all; these are. /// Open `path` read-only for *verification*, refusing to traverse a symlink at /// the final component and refusing anything that is not a regular file. @@ -418,6 +419,71 @@ pub(crate) fn create_new_nofollow(path: &Path) -> io::Result> { } } +/// Open `path` read-write, creating it if absent, refusing to traverse a +/// symlink at the final component and refusing anything that is not a regular +/// file. Never truncates. +/// +/// `Ok(None)` means "the name is occupied by something that is not a regular +/// file": a symlink, a directory, a fifo, a socket, a device. Absent is not one +/// of them — absent is the create case, and it returns the new file. +/// +/// This is the shape [`open_regular_nofollow`] and [`create_new_nofollow`] do +/// not cover: a name the store must *own and keep*, where an existing file is +/// adopted rather than replaced and an absent one is brought into being. The +/// only caller is `segment::lock_root`, which needs exactly this because +/// `/LOCK` is created as a side effect of asking whether a root is busy. +/// +/// # Why the type check is not cosmetic here +/// +/// `flock` locks the *inode*, not the name. A symlink at `LOCK` that a +/// follow-through open traverses therefore takes the root lock on a foreign +/// inode, and the root's own lock file is left unlocked — so a second process +/// arriving at the same root takes it too, and both hold what each believes is +/// exclusive ownership. That defeats the exclusion scope 3.1 is built on, which +/// no amount of care at the `flock` call itself can restore. `O_NOFOLLOW` plus +/// this `fstat` is what makes the locked inode provably the one the caller +/// named. +/// +/// One `O_CREAT` open, not an `O_EXCL` create followed by a plain open on +/// `EEXIST`: the single call has no window between deciding the name is taken +/// and opening what is there, so there is nothing for an adversary replacing +/// the name to land in. +/// +/// # What this still permits +/// +/// A device node at the name receives one `open(2)` before the `fstat` refuses +/// it. `O_NONBLOCK` is passed so that open cannot hang — the fifo case is a +/// startup denial of service otherwise — but a driver that acts on being opened +/// has still been opened. Closing that would need `O_PATH` for classification +/// and a re-open of the same inode, which is more machinery than the hazard +/// 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> { + use rustix::fs::{FileType, Mode, OFlags}; + let fd = match rustix::fs::open( + path, + OFlags::RDWR | OFlags::CREATE | OFlags::NOFOLLOW | OFlags::CLOEXEC | OFlags::NONBLOCK, + Mode::from_raw_mode(0o644), + ) { + Ok(fd) => fd, + // A symlink at the final component: `ELOOP` on Linux, `EMLINK` on some + // BSDs. `O_CREAT` through a *dangling* link is the same refusal, which + // is what stops this call from creating a file outside the root. + Err(rustix::io::Errno::LOOP) | Err(rustix::io::Errno::MLINK) => return Ok(None), + // A directory refuses `O_RDWR` before any type check runs. + Err(rustix::io::Errno::ISDIR) => return Ok(None), + 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)) + } else { + Ok(None) + } +} + /// Truncate a file to `len`. /// /// In the funnel because it changes what is durable. `journal.rs` was calling diff --git a/crates/levcs-store/tests/d0_contract.rs b/crates/levcs-store/tests/d0_contract.rs index fd31c1f..8215bf0 100644 --- a/crates/levcs-store/tests/d0_contract.rs +++ b/crates/levcs-store/tests/d0_contract.rs @@ -300,16 +300,34 @@ fn durability_syscalls_go_only_through_the_sys_funnel() { continue; } let text = std::fs::read_to_string(&path).expect("read source"); - let mut in_test_module = false; + // Test-only code is exempt, and how that is decided matters twice over. + // + // The trigger is the `#[cfg(test)]` attribute at column zero — the thing + // that actually removes the code from a release build — and not the + // module's *name*. Matching `mod tests` exempted only modules that + // happen to be called that, so `segment.rs`'s `root_lock_tests` and + // `recovery.rs`'s `production_session_tests` were scanned as production + // code, while a file could equally have evaded the guard by naming a + // module `tests` and putting real code in it. + // + // The exemption also *ends*, at the next column-zero `}`. Latching it on + // for the rest of the file meant anything appended after a test module + // was unscanned — the one place a durability call is least likely to be + // noticed. rustfmt puts every top-level item's closing brace at column + // zero, so that boundary is mechanical here. + let mut in_test_item = false; for (n, line) in text.lines().enumerate() { let code = line.trim_start(); - if code.starts_with("//") { + if line.starts_with("#[cfg(test)]") { + in_test_item = true; + } + if in_test_item { + if line == "}" { + in_test_item = false; + } continue; } - if code.contains("mod tests") { - in_test_module = true; - } - if in_test_module { + if code.starts_with("//") { continue; } for needle in FORBIDDEN { @@ -319,6 +337,11 @@ fn durability_syscalls_go_only_through_the_sys_funnel() { } } } + // `std::fs::remove_file(` and `fs::remove_file(` both match one line, so an + // offender is reported once per needle it happens to satisfy. That made a + // real report ambiguous to read; the finding is the line, not the needle. + offenders.sort(); + offenders.dedup(); assert!( offenders.is_empty(), "durability syscalls must go through sys.rs so DurabilityCounters sees \ diff --git a/doc/instance-throughput-rewrite-plan.md b/doc/instance-throughput-rewrite-plan.md index 7c3afa4..c0605d4 100644 --- a/doc/instance-throughput-rewrite-plan.md +++ b/doc/instance-throughput-rewrite-plan.md @@ -1217,6 +1217,53 @@ shard tree outside the root. `read_format` follows a link at `FORMAT`, read-only priority. The first is a correctness defect in the locking discipline and should be scheduled on its own, not folded into a later pass. +##### Contract review 2026-07-29-A + +**The first of 2026-07-28-D's four recorded hazards is closed at the frozen surface.** Granted and +landed by the lead, not by a package: `segment::lock_root` now takes `/LOCK` through a third +`sys.rs` primitive, `open_or_create_regular_nofollow` — `O_RDWR | O_CREAT | O_NOFOLLOW | O_CLOEXEC | +O_NONBLOCK`, no `O_TRUNC`, followed by an `fstat` on the descriptor already held. A name occupied by +anything that is not a regular file is `UnrecognizedLayout`, and the type is established **before** +`flock` is attempted, so a refused name is never locked even momentarily. + +Why this one was scheduled alone rather than folded into a later pass. The other three hazards risk +a file. This one breaks a **safety property**: `flock` locks the inode a descriptor reached, not the +name that was asked for, so a link at `LOCK` puts the root lock on a foreign inode and leaves the +root's own lock file unlocked. Every single-writer argument above this layer — the shard writer, the +publication window of scope §6.3, the poison-window reasoning — rests on §3.1 exclusion holding. +Nothing is destroyed and everything downstream is permitted to race. + +Why `classify_root` was not sufficient, which is the whole reason this could not stay a B1 fix. +B1's classifier does refuse a non-regular `LOCK`, but only on the `StoreEngine::open` path. +`RecoverySession::open` and `drive.rs` call `lock_root` directly, with no classification anywhere +ahead of them, and those are the callers a future `migrate-store` and every recovery tool reach +through. A guarantee that holds only for one of three entry points is not a guarantee. + +Measured on a reverted copy, all three distinct failures observed rather than predicted: + +- **Two owners of one root.** First lock taken and *still held*; `LOCK` then replaced with a link to + an unlocked file elsewhere; the second `lock_root` returned `Ok`. A state, not a race. +- **A dangling link at `LOCK` created a file outside the root**, because `create(true)` through an + unresolved link is a create at the target. +- **A fifo at `LOCK` returned `Ok(RootLock)`** — the store reported holding the root lock on a pipe. + This was not in the ranked hazard; it was found by writing the test for the `fstat` arm. + +`FileType::RegularFile` is checked from the descriptor, never from a second path lookup, so the +answer is about the object the caller holds and cannot be changed underneath it. One `O_CREAT` open +rather than an `O_EXCL` create with a plain open on `EEXIST`: the single call leaves no window +between deciding a name is taken and opening what is there. + +**Disclosed and not closed.** A device node at `LOCK` still receives one `open(2)` before the +`fstat` refuses it. `O_NONBLOCK` stops that open from hanging — the fifo case is otherwise a startup +denial of service — but a driver that acts on being opened has been opened. Closing it needs `O_PATH` +classification plus a re-open of the same inode, which is more machinery than a hazard gated behind +the privilege to `mknod` inside a configured store root warrants. Recorded, not silently accepted. + +The remaining three hazards from 2026-07-28-D stand unchanged and unfixed: `write_fenced` for +`FORMAT.tmp`, `initialize_root`'s `create_dir_all`, and `read_format`. Their common shape is now +one primitive away from a fix, but each needs its own refusal semantics decided, and none of them +breaks an exclusion property. + ##### Contract review 2026-07-28-C B4 re-pointed `store-bench` at a real `StoreEngine::submit` and found that four verification