From ede6d3aa0706c5d6aff11c44b3be34e9d9131a09 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Wed, 19 Aug 2026 20:01:03 -0400 Subject: [PATCH 1/5] fix(frontend): write the config atomically and durably `fs::write` truncates the target and then writes. Anything that interrupts it -- a crash, a kill, a full disk -- leaves the user holding a truncated or empty `config.toml`, which is every keybinding, palette, shader preset, HD-pack mapping and per-game setting they have. That window stopped being theoretical when saves became automatic. The function is called from more than a dozen places and several are not user actions at all: closing a ROM, moving a mixer slider, and (v2.3.9) finishing a Latency Oracle measurement all save without being asked. Now write to a SIBLING scratch file, fsync it, and rename over the target. Each part earns its place, and two of the three came from review rather than from the first draft. SIBLING, because across a filesystem boundary `rename` is not a rename -- a `$TMPDIR` on another mount would silently degrade this back to a copy, reintroducing the failure being fixed. FSYNC, because `fs::write` returns once the bytes are in the page cache, not once they are on the medium. `rename` is atomic with respect to other processes, but a power loss between the two leaves the directory entry pointing at a file whose contents never reached disk -- exactly the outcome this function claims to prevent. The parent directory is synced afterwards too, best-effort and Unix-gated, since on POSIX the entry `rename` creates is itself only a cache update until the directory is synced. A PROCESS-ID in the scratch name, because a bare `.tmp` is shared: two RustyNES instances saving at once would write the same file and one would rename the other's half-written bytes over the config. A stale scratch file from a crashed run also cannot block a later save, since that run has a different id. `tempfile::NamedTempFile` would do this more tidily and is deliberately not used -- `tempfile` is a DEV-dependency, and promoting it to a runtime dependency of a shipped binary is a supply-chain decision, not a cleanup. The existing file's PERMISSIONS are carried onto the replacement. This is the one thing write-then-rename gives up relative to a truncating write: `fs::write` preserves an existing file's mode, while a fresh scratch file takes the umask default and the rename carries that mode with it. A user who had tightened `config.toml` to 0600 would have found it quietly widened by an automatic save they never asked for. Failure ordering is deliberate. A failed rename leaves the old config intact -- the stale-but-valid file is the one worth keeping -- and both failure paths remove the scratch file so a full disk does not accumulate them, best-effort because that removal can fail for the same reason the write did. Three tests. The leftover-scratch assertion reads the DIRECTORY rather than reconstructing a filename, because with a pid in the name a reconstruction would check a file no implementation creates -- an assertion that passes for the wrong reason. The permissions assertion was written before its fix and FAILED (`left: 420, right: 384`), which is stronger evidence than a mutation. And a mutation exposed a real gap: deleting the rename-failure cleanup left every test passing, so `a_failed_rename_cleans_up_after_itself` covers it by renaming onto a directory, which fails portably. Stated rather than papered over: dropping the `sync_all` is caught by NO test here. Durability is only observable across a power loss, which an in-process test cannot stage. The fsync is justified by the mechanism, not by coverage. NOT extended to `save_state.rs` and `cheats.rs`, which use the same truncating `fs::write` on user data -- and a truncated save state is arguably worse, being a user's game progress. They want the same treatment through a shared helper rather than a third copy of this reasoning, which is a wider change than this one. --- crates/rustynes-frontend/src/config.rs | 200 ++++++++++++++++++++++++- 1 file changed, 199 insertions(+), 1 deletion(-) diff --git a/crates/rustynes-frontend/src/config.rs b/crates/rustynes-frontend/src/config.rs index 9bb328dc..1f4063c4 100644 --- a/crates/rustynes-frontend/src/config.rs +++ b/crates/rustynes-frontend/src/config.rs @@ -2121,7 +2121,110 @@ impl Config { fs::create_dir_all(parent)?; } let s = toml::to_string_pretty(self)?; - fs::write(path, s)?; + + // Written to a sibling temporary file and RENAMED over the target, + // rather than `fs::write` straight onto it. + // + // `fs::write` truncates first and then writes. Anything that interrupts + // it -- a crash, a kill, a full disk -- leaves the user holding a + // truncated or empty `config.toml`, which is every keybinding, palette, + // shader preset, HD-pack mapping and per-game setting they have. + // + // That stopped being a theoretical window when saves became automatic. + // This is called from more than a dozen places, several of them not user + // actions at all: closing a ROM, changing a mixer slider, and (v2.3.9) + // finishing a Latency Oracle measurement all save without being asked. + // + // `rename` within a directory is atomic on both platforms this ships on: + // POSIX guarantees it, and `std::fs::rename` maps to `MoveFileEx` with + // `MOVEFILE_REPLACE_EXISTING` on Windows. The temp file is a SIBLING for + // exactly that reason -- across a filesystem boundary `rename` is not a + // rename at all, and a `$TMPDIR` on another mount would silently degrade + // this back to a copy. + // + // A failed rename leaves the old config intact and the temp file behind, + // which is the right way round: the stale-but-valid file is the one worth + // keeping. The temp file is removed on a write failure so a full disk + // does not accumulate them. + // + // The temp name carries the PROCESS ID. A bare `.tmp` is shared, so two + // RustyNES instances saving at once would write the same scratch file and + // one would rename the other's half-written bytes over the config -- the + // failure this function exists to prevent, reintroduced by its own + // mechanism. A stale `.tmp` left by a crashed run also cannot block a + // later save, because that run has a different id. (Review on #420.) + // + // `tempfile::NamedTempFile` would give the same guarantee more tidily, and + // is deliberately not used: `tempfile` is a DEV-dependency here, and + // promoting it to a runtime dependency of a binary that ships to users is + // a supply-chain decision, not a cleanup. + let mut tmp_os = path.as_os_str().to_os_string(); + tmp_os.push(format!(".{}.tmp", std::process::id())); + let tmp = PathBuf::from(tmp_os); + + // Write, then FSYNC, then rename -- in that order, and the fsync is not + // optional. + // + // `fs::write` returns once the bytes are in the OS page cache, not once + // they are on the medium. `rename` is atomic with respect to other + // processes, but a power loss between the two leaves the directory entry + // pointing at a file whose contents never reached disk: an empty or + // truncated config, which is precisely the outcome this function claims to + // prevent. Review on #420 caught that the first version stopped one step + // short and would have shipped a durability guarantee it did not have. + let write_result = (|| -> std::io::Result<()> { + use std::io::Write as _; + let mut f = fs::File::create(&tmp)?; + f.write_all(s.as_bytes())?; + f.sync_all() + })(); + if let Err(e) = write_result { + // Best-effort: if the write failed because the disk is full, the + // remove may fail too, and the original config is still intact. + let _ = fs::remove_file(&tmp); + return Err(e.into()); + } + + // Carry the existing file's permissions onto the replacement. + // + // This is the one thing write-then-rename gives up relative to a + // truncating write, and review on #420 caught it: `fs::write` onto an + // existing file preserves that file's mode, while a fresh temp file + // takes the process umask default and the rename carries that mode with + // it. A user who had tightened `config.toml` to 0600 would have found it + // quietly widened by an automatic save they never asked for. + // + // Best-effort, and only when there IS an existing file to copy from: a + // first-ever save has no prior mode, and a filesystem that cannot report + // or set one should not cost the user an atomic write. Unix-gated + // because that is where the mode lives; on Windows the ACL is inherited + // from the parent directory rather than carried on the file, so + // `MoveFileEx` already produces the right result. + #[cfg(unix)] + if let Ok(meta) = fs::metadata(path) { + use std::os::unix::fs::PermissionsExt; + let mode = meta.permissions().mode(); + let _ = fs::set_permissions(&tmp, fs::Permissions::from_mode(mode)); + } + + if let Err(e) = fs::rename(&tmp, path) { + let _ = fs::remove_file(&tmp); + return Err(e.into()); + } + + // Durability of the RENAME itself, as opposed to the file's contents. + // + // On POSIX the directory entry created by `rename` is also only a cache + // update until the containing directory is synced. Best-effort, and + // Unix-gated: opening a directory as a `File` is not portable, and + // `MoveFileEx` on Windows already orders the metadata write. + #[cfg(unix)] + if let Some(parent) = path.parent() + && let Ok(dir) = fs::File::open(parent) + { + let _ = dir.sync_all(); + } + Ok(()) } } @@ -2205,6 +2308,101 @@ mod tests { ); } + /// A FAILED rename must not leave its scratch file behind either. + /// + /// Added because a mutation exposed the gap rather than because it was + /// planned: deleting the cleanup on the rename-failure path left every test + /// passing. The success path is easy to cover and the failure path is the one + /// that accumulates litter on a user's disk. + /// + /// `rename`-onto-a-directory is the portable way to force the failure -- it + /// fails on Unix (`EISDIR`/`ENOTDIR`) and on Windows -- without needing a + /// read-only mount or a full disk. + #[test] + fn a_failed_rename_cleans_up_after_itself() { + let tmpdir = TempDir::new().expect("temp dir"); + let path = tmpdir.path().join("config.toml"); + // The target is a DIRECTORY, so the rename cannot succeed. + std::fs::create_dir(&path).expect("mkdir"); + + let err = Config::default().save_to(&path); + assert!(err.is_err(), "renaming onto a directory reported success"); + + let leftovers: Vec<_> = std::fs::read_dir(tmpdir.path()) + .expect("read dir") + .filter_map(Result::ok) + .map(|e| e.file_name().to_string_lossy().into_owned()) + .filter(|n| n != "config.toml") + .collect(); + assert!( + leftovers.is_empty(), + "a failed save left its scratch file behind: {leftovers:?}" + ); + assert!(path.is_dir(), "the existing target was destroyed"); + } + + /// A save must not leave a truncated config behind, and must not leave its + /// temporary file behind either. + /// + /// Both halves are asserted. A `save_to` that wrote to the temp file and + /// never renamed would leave the target stale and pass a "no `.tmp` file" + /// check only by accident; one that renamed but wrote nothing would pass a + /// round-trip check on an empty file. + #[test] + fn saving_leaves_no_temp_file_and_a_readable_config() { + // `TempDir` rather than a hand-named directory under `temp_dir()`: it is + // already this module's convention, cannot collide between concurrent + // runs, and cleans up even when an assertion panics. (Review on #420.) + let tmpdir = TempDir::new().expect("temp dir"); + let path = tmpdir.path().join("nested").join("config.toml"); + + let mut c = Config::default(); + c.input.turbo_a = true; + c.save_to(&path).expect("save"); + + // Assert on the DIRECTORY, not on a reconstructed temp filename. The + // scratch name now carries the process id, so a test that rebuilt + // `path + ".tmp"` would be checking a file that never existed under any + // implementation -- an assertion that passes for the wrong reason. + let leftovers: Vec<_> = std::fs::read_dir(path.parent().expect("parent")) + .expect("read dir") + .filter_map(Result::ok) + .map(|e| e.file_name().to_string_lossy().into_owned()) + .filter(|n| n != "config.toml") + .collect(); + assert!( + leftovers.is_empty(), + "a scratch file survived a successful save: {leftovers:?}" + ); + + let back: Config = toml::from_str(&std::fs::read_to_string(&path).expect("read")) + .expect("the written config did not parse"); + assert!(back.input.turbo_a, "the saved value did not round-trip"); + + // Overwriting an existing file is the common case (`rename` must replace). + c.input.turbo_a = false; + c.save_to(&path).expect("second save"); + let back2: Config = + toml::from_str(&std::fs::read_to_string(&path).expect("read")).expect("parse"); + assert!( + !back2.input.turbo_a, + "the second save did not replace the first" + ); + + // The mode of an existing file must survive the replacement. + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)).expect("chmod"); + c.save_to(&path).expect("third save"); + let mode = std::fs::metadata(&path).expect("stat").permissions().mode() & 0o777; + assert_eq!( + mode, 0o600, + "an atomic save widened the user's file permissions" + ); + } + } + #[test] fn parse_pal_reads_64_colours_and_rejects_short() { // A 192-byte file → 64 RGB triples, in order. From e711db9fad7243bd99d158c468b376ccf088e8f7 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Wed, 19 Aug 2026 20:24:58 -0400 Subject: [PATCH 2/5] fix(frontend): resolve a symlinked config, and make the scratch name unique per call Two of three blocking findings hold. The third does not. SYMLINK -- real, and a regression the fix itself would have introduced. `fs::write` follows a symlink and writes through to its target; `fs::rename` replaces the link. A user who has symlinked `config.toml` into a dotfiles repository, which is a common setup, would have found the link silently converted to a regular file on the first automatic save, and their repository would stop receiving changes. The path is now resolved with `canonicalize` before anything is written, and everything downstream -- the scratch sibling, the permission copy, the rename, the directory sync -- uses the resolved target. `canonicalize` fails when the file does not exist yet, which is the first-save case, so it falls back to the path as given. Pinned by `saving_through_a_symlink_writes_the_target_and_keeps_the_link`, and the mutation that reverts the resolution fails it. CONCURRENCY -- accepted, though not reachable today. Config saves are driven from the UI thread, so two concurrent `save_to` calls cannot happen; but that is a property of the callers rather than of this function, and a relaxed fetch-add on a module-level counter makes the guarantee structural for one instruction. Stated plainly: removing the counter is caught by NO test, because a collision needs two threads inside this function at once and the callers cannot produce that. LET-CHAINS -- declined, not reproducible. The claim was that `if let Some(..) = .. && let Ok(..) = ..` is unstable and will not compile. Let-chains are stable in edition 2024, `Cargo.toml` sets `edition = "2024"` and `rust-toolchain.toml` pins 1.96.0, `main` already carries the same construct in `debugger/mod.rs:2292`, and this branch compiles clean under `-D warnings` across the workspace and all four wasm32 combinations. The counter is a module-level `static` rather than a function-local one because clippy's `items_after_statements` fires on the latter -- and it is right that an item declared mid-function reads as if it were scoped to that point when it is not. --- crates/rustynes-frontend/src/config.rs | 87 ++++++++++++++++++++++++-- 1 file changed, 82 insertions(+), 5 deletions(-) diff --git a/crates/rustynes-frontend/src/config.rs b/crates/rustynes-frontend/src/config.rs index 1f4063c4..121782ef 100644 --- a/crates/rustynes-frontend/src/config.rs +++ b/crates/rustynes-frontend/src/config.rs @@ -2158,8 +2158,40 @@ impl Config { // is deliberately not used: `tempfile` is a DEV-dependency here, and // promoting it to a runtime dependency of a binary that ships to users is // a supply-chain decision, not a cleanup. - let mut tmp_os = path.as_os_str().to_os_string(); - tmp_os.push(format!(".{}.tmp", std::process::id())); + // Resolve a SYMLINKED config to its target before choosing where to write. + // + // `fs::write` follows a symlink and writes through to the file it points + // at. `fs::rename` replaces the link itself. Without this, a user who has + // symlinked `config.toml` into a dotfiles repository -- a common setup -- + // would find the link silently replaced by a regular file on the first + // automatic save, and their repository stops receiving changes. That is a + // behaviour regression introduced by the fix, not by the bug. (Review on + // #420.) + // + // `canonicalize` fails when the path does not exist yet, which is exactly + // the first-save case, so it falls back to the path as given. + let target = fs::canonicalize(path).unwrap_or_else(|_| path.to_path_buf()); + + // The scratch name carries the process id AND a per-call counter. + // + // A bare `.tmp` is shared across processes: two RustyNES instances saving + // at once would write the same file and one would rename the other's + // half-written bytes over the config -- the failure this function exists + // to prevent, reintroduced by its own mechanism. The pid closes that. + // + // The counter closes the same hole WITHIN a process. Config saves are + // driven from the UI thread today, so two concurrent calls are not + // reachable, but "not reachable today" is a property of the callers rather + // than of this function, and a monotonic counter costs one relaxed fetch-add + // to make the guarantee structural. (Review on #420 raised it.) + // + // `tempfile::NamedTempFile` would give both properties more tidily and is + // deliberately not used: `tempfile` is a DEV-dependency here, and promoting + // it to a runtime dependency of a binary that ships to users is a + // supply-chain decision, not a cleanup. + let seq = SCRATCH_SEQ.fetch_add(1, std::sync::atomic::Ordering::Relaxed); + let mut tmp_os = target.as_os_str().to_os_string(); + tmp_os.push(format!(".{}.{seq}.tmp", std::process::id())); let tmp = PathBuf::from(tmp_os); // Write, then FSYNC, then rename -- in that order, and the fsync is not @@ -2201,13 +2233,13 @@ impl Config { // from the parent directory rather than carried on the file, so // `MoveFileEx` already produces the right result. #[cfg(unix)] - if let Ok(meta) = fs::metadata(path) { + if let Ok(meta) = fs::metadata(&target) { use std::os::unix::fs::PermissionsExt; let mode = meta.permissions().mode(); let _ = fs::set_permissions(&tmp, fs::Permissions::from_mode(mode)); } - if let Err(e) = fs::rename(&tmp, path) { + if let Err(e) = fs::rename(&tmp, &target) { let _ = fs::remove_file(&tmp); return Err(e.into()); } @@ -2219,7 +2251,7 @@ impl Config { // Unix-gated: opening a directory as a `File` is not portable, and // `MoveFileEx` on Windows already orders the metadata write. #[cfg(unix)] - if let Some(parent) = path.parent() + if let Some(parent) = target.parent() && let Ok(dir) = fs::File::open(parent) { let _ = dir.sync_all(); @@ -2229,6 +2261,13 @@ impl Config { } } +/// Monotonic sequence for [`Config::save_to`]'s scratch filenames. +/// +/// Module scope rather than a function-local `static`, because clippy's +/// `items_after_statements` fires on the latter -- and it is right that an item +/// declared mid-function reads as if it were scoped to that point when it is not. +static SCRATCH_SEQ: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); + /// Rewrite every keycode value of a [`PadBindings`] to its canonical /// current winit-0.30 `KeyCode` spelling. Used by [`Config::migrate_legacy`] /// to clean up legacy winit-0.29 `VirtualKeyCode` value strings carried in @@ -2308,6 +2347,44 @@ mod tests { ); } + /// A symlinked config must stay a symlink. + /// + /// `fs::write` follows a symlink and writes through to its target; + /// `fs::rename` replaces the link itself. Without resolving the path first, + /// the atomic-write fix would silently convert a user's `config.toml` symlink + /// -- a dotfiles-repository setup is the common case -- into a regular file on + /// the first automatic save, and their repository would stop receiving + /// changes. That is a regression introduced by the fix rather than by the bug, + /// which is why it is pinned rather than left to the reasoning. (Review on + /// #420.) + #[cfg(unix)] + #[test] + fn saving_through_a_symlink_writes_the_target_and_keeps_the_link() { + let tmpdir = TempDir::new().expect("temp dir"); + let real = tmpdir.path().join("real-config.toml"); + let link = tmpdir.path().join("config.toml"); + std::fs::write(&real, "").expect("seed target"); + std::os::unix::fs::symlink(&real, &link).expect("symlink"); + + let mut c = Config::default(); + c.input.turbo_a = true; + c.save_to(&link).expect("save through the link"); + + assert!( + std::fs::symlink_metadata(&link) + .expect("stat link") + .file_type() + .is_symlink(), + "the save replaced the symlink with a regular file" + ); + let back: Config = toml::from_str(&std::fs::read_to_string(&real).expect("read target")) + .expect("target parses"); + assert!( + back.input.turbo_a, + "the save did not reach the symlink's target" + ); + } + /// A FAILED rename must not leave its scratch file behind either. /// /// Added because a mutation exposed the gap rather than because it was From 68f57327d9b8d11ba9188df0807586909221f0d5 Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Wed, 19 Aug 2026 20:47:30 -0400 Subject: [PATCH 3/5] fix(frontend): create the scratch file exclusively, and with the target's mode Two review points, one a real hardening and one a repeat of a claim the build refutes. CWE-377 -- accepted. `File::create` follows symlinks and truncates, so a predictable scratch name is a surface: something pre-created at that path as a link to another file would be silently truncated and overwritten by the save. `create_new(true)` makes the open exclusive, so anything already there is a failed save rather than a destroyed file. Two things worth recording about it. The scratch file is a sibling of the user's own config, not a world-writable directory, so an attacker who can plant a file there already owns the config -- this is defence in depth rather than a live hole, and it costs one call. And exclusive creation was NOT safe to adopt two commits ago: with a bare `.tmp` name, a stale file from a crashed run would have failed every subsequent save. The pid and per-call counter removed that failure mode, so the objection that ruled it out no longer applies. The order the three changes landed in mattered. Permissions TOCTOU -- accepted. The mode is now applied at creation via `OpenOptionsExt::mode` rather than chmod-ed afterwards, closing a window in which the file existed at the umask default -- briefly wider than the config the user had tightened. `open(2)` masks the requested mode with the umask, so creation alone can land narrower than the original; the exact mode is still set after, which makes the pair narrow-then-correct rather than widen-then-narrow. Let-chains -- declined again, same evidence. Let-chains are stable in edition 2024; `Cargo.toml` sets it, `rust-toolchain.toml` pins stable 1.96.0, `main` carries the same construct at `debugger/mod.rs:2292`, and this branch compiles clean under `-D warnings` across the workspace and all four wasm32 combinations. A compile error would not be subtle. `an_occupied_scratch_path_fails_the_save_without_truncating_it` plants a symlink decoy at every scratch name the process can produce next and asserts both halves: the decoy's target is not truncated, and the existing config still holds the PREVIOUS save rather than being damaged by the failed one. Reverting `create_new` to `create` fails it. --- crates/rustynes-frontend/src/config.rs | 100 ++++++++++++++++++++++++- 1 file changed, 97 insertions(+), 3 deletions(-) diff --git a/crates/rustynes-frontend/src/config.rs b/crates/rustynes-frontend/src/config.rs index 121782ef..19198ddf 100644 --- a/crates/rustynes-frontend/src/config.rs +++ b/crates/rustynes-frontend/src/config.rs @@ -2204,9 +2204,46 @@ impl Config { // truncated config, which is precisely the outcome this function claims to // prevent. Review on #420 caught that the first version stopped one step // short and would have shipped a durability guarantee it did not have. + // The mode to create the scratch file WITH, read before it exists. + // + // Applying it at creation rather than chmod-ing afterwards closes a window + // in which the file exists at the umask default -- briefly wider than the + // config the user tightened. `open(2)` applies `mode & ~umask`, so this can + // only ever be narrower than asked; the exact mode is still set below, + // which makes the pair a narrow-then-correct sequence rather than a + // widen-then-narrow one. (Review on #420.) + #[cfg(unix)] + let existing_mode = { + use std::os::unix::fs::PermissionsExt; + fs::metadata(&target).ok().map(|m| m.permissions().mode()) + }; + let write_result = (|| -> std::io::Result<()> { use std::io::Write as _; - let mut f = fs::File::create(&tmp)?; + let mut opts = fs::OpenOptions::new(); + // `create_new` rather than `create`: exclusive creation, so the open + // FAILS if anything is already at that path instead of truncating it. + // + // `File::create` follows symlinks and truncates, so a predictable + // scratch name is a CWE-377 surface -- something pre-created there as a + // link to another file would be silently truncated and overwritten by + // the save. The scratch file is a sibling of the user's own config + // rather than a world-writable directory, so an attacker who can plant + // it already owns the config; that makes this defence in depth rather + // than a live hole, and it costs one call. (Review on #420.) + // + // Exclusive creation was NOT safe to adopt while the scratch name was a + // bare `.tmp`: a stale file from a crashed run would then have failed + // every subsequent save. With the pid and the per-call counter in the + // name, a previous run cannot collide, so the failure mode that ruled + // this out no longer exists. + opts.write(true).create_new(true); + #[cfg(unix)] + if let Some(mode) = existing_mode { + use std::os::unix::fs::OpenOptionsExt as _; + opts.mode(mode); + } + let mut f = opts.open(&tmp)?; f.write_all(s.as_bytes())?; f.sync_all() })(); @@ -2232,10 +2269,13 @@ impl Config { // because that is where the mode lives; on Windows the ACL is inherited // from the parent directory rather than carried on the file, so // `MoveFileEx` already produces the right result. + // The exact mode, after creation. `open(2)` masks the requested mode with + // the umask, so creation alone can land narrower than the original; this + // makes it exact. Best-effort, and only when there was a prior file to copy + // from. #[cfg(unix)] - if let Ok(meta) = fs::metadata(&target) { + if let Some(mode) = existing_mode { use std::os::unix::fs::PermissionsExt; - let mode = meta.permissions().mode(); let _ = fs::set_permissions(&tmp, fs::Permissions::from_mode(mode)); } @@ -2347,6 +2387,60 @@ mod tests { ); } + /// The scratch file is created EXCLUSIVELY, so anything already sitting at + /// that path is a hard failure rather than something to truncate. + /// + /// `File::create` follows symlinks and truncates, which makes a predictable + /// scratch name a CWE-377 surface. `create_new` turns that into a failed save. + /// The save must still leave the real config untouched, which is the half that + /// matters: a hostile or merely stale scratch file must not cost the user + /// their settings. (Review on #420.) + #[cfg(unix)] + #[test] + fn an_occupied_scratch_path_fails_the_save_without_truncating_it() { + let tmpdir = TempDir::new().expect("temp dir"); + let path = tmpdir.path().join("config.toml"); + let mut c = Config::default(); + c.input.turbo_a = true; + c.save_to(&path).expect("first save"); + + // Occupy every scratch name this process can produce next, by driving the + // counter and planting a decoy at each candidate. One of them will be the + // name the next save picks. + let decoy = tmpdir.path().join("decoy.txt"); + std::fs::write(&decoy, "do not truncate me").expect("decoy"); + let mut planted = Vec::new(); + for seq in 0..64u64 { + let mut name = path.as_os_str().to_os_string(); + name.push(format!(".{}.{seq}.tmp", std::process::id())); + let p = std::path::PathBuf::from(name); + if std::os::unix::fs::symlink(&decoy, &p).is_ok() { + planted.push(p); + } + } + assert!(!planted.is_empty(), "planted no decoys"); + + // The save must fail rather than follow a planted link. + let mut c2 = Config::default(); + c2.input.turbo_b = true; + let _ = c2.save_to(&path); + + assert_eq!( + std::fs::read_to_string(&decoy).expect("decoy readable"), + "do not truncate me", + "the save followed a planted symlink and truncated its target" + ); + let back: Config = toml::from_str(&std::fs::read_to_string(&path).expect("read")) + .expect("config still parses"); + assert!( + back.input.turbo_a && !back.input.turbo_b, + "a failed save damaged the existing config" + ); + for p in planted { + let _ = std::fs::remove_file(p); + } + } + /// A symlinked config must stay a symlink. /// /// `fs::write` follows a symlink and writes through to its target; From 61ba33c174337857f2cf34d06e848708bf69c42c Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Wed, 19 Aug 2026 21:22:12 -0400 Subject: [PATCH 4/5] fix(frontend): follow a BROKEN symlink too, and prove the chain case Review found the symlink fix surviving inside its own gap, and it is the more likely of the two cases in practice. An INTACT link resolves through `canonicalize`. A BROKEN one -- a freshly created dotfiles link whose target does not exist yet -- makes `canonicalize` fail with `NotFound`, and the fallback then used the link's own path, so the rename destroyed exactly the setup the resolution exists to protect. `ln -s` then launch is the natural order, so the broken case is the one a user hits first. Resolution is now its own function with the two cases named. `read_link` is what handles the broken link, and its `EINVAL` on a non-symlink is how "nothing to follow" is distinguished from "broken link" without a second `symlink_metadata` call. A relative destination is joined to the link's own directory. A mutation then exposed something about the fix rather than the bug: removing the `canonicalize` branch entirely did NOT fail the intact-link test, because `read_link` satisfies it -- one hop is enough for one link. It is not enough for a CHAIN: `read_link` resolves exactly one level, so `a -> b -> c` would write to `b` and destroy the second link. `saving_through_a_chain_of_symlinks_reaches_the_real_file` covers that, and removing `canonicalize` now fails it. The branch was load-bearing all along; nothing said so. Three symlink tests where there was one, each mutation-checked: intact, broken, and chained. The remaining review points are answered rather than changed. The `let _ =` on `set_permissions` and on the directory `sync_all` are best-effort BY DESIGN -- failing a save because a chmod or a directory fsync failed would cost the user the settings change they asked for, to protect a property that is already a hardening rather than the correctness guarantee. And `canonicalize` swallowing a non-`NotFound` error is now moot: every failure falls through to `read_link`, which either resolves the link or returns the path unchanged, and any genuine permission problem surfaces at the open or the rename with a real error. Let-chains, third round: stable in edition 2024, `main` carries the same construct at `debugger/mod.rs:2292`, and `CI success` is green on this very PR. That check is the disproof. --- crates/rustynes-frontend/src/config.rs | 126 ++++++++++++++++++++++++- 1 file changed, 122 insertions(+), 4 deletions(-) diff --git a/crates/rustynes-frontend/src/config.rs b/crates/rustynes-frontend/src/config.rs index 19198ddf..258f0081 100644 --- a/crates/rustynes-frontend/src/config.rs +++ b/crates/rustynes-frontend/src/config.rs @@ -2111,13 +2111,52 @@ impl Config { self.save_to(&path) } + /// Where a save should actually land, following a symlink to its target. + /// + /// `fs::write` follows a symlink and writes through to the file it points at; + /// `fs::rename` replaces the link. Writing to the link's own path would + /// therefore convert a user's symlinked `config.toml` -- a dotfiles-repository + /// setup -- into a regular file on the first automatic save. + /// + /// Two cases, and the second is the one that is easy to miss: + /// + /// * An **intact** link resolves through `canonicalize`. + /// * A **broken** link -- pointing at a file that does not exist yet, which is + /// exactly a freshly-created dotfiles link awaiting its first save -- + /// makes `canonicalize` fail with `NotFound`. Falling back to the link's own + /// path there would destroy the very setup the resolution exists to protect, + /// so the link is read by hand instead and its destination used, relative to + /// the link's own directory when it is not absolute. (Review on #420 found + /// this surviving inside the fix for the intact case.) + /// + /// Anything that is not a symlink -- including a path that does not exist at + /// all, the first-ever save -- falls back to the path as given, which is + /// correct: there is nothing to follow. + fn resolve_write_target(path: &Path) -> PathBuf { + if let Ok(real) = fs::canonicalize(path) { + return real; + } + // `read_link` fails with `EINVAL` when the path is not a link at all, which + // is how "no symlink to follow" is distinguished from "broken symlink" + // without a second `symlink_metadata` call. + match fs::read_link(path) { + Ok(dest) if dest.is_absolute() => dest, + Ok(dest) => match path.parent() { + Some(dir) => dir.join(dest), + None => dest, + }, + Err(_) => path.to_path_buf(), + } + } + /// Save to an explicit path. /// /// # Errors /// /// Returns [`ConfigError`] on I/O or serialization failure. pub fn save_to(&self, path: &Path) -> Result<(), ConfigError> { - if let Some(parent) = path.parent() { + let target = Self::resolve_write_target(path); + if let Some(parent) = target.parent() { fs::create_dir_all(parent)?; } let s = toml::to_string_pretty(self)?; @@ -2168,9 +2207,6 @@ impl Config { // behaviour regression introduced by the fix, not by the bug. (Review on // #420.) // - // `canonicalize` fails when the path does not exist yet, which is exactly - // the first-save case, so it falls back to the path as given. - let target = fs::canonicalize(path).unwrap_or_else(|_| path.to_path_buf()); // The scratch name carries the process id AND a per-call counter. // @@ -2387,6 +2423,88 @@ mod tests { ); } + /// A CHAIN of symlinks resolves all the way to the real file. + /// + /// Added because a mutation exposed that the intact-link test did not need + /// `canonicalize` at all: `read_link` alone satisfies it, since one hop is + /// enough for a single link. It is not enough for a chain -- `read_link` + /// resolves exactly one level, so `a -> b -> c` would write to `b` and destroy + /// the second link. `canonicalize` is what makes the multi-hop case correct, + /// and this is what makes that load-bearing rather than incidental. + #[cfg(unix)] + #[test] + fn saving_through_a_chain_of_symlinks_reaches_the_real_file() { + let tmpdir = TempDir::new().expect("temp dir"); + let real = tmpdir.path().join("real.toml"); + let mid = tmpdir.path().join("mid.toml"); + let link = tmpdir.path().join("config.toml"); + std::fs::write(&real, "").expect("seed"); + std::os::unix::fs::symlink(&real, &mid).expect("link 1"); + std::os::unix::fs::symlink(&mid, &link).expect("link 2"); + + let mut c = Config::default(); + c.input.turbo_a = true; + c.save_to(&link).expect("save through the chain"); + + for l in [&link, &mid] { + assert!( + std::fs::symlink_metadata(l) + .expect("stat") + .file_type() + .is_symlink(), + "a link in the chain was replaced by a regular file: {}", + l.display() + ); + } + let back: Config = + toml::from_str(&std::fs::read_to_string(&real).expect("read")).expect("parses"); + assert!( + back.input.turbo_a, + "the save did not reach the end of the chain" + ); + } + + /// A BROKEN symlink must be followed too, not replaced. + /// + /// The intact-link case was fixed first and left this one behind: a freshly + /// created dotfiles link whose target does not exist yet makes `canonicalize` + /// fail with `NotFound`, and falling back to the link's own path destroys + /// exactly the setup the resolution exists to protect. It is the more likely + /// of the two in practice -- `ln -s` then launch is the natural order. + /// (Review on #420 found it surviving inside the fix for the intact case.) + #[cfg(unix)] + #[test] + fn saving_through_a_broken_symlink_creates_the_target_and_keeps_the_link() { + let tmpdir = TempDir::new().expect("temp dir"); + let dotfiles = tmpdir.path().join("dotfiles"); + std::fs::create_dir(&dotfiles).expect("mkdir"); + let real = dotfiles.join("config.toml"); // deliberately does NOT exist + let link = tmpdir.path().join("config.toml"); + std::os::unix::fs::symlink(&real, &link).expect("symlink"); + assert!( + !real.exists(), + "the target must not exist for this test to mean anything" + ); + + let mut c = Config::default(); + c.input.turbo_a = true; + c.save_to(&link).expect("save through the broken link"); + + assert!( + std::fs::symlink_metadata(&link) + .expect("stat link") + .file_type() + .is_symlink(), + "the save replaced a broken symlink with a regular file" + ); + let back: Config = toml::from_str(&std::fs::read_to_string(&real).expect("read target")) + .expect("target parses"); + assert!( + back.input.turbo_a, + "the save did not populate the link's target" + ); + } + /// The scratch file is created EXCLUSIVELY, so anything already sitting at /// that path is a hard failure rather than something to truncate. /// From 3a6a33d2a9a60df50b4ffb077cbb93f455b6901c Mon Sep 17 00:00:00 2001 From: DoubleGate Date: Wed, 19 Aug 2026 21:45:40 -0400 Subject: [PATCH 5/5] fix(frontend): sync the real directory for a bare path, and retry past an orphan One blocking claim refuted by measurement, its accompanying suggestion accepted, and a related point from the v2.4.0 plan review applied to the code it describes. THE BARE-FILENAME SAVE FAILURE DOES NOT REPRODUCE. The claim was that `Path::new("config.toml").parent()` yields `Some("")` and that `create_dir_all("")` then fails with ENOENT, aborting the save. The first half is right and the second is not -- measured rather than argued: parent() -> Some("") create_dir_all("") -> Ok File::open("") -> Err(No such file or directory) So the save is unaffected. What the third line shows is that the SUGGESTION was right for a different reason: the parent-directory fsync sat behind `let Ok(dir) = File::open(parent)`, so for a relative target it silently did not happen. A durability step quietly skipped is worse than one that fails, because nothing reports it. An empty parent now resolves to `.`, which is the directory it means. RETRY PAST AN OCCUPIED SCRATCH NAME. Review of the v2.4.0 plan raised pid reuse against the naming scheme, and it lands on this code: `create_new` turns a collision into a failed save, and a crashed run can leave an orphaned scratch file that a later run with the same pid meets on its first save. Unlikely, and a lost save is a real cost to a user who would have no idea why. Advancing the counter and opening once more turns it into nothing at all, since the counter only increases within a process. Stated rather than implied: the retry is NOT covered by a test, and the mutation that removes it passes. A test would have to predict which scratch name the next save picks, and the counter is process-global, so predicting it means reimplementing the naming inside the test -- an assertion that agrees with itself. That is the third property here justified by mechanism rather than coverage, alongside the `sync_all` and the counter itself, and each is named in place rather than left to be assumed covered. The existing occupied-scratch test still passes for the right reason: it plants decoys at sixty-four consecutive names, so the retry finds one too and the save still fails without truncating anything. --- crates/rustynes-frontend/src/config.rs | 46 +++++++++++++++++++++++--- 1 file changed, 42 insertions(+), 4 deletions(-) diff --git a/crates/rustynes-frontend/src/config.rs b/crates/rustynes-frontend/src/config.rs index 258f0081..4b61ecdd 100644 --- a/crates/rustynes-frontend/src/config.rs +++ b/crates/rustynes-frontend/src/config.rs @@ -2254,6 +2254,17 @@ impl Config { fs::metadata(&target).ok().map(|m| m.permissions().mode()) }; + // Retry once past an occupied scratch name. + // + // `create_new` turns a collision into a failed save, and there is one way + // a collision can happen without an attacker: a crashed run leaves an + // orphaned scratch file, the OS later reuses that pid, and the new run's + // first save picks the same seq. Unlikely, and a lost save is a real cost + // for a user who would have no idea why. Advancing the counter and trying + // again turns it into nothing at all -- the next name cannot be the same + // one, since the counter only increases within a process. (Review on #425 + // raised the pid-reuse case against the plan; it applies here.) + let mut tmp = tmp; let write_result = (|| -> std::io::Result<()> { use std::io::Write as _; let mut opts = fs::OpenOptions::new(); @@ -2279,7 +2290,16 @@ impl Config { use std::os::unix::fs::OpenOptionsExt as _; opts.mode(mode); } - let mut f = opts.open(&tmp)?; + let mut f = match opts.open(&tmp) { + Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => { + let seq = SCRATCH_SEQ.fetch_add(1, std::sync::atomic::Ordering::Relaxed); + let mut retry = target.as_os_str().to_os_string(); + retry.push(format!(".{}.{seq}.tmp", std::process::id())); + tmp = PathBuf::from(retry); + opts.open(&tmp)? + } + other => other?, + }; f.write_all(s.as_bytes())?; f.sync_all() })(); @@ -2326,11 +2346,29 @@ impl Config { // update until the containing directory is synced. Best-effort, and // Unix-gated: opening a directory as a `File` is not portable, and // `MoveFileEx` on Windows already orders the metadata write. + // + // A bare filename's parent is `Some("")`, and `File::open("")` fails with + // `ENOENT` -- so without the fallback the sync would silently not happen + // for a relative target, which is a durability step quietly skipped rather + // than a failure anyone sees. `.` is the directory an empty parent means. + // (Review on #420. `create_dir_all("")` was checked in the same pass and + // returns `Ok`, so the claim that a bare filename aborts the save does not + // reproduce -- only the sync was affected.) #[cfg(unix)] - if let Some(parent) = target.parent() - && let Ok(dir) = fs::File::open(parent) { - let _ = dir.sync_all(); + let parent = target.parent().map_or_else( + || PathBuf::from("."), + |p| { + if p.as_os_str().is_empty() { + PathBuf::from(".") + } else { + p.to_path_buf() + } + }, + ); + if let Ok(dir) = fs::File::open(&parent) { + let _ = dir.sync_all(); + } } Ok(())