From 661144021e2896587b5f9c6a9c740ff8aa6db45f Mon Sep 17 00:00:00 2001 From: notdezzi Date: Wed, 14 Oct 2026 13:11:28 +0200 Subject: [PATCH] fix(format): exclusive unique staging files for atomic save/export Saving and exporting wrote a truncating std::fs::write to a deterministic staging name (.tmp-figcraft, .figcraft.tmp) before renaming, so an unrelated file or a planted symlink at that name was clobbered or written through, and separate writers of one destination shared a staging file they could corrupt. Both paths now go through figcraft_format::write_file_atomic: a uniquely named sibling staging file is created exclusively (create_new) and written through its own handle, renamed over the destination, and removed again on any failure, so only the staging file a write created is ever touched. Tests cover pre-existing staging files and symlinks, concurrent writers, and injected rename and permission failures. --- crates/engine/src/cmd/file.rs | 134 +++++++++++++++++++++++++++-- crates/format/src/lib.rs | 51 +++++++++-- crates/format/src/staging_tests.rs | 125 +++++++++++++++++++++++++++ 3 files changed, 298 insertions(+), 12 deletions(-) create mode 100644 crates/format/src/staging_tests.rs diff --git a/crates/engine/src/cmd/file.rs b/crates/engine/src/cmd/file.rs index 781ed6b0..922729f0 100644 --- a/crates/engine/src/cmd/file.rs +++ b/crates/engine/src/cmd/file.rs @@ -437,15 +437,12 @@ pub(crate) fn read_capped(path: &str, max: u64) -> Result> { read(path) } +/// Write `bytes` to `path` through a uniquely named, exclusively created staging file and one +/// atomic rename (see [`figcraft_format::write_file_atomic`]): a failed or interrupted write never +/// truncates `path`, and no file or symlink at a staging name is ever written through. #[cfg(not(target_arch = "wasm32"))] pub(crate) fn write(path: &str, bytes: &[u8]) -> Result<()> { - let tmp = format!("{path}.tmp-figcraft"); - std::fs::write(&tmp, bytes).map_err(|e| EngineError::Other(format!("{tmp}: {e}")))?; - std::fs::rename(&tmp, path).map_err(|e| { - // Leave nothing behind when the file can't take its place. - let _ = std::fs::remove_file(&tmp); - EngineError::Other(format!("{path}: {e}")) - }) + figcraft_format::write_file_atomic(std::path::Path::new(path), bytes).map_err(|e| EngineError::Other(format!("{path}: {e}"))) } #[cfg(target_arch = "wasm32")] @@ -1327,3 +1324,126 @@ mod rest_import_tests { let _ = std::fs::remove_dir_all(&dir); } } + +#[cfg(test)] +mod staging_write_tests { + use super::*; + + fn dir(name: &str) -> std::path::PathBuf { + let d = std::env::temp_dir().join(format!("figcraft-engine-staging-{name}-{}", std::process::id())); + std::fs::create_dir_all(&d).unwrap(); + d + } + + /// A symlink `from` → `to`; false when the platform won't make one (Windows needs privileges). + #[cfg(unix)] + fn symlink(from: &std::path::Path, to: &std::path::Path) -> bool { + std::os::unix::fs::symlink(to, from).is_ok() + } + + #[cfg(windows)] + fn symlink(from: &std::path::Path, to: &std::path::Path) -> bool { + std::os::windows::fs::symlink_file(to, from).is_ok() + } + + #[test] + fn an_existing_staging_file_is_never_written_through() { + let d = dir("existing"); + let path = d.join("shot.svg"); + // The old deterministic staging name, held by an unrelated file: the write must pick its + // own staging name instead of truncating this one. + let old = d.join("shot.svg.tmp-figcraft"); + std::fs::write(&old, b"not yours").unwrap(); + write(&path.to_string_lossy(), b"").unwrap(); + assert_eq!(std::fs::read(&path).unwrap(), b""); + assert_eq!(std::fs::read(&old).unwrap(), b"not yours", "an unrelated file at a staging name is untouched"); + // Just the file and the untouched staging file: no new staging leftovers. + assert_eq!(std::fs::read_dir(&d).unwrap().count(), 2); + let _ = std::fs::remove_dir_all(&d); + } + + #[test] + fn a_staging_symlink_is_never_followed() { + let d = dir("symlink"); + let path = d.join("shot.svg"); + let victim = d.join("victim.txt"); + std::fs::write(&victim, b"innocent").unwrap(); + let old = d.join("shot.svg.tmp-figcraft"); + if !symlink(&old, &victim) { + return; // Making a symlink needs privileges on some Windows setups. + } + write(&path.to_string_lossy(), b"").unwrap(); + assert_eq!(std::fs::read(&victim).unwrap(), b"innocent", "the symlink was never written through"); + assert_eq!(std::fs::read(&path).unwrap(), b""); + let _ = std::fs::remove_dir_all(&d); + } + + #[test] + fn concurrent_writers_of_one_file_never_interleave() { + let d = dir("concurrent"); + let path = d.join("shot.svg"); + for round in 0..5u8 { + let mut handles = Vec::new(); + for w in 0u8..4 { + let (path, bytes) = (path.clone(), vec![b'0' + w; 8192]); + handles.push(std::thread::spawn(move || write(&path.to_string_lossy(), &bytes))); + } + let mut wins = 0; + for h in handles { + // On Windows a rename over a file another writer just replaced can lose the race. + if h.join().unwrap().is_ok() { + wins += 1; + } + } + assert!(wins > 0, "round {round}: every writer lost"); + // Whatever landed is one writer's complete payload, never a mix. + let got = std::fs::read(&path).unwrap(); + assert_eq!(got.len(), 8192, "round {round}: a complete payload landed"); + assert!(got.windows(2).all(|w| w[0] == w[1]), "round {round}: one writer's payload, not a mix"); + } + // No staging file was left stuck (a failed writer removes its own). + let left = std::fs::read_dir(&d).unwrap().count(); + assert!(left <= if cfg!(windows) { 5 } else { 1 }, "{left} files left in the staging directory"); + let _ = std::fs::remove_dir_all(&d); + } + + #[test] + fn a_failed_write_leaves_the_destination_alone_and_cleans_up() { + let d = dir("failure"); + // The destination is a directory: the rename fails, our staging file is removed and the + // directory is untouched. + let taken = d.join("over-a-directory"); + std::fs::create_dir_all(&taken).unwrap(); + assert!(write(&taken.to_string_lossy(), b"payload").is_err()); + assert!(taken.is_dir()); + assert_eq!(std::fs::read_dir(&d).unwrap().count(), 1, "no staging file left behind"); + // A missing parent directory fails without creating anything anywhere. + assert!(write(&d.join("no").join("such.svg").to_string_lossy(), b"payload").is_err()); + assert!(!d.join("no").exists()); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + // A read-only directory refuses the staging file, and the old destination stays. + let ro = d.join("read-only"); + std::fs::create_dir_all(&ro).unwrap(); + let dest = ro.join("keep.svg"); + std::fs::write(&dest, b"keep").unwrap(); + let mut perms = std::fs::metadata(&ro).unwrap().permissions(); + perms.set_mode(0o500); + std::fs::set_permissions(&ro, perms).unwrap(); + // Running as root would defeat the read-only directory: skip instead of failing. + let probe = ro.join(".probe"); + if std::fs::write(&probe, b"").is_ok() { + let _ = std::fs::remove_file(&probe); + } else { + assert!(write(&dest.to_string_lossy(), b"payload").is_err()); + assert_eq!(std::fs::read(&dest).unwrap(), b"keep"); + assert_eq!(std::fs::read_dir(&ro).unwrap().count(), 1, "no staging file left behind"); + } + let mut perms = std::fs::metadata(&ro).unwrap().permissions(); + perms.set_mode(0o700); + std::fs::set_permissions(&ro, perms).unwrap(); + } + let _ = std::fs::remove_dir_all(&d); + } +} diff --git a/crates/format/src/lib.rs b/crates/format/src/lib.rs index a989f3f6..56798d14 100644 --- a/crates/format/src/lib.rs +++ b/crates/format/src/lib.rs @@ -654,14 +654,53 @@ fn read_branches(z: &mut zip::ZipArchive, images: &BTreeMap) -> Result<(), FormatError> { let bytes = save_bytes(doc, thumbnail_png)?; - let tmp = path.with_extension("figcraft.tmp"); - std::fs::write(&tmp, &bytes).map_err(|e| FormatError::Io(format!("{}: {e}", tmp.display())))?; - std::fs::rename(&tmp, path).map_err(|e| FormatError::Io(format!("{}: {e}", path.display())))?; - Ok(()) + write_file_atomic(path, &bytes).map_err(|e| FormatError::Io(format!("{}: {e}", path.display()))) +} + +/// Write `bytes` to `path` through a staging file and one atomic rename, so a reader of `path` +/// sees either the previous file or the complete new one, never a partial write. The staging file +/// is a uniquely named sibling (`.tmp-figcraft--`) created exclusively +/// (`create_new`) and written through its own handle: a file or symlink that already exists at a +/// staging name is never truncated or followed, and separate writers (also of separate processes) +/// never share a staging file. On failure only the staging file this call created is removed, and +/// `path` is replaced only on success. The rename replaces an existing destination on every +/// platform (`std::fs::rename` uses `MoveFileEx` with `MOVEFILE_REPLACE_EXISTING` on Windows). +/// (No `sync_all` yet: nothing around here promises durability across a power cut.) +#[cfg(not(target_arch = "wasm32"))] +pub fn write_file_atomic(path: &std::path::Path, bytes: &[u8]) -> std::io::Result<()> { + use std::sync::atomic::{AtomicU64, Ordering::Relaxed}; + + static STAGING: AtomicU64 = AtomicU64::new(0); + // Three tries at a fresh name: the counter is unique within this process, so only a leftover + // of a dead process that now shares our process id can collide, and the next counter won't. + for _ in 0..3 { + let mut name = path.as_os_str().to_os_string(); + name.push(format!(".tmp-figcraft-{}-{}", std::process::id(), STAGING.fetch_add(1, Relaxed))); + let tmp = std::path::PathBuf::from(name); + match std::fs::OpenOptions::new().write(true).create_new(true).open(&tmp) { + Ok(mut file) => { + if let Err(e) = file.write_all(bytes) { + let _ = std::fs::remove_file(&tmp); + return Err(e); + } + return match std::fs::rename(&tmp, path) { + Ok(()) => Ok(()), + // Leave nothing behind when the file can't take its place. + Err(e) => { + let _ = std::fs::remove_file(&tmp); + Err(e) + } + }; + } + Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => continue, + Err(e) => return Err(e), + } + } + Err(std::io::Error::new(std::io::ErrorKind::AlreadyExists, "no free staging name next to the file")) } #[cfg(not(target_arch = "wasm32"))] @@ -677,6 +716,8 @@ mod image_tests; #[cfg(test)] mod large_tests; #[cfg(test)] +mod staging_tests; +#[cfg(test)] mod thumbnail_tests; #[cfg(test)] mod version_tests; diff --git a/crates/format/src/staging_tests.rs b/crates/format/src/staging_tests.rs new file mode 100644 index 00000000..1bc199e2 --- /dev/null +++ b/crates/format/src/staging_tests.rs @@ -0,0 +1,125 @@ +//! The staging write behind `save_file`: a pre-existing file or symlink at a staging name is never +//! truncated or followed, concurrent savers of one file never corrupt it, and a failed write +//! cleans up after itself and leaves the destination alone. + +use figcraft_doc::Document; + +use crate::{save_file, write_file_atomic}; + +fn dir(name: &str) -> std::path::PathBuf { + let d = std::env::temp_dir().join(format!("figcraft-staging-tests-{name}-{}", std::process::id())); + std::fs::create_dir_all(&d).unwrap(); + d +} + +/// A symlink `from` → `to`; false when the platform won't make one (Windows needs privileges). +#[cfg(unix)] +fn symlink(from: &std::path::Path, to: &std::path::Path) -> bool { + std::os::unix::fs::symlink(to, from).is_ok() +} + +#[cfg(windows)] +fn symlink(from: &std::path::Path, to: &std::path::Path) -> bool { + std::os::windows::fs::symlink_file(to, from).is_ok() +} + +#[test] +fn an_existing_staging_file_is_never_written_through() { + let d = dir("existing"); + let path = d.join("a.figcraft"); + // The old deterministic staging name, held by an unrelated file: saving must pick its own + // staging name instead of truncating this one. + let old = d.join("a.figcraft.tmp"); + std::fs::write(&old, b"not yours").unwrap(); + save_file(&Document::new("Mine"), &path, None).unwrap(); + assert_eq!(std::fs::read(&old).unwrap(), b"not yours", "an unrelated file at a staging name is untouched"); + assert_eq!(crate::load_file(&path).unwrap().0.name, "Mine"); + // Just the file and the untouched staging file: no new staging leftovers. + assert_eq!(std::fs::read_dir(&d).unwrap().count(), 2); + let _ = std::fs::remove_dir_all(&d); +} + +#[test] +fn a_staging_symlink_is_never_followed() { + let d = dir("symlink"); + let path = d.join("a.figcraft"); + let victim = d.join("victim.txt"); + std::fs::write(&victim, b"innocent").unwrap(); + let old = d.join("a.figcraft.tmp"); + if !symlink(&old, &victim) { + return; // Making a symlink needs privileges on some Windows setups. + } + save_file(&Document::new("Mine"), &path, None).unwrap(); + assert_eq!(std::fs::read(&victim).unwrap(), b"innocent", "the symlink was never written through"); + assert_eq!(crate::load_file(&path).unwrap().0.name, "Mine"); + let _ = std::fs::remove_dir_all(&d); +} + +#[test] +fn concurrent_savers_of_one_file_never_interleave() { + let d = dir("concurrent"); + let path = d.join("a.figcraft"); + let names = ["W0", "W1", "W2", "W3"]; + for round in 0..5 { + let mut handles = Vec::new(); + for name in names { + let (path, doc) = (path.clone(), Document::new(name)); + handles.push(std::thread::spawn(move || save_file(&doc, &path, None))); + } + for h in handles { + // On Windows a rename over a file another writer just replaced can lose the race. + if cfg!(windows) { + let _ = h.join().unwrap(); + } else { + h.join().unwrap().unwrap(); + } + } + // Whatever landed is one writer's complete file, never a mix: a torn archive wouldn't load. + let (doc, _) = crate::load_file(&path).unwrap(); + assert!(names.contains(&doc.name.as_str()), "round {round}: {:?}", doc.name); + } + // No staging file was left stuck. + assert_eq!(std::fs::read_dir(&d).unwrap().count(), 1); + let _ = std::fs::remove_dir_all(&d); +} + +#[test] +fn a_failed_write_leaves_the_destination_alone_and_cleans_up() { + let d = dir("failure"); + // The destination is a directory: the rename fails, our staging file is removed, the + // directory is untouched. + let taken = d.join("folder.figcraft"); + std::fs::create_dir_all(&taken).unwrap(); + assert!(save_file(&Document::new("Mine"), &taken, None).is_err()); + assert!(write_file_atomic(&taken, b"payload").is_err()); + assert!(taken.is_dir()); + assert_eq!(std::fs::read_dir(&d).unwrap().count(), 1, "no staging file left behind"); + // A missing parent directory fails without creating anything anywhere. + assert!(write_file_atomic(&d.join("no").join("such.figcraft"), b"payload").is_err()); + assert!(!d.join("no").exists()); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + // A read-only directory refuses the staging file, and the old destination stays. + let ro = d.join("read-only"); + std::fs::create_dir_all(&ro).unwrap(); + let dest = ro.join("keep.figcraft"); + std::fs::write(&dest, b"keep").unwrap(); + let mut perms = std::fs::metadata(&ro).unwrap().permissions(); + perms.set_mode(0o500); + std::fs::set_permissions(&ro, perms).unwrap(); + // Running as root would defeat the read-only directory: skip instead of failing. + let probe = ro.join(".probe"); + if std::fs::write(&probe, b"").is_ok() { + let _ = std::fs::remove_file(&probe); + } else { + assert!(write_file_atomic(&dest, b"payload").is_err()); + assert_eq!(std::fs::read(&dest).unwrap(), b"keep"); + assert_eq!(std::fs::read_dir(&ro).unwrap().count(), 1, "no staging file left behind"); + } + let mut perms = std::fs::metadata(&ro).unwrap().permissions(); + perms.set_mode(0o700); + std::fs::set_permissions(&ro, perms).unwrap(); + } + let _ = std::fs::remove_dir_all(&d); +}