From d6579c2befe8c6ec9a4d04d7fa8a6975a998e8e8 Mon Sep 17 00:00:00 2001 From: Christian Lappin <36496308+christianlappin@users.noreply.github.com> Date: Thu, 12 Nov 2026 00:11:53 -0500 Subject: [PATCH] Fix macOS extended attribute sizing during save --- crates/platform/Cargo.toml | 4 +-- crates/platform/src/staging.rs | 15 ++++++--- crates/platform/src/staging/tests.rs | 48 +++++++++++++++------------- 3 files changed, 37 insertions(+), 30 deletions(-) diff --git a/crates/platform/Cargo.toml b/crates/platform/Cargo.toml index 1808c4128f..4342565a0d 100644 --- a/crates/platform/Cargo.toml +++ b/crates/platform/Cargo.toml @@ -12,8 +12,8 @@ workspace = true log = { workspace = true } [target.'cfg(target_os = "macos")'.dependencies] -# `write_atomically`: a volume that refuses F_FULLFSYNC gets a plain fsync. -rustix = { version = "1", features = ["fs"] } +# Atomic saves use explicit xattr size queries and fall back to plain fsync if F_FULLFSYNC is refused. +rustix = { version = "1.1.6", features = ["fs"] } [target.'cfg(target_os = "linux")'.dependencies] # Already in the dependency graph through the accessibility integration. The portal watcher diff --git a/crates/platform/src/staging.rs b/crates/platform/src/staging.rs index b9b2a80443..a604dd58b5 100644 --- a/crates/platform/src/staging.rs +++ b/crates/platform/src/staging.rs @@ -220,7 +220,7 @@ pub fn write_atomically_with( /// Provenance is managed by macOS for the new file, not writable document metadata. #[cfg(target_os = "macos")] fn preserve_target_xattrs(path: &Path, staged: &std::fs::File) -> std::io::Result<()> { - use rustix::fs::{Mode, OFlags, XattrFlags, fgetxattr, flistxattr, fsetxattr, open}; + use rustix::fs::{Mode, OFlags, XattrFlags, fgetxattr, fgetxattr_query, flistxattr, fsetxattr, open}; use rustix::io::Errno; // A new destination has nothing to retain. Replacing a symlink has always replaced the @@ -257,12 +257,13 @@ fn preserve_target_xattrs(path: &Path, staged: &std::fs::File) -> std::io::Resul continue; } let name = std::ffi::CString::new(name).map_err(|_| std::io::Error::other("invalid extended attribute name"))?; - let size = fgetxattr(&source, &name, &mut [] as &mut [u8])?; + // rustix treats an empty read buffer as reading zero bytes, not a size query. + let size = fgetxattr_query(&source, &name)?; if size > MAX_VALUE_BYTES || size > MAX_TOTAL_BYTES.saturating_sub(total) { return Err(std::io::Error::other("extended attributes exceed the 16 MiB per-value or 64 MiB total limit")); } // Resource forks may return a prefix instead of ERANGE on growth; check the size again. - let value = read_xattr_value(size, |buffer| Ok(fgetxattr(&source, &name, buffer)?))?; + let value = read_xattr_value(size, |buffer| Ok(fgetxattr(&source, &name, buffer)?), || Ok(fgetxattr_query(&source, &name)?))?; fsetxattr(staged, &name, &value, XattrFlags::empty())?; total = total.saturating_add(value.len()); } @@ -271,10 +272,14 @@ fn preserve_target_xattrs(path: &Path, staged: &std::fs::File) -> std::io::Resul /// Read an already size-bounded attribute, refusing a partial result or a changed post-read size. #[cfg(target_os = "macos")] -fn read_xattr_value(size: usize, mut read: impl FnMut(&mut [u8]) -> std::io::Result) -> std::io::Result> { +fn read_xattr_value( + size: usize, + read: impl FnOnce(&mut [u8]) -> std::io::Result, + query_size: impl FnOnce() -> std::io::Result, +) -> std::io::Result> { let mut value = vec![0u8; size.max(1)]; let len = read(value.as_mut_slice())?; - let after = read(&mut [])?; + let after = query_size()?; if len != size || after != size { return Err(std::io::Error::other("extended attribute changed size while saving")); } diff --git a/crates/platform/src/staging/tests.rs b/crates/platform/src/staging/tests.rs index da1e015e15..699ba9bbf9 100644 --- a/crates/platform/src/staging/tests.rs +++ b/crates/platform/src/staging/tests.rs @@ -130,6 +130,7 @@ fn atomic_saves_keep_finder_tags_and_binary_extended_attributes() { ("com.apple.metadata:_kMDItemUserTags", b"PdfCraft\n6"), ("com.pdfcraft.test.binary", b"\0\xff\x80original\0"), ("com.pdfcraft.test.empty", b""), + ("com.apple.ResourceFork", b"\0\xfffork"), ]; std::fs::write(&target, b"OLD").unwrap(); for (name, value) in attributes { @@ -188,7 +189,7 @@ fn a_failed_flush_keeps_the_original_extended_attributes() { #[cfg(target_os = "macos")] #[test] fn a_growing_resource_fork_refuses_publication_and_keeps_original() { - use rustix::fs::{XattrFlags, fgetxattr, fsetxattr, getxattr, setxattr}; + use rustix::fs::{XattrFlags, fgetxattr_query, fsetxattr, getxattr, setxattr}; let dir = test_dir("xattrs-growing-fork"); let target = dir.join("out.pdf"); let name = "com.apple.ResourceFork"; @@ -197,17 +198,18 @@ fn a_growing_resource_fork_refuses_publication_and_keeps_original() { setxattr(&target, name, b"ABC", XattrFlags::empty()).unwrap(); let source = std::fs::File::open(&target).unwrap(); let result = write_atomically_with(&target, b"REPLACEMENT PDF", form, [23], |staged| { - let size = fgetxattr(&source, name, &mut [] as &mut [u8])?; - let value = read_xattr_value(size, |buffer| { - if buffer.is_empty() { - return Ok(fgetxattr(&source, name, buffer)?); - } - // HFS can return the prefix that fits instead of ERANGE. APFS need not - // have that behaviour: model the kernel response, not a timing race. - setxattr(&target, name, b"ABCD", XattrFlags::empty())?; - buffer.get_mut(..3).unwrap().copy_from_slice(b"ABC"); - Ok(3) - })?; + let size = fgetxattr_query(&source, name)?; + let value = read_xattr_value( + size, + |buffer| { + // HFS can return the prefix that fits instead of ERANGE. APFS need not + // have that behaviour: model the kernel response, not a timing race. + setxattr(&target, name, b"ABCD", XattrFlags::empty())?; + buffer.get_mut(..3).unwrap().copy_from_slice(b"ABC"); + Ok(3) + }, + || Ok(fgetxattr_query(&source, name)?), + )?; fsetxattr(staged, name, &value, XattrFlags::empty())?; Ok(()) }); @@ -227,29 +229,29 @@ fn a_growing_resource_fork_refuses_publication_and_keeps_original() { #[test] fn attribute_reads_require_a_complete_value_and_stable_size() { for bytes in [&b""[..], &b"\0\xffB"[..]] { - let value = read_xattr_value(bytes.len(), |buffer| { - if !buffer.is_empty() { + let value = read_xattr_value( + bytes.len(), + |buffer| { buffer.get_mut(..bytes.len()).unwrap().copy_from_slice(bytes); - } - Ok(bytes.len()) - }) + Ok(bytes.len()) + }, + || Ok(bytes.len()), + ) .unwrap(); assert_eq!(value, bytes); } for (before, read_len, after) in [(3, 2, 3), (3, 3, 4), (0, 1, 1), (3, 2, 2)] { - let result = read_xattr_value(before, |buffer| Ok(if buffer.is_empty() { after } else { read_len })); + let result = read_xattr_value(before, |_| Ok(read_len), || Ok(after)); assert!(result.unwrap_err().to_string().contains("changed size")); } - let result = read_xattr_value(3, |buffer| { - if buffer.is_empty() { Err(std::io::Error::new(std::io::ErrorKind::PermissionDenied, "size recheck refused")) } else { Ok(3) } - }); + let result = read_xattr_value(3, |_| Ok(3), || Err(std::io::Error::new(std::io::ErrorKind::PermissionDenied, "size recheck refused"))); assert_eq!(result.unwrap_err().kind(), std::io::ErrorKind::PermissionDenied); } #[cfg(target_os = "macos")] #[test] fn oversized_extended_attributes_refuse_the_save_without_replacing_the_target() { - use rustix::fs::{XattrFlags, getxattr, setxattr}; + use rustix::fs::{XattrFlags, getxattr_query, setxattr}; let dir = test_dir("xattrs-limit"); let target = dir.join("out.pdf"); std::fs::write(&target, b"OLD").unwrap(); @@ -267,7 +269,7 @@ fn oversized_extended_attributes_refuse_the_save_without_replacing_the_target() assert!(e.to_string().contains("Save As"), "{e}"); assert!(!flushed, "metadata failure must stop before flush and rename"); assert_eq!(std::fs::read(&target).unwrap(), b"OLD"); - let size = getxattr(&target, "com.apple.ResourceFork", &mut [] as &mut [u8]).unwrap(); + let size = getxattr_query(&target, "com.apple.ResourceFork").unwrap(); assert_eq!(size, fork.len()); assert_eq!(std::fs::read_dir(&dir).unwrap().count(), 1); std::fs::remove_dir_all(dir).unwrap();