neuwirth
/
LeVCS
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Take the root lock on the inode the caller named 

`flock` locks the inode a descriptor reached, not the name that was asked
for, and `lock_root` opened `<root>/LOCK` with `create(true)` — a
follow-through open. A symlink at that name therefore put the root lock on
a foreign inode and left the root's own lock file unlocked, so a second
process arriving at the same root took the lock as well: two owners, each
believing it held one root exclusively. Nothing is destroyed and everything
downstream is permitted to race, which is why this is scheduled on its own
rather than folded into the remaining symlink work.

`classify_root` already refused a non-regular `LOCK`, but only on the
`StoreEngine::open` path. `RecoverySession::open` and `drive.rs` reach
`lock_root` directly with no classification ahead of them, so the guarantee
had to move down to the open itself.

`sys.rs` gains a third no-follow primitive for the shape the other two do
not cover — a name the store must own and keep, adopting an existing regular
file and creating an absent one, never truncating. The type is established
by `fstat` on the descriptor already held, before `flock` is attempted, so a
refused name is never locked even momentarily. A non-regular occupant is
`UnrecognizedLayout`, not `AlreadyLocked`: the root is malformed, not busy.

Measured on a reverted copy, three distinct failures rather than one:

  - two owners of one root, with the first lock still held;
  - a dangling link at `LOCK` created a file outside the root;
  - a fifo at `LOCK` returned `Ok(RootLock)`, the store reporting that it
    held the root lock on a pipe. That one was found by writing the test
    for the type check, not predicted.

The funnel guard needed amending to accept these tests, and the reason it
did is a defect in the guard: it exempted test code by matching the literal
name `mod tests`, so the two modules named otherwise were scanned as
production code while a file could have evaded the guard entirely by naming
a module `tests`. It now keys on the `#[cfg(test)]` attribute and, unlike
before, the exemption ends at the module's closing brace — code appended
after a test module used to be unscanned. Both directions mutation-checked.

Disclosed, not closed: a device node at `LOCK` still receives one
`O_NONBLOCK` open before the `fstat` refuses it. The three remaining
symlink hazards in `segment.rs` stand unfixed; contract review 2026-07-29-A
records both.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XKzM69CHmBuDcA3qN1jFdh
This commit is contained in:
Levi Neuwirth 2026-07-29 18:44:20 -04:00
parent 062797d4b8
commit fcf8dc680d
5 changed files with 322 additions and 29 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

27
crates/levcs-store/src/engine.rs
View File

@ -653,7 +653,7 @@ fn initializing_marker_is_ours(layout: &RootLayout) -> Result<bool, StoreError>
/// 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();

164
crates/levcs-store/src/segment.rs
View File

@ -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<RootLock, StoreError> {
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() {

78
crates/levcs-store/src/sys.rs
View File

@ -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<Option<File>> {
}
}
/// 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
/// `<root>/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<Option<File>> {
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

35
crates/levcs-store/tests/d0_contract.rs
View File

@ -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 \

47
doc/instance-throughput-rewrite-plan.md
View File

@ -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 `<root>/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