diff --git a/crates/engine/src/config.rs b/crates/engine/src/config.rs index c333c973..68f403dd 100644 --- a/crates/engine/src/config.rs +++ b/crates/engine/src/config.rs @@ -164,10 +164,15 @@ impl ConfigStore for DirConfig { } } -/// The temporary file an atomic write of `path` goes through. +/// The temporary file an atomic write of `path` goes through. Unique per call (process id plus a +/// counter), so two writers saving the same destination never share a temporary — one cannot +/// truncate, rename or clean up another's — and there is no predictable name for a pre-existing +/// file or symlink to sit at. pub fn temp_path(path: &Path) -> PathBuf { + use std::sync::atomic::{AtomicU64, Ordering}; + static NEXT: AtomicU64 = AtomicU64::new(0); let mut name = path.file_name().map(|n| n.to_os_string()).unwrap_or_default(); - name.push(".tmp"); + name.push(format!(".{}-{}.tmp", std::process::id(), NEXT.fetch_add(1, Ordering::Relaxed))); path.with_file_name(name) } @@ -177,19 +182,30 @@ pub fn temp_path(path: &Path) -> PathBuf { pub fn atomic_write(path: &Path, data: &[u8]) -> std::io::Result<()> { atomic_write_with(path, data, |tmp, data| { use std::io::Write; - let mut f = std::fs::File::create(tmp)?; + // Exclusive creation: an entry already at the temporary's name (a leftover from a crashed + // writer, or something planted there) is never opened, truncated or written through. + let mut f = std::fs::OpenOptions::new().write(true).create_new(true).open(tmp)?; f.write_all(data)?; f.sync_all() }) } -/// [`atomic_write`] with an injectable temp-file writer (tests simulate a crash mid-write). A -/// failed write removes the temporary file and leaves `path` untouched. -pub fn atomic_write_with(path: &Path, data: &[u8], write: impl FnOnce(&Path, &[u8]) -> std::io::Result<()>) -> std::io::Result<()> { - let tmp = temp_path(path); - if let Err(e) = write(&tmp, data) { - let _ = std::fs::remove_file(&tmp); - return Err(e); +/// [`atomic_write`] with an injectable temp-file writer (tests simulate a crash mid-write). The +/// temporary belongs to this call alone: the writer must create it exclusively, and a failed +/// write removes only its own temporary and leaves `path` untouched. An `AlreadyExists` from the +/// writer means the name was taken (never opened by us): that entry is left alone and another +/// unique name is tried. +pub fn atomic_write_with(path: &Path, data: &[u8], mut write: impl FnMut(&Path, &[u8]) -> std::io::Result<()>) -> std::io::Result<()> { + for _ in 0..16 { + let tmp = temp_path(path); + match write(&tmp, data) { + Ok(()) => return std::fs::rename(&tmp, path), + Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => continue, + Err(e) => { + let _ = std::fs::remove_file(&tmp); + return Err(e); + } + } } - std::fs::rename(&tmp, path) + Err(std::io::Error::new(std::io::ErrorKind::AlreadyExists, format!("{}: no free temporary name", path.display()))) } diff --git a/crates/engine/src/tests_settings.rs b/crates/engine/src/tests_settings.rs index 458c886f..4ce956ca 100644 --- a/crates/engine/src/tests_settings.rs +++ b/crates/engine/src/tests_settings.rs @@ -5,7 +5,7 @@ use std::sync::Arc; use serde_json::json; -use crate::config::{ConfigStore, DirConfig, MemoryConfig, atomic_write, atomic_write_with, temp_path}; +use crate::config::{ConfigStore, DirConfig, MemoryConfig, atomic_write, atomic_write_with}; use crate::prefs::{PREFS_FILE, Prefs}; use crate::shortcuts::{DEFAULT_PRESET, Keymaps, normalize}; use crate::{Session, autosave}; @@ -560,7 +560,9 @@ fn atomic_write_survives_a_crash_mid_write() { }); assert!(r.is_err()); assert_eq!(std::fs::read(&f).unwrap(), b"version 1"); - assert!(!temp_path(&f).exists(), "the partial temp file is cleaned up"); + let leftovers: Vec = + std::fs::read_dir(&d).unwrap().flatten().map(|e| e.file_name().to_string_lossy().to_string()).filter(|n| n != "p.ecproj").collect(); + assert!(leftovers.is_empty(), "the partial temporary is cleaned up: {leftovers:?}"); // A temp file left by a hard kill doesn't count as an auto-save. std::fs::write(d.join("p auto-save 1.ecproj.tmp"), b"junk").unwrap(); assert!(autosave::existing(&d, "p").is_empty()); @@ -569,6 +571,54 @@ fn atomic_write_survives_a_crash_mid_write() { let _ = std::fs::remove_dir_all(d); } +#[cfg(unix)] +#[test] +fn atomic_write_does_not_follow_a_planted_temporary_symlink() { + let d = tmp("atomic-link"); + let f = d.join("p.ecproj"); + let sentinel = d.join("sentinel.txt"); + std::fs::write(&sentinel, b"precious").unwrap(); + // Someone plants a symlink at the temporary name the write used to go through. + std::os::unix::fs::symlink(&sentinel, d.join("p.ecproj.tmp")).unwrap(); + atomic_write(&f, b"new").unwrap(); + assert_eq!(std::fs::read(&sentinel).unwrap(), b"precious", "a symlink at the temporary name is never followed"); + assert_eq!(std::fs::read(&f).unwrap(), b"new"); + let _ = std::fs::remove_dir_all(d); +} + +#[cfg(unix)] +#[test] +fn atomic_writes_in_parallel_never_publish_a_mixture() { + use std::sync::{Arc, Barrier}; + let d = tmp("atomic-race"); + let f = d.join("p.ecproj"); + let a = vec![b'A'; 64]; + let b = vec![b'B'; 64]; + // Writer A writes the first half of its temporary, then waits while writer B runs against + // the same destination, and only then finishes. + let (half, published) = (Arc::new(Barrier::new(2)), Arc::new(Barrier::new(2))); + let (f2, a2) = (f.clone(), a.clone()); + let (half2, published2) = (half.clone(), published.clone()); + let t = std::thread::spawn(move || { + atomic_write_with(&f2, &a2, |tmp, data| { + use std::io::Write; + let mut fh = std::fs::File::create(tmp)?; + fh.write_all(&data[..32])?; + half2.wait(); + published2.wait(); + fh.write_all(&data[32..])?; + fh.sync_all() + }) + }); + half.wait(); + atomic_write(&f, &b).unwrap(); + published.wait(); + let _ = t.join(); + let got = std::fs::read(&f).unwrap(); + assert!(got == a || got == b, "the destination holds one complete write, never a mixture: {}", String::from_utf8_lossy(&got)); + let _ = std::fs::remove_dir_all(d); +} + #[test] fn autosave_rotates_through_max_versions() { let d = tmp("rotate");