Bound the funnel exemption by shape and state what the root lock excludes
Two findings against the previous commit, both upheld. The funnel guard's test exemption could still latch. Ending it at the next column-zero `}` is right for a braced item and wrong for every other shape: after `#[cfg(test)] use crate::test_support;` the first such brace belongs to the *next* function, so all of it went unscanned. The scanner now reads the attributed item's shape — braced items are exempt to their closing brace, semicolon-terminated items exempt only themselves, and any third shape, including an item header rustfmt split across lines, fails the guard. A shape it cannot bound is not a shape it may assume is harmless. It is now a function over `&str` with synthetic tests, which is the more important half. Mutating real sources only probes the shapes those sources happen to contain: no file in this crate has a semicolon-terminated `#[cfg(test)]` item followed by production code, so no mutation of a real file could have produced this defect. Charter item 8's analogue for tooling. The two-owner claim was overstated. The regression arranges its wrong-typed name by replacing `LOCK` under a live holder — and replacing it with a fresh *regular* file succeeds just as well, since both opens are then of a regular file at the right name with nothing to tell them apart. The type check closes "the name already resolves to the wrong kind of object", the operator-error and stale-state case; it does not close "the name is replaced under a holder", and no check at this layer can. So scope 3.1 now separates the two, says which is in scope, and states the replacement case as an explicit deployment assumption rather than leaving it implied: anything able to replace `LOCK` can equally unlink a journal, so advisory locking was never the boundary that would stop it. The assumption is pinned by a test asserting the current behavior on purpose — if a stable locking object is ever adopted, that test is meant to fail, and the failure is the signal that the documented assumption changed. §3.1 records locking the root directory as the candidate and what it would cost. Also exact rather than caveated: a Unix socket fails `open(2)` with `ENXIO` before any `fstat`, so it surfaced as `Io` while the documentation promised `UnrecognizedLayout`. `ENXIO` and `EISDIR` both now mean "not a regular file", and the socket is one of four occupants the test loop covers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XKzM69CHmBuDcA3qN1jFdh
This commit is contained in:
parent
64dc949370
commit
c0df18f15f
|
|
@ -1053,6 +1053,18 @@ mod root_lock_tests {
|
|||
/// believes is exclusive ownership of one root. That is the scope 3.1
|
||||
/// property every single-writer argument above this layer depends on.
|
||||
///
|
||||
/// # What this proves, and what it does not
|
||||
///
|
||||
/// It proves that a `LOCK` whose name resolves to something other than a
|
||||
/// regular file cannot be locked, which closes case 1 of the two in scope
|
||||
/// 3.1. It is **not** proof of exclusion against *replacement*: planting a
|
||||
/// fresh regular file here instead of a symlink still produces a second
|
||||
/// holder, because the second caller opens and locks a new, unlocked inode
|
||||
/// and no check at this layer can distinguish that from the first open.
|
||||
/// [`replacing_the_lock_file_still_admits_a_second_holder`] pins that limit
|
||||
/// deliberately. The lock method here is what is under test; the planting is
|
||||
/// only how a wrong-typed name is arranged.
|
||||
///
|
||||
/// 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
|
||||
|
|
@ -1078,14 +1090,50 @@ mod root_lock_tests {
|
|||
}
|
||||
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."
|
||||
the symlink at LOCK sent its flock to a foreign inode. A name that does not \
|
||||
resolve to a regular file must be refused before flock."
|
||||
),
|
||||
Err(other) => panic!("expected UnrecognizedLayout, got {other:?}"),
|
||||
}
|
||||
drop(held);
|
||||
}
|
||||
|
||||
/// The limit of what a lock on a file inside the root can give, pinned so it
|
||||
/// cannot be mistaken for a property.
|
||||
///
|
||||
/// Replacing `LOCK` with a fresh **regular** file while a holder holds it
|
||||
/// produces a second holder. The no-follow open cannot help: both opens are
|
||||
/// of a regular file at exactly the right name, and the only difference is
|
||||
/// which inode the name resolved to, which the second caller has no way to
|
||||
/// know was ever different. This is true of any regular file at any name.
|
||||
///
|
||||
/// Scope 3.1 therefore states the assumption explicitly — no noncooperating
|
||||
/// mutation of the root directory's entries while the root is held — and this
|
||||
/// test is the machine-readable form of it. It asserts the *current* behavior
|
||||
/// on purpose: if a stable locking object is ever adopted (§3.1 names locking
|
||||
/// the root directory as the candidate), this test is expected to fail, and
|
||||
/// that failure is the signal that the assumption changed.
|
||||
#[test]
|
||||
fn replacing_the_lock_file_still_admits_a_second_holder() {
|
||||
let dir = tempfile::tempdir().expect("temp root");
|
||||
let layout = RootLayout::new(dir.path());
|
||||
let held = lock_root(&layout).expect("the first holder");
|
||||
|
||||
std::fs::remove_file(layout.lock_path()).expect("unlink the locked name");
|
||||
let second = lock_root(&layout);
|
||||
|
||||
match second {
|
||||
Ok(_) => {}
|
||||
Err(other) => panic!(
|
||||
"a stable locking object appears to have been adopted, or lock_root changed: a \
|
||||
replaced LOCK was refused with {other:?}. If that is intended, scope 3.1's \
|
||||
assumption about noncooperating mutation of the root directory is now \
|
||||
stronger than documented and both should be updated together."
|
||||
),
|
||||
}
|
||||
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]
|
||||
|
|
@ -1126,6 +1174,12 @@ mod root_lock_tests {
|
|||
"directory",
|
||||
(|path: &Path| std::fs::create_dir(path).expect("mkdir")) as fn(&Path),
|
||||
),
|
||||
// Refused by `open(2)` itself with `ENXIO`, so it never reaches the
|
||||
// `fstat` — which is why the primitive maps that errno rather than
|
||||
// letting it surface as an `Io` the caller would have to know about.
|
||||
("unix socket", |path: &Path| {
|
||||
std::os::unix::net::UnixListener::bind(path).expect("bind");
|
||||
}),
|
||||
("fifo", |path: &Path| {
|
||||
let name = std::ffi::CString::new(path.as_os_str().as_encoded_bytes())
|
||||
.expect("a path with no interior NUL");
|
||||
|
|
|
|||
|
|
@ -441,8 +441,14 @@ pub(crate) fn create_new_nofollow(path: &Path) -> io::Result<Option<File>> {
|
|||
/// 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.
|
||||
/// this `fstat` is what makes the locked inode the one the caller's *name*
|
||||
/// resolved to.
|
||||
///
|
||||
/// It does not make the binding permanent, and scope 3.1 is explicit about the
|
||||
/// difference: a name replaced with a fresh regular file while a holder holds it
|
||||
/// still yields a second holder, because both opens are then of a regular file at
|
||||
/// exactly the right name. That case is an assumption about the deployment, not a
|
||||
/// property this function can supply.
|
||||
///
|
||||
/// 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
|
||||
|
|
@ -470,8 +476,14 @@ pub(crate) fn open_or_create_regular_nofollow(path: &Path) -> io::Result<Option<
|
|||
// 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),
|
||||
// A directory refuses `O_RDWR` before any type check runs, and a Unix
|
||||
// socket refuses `open(2)` with `ENXIO` — so neither ever reaches the
|
||||
// `fstat`. Both are mapped here rather than left to surface as `Io`,
|
||||
// because "the name is occupied by something that is not a regular file"
|
||||
// is precisely what they mean, and a caller that has to handle that
|
||||
// answer in two shapes will handle one of them wrongly. `ENXIO` on a
|
||||
// deviceless device node says the same thing.
|
||||
Err(rustix::io::Errno::ISDIR) | Err(rustix::io::Errno::NXIO) => return Ok(None),
|
||||
Err(e) => return Err(io::Error::from_raw_os_error(e.raw_os_error())),
|
||||
};
|
||||
let file = File::from(fd);
|
||||
|
|
|
|||
|
|
@ -262,6 +262,137 @@ fn privileged_construction_is_not_reachable_from_an_engine() {
|
|||
}
|
||||
}
|
||||
|
||||
/// Calls the durability funnel exists to intercept.
|
||||
///
|
||||
/// Writes are here for a reason found in review: `checkpoint.rs` used
|
||||
/// `File::write_all` directly, so checkpoint bytes and short writes were
|
||||
/// invisible to `DurabilityCounters` and the ENOSPC / short-write / cursor fault
|
||||
/// seam could not reach checkpoint installation at all. The original guard
|
||||
/// scanned only sync, rename, and unlink, so it passed. A funnel that covers
|
||||
/// durability but not the writes being made durable is not a funnel.
|
||||
const FORBIDDEN: &[&str] = &[
|
||||
"sync_all(",
|
||||
"sync_data(",
|
||||
"std::fs::rename(",
|
||||
"std::fs::remove_file(",
|
||||
"fs::rename(",
|
||||
"fs::remove_file(",
|
||||
".write_all(",
|
||||
".write_vectored(",
|
||||
".set_len(",
|
||||
"std::fs::write(",
|
||||
"fs::write(",
|
||||
];
|
||||
|
||||
/// Scan one file's text for [`FORBIDDEN`] calls outside test-only code.
|
||||
///
|
||||
/// `Err` is a scanner failure and fails the guard exactly as an offender does:
|
||||
/// a shape it cannot reason about is not a shape it may assume is safe. Split
|
||||
/// out of the test so the scanner itself is testable on synthetic input —
|
||||
/// mutating real sources only ever probes the shapes those sources happen to
|
||||
/// contain, which is how the two defects below survived.
|
||||
///
|
||||
/// # How test-only code is exempted, and why not by name
|
||||
///
|
||||
/// The trigger is the `#[cfg(test)]` attribute at column zero — the thing that
|
||||
/// actually removes code from a release build. An earlier version matched the
|
||||
/// literal `mod tests`, which exempted only modules that happen to be called
|
||||
/// that (`segment.rs`'s `root_lock_tests` and `recovery.rs`'s
|
||||
/// `production_session_tests` were scanned as production code) while letting a
|
||||
/// file evade the guard entirely by naming a module `tests`.
|
||||
///
|
||||
/// The exemption is then bounded by the **shape of the attributed item**, which
|
||||
/// is the second defect. Ending it at the next column-zero `}` is right for a
|
||||
/// braced item and wrong for anything else: after
|
||||
///
|
||||
/// ```text
|
||||
/// #[cfg(test)]
|
||||
/// use crate::test_support;
|
||||
///
|
||||
/// fn shipping_code() {
|
||||
/// std::fs::write(..);
|
||||
/// }
|
||||
/// ```
|
||||
///
|
||||
/// the first column-zero `}` is *`shipping_code`'s*, so every line of it was
|
||||
/// skipped. A semicolon-terminated item therefore exempts only itself, and any
|
||||
/// third shape — including an item header rustfmt has split across lines — is a
|
||||
/// scanner error rather than a guess.
|
||||
fn scan_for_unfunnelled_calls(text: &str) -> Result<Vec<(usize, String)>, String> {
|
||||
#[derive(PartialEq)]
|
||||
enum Exempt {
|
||||
No,
|
||||
/// Until the item's closing brace at column zero.
|
||||
UntilUnindentedBrace,
|
||||
}
|
||||
|
||||
let lines: Vec<&str> = text.lines().collect();
|
||||
let mut offenders = Vec::new();
|
||||
let mut exempt = Exempt::No;
|
||||
let mut index = 0;
|
||||
while index < lines.len() {
|
||||
let line = lines[index];
|
||||
let code = line.trim_start();
|
||||
|
||||
if exempt == Exempt::UntilUnindentedBrace {
|
||||
if line == "}" {
|
||||
exempt = Exempt::No;
|
||||
}
|
||||
index += 1;
|
||||
continue;
|
||||
}
|
||||
|
||||
if line.starts_with("#[cfg(test)]") {
|
||||
// The item this attribute applies to, past any further attributes,
|
||||
// doc comments and blank lines.
|
||||
let mut head = index + 1;
|
||||
while head < lines.len() {
|
||||
let candidate = lines[head].trim_start();
|
||||
if candidate.is_empty() || candidate.starts_with('#') || candidate.starts_with("//")
|
||||
{
|
||||
head += 1;
|
||||
continue;
|
||||
}
|
||||
break;
|
||||
}
|
||||
let Some(item) = lines.get(head).map(|l| l.trim_end()) else {
|
||||
return Err(format!(
|
||||
"line {}: `#[cfg(test)]` with no item after it",
|
||||
index + 1
|
||||
));
|
||||
};
|
||||
if item.ends_with('{') {
|
||||
exempt = Exempt::UntilUnindentedBrace;
|
||||
index = head + 1;
|
||||
continue;
|
||||
}
|
||||
if item.ends_with(';') {
|
||||
// Only the item itself. Whatever follows is production code
|
||||
// until something says otherwise.
|
||||
index = head + 1;
|
||||
continue;
|
||||
}
|
||||
return Err(format!(
|
||||
"line {}: the scanner cannot bound a `#[cfg(test)]` item of this shape, so it \
|
||||
cannot tell where the exemption ends: `{}`. Keep the item header on one line, \
|
||||
or teach the scanner the shape — do not leave it guessing.",
|
||||
head + 1,
|
||||
item.trim()
|
||||
));
|
||||
}
|
||||
|
||||
if !code.starts_with("//") {
|
||||
for needle in FORBIDDEN {
|
||||
if code.contains(needle) {
|
||||
offenders.push((index + 1, line.trim().to_string()));
|
||||
}
|
||||
}
|
||||
}
|
||||
index += 1;
|
||||
}
|
||||
Ok(offenders)
|
||||
}
|
||||
|
||||
/// Nothing outside `sys.rs` may call a durability syscall directly.
|
||||
///
|
||||
/// The counters are what turn "exactly one fence per group" and "no per-object
|
||||
|
|
@ -269,26 +400,6 @@ fn privileged_construction_is_not_reachable_from_an_engine() {
|
|||
/// that bypasses the funnel is invisible to them (scope 2.3).
|
||||
#[test]
|
||||
fn durability_syscalls_go_only_through_the_sys_funnel() {
|
||||
// Writes are here for a reason found in review: `checkpoint.rs` used
|
||||
// `File::write_all` directly, so checkpoint bytes and short writes were
|
||||
// invisible to `DurabilityCounters` and the ENOSPC / short-write / cursor
|
||||
// fault seam could not reach checkpoint installation at all. The original
|
||||
// guard scanned only sync, rename, and unlink, so it passed. A funnel that
|
||||
// covers durability but not the writes being made durable is not a funnel.
|
||||
const FORBIDDEN: &[&str] = &[
|
||||
"sync_all(",
|
||||
"sync_data(",
|
||||
"std::fs::rename(",
|
||||
"std::fs::remove_file(",
|
||||
"fs::rename(",
|
||||
"fs::remove_file(",
|
||||
".write_all(",
|
||||
".write_vectored(",
|
||||
".set_len(",
|
||||
"std::fs::write(",
|
||||
"fs::write(",
|
||||
];
|
||||
|
||||
let mut offenders = Vec::new();
|
||||
for path in rust_sources(&crate_src()) {
|
||||
// `sys.rs` is the funnel itself. `src/bin/**` are harness binaries that
|
||||
|
|
@ -300,41 +411,13 @@ fn durability_syscalls_go_only_through_the_sys_funnel() {
|
|||
continue;
|
||||
}
|
||||
let text = std::fs::read_to_string(&path).expect("read source");
|
||||
// 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 line.starts_with("#[cfg(test)]") {
|
||||
in_test_item = true;
|
||||
}
|
||||
if in_test_item {
|
||||
if line == "}" {
|
||||
in_test_item = false;
|
||||
}
|
||||
continue;
|
||||
}
|
||||
if code.starts_with("//") {
|
||||
continue;
|
||||
}
|
||||
for needle in FORBIDDEN {
|
||||
if code.contains(needle) {
|
||||
offenders.push(format!("{}:{}: {}", path.display(), n + 1, line.trim()));
|
||||
}
|
||||
}
|
||||
match scan_for_unfunnelled_calls(&text) {
|
||||
Ok(found) => offenders.extend(
|
||||
found
|
||||
.into_iter()
|
||||
.map(|(line, text)| format!("{}:{line}: {text}", path.display())),
|
||||
),
|
||||
Err(reason) => panic!("{}: {reason}", path.display()),
|
||||
}
|
||||
}
|
||||
// `std::fs::remove_file(` and `fs::remove_file(` both match one line, so an
|
||||
|
|
@ -350,6 +433,109 @@ fn durability_syscalls_go_only_through_the_sys_funnel() {
|
|||
);
|
||||
}
|
||||
|
||||
/// The scanner above, against the shapes it has to get right.
|
||||
///
|
||||
/// Synthetic input rather than mutated sources. Mutating `segment.rs` proves the
|
||||
/// scanner works on the shapes `segment.rs` happens to contain, which is exactly
|
||||
/// how both of its exemption defects survived a mutation check: no file in the
|
||||
/// crate currently has a semicolon-terminated `#[cfg(test)]` item followed by
|
||||
/// production code, so no mutation of a real file could produce one.
|
||||
#[test]
|
||||
fn the_funnel_scanner_bounds_a_test_exemption_by_the_shape_of_the_item() {
|
||||
let lines = |found: Vec<(usize, String)>| -> Vec<usize> {
|
||||
let mut out: Vec<usize> = found.into_iter().map(|(line, _)| line).collect();
|
||||
out.dedup();
|
||||
out
|
||||
};
|
||||
|
||||
// Production code is found.
|
||||
assert_eq!(
|
||||
lines(
|
||||
scan_for_unfunnelled_calls("fn ship() {\n std::fs::write(p, b\"\");\n}\n")
|
||||
.expect("a plain file scans")
|
||||
),
|
||||
vec![2]
|
||||
);
|
||||
|
||||
// A braced test item is exempt, and the exemption ends with it.
|
||||
let braced = "\
|
||||
#[cfg(test)]
|
||||
mod named_anything {
|
||||
fn setup() {
|
||||
std::fs::write(p, b\"\");
|
||||
}
|
||||
}
|
||||
|
||||
fn ship() {
|
||||
std::fs::remove_file(p);
|
||||
}
|
||||
";
|
||||
assert_eq!(
|
||||
lines(scan_for_unfunnelled_calls(braced).expect("a braced item scans")),
|
||||
vec![9],
|
||||
"the call inside the test module must be exempt and the one after it must not"
|
||||
);
|
||||
|
||||
// The defect this shape check exists for: a semicolon-terminated test item
|
||||
// exempts itself and nothing else. Under the previous rule the first
|
||||
// column-zero `}` was `ship`'s, so the whole function was skipped.
|
||||
let terminated = "\
|
||||
#[cfg(test)]
|
||||
use crate::test_support;
|
||||
|
||||
fn ship() {
|
||||
std::fs::write(p, b\"\");
|
||||
}
|
||||
";
|
||||
assert_eq!(
|
||||
lines(scan_for_unfunnelled_calls(terminated).expect("a semicolon-terminated item scans")),
|
||||
vec![5],
|
||||
"a `#[cfg(test)] use ...;` must not exempt the function that follows it"
|
||||
);
|
||||
|
||||
// Stacked attributes and doc comments between the trigger and the item.
|
||||
let stacked = "\
|
||||
#[cfg(test)]
|
||||
#[allow(dead_code)]
|
||||
/// A helper.
|
||||
mod support {
|
||||
fn setup() {
|
||||
std::fs::write(p, b\"\");
|
||||
}
|
||||
}
|
||||
";
|
||||
assert!(
|
||||
lines(scan_for_unfunnelled_calls(stacked).expect("stacked attributes scan")).is_empty(),
|
||||
"further attributes and doc comments must not hide the item's shape"
|
||||
);
|
||||
|
||||
// A shape the scanner cannot bound is a failure, not an assumption. A
|
||||
// multi-line item header is the realistic way to produce one.
|
||||
let unsupported = "\
|
||||
#[cfg(test)]
|
||||
fn helper(
|
||||
argument: usize,
|
||||
) {
|
||||
std::fs::write(p, b\"\");
|
||||
}
|
||||
";
|
||||
let reason = scan_for_unfunnelled_calls(unsupported)
|
||||
.expect_err("an unbounded exemption must fail the guard rather than be guessed at");
|
||||
assert!(reason.contains("cannot bound"), "{reason}");
|
||||
|
||||
// And a trailing attribute with nothing after it.
|
||||
assert!(scan_for_unfunnelled_calls("#[cfg(test)]\n")
|
||||
.expect_err("a dangling attribute is a scanner failure")
|
||||
.contains("no item after it"));
|
||||
|
||||
// Comments are not code, in either position.
|
||||
assert!(lines(
|
||||
scan_for_unfunnelled_calls("fn ship() {\n // std::fs::write(p, b\"\");\n}\n")
|
||||
.expect("a commented call scans")
|
||||
)
|
||||
.is_empty());
|
||||
}
|
||||
|
||||
/// `StoreError` is for inability to answer; `TransactionStatus` is for every
|
||||
/// state the store can actually report. Plan §5.1 makes the split normative.
|
||||
///
|
||||
|
|
|
|||
|
|
@ -1241,8 +1241,8 @@ through. A guarantee that holds only for one of three entry points is not a guar
|
|||
|
||||
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 wrong-typed name is locked.** 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.
|
||||
|
|
@ -1264,6 +1264,46 @@ The remaining three hazards from 2026-07-28-D stand unchanged and unfixed: `writ
|
|||
one primitive away from a fix, but each needs its own refusal semantics decided, and none of them
|
||||
breaks an exclusion property.
|
||||
|
||||
###### Amended after review, 2026-07-29
|
||||
|
||||
Two findings against the above, both upheld.
|
||||
|
||||
**The two-owner claim was overstated, and the correction is a scope amendment rather than a code
|
||||
change.** The regression arranges its wrong-typed name by *replacing* `LOCK` while the first holder
|
||||
holds it — and replacing it with a fresh **regular** file succeeds just as well, since both opens
|
||||
are then of a regular file at exactly the right name and nothing distinguishes the second from the
|
||||
first. So the type check closes "the name already resolves to the wrong kind of object", which is
|
||||
the operator-error and stale-state case, and does **not** close "the name is replaced under a
|
||||
holder". Scope 3.1 now states the two cases apart, says which one is in scope, and states the
|
||||
replacement case as an explicit deployment assumption: anything able to replace `LOCK` can equally
|
||||
unlink a journal, so advisory locking was never the boundary that would stop it. The assumption is
|
||||
pinned by `replacing_the_lock_file_still_admits_a_second_holder`, which asserts the *current*
|
||||
behavior deliberately — if a stable locking object is ever adopted, that test is expected to fail,
|
||||
and the failure is the signal that the documented assumption changed. §3.1 records locking the root
|
||||
**directory** as the candidate, and what it would cost: a frozen-surface change, and the removal of
|
||||
the "directory holding only `LOCK` is empty" special case that startup state 1 depends on.
|
||||
|
||||
**The funnel guard's exemption could still latch, and the fix is a scanner that refuses to guess.**
|
||||
Bounding a `#[cfg(test)]` exemption at the next column-zero `}` is right for a braced item and wrong
|
||||
for every other shape: after `#[cfg(test)] use crate::test_support;` the first such brace belongs to
|
||||
the *next* function, so all of it was skipped. The scanner now reads the shape of the attributed
|
||||
item — braced items are exempt to their closing brace, semicolon-terminated items exempt only
|
||||
themselves, and any third shape (including an item header rustfmt split across lines) **fails the
|
||||
guard** rather than being assumed safe. A shape the scanner cannot bound is not a shape it may
|
||||
treat as harmless.
|
||||
|
||||
It is also now a function over `&str` with synthetic tests, which is the load-bearing half. Mutating
|
||||
real sources only ever probes the shapes those sources happen to contain: no file in the crate has a
|
||||
semicolon-terminated `#[cfg(test)]` item followed by production code, so no mutation of a real file
|
||||
could have produced this defect. That is a general lesson about source-scanning guards and belongs
|
||||
with the charter's item 8 — assert against the path that runs — as its analogue for tooling.
|
||||
|
||||
**One promise made exact rather than caveated.** A Unix socket at the name fails `open(2)` with
|
||||
`ENXIO` before any `fstat`, so it surfaced as `Io` while the documentation promised
|
||||
`UnrecognizedLayout`. `ENXIO` and `EISDIR` are both mapped to "not a regular file", which is what
|
||||
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-28-C
|
||||
|
||||
B4 re-pointed `store-bench` at a real `StoreEngine::submit` and found that four verification
|
||||
|
|
|
|||
|
|
@ -408,6 +408,40 @@ created_at_micros, checksum }`, written then directory-synced. `root_uuid` binds
|
|||
`LOCK` is held with `flock(LOCK_EX|LOCK_NB)` for process lifetime. Failure is
|
||||
`AlreadyLocked`, never a wait.
|
||||
|
||||
**What the root lock does and does not exclude** (contract review 2026-07-29-A, second
|
||||
finding). `flock` locks an *inode*. The name `<root>/LOCK` resolves to an inode once, at
|
||||
open, and nothing binds the name to that inode afterwards — so exclusion holds exactly as
|
||||
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:
|
||||
|
||||
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
|
||||
lock on a foreign inode, or on no inode the store owns, while the root itself stays
|
||||
unlocked. **In scope and closed**: `segment::lock_root` opens through
|
||||
`sys::open_or_create_regular_nofollow` and refuses anything that is not a regular file
|
||||
at exactly that name, before `flock`.
|
||||
2. **The name is replaced while a holder holds it.** `unlink` plus `create`, or a `rename`
|
||||
over it, gives the next arrival a fresh unlocked inode and a second holder — and this is
|
||||
true of *any* regular file at any name, so the type check does not address it and no
|
||||
check at this layer can. **Out of scope, and stated here as an assumption rather than
|
||||
left implied**: scope 3.1 exclusion assumes no noncooperating mutation of the root
|
||||
directory's entries while the root is held. The assumption is sound for the deployment
|
||||
this store is built for — anything able to replace `LOCK` can equally unlink a journal or
|
||||
a manifest, so advisory locking was never the boundary that would stop it — but it is an
|
||||
assumption, not a property, and a test in `segment.rs` pins it so it cannot quietly be
|
||||
believed to be stronger than it is.
|
||||
|
||||
The strongest available strengthening, if case 2 is ever brought into scope, is to hold the
|
||||
lock on the **root directory** rather than on a file inside it: `rename` over a non-empty
|
||||
directory fails `ENOTEMPTY`, so an adversary must move the whole root aside, after which
|
||||
every subsequent path resolves into a different tree and the failure stops resembling
|
||||
successful exclusion. It is not free — it changes a frozen surface, and it removes the
|
||||
"directory holding only `LOCK` is empty" special case that startup state 1 currently needs,
|
||||
which is a simplification but a behavioral change to `classify_root` and its tests. Not
|
||||
scheduled here.
|
||||
|
||||
Startup states (§5.2), decided in this order and with no inference:
|
||||
|
||||
1. Path absent, or present and empty → initialize v2. Create the tree, write `FORMAT`,
|
||||
|
|
|
|||
Loading…
Reference in New Issue