diff --git a/crates/format/src/atomic.rs b/crates/format/src/atomic.rs index bf39fe2761..348374d3cf 100644 --- a/crates/format/src/atomic.rs +++ b/crates/format/src/atomic.rs @@ -13,6 +13,10 @@ //! if it does anyway, the save fails with a clear error. It never falls back to writing the //! destination in place. //! - **Unwritable folder:** the save fails with a clear error instead of writing in place. +//! - **Read-only file:** the save is refused before anything is written (#6600). A rename only +//! needs the folder to be writable, so without this check Linux and macOS would silently replace +//! a `chmod 444` file while Windows refuses; now every platform refuses, as Photoshop does, and +//! the user saves a copy with Save As. //! - **Network shares (macOS):** `sync_all` asks for a full flush to the disk's platters //! (`F_FULLFSYNC`), which SMB shares and some virtual file systems refuse while a plain `fsync` //! works. [`sync_file`] falls back to the plain one there (#1336). @@ -30,6 +34,12 @@ use std::sync::atomic::{AtomicU64, Ordering}; /// The file-system steps [`atomic_write_with`] performs. [`RealFs`] is the real thing; tests /// wrap it to inject failures. pub trait AtomicFs { + /// Whether the existing file at `path` is marked read-only (a missing file is not). A + /// read-only destination is never replaced (#6600). By default nothing is read-only. + fn is_readonly(&self, path: &Path) -> bool { + let _ = path; + false + } /// Create `tmp` (it must not exist), write all of `bytes` and flush it ([`sync_file`]). fn write_new(&self, tmp: &Path, bytes: &[u8]) -> io::Result<()>; /// [`write_new`](Self::write_new), reporting the fraction of `bytes` written so far to @@ -91,6 +101,9 @@ impl AtomicFs for RealFs { let perms = std::fs::metadata(from)?.permissions(); std::fs::set_permissions(to, perms) } + fn is_readonly(&self, path: &Path) -> bool { + std::fs::metadata(path).is_ok_and(|m| m.is_file() && m.permissions().readonly()) + } } /// Flush `file` to stable storage (`sync_all`). On Apple platforms that is a full flush @@ -169,6 +182,9 @@ pub fn atomic_write_with_progress(fs: &dyn AtomicFs, retry: RenameRetry, path: & _ => PathBuf::from("."), }; std::fs::create_dir_all(&dir).map_err(|e| context(e, format!("cannot create the folder {}", dir.display())))?; + if fs.is_readonly(&target) { + return Err(io::Error::new(io::ErrorKind::PermissionDenied, format!("cannot save {}: the file is read-only; {UNCHANGED}", path.display()))); + } // Create a fresh temporary file next to the target (same directory, so the rename never // crosses a device). The name is unique per process and call; retry a few on collisions. diff --git a/crates/format/src/atomic/tests.rs b/crates/format/src/atomic/tests.rs index 8e71bc7ce3..a85f998a16 100644 --- a/crates/format/src/atomic/tests.rs +++ b/crates/format/src/atomic/tests.rs @@ -176,6 +176,81 @@ fn lock_that_outlasts_the_retries_keeps_original() { assert_no_temp(&d.0); } +/// A file system that reports the target as read-only and records every write and rename. +#[derive(Default)] +struct ReadOnlyTarget { + writes: Cell, + renames: Cell, +} + +impl AtomicFs for ReadOnlyTarget { + fn is_readonly(&self, _path: &Path) -> bool { + true + } + fn write_new(&self, tmp: &Path, bytes: &[u8]) -> io::Result<()> { + self.writes.set(self.writes.get() + 1); + RealFs.write_new(tmp, bytes) + } + fn rename(&self, from: &Path, to: &Path) -> io::Result<()> { + self.renames.set(self.renames.get() + 1); + RealFs.rename(from, to) + } + fn remove(&self, path: &Path) -> io::Result<()> { + RealFs.remove(path) + } + fn sync_dir(&self, dir: &Path) -> io::Result<()> { + RealFs.sync_dir(dir) + } + fn copy_permissions(&self, from: &Path, to: &Path) -> io::Result<()> { + RealFs.copy_permissions(from, to) + } +} + +/// #6600: a read-only destination is refused before anything is written, with an error that says +/// so, instead of being replaced by the rename (which only needs the folder to be writable). +#[test] +fn read_only_target_is_refused_before_anything_is_written() { + let d = TempDir::new("rofake"); + let p = d.0.join("a.psd"); + std::fs::write(&p, ORIGINAL).unwrap(); + let fs = ReadOnlyTarget::default(); + let e = atomic_write_with(&fs, NO_RETRY, &p, b"new").unwrap_err(); + assert_eq!(e.kind(), io::ErrorKind::PermissionDenied); + assert!(e.to_string().contains("a.psd") && e.to_string().contains("read-only") && e.to_string().contains("not changed"), "{e}"); + assert_eq!((fs.writes.get(), fs.renames.get()), (0, 0), "nothing is written or renamed"); + assert_eq!(std::fs::read(&p).unwrap(), ORIGINAL); + assert_no_temp(&d.0); +} + +/// #6600 on the real file system: a file marked read-only (`chmod 444`, or the Windows read-only +/// attribute) keeps its bytes and the save says why; a writable one still saves. +#[test] +fn read_only_file_on_disk_is_never_replaced() { + let d = TempDir::new("rofile"); + let p = d.0.join("a.psd"); + std::fs::write(&p, ORIGINAL).unwrap(); + let mut perms = std::fs::metadata(&p).unwrap().permissions(); + perms.set_readonly(true); + std::fs::set_permissions(&p, perms.clone()).unwrap(); + let r = atomic_write(&p, b"new"); + let unchanged = std::fs::read(&p).unwrap(); + let still_readonly = std::fs::metadata(&p).unwrap().permissions().readonly(); + // Make it writable again so the folder can be removed (Windows refuses to delete it otherwise). + #[allow(clippy::permissions_set_readonly_false)] + perms.set_readonly(false); + std::fs::set_permissions(&p, perms).unwrap(); + let e = r.unwrap_err(); + assert_eq!(e.kind(), io::ErrorKind::PermissionDenied); + assert!(e.to_string().contains("the file is read-only;"), "a clear reason, not a failed rename: {e}"); + assert_eq!(unchanged, ORIGINAL); + assert!(still_readonly); + assert_no_temp(&d.0); + // New files and writable files are unaffected. + atomic_write(&p, b"new").unwrap(); + assert_eq!(std::fs::read(&p).unwrap(), b"new"); + atomic_write(&d.0.join("fresh.psd"), b"x").unwrap(); +} + #[cfg(unix)] #[test] fn read_only_folder_errors_clearly_and_never_writes_in_place() {