Merge pull request #7203 from christianlappin/fix/7050-macos-save-xattrs

Fix macOS saves for PDFs with extended attributes

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

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<usize>) -> std::io::Result<Vec<u8>> {
+fn read_xattr_value(
+    size: usize,
+    read: impl FnOnce(&mut [u8]) -> std::io::Result<usize>,
+    query_size: impl FnOnce() -> std::io::Result<usize>,
+) -> std::io::Result<Vec<u8>> {
     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"));
     }

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"<plist version=\"1.0\"><array><string>PdfCraft\n6</string></array></plist>"),
         ("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();

crates/ui-egui/Cargo.toml

@@ -75,9 +75,8 @@ objc2-app-kit = { version = "0.3", default-features = false, features = ["std",
 objc2-foundation = { version = "0.3", default-features = false, features = ["std", "NSArray", "NSGeometry", "NSBundle", "NSURL", "NSString", "NSError"] }
 block2 = "0.6"
 plist = "1.10"
-# A plain fsync where a volume refuses the full flush `sync_all` asks for (SMB, FUSE); already in
-# the tree through pdfcraft-cli.
-rustix = { version = "1", features = ["fs"] }
+# Query and read Finder association attributes without following symbolic links.
+rustix = { version = "1.1.6", features = ["fs"] }
 
 [target.'cfg(windows)'.dependencies]
 # Auto interface language: the Windows display language, without starting a process.

crates/ui-egui/src/platform/file_watch.rs

@@ -623,14 +623,18 @@ mod tests {
     #[test]
     fn a_large_binding_does_not_block_a_document_save() {
         let (_dir, path) = TempDir::new("large-binding-save", &fixture(2));
-        xattr::set(&path, "com.apple.LaunchServices.OpenWith", &vec![b'x'; 65537]).unwrap();
+        let binding = vec![b'x'; 65537];
+        xattr::set(&path, "com.apple.LaunchServices.OpenWith", &binding).unwrap();
         xattr::set(&path, "com.apple.quarantine", b"0082;1234;Preview;").unwrap();
         let mut h = harness(&path);
         let app = h.state_mut();
         app.views[0].select_pages(&[0]);
         app.apply_edit(pdfcraft_engine::Edit::RotatePages { pages: vec![0], degrees: 90 });
         assert!(app.save_active(SaveTarget::InPlace), "the document save uses upstream preservation limits");
-        assert_eq!(xattr::get(&path, "com.apple.LaunchServices.OpenWith").unwrap().unwrap().len(), 65537);
+        let mut saved = vec![0; binding.len()];
+        let count = rustix::fs::lgetxattr(&path, "com.apple.LaunchServices.OpenWith", saved.as_mut_slice()).unwrap();
+        assert_eq!(count, binding.len());
+        assert_eq!(saved, binding);
         assert_eq!(xattr::get(&path, "com.apple.quarantine").unwrap(), Some(b"0082;1234;Preview;".to_vec()));
     }
 

crates/ui-egui/src/platform/macos_association.rs

@@ -41,26 +41,30 @@ impl OpeningMetadata {
 
 /// Read a last-component-symlink-safe attribute with a strict allocation cap.
 fn read_attribute(path: &Path, name: &str) -> io::Result<Option<Vec<u8>>> {
-    read_attribute_with(|buffer| rustix::fs::lgetxattr(path, name, buffer))
+    read_attribute_with(|| rustix::fs::lgetxattr_query(path, name), |buffer| rustix::fs::lgetxattr(path, name, buffer))
 }
 
 /// Probe the length before allocating. A concurrently grown value fails safely; unreadable
 /// attributes or values beyond the parser cap skip recovery without affecting open/save.
-fn read_attribute_with(mut get: impl FnMut(&mut [u8]) -> rustix::io::Result<usize>) -> io::Result<Option<Vec<u8>>> {
+fn read_attribute_with(
+    query_size: impl FnOnce() -> rustix::io::Result<usize>,
+    read: impl FnOnce(&mut [u8]) -> rustix::io::Result<usize>,
+) -> io::Result<Option<Vec<u8>>> {
     use rustix::io::Errno;
     let unreadable = |error| match error {
         Errno::RANGE => Err(io::Error::new(io::ErrorKind::InvalidData, "opening metadata exceeds 64 KiB or changed while reading")),
         _ => Ok(None),
     };
-    let len = match get(&mut []) {
+    let len = match query_size() {
         Ok(len) if len <= MAX_ATTRIBUTE => len,
         Ok(_) => return unreadable(Errno::RANGE),
         Err(e) => return unreadable(e),
     };
-    let mut bytes = vec![0; len];
-    match get(bytes.as_mut_slice()) {
-        Ok(len) if len <= bytes.len() => {
-            bytes.truncate(len);
+    // Read through a nonempty buffer so a zero-length attribute can still be checked for growth.
+    let mut bytes = vec![0; len.max(1)];
+    match read(bytes.as_mut_slice()) {
+        Ok(count) if count <= len => {
+            bytes.truncate(count);
             Ok(Some(bytes.into_boxed_slice().into_vec()))
         }
         Ok(_) => unreadable(Errno::RANGE),
@@ -285,12 +289,16 @@ mod tests {
         let dir = directory();
         let file = dir.join("out.pdf");
         std::fs::write(&file, b"original PDF").unwrap();
-        xattr::set(&file, OPEN_WITH, &vec![b'x'; MAX_ATTRIBUTE + 1]).unwrap();
+        let binding = vec![b'x'; MAX_ATTRIBUTE + 1];
+        xattr::set(&file, OPEN_WITH, &binding).unwrap();
         xattr::set(&file, QUARANTINE, b"0082;1234;Preview;").unwrap();
         assert!(OpeningMetadata::read(&file).is_err());
         crate::editing::write_atomically(file.to_str().unwrap(), b"new PDF").unwrap();
         assert_eq!(std::fs::read(&file).unwrap(), b"new PDF");
-        assert_eq!(xattr::get(&file, OPEN_WITH).unwrap().unwrap().len(), MAX_ATTRIBUTE + 1);
+        let mut saved = vec![0; binding.len()];
+        let count = rustix::fs::lgetxattr(&file, OPEN_WITH, saved.as_mut_slice()).unwrap();
+        assert_eq!(count, binding.len());
+        assert_eq!(saved, binding);
         assert_eq!(xattr::get(&file, QUARANTINE).unwrap(), Some(b"0082;1234;Preview;".to_vec()));
         std::fs::remove_dir_all(dir).unwrap();
     }
@@ -393,30 +401,29 @@ mod tests {
     /// and skips recovery if the value grows between the probe and read.
     #[test]
     fn attribute_probes_bound_allocation_and_detect_growth() {
-        let mut calls = 0;
-        let small = read_attribute_with(|out| {
-            calls += 1;
-            if calls == 1 {
-                assert!(out.is_empty());
-                return Ok(3);
-            }
-            assert_eq!(out.len(), 3);
-            out.copy_from_slice(b"abc");
-            Ok(3)
-        })
+        let small = read_attribute_with(
+            || Ok(3),
+            |out| {
+                assert_eq!(out.len(), 3);
+                out.copy_from_slice(b"abc");
+                Ok(3)
+            },
+        )
         .unwrap()
         .unwrap();
         assert_eq!(small, b"abc");
         assert_eq!(small.capacity(), 3);
-        assert!(read_attribute_with(|_| Ok(MAX_ATTRIBUTE + 1)).is_err());
-        let mut calls = 0;
-        assert!(
-            read_attribute_with(|_| {
-                calls += 1;
-                if calls == 1 { Ok(3) } else { Err(rustix::io::Errno::RANGE) }
-            })
-            .is_err()
+        assert!(read_attribute_with(|| Ok(MAX_ATTRIBUTE + 1), |_| panic!("oversized attribute must not be read")).is_err());
+        assert!(read_attribute_with(|| Ok(3), |_| Err(rustix::io::Errno::RANGE)).is_err());
+        let empty = read_attribute_with(
+            || Ok(0),
+            |out| {
+                assert_eq!(out.len(), 1);
+                Ok(0)
+            },
         );
+        assert_eq!(empty.unwrap(), Some(Vec::new()));
+        assert!(read_attribute_with(|| Ok(0), |_| Ok(1)).is_err());
     }
 
     /// Volumes may expose no readable attributes with errors beyond ENOTSUP.
@@ -424,7 +431,7 @@ mod tests {
     fn unreadable_attributes_are_best_effort() {
         use rustix::io::Errno;
         for errno in [Errno::NOSYS, Errno::INVAL, Errno::PERM, Errno::ACCESS, Errno::IO] {
-            assert_eq!(read_attribute_with(|_| Err(errno)).unwrap(), None);
+            assert_eq!(read_attribute_with(|| Err(errno), |_| panic!("unreadable attribute must not be read")).unwrap(), None);
         }
     }
 
@@ -433,8 +440,8 @@ mod tests {
     fn unsupported_attribute_volumes_are_distinct_from_read_failures() {
         use rustix::io::Errno;
         for errno in [Errno::NOATTR, Errno::NOTSUP, Errno::OPNOTSUPP] {
-            assert_eq!(read_attribute_with(|_| Err(errno)).unwrap(), None, "{errno}");
+            assert_eq!(read_attribute_with(|| Err(errno), |_| panic!("unsupported attribute must not be read")).unwrap(), None, "{errno}");
         }
-        assert!(read_attribute_with(|_| Err(Errno::RANGE)).is_err());
+        assert!(read_attribute_with(|| Err(Errno::RANGE), |_| panic!("failed size query must not read the attribute")).is_err());
     }
 }