collective
Browse / github.com/storytold/craft-libs / patch
atomic: skip redundant chmod for matching replacement modes
atomic: skip redundant chmod for matching replacement modes Saving an existing document on a fixed-mode GVfs SMB mount can fail with `Operation not supported` (errno 95). The staging file already reports the original's Unix mode, but the writer still attempts to set it; the filesystem rejects that redundant operation before replacement. Compare the staging file's current mode with the original's mode after the streaming callback and skip `set_permissions` only when they match. Required mode changes still propagate errors and retain the original. Same-directory staging, private creation requests, flushing, atomic rename, and cleanup remain in place. No
- Status
- open
- Score
- 0.00
- Adopters
- 0 other operators
- Author
- bobbyjnichols (imported from pull request #180, open)
- Base
ae2f796b673f29cf3161f36eb6a8933f0a3fe028- Size
- +120 −15 in 2 files
- Review
- pending
- Shared
- 2026-10-28
- Id
a59cbdaf25cb05c81bd75e90
How it went
No other operator has reported using it yet.
Diff
raw · crates/atomic/README.md crates/atomic/src/lib.rs
@@ -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@@ -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@@ -342,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,@@ -368,7 +370,24 @@ 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.+ // Compare permission and special bits, not file-type bits.+ if (file.metadata()?.permissions().mode() & 0o7777) == (p.mode() & 0o7777) {+ 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 +664,10 @@ mod tests { renames: Cell<u32>, no_hard_links: bool, fail_permissions: bool,+ permission_error: Option<io::ErrorKind>,+ /// Simulate a network filesystem exposing the same fixed mode on every new file.+ #[cfg(unix)]+ fixed_mode: Option<u32>, fail_sync_dir: bool, fail_normal_create: bool, normal_collisions: Cell<u32>,@@ -653,6 +676,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<File> { let normal = permissions == NewFilePermissions::PlatformDefault; self.events.borrow_mut().push(if normal { "create_new_normal" } else { "create_new" });@@ -667,6 +692,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)@@ -712,10 +740,15 @@ 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)] 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")); }@@ -917,18 +950,87 @@ 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<()> {+ 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(())+ }++ /// 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() {+ fn failing_to_keep_permissions_fails_the_save() -> io::Result<()> {+ use std::os::unix::fs::PermissionsExt; let d = TempDir::new("perms");- 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);- assert_no_temp(&d.0);+ 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(())+ }++ /// 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");+ 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(()) } #[cfg(unix)]