From 24f4ab92b61caec3e7e0b5ed7f6e4d74d0216c04 Mon Sep 17 00:00:00 2001 From: Bobby Nichols Date: Sun, 18 Oct 2026 16:00:28 -0700 Subject: [PATCH 1/3] atomic: skip redundant chmod for matching replacement modes --- crates/atomic/README.md | 7 ++- crates/atomic/src/lib.rs | 104 +++++++++++++++++++++++++++++++++++---- 2 files changed, 100 insertions(+), 11 deletions(-) diff --git a/crates/atomic/README.md b/crates/atomic/README.md index bd4c4215..e43d1c58 100644 --- a/crates/atomic/README.md +++ b/crates/atomic/README.md @@ -97,8 +97,11 @@ sensitive payloads; changing permissions inside a callback is too late to guaran other process opened the stage. Private stages request `0600`; actual enforcement depends on the filesystem's permission support. Neither policy adds Windows ACL guarantees. -Replacements still stage privately and restore the existing Unix mode after writing. A new -file keeps any mode selected by its callback. In the normal exclusive-copy fallback, a changed +Replacements still stage privately and restore the existing Unix mode after writing. When the +stage already has that mode, no `chmod` is attempted. This lets replacements work on fixed-mode +GVfs/SMB mounts and document portals that reject permission changes even when the reported mode +already matches. The comparison happens after the callback, so its mode changes are still +restored. A new file keeps any mode selected by its callback. In the normal exclusive-copy fallback, a changed mode is applied before copying bytes; explicit private copies stay private during copying and apply callback-selected modes afterward. A required mode change that fails is reported, not silently ignored. Existing-file replacements and deliberate callback changes can therefore diff --git a/crates/atomic/src/lib.rs b/crates/atomic/src/lib.rs index 36168b31..7755734a 100644 --- a/crates/atomic/src/lib.rs +++ b/crates/atomic/src/lib.rs @@ -31,7 +31,8 @@ //! ### Permissions //! New documents and exports use normal OS creation permissions, including while staging. //! Use [`WriteOptions`] with [`NewFilePermissions::Private`] for settings or sensitive payloads. -//! Existing destinations always stage privately and retain their original Unix mode. +//! Existing destinations always stage privately and retain their original Unix mode. Restoring +//! an already-matching mode needs no chmod, including on fixed-mode network filesystems. //! //! ### Platform Characteristics //! - **Windows**: Antivirus scanners, search indexers, and backup utilities briefly hold files @@ -368,7 +369,23 @@ fn stage( }; let written = fill(&mut file) .map_err(|e| write_error(e, path, dir, existed)) - .and_then(|()| perms.map_or(Ok(()), |p| fs.set_permissions(&file, p).map_err(|e| permissions_error(e, path)))) + .and_then(|()| { + perms + .map_or(Ok(()), |p| { + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + // GVfs/SMB and document portals can expose a fixed mode but reject + // chmod, even when the requested mode is already in effect. Check + // after fill: a streaming callback may have changed the stage's mode. + if file.metadata()?.permissions().mode() == p.mode() { + return Ok(()); + } + } + fs.set_permissions(&file, p) + }) + .map_err(|e| permissions_error(e, path)) + }) .and_then(|()| fs.sync(&file).map_err(|e| write_error(e, path, dir, existed))); drop(file); if written.is_err() { @@ -645,6 +662,10 @@ mod tests { renames: Cell, no_hard_links: bool, fail_permissions: bool, + permission_error: Option, + /// Simulate a network filesystem exposing the same fixed mode on every new file. + #[cfg(unix)] + fixed_mode: Option, fail_sync_dir: bool, fail_normal_create: bool, normal_collisions: Cell, @@ -667,6 +688,9 @@ mod tests { #[cfg(unix)] { use std::os::unix::fs::PermissionsExt; + if let Some(mode) = self.fixed_mode { + file.set_permissions(std::fs::Permissions::from_mode(mode))?; + } self.created_modes.borrow_mut().push(file.metadata()?.permissions().mode() & 0o777); } Ok(file) @@ -716,6 +740,9 @@ mod tests { self.events.borrow_mut().push("set_permissions"); #[cfg(unix)] self.permission_lengths.borrow_mut().push(file.metadata()?.len()); + if let Some(kind) = self.permission_error { + return Err(io::Error::new(kind, "injected permission failure")); + } if self.fail_permissions { return Err(io::Error::new(io::ErrorKind::PermissionDenied, "injected: operation not permitted")); } @@ -919,16 +946,75 @@ mod tests { #[cfg(unix)] #[test] - fn failing_to_keep_permissions_fails_the_save() { + fn unchanged_replacement_modes_need_no_chmod() -> io::Result<()> { + use std::os::unix::fs::PermissionsExt; + let d = TempDir::new("fixed-replacement-mode"); + // GVfs SMB reports 0700 regardless of the requested creation mode; also cover a + // private stage whose normal 0600 mode already matches the original. + for mode in [0o600, 0o700] { + for policy in [NewFilePermissions::Private, NewFilePermissions::PlatformDefault] { + for streaming in [false, true] { + let p = d.0.join(format!("{mode:o}-{policy:?}-{streaming}")); + std::fs::write(&p, ORIGINAL)?; + std::fs::set_permissions(&p, std::fs::Permissions::from_mode(mode))?; + let fs = Faulty { fixed_mode: Some(mode), permission_error: Some(io::ErrorKind::Unsupported), ..Default::default() }; + let options = WriteOptions::default().with_new_file_permissions(policy); + if streaming { + options.atomic_write_with_custom(&fs, NO_RETRY, &p, |f| f.write_all(b"replacement"))?; + } else { + options.atomic_write_custom(&fs, NO_RETRY, &p, b"replacement")?; + } + assert_eq!(std::fs::read(&p)?, b"replacement"); + assert_eq!(std::fs::metadata(&p)?.permissions().mode() & 0o7777, mode); + assert!(!fs.events.borrow().contains(&"set_permissions")); + assert_eq!(fs.renames.get(), 1); + assert_no_temp(&d.0); + } + } + } + Ok(()) + } + + #[cfg(unix)] + #[test] + fn failing_to_keep_permissions_fails_the_save() -> io::Result<()> { + use std::os::unix::fs::PermissionsExt; let d = TempDir::new("perms"); + for kind in [io::ErrorKind::Unsupported, io::ErrorKind::PermissionDenied, io::ErrorKind::Other] { + let p = d.0.join(format!("{kind:?}.craft")); + std::fs::write(&p, ORIGINAL)?; + std::fs::set_permissions(&p, std::fs::Permissions::from_mode(0o640))?; + let fs = Faulty { permission_error: Some(kind), ..Default::default() }; + let error = + atomic_write_custom(&fs, NO_RETRY, &p, b"new").err().ok_or_else(|| io::Error::other("a required mode change must succeed"))?; + assert_eq!(error.kind(), kind); + assert!(error.to_string().contains("permissions"), "{error}"); + assert_eq!(std::fs::read(&p)?, ORIGINAL); + assert_eq!(std::fs::metadata(&p)?.permissions().mode() & 0o7777, 0o640); + assert_eq!(fs.renames.get(), 0); + assert_no_temp(&d.0); + } + Ok(()) + } + + #[cfg(unix)] + #[test] + fn replacement_checks_mode_after_streaming_callback() -> io::Result<()> { + use std::os::unix::fs::PermissionsExt; + let d = TempDir::new("callback-mode-change"); let p = d.0.join("doc.craft"); - std::fs::write(&p, ORIGINAL).ok(); - let fs = Faulty { fail_permissions: true, ..Default::default() }; - let msg = atomic_write_custom(&fs, NO_RETRY, &p, b"new").err().map(|e| e.to_string()).unwrap_or_default(); - assert!(msg.contains("permissions"), "{msg}"); - assert_eq!(std::fs::read(&p).ok(), Some(ORIGINAL.to_vec())); - assert_eq!(fs.renames.get(), 0); + std::fs::write(&p, ORIGINAL)?; + std::fs::set_permissions(&p, std::fs::Permissions::from_mode(0o600))?; + let fs = Faulty::default(); + WriteOptions::default().atomic_write_with_custom(&fs, NO_RETRY, &p, |f| { + f.write_all(b"replacement")?; + f.set_permissions(std::fs::Permissions::from_mode(0o640)) + })?; + assert_eq!(std::fs::read(&p)?, b"replacement"); + assert_eq!(std::fs::metadata(&p)?.permissions().mode() & 0o7777, 0o600); + assert!(fs.events.borrow().contains(&"set_permissions")); assert_no_temp(&d.0); + Ok(()) } #[cfg(unix)] From 595b5e636ecf391ee08d66a41097276e9a36da30 Mon Sep 17 00:00:00 2001 From: Bobby Nichols Date: Sun, 18 Oct 2026 16:17:04 -0700 Subject: [PATCH 2/3] atomic: document permission helpers and regression tests --- crates/atomic/src/lib.rs | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/crates/atomic/src/lib.rs b/crates/atomic/src/lib.rs index 7755734a..b2bcf150 100644 --- a/crates/atomic/src/lib.rs +++ b/crates/atomic/src/lib.rs @@ -343,8 +343,9 @@ fn split(target: &Path, path: &Path) -> io::Result<(PathBuf, String)> { } /// Create a temporary file in `dir`, fill it with `fill`, give it the permissions of the file it -/// replaces (`replaced`, Unix) and flush it. A leftover file with the same name is skipped. When -/// any step fails, the temporary file is removed. +/// replaces (`replaced`, Unix) and flush it. Permission restoration is skipped when the mode +/// already matches after `fill`. A leftover file with the same name is skipped. When any step +/// fails, the temporary file is removed. fn stage( fs: &dyn AtomicFs, dir: &Path, @@ -674,6 +675,8 @@ mod tests { } impl AtomicFs for Faulty { + /// Create a real stage with injected failures or a simulated fixed network mode, + /// recording the creation policy and the Unix mode exposed by the filesystem. fn create_new(&self, path: &Path, permissions: NewFilePermissions) -> io::Result { let normal = permissions == NewFilePermissions::PlatformDefault; self.events.borrow_mut().push(if normal { "create_new_normal" } else { "create_new" }); @@ -736,6 +739,8 @@ mod tests { } RealFs.sync_dir(dir) } + /// Record permission restoration and payload length, then inject a configured error + /// or delegate the mode change to the real filesystem. fn set_permissions(&self, file: &File, perms: std::fs::Permissions) -> io::Result<()> { self.events.borrow_mut().push("set_permissions"); #[cfg(unix)] @@ -944,6 +949,8 @@ mod tests { Ok(()) } + /// Matching fixed modes permit byte and streaming replacements under both creation + /// policies without calling a permission operation that the mount rejects. #[cfg(unix)] #[test] fn unchanged_replacement_modes_need_no_chmod() -> io::Result<()> { @@ -975,6 +982,8 @@ mod tests { Ok(()) } + /// A required mode change that fails must preserve the original bytes and mode, + /// report the error, and remove the stage without publishing it. #[cfg(unix)] #[test] fn failing_to_keep_permissions_fails_the_save() -> io::Result<()> { @@ -997,6 +1006,8 @@ mod tests { Ok(()) } + /// A streaming callback that changes the stage mode must not bypass restoration + /// of the original mode, even when the modes matched at creation. #[cfg(unix)] #[test] fn replacement_checks_mode_after_streaming_callback() -> io::Result<()> { From d5099dacd3596303a6c0f8438a8c86e8bbc21adf Mon Sep 17 00:00:00 2001 From: Bobby Nichols Date: Wed, 21 Oct 2026 22:59:44 -0700 Subject: [PATCH 3/3] atomic: compare permission bits without file-type bits --- crates/atomic/src/lib.rs | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/crates/atomic/src/lib.rs b/crates/atomic/src/lib.rs index b2bcf150..2e448abc 100644 --- a/crates/atomic/src/lib.rs +++ b/crates/atomic/src/lib.rs @@ -379,7 +379,8 @@ fn stage( // GVfs/SMB and document portals can expose a fixed mode but reject // chmod, even when the requested mode is already in effect. Check // after fill: a streaming callback may have changed the stage's mode. - if file.metadata()?.permissions().mode() == p.mode() { + // Compare permission and special bits, not file-type bits. + if (file.metadata()?.permissions().mode() & 0o7777) == (p.mode() & 0o7777) { return Ok(()); } } @@ -1006,25 +1007,29 @@ mod tests { Ok(()) } - /// A streaming callback that changes the stage mode must not bypass restoration - /// of the original mode, even when the modes matched at creation. + /// A streaming callback that changes permissions or setuid/setgid/sticky bits must not + /// bypass restoration of the original mode, even when the modes matched at creation. #[cfg(unix)] #[test] fn replacement_checks_mode_after_streaming_callback() -> io::Result<()> { use std::os::unix::fs::PermissionsExt; let d = TempDir::new("callback-mode-change"); - let p = d.0.join("doc.craft"); - std::fs::write(&p, ORIGINAL)?; - std::fs::set_permissions(&p, std::fs::Permissions::from_mode(0o600))?; - let fs = Faulty::default(); - WriteOptions::default().atomic_write_with_custom(&fs, NO_RETRY, &p, |f| { - f.write_all(b"replacement")?; - f.set_permissions(std::fs::Permissions::from_mode(0o640)) - })?; - assert_eq!(std::fs::read(&p)?, b"replacement"); - assert_eq!(std::fs::metadata(&p)?.permissions().mode() & 0o7777, 0o600); - assert!(fs.events.borrow().contains(&"set_permissions")); - assert_no_temp(&d.0); + for callback_mode in [0o640, 0o1600, 0o2600, 0o4600] { + let p = d.0.join(format!("doc-{callback_mode:o}.craft")); + std::fs::write(&p, ORIGINAL)?; + std::fs::set_permissions(&p, std::fs::Permissions::from_mode(0o600))?; + let fs = Faulty::default(); + WriteOptions::default().atomic_write_with_custom(&fs, NO_RETRY, &p, |f| { + f.write_all(b"replacement")?; + f.set_permissions(std::fs::Permissions::from_mode(callback_mode))?; + assert_eq!(f.metadata()?.permissions().mode() & 0o7777, callback_mode); + Ok(()) + })?; + assert_eq!(std::fs::read(&p)?, b"replacement"); + assert_eq!(std::fs::metadata(&p)?.permissions().mode() & 0o7777, 0o600); + assert!(fs.events.borrow().contains(&"set_permissions")); + assert_no_temp(&d.0); + } Ok(()) }