From d78b3520042b0f5509d2d3bc17c2c1aab2a50eb3 Mon Sep 17 00:00:00 2001 From: aiongg Date: Sun, 11 Oct 2026 15:22:34 +0000 Subject: [PATCH] Refuse to replace a file the user cannot write (Unix) A rename needs only a writable folder, so a read-only file would be replaced where writing it in place would fail. Before staging, the existing file is opened for writing without truncating; when that fails the save fails with a read-only error. --- crates/atomic/src/lib.rs | 30 ++++++++++++++++++++++++++++- crates/atomic/tests/atomic_tests.rs | 18 +++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/crates/atomic/src/lib.rs b/crates/atomic/src/lib.rs index 7506d58f..e7143134 100644 --- a/crates/atomic/src/lib.rs +++ b/crates/atomic/src/lib.rs @@ -156,7 +156,11 @@ pub fn atomic_write_custom(fs: &dyn AtomicFs, retry: RenameRetry, path: &Path, b fn replace(fs: &dyn AtomicFs, retry: RenameRetry, path: &Path, fill: impl FnOnce(&mut File) -> io::Result<()>) -> io::Result<()> { let target = resolve_links(path)?; let (dir, name) = split(&target, path)?; - let existed = target.exists(); + let existing = std::fs::metadata(&target).ok(); + let existed = existing.is_some(); + if existing.as_ref().is_some_and(|m| m.is_file()) { + check_writable(&target).map_err(|e| read_only_error(e, path))?; + } let tmp = stage(fs, &dir, &name, path, existed, fill)?; @@ -173,6 +177,19 @@ fn replace(fs: &dyn AtomicFs, retry: RenameRetry, path: &Path, fill: impl FnOnce Ok(()) } +/// A rename replaces a file the user can't write (Unix only needs the folder to be writable), so +/// refuse, as writing it in place would. Opening for writing without truncating changes nothing. +#[cfg(unix)] +fn check_writable(target: &Path) -> io::Result<()> { + OpenOptions::new().write(true).open(target).map(drop) +} + +/// Windows refuses to rename over a read-only file by itself. +#[cfg(not(unix))] +fn check_writable(_: &Path) -> io::Result<()> { + Ok(()) +} + /// The folder `target` is in and its file name. `path` is the caller's path, for the error. fn split(target: &Path, path: &Path) -> io::Result<(PathBuf, String)> { let name = target.file_name().ok_or_else(|| io::Error::new(io::ErrorKind::InvalidInput, format!("{}: not a file path", path.display())))?; @@ -346,6 +363,17 @@ fn write_error(e: io::Error, path: &Path, dir: &Path, existed: bool) -> io::Erro io::Error::new(e.kind(), msg) } +fn read_only_error(e: io::Error, path: &Path) -> io::Error { + let kept = unchanged(true); + let msg = match e.kind() { + io::ErrorKind::PermissionDenied | io::ErrorKind::ReadOnlyFilesystem => { + format!("cannot save {}: the file is read-only ({e}){kept}", path.display()) + } + _ => format!("cannot save {}: {e}{kept}", path.display()), + }; + io::Error::new(e.kind(), msg) +} + fn rename_error(e: io::Error, path: &Path, existed: bool) -> io::Error { let kept = unchanged(existed); let msg = if is_cross_device(&e) { diff --git a/crates/atomic/tests/atomic_tests.rs b/crates/atomic/tests/atomic_tests.rs index 8f9f3323..ba6e27ae 100644 --- a/crates/atomic/tests/atomic_tests.rs +++ b/crates/atomic/tests/atomic_tests.rs @@ -152,3 +152,21 @@ fn symlink_loop_is_an_error() { assert!(std::fs::symlink_metadata(d.0.join("a")).map(|m| m.file_type().is_symlink()).unwrap_or(false)); assert_no_temp(&d.0); } + +#[cfg(unix)] +#[test] +fn read_only_target_is_not_replaced() { + use std::os::unix::fs::PermissionsExt; + let d = TempDir::new("read_only"); + let p = d.0.join("locked.txt"); + std::fs::write(&p, ORIGINAL).ok(); + std::fs::set_permissions(&p, std::fs::Permissions::from_mode(0o444)).ok(); + // Root may write the file anyway; the check only means something for an ordinary user. + if std::fs::OpenOptions::new().write(true).open(&p).is_ok() { + return; + } + let msg = atomic_write(&p, b"new").err().map(|e| e.to_string()).unwrap_or_default(); + assert!(msg.contains("read-only"), "{msg}"); + assert_eq!(std::fs::read(&p).ok(), Some(ORIGINAL.to_vec())); + assert_no_temp(&d.0); +}