fix(format): harden directory-bundle cleanup

Protect Unix bundle cleanup with pinned directory handles and remove only recognized content-addressed files. Keep legitimate linked roots and write/search-only directories working, with regression coverage for GC races and permission failures.

Co-authored-by: skylar taylor-barrick <[email protected]>

Cargo.lock

@@ -4397,6 +4397,7 @@ dependencies = [
  "blake3",
  "crc32fast",
  "flate2",
+ "libc",
  "photocraft-codecs",
  "photocraft-color",
  "photocraft-doc",

crates/format/Cargo.toml

@@ -24,10 +24,15 @@ crc32fast = "1.5"
 blake3 = { version = "1.8", default-features = false, features = ["pure"] }
 ruzstd = "0.9"
 
-# A plain fsync where a file system refuses the full flush (`sync_file`, atomic.rs). Already in
-# Cargo.lock on Apple targets (cap-std), so this adds no new crate.
-[target.'cfg(target_vendor = "apple")'.dependencies]
+# Unix directory cleanup uses no-follow descriptor-relative operations; the same
+# dependency provides the plain-fsync fallback on Apple file systems.
+[target.'cfg(unix)'.dependencies]
 rustix = { version = "1.1", default-features = false, features = ["std", "fs"] }
 
+# rustix does not expose Apple's directory-search open mode. Only the named
+# constant is used; all descriptor operations remain in safe rustix APIs.
+[target.'cfg(target_vendor = "apple")'.dependencies]
+libc = "0.2"
+
 [dev-dependencies]
 proptest = { workspace = true }

crates/format/src/store.rs

@@ -570,10 +570,82 @@ fn write_bundle_file(root: &Path, path: &Path, data: &[u8]) -> Result<()> {
     result
 }
 
-fn remove_bundle_file(root: &Path, path: &Path) -> Result<()> {
+/// Keep the existing leaf/path validation and pin the actual unlink to opened
+/// directory handles on Unix. This closes the check-then-remove path-swap gap.
+fn remove_bundle_file(root: &Path, path: &Path, gc: &BundleGc) -> Result<()> {
     validate_bundle_file(root, path)?;
-    std::fs::remove_file(path)?;
-    Ok(())
+    let relative = path.strip_prefix(root).map_err(|_| unsafe_bundle_entry(path))?;
+    gc.remove(relative)
+}
+
+#[cfg(unix)]
+struct BundleGc {
+    root: rustix::fd::OwnedFd,
+    tiles: rustix::fd::OwnedFd,
+    blobs: rustix::fd::OwnedFd,
+    composite: rustix::fd::OwnedFd,
+}
+
+#[cfg(unix)]
+fn gc_directory_flags() -> rustix::fs::OFlags {
+    use rustix::fs::OFlags;
+
+    // GC needs directory traversal, not permission to read a directory listing.
+    // Both modes retain a descriptor for the directory opened here, so a later
+    // pathname replacement cannot redirect descriptor-relative cleanup.
+    #[cfg(any(target_os = "linux", target_os = "android"))]
+    let access = OFlags::PATH;
+    #[cfg(target_vendor = "apple")]
+    let access = OFlags::from_bits_retain(libc::O_SEARCH.cast_unsigned());
+    #[cfg(not(any(target_os = "linux", target_os = "android", target_vendor = "apple")))]
+    let access = OFlags::RDONLY;
+    access | OFlags::DIRECTORY | OFlags::NOFOLLOW | OFlags::CLOEXEC
+}
+
+#[cfg(unix)]
+impl BundleGc {
+    fn new(root: &Path) -> Result<Self> {
+        use rustix::fs::{Mode, open, openat};
+
+        let flags = gc_directory_flags();
+        let root = open(root, flags, Mode::empty()).map_err(std::io::Error::from)?;
+        let tiles = openat(&root, "tiles", flags, Mode::empty()).map_err(std::io::Error::from)?;
+        let blobs = openat(&root, "blobs", flags, Mode::empty()).map_err(std::io::Error::from)?;
+        let composite = openat(&root, "composite", flags, Mode::empty()).map_err(std::io::Error::from)?;
+        Ok(Self { root, tiles, blobs, composite })
+    }
+
+    fn remove(&self, relative: &Path) -> Result<()> {
+        let relative = relative.to_str().ok_or_else(|| unsafe_bundle_entry(relative))?;
+        let (handle, name) = match relative {
+            THUMB => (&self.root, THUMB),
+            COMPOSITE => (&self.composite, "preview.png"),
+            _ => match relative.split_once('/') {
+                Some(("tiles", name)) if name.strip_suffix(".zst").is_some_and(is_valid_hash) => (&self.tiles, name),
+                Some(("blobs", name)) if name.strip_suffix(".zst").is_some_and(is_valid_hash) => (&self.blobs, name),
+                _ => return Err(FormatError::Unsupported(format!("invalid bundle cleanup target: {relative}"))),
+            },
+        };
+        rustix::fs::unlinkat(handle, name, rustix::fs::AtFlags::empty()).map_err(std::io::Error::from)?;
+        Ok(())
+    }
+}
+
+#[cfg(not(unix))]
+struct BundleGc {
+    root: PathBuf,
+}
+
+#[cfg(not(unix))]
+impl BundleGc {
+    fn new(root: &Path) -> Result<Self> {
+        Ok(Self { root: root.to_path_buf() })
+    }
+
+    fn remove(&self, relative: &Path) -> Result<()> {
+        std::fs::remove_file(self.root.join(relative))?;
+        Ok(())
+    }
 }
 
 fn existing_object_is_valid(path: &Path, object: &Object, what: &str) -> Result<(bool, Option<FileSignature>)> {
@@ -802,11 +874,14 @@ impl PcraftWriter {
         for (name, data) in &p.previews {
             write_bundle_file(dir, &dir.join(name), data)?;
         }
+        // Pin all GC directories before removing stale previews or objects.
+        // Unix cleanup then stays within these directories even if a pathname is swapped.
+        let gc = BundleGc::new(dir)?;
         for stale in [THUMB, COMPOSITE] {
             if !p.previews.iter().any(|(n, _)| *n == stale) {
                 let path = dir.join(stale);
                 validate_bundle_file(dir, &path)?;
-                match remove_bundle_file(dir, &path) {
+                match remove_bundle_file(dir, &path, &gc) {
                     Err(FormatError::Io(e)) if e.kind() == std::io::ErrorKind::NotFound => {}
                     other => other?,
                 }
@@ -818,7 +893,7 @@ impl PcraftWriter {
         // complete save into a reported failure); a leftover is retried on the next save.
         for path in existing {
             if !p.objects.contains_key(&path) {
-                if remove_bundle_file(dir, &dir.join(&path)).is_ok() {
+                if remove_bundle_file(dir, &dir.join(&path), &gc).is_ok() {
                     stats.objects_removed += 1;
                 }
                 if let Some(cache) = &mut self.verified_directory {
@@ -866,6 +941,36 @@ impl PcraftWriter {
     }
 }
 
+#[cfg(all(test, unix))]
+#[test]
+fn gc_pinned_directory_handles_resist_subdirectory_swap() {
+    use std::os::unix::fs::symlink;
+    use std::sync::atomic::{AtomicU64, Ordering};
+
+    static NEXT: AtomicU64 = AtomicU64::new(0);
+    let root = std::env::temp_dir().join(format!("pcraft-gc-swap-{}-{}", std::process::id(), NEXT.fetch_add(1, Ordering::Relaxed)));
+    let bundle = root.join("project.pcraft");
+    let outside = root.join("outside");
+    for sub in ["tiles", "blobs", "composite"] {
+        std::fs::create_dir_all(bundle.join(sub)).unwrap();
+    }
+    std::fs::create_dir(&outside).unwrap();
+    let filename = format!("{}.zst", "c".repeat(64));
+    std::fs::write(bundle.join("tiles").join(&filename), b"stale bundle file").unwrap();
+    std::fs::write(outside.join(&filename), b"outside file must survive").unwrap();
+
+    let gc = BundleGc::new(&bundle).unwrap();
+    std::fs::rename(bundle.join("tiles"), bundle.join("old-tiles")).unwrap();
+    symlink(&outside, bundle.join("tiles")).unwrap();
+
+    // Exercise the descriptor-relative unlink directly: the public save path
+    // also validates the leaf, but that check itself cannot prevent a later swap.
+    gc.remove(Path::new(&format!("tiles/{filename}"))).unwrap();
+    assert!(!bundle.join("old-tiles").join(&filename).exists());
+    assert_eq!(std::fs::read(outside.join(&filename)).unwrap(), b"outside file must survive");
+    std::fs::remove_dir_all(root).unwrap();
+}
+
 fn list_objects(dir: &Path) -> Result<HashSet<String>> {
     let mut out = HashSet::new();
     for sub in ["tiles", "blobs"] {
@@ -878,7 +983,7 @@ fn list_objects(dir: &Path) -> Result<HashSet<String>> {
         for e in rd {
             let e = e?;
             let name = e.file_name().to_string_lossy().into_owned();
-            if name.ends_with(".zst") {
+            if name.strip_suffix(".zst").is_some_and(is_valid_hash) {
                 let path = e.path();
                 match std::fs::symlink_metadata(&path) {
                     Ok(metadata) if metadata.file_type().is_symlink() => return Err(unsafe_bundle_entry(&path)),

crates/format/tests/directory_symlinks.rs

@@ -0,0 +1,176 @@
+//! Directory cleanup only owns content-addressed objects; unrelated files survive.
+
+mod common;
+
+use common::temp_dir;
+use photocraft_color::{ColorMode, SampleType};
+use photocraft_doc::Document;
+use photocraft_format::{PcraftWriter, SaveOptions};
+use photocraft_geom::Size;
+
+fn empty_doc() -> Document {
+    Document::new("clean", Size::new(1, 1), ColorMode::Rgb, SampleType::U8)
+}
+
+#[test]
+fn directory_cleanup_preserves_unrelated_zst_files() {
+    let root = temp_dir("unrelated-object");
+    let bundle = root.join("project.pcraft");
+    let mut writer = PcraftWriter::new();
+    writer.save_dir(&empty_doc(), &bundle, &SaveOptions::default()).unwrap();
+
+    let unrelated = bundle.join("tiles/personal-notes.zst");
+    let orphan = bundle.join("blobs").join(format!("{}.zst", "b".repeat(64)));
+    std::fs::write(&unrelated, b"unrelated file").unwrap();
+    std::fs::write(&orphan, b"unused content-addressed object").unwrap();
+
+    let stats = writer.save_dir(&empty_doc(), &bundle, &SaveOptions::default()).unwrap();
+    assert_eq!(std::fs::read(&unrelated).unwrap(), b"unrelated file");
+    assert!(!orphan.exists(), "a valid but unreferenced object is removed");
+    assert_eq!(stats.objects_removed, 1);
+    std::fs::remove_dir_all(root).unwrap();
+}
+
+#[cfg(any(target_os = "linux", target_os = "android", target_vendor = "apple"))]
+mod permissions {
+    use super::*;
+    use photocraft_format::load_path;
+    use photocraft_raster::Rgba8Image;
+    use std::os::unix::fs::{PermissionsExt, symlink};
+    use std::path::{Path, PathBuf};
+
+    struct RestoreMode {
+        path: PathBuf,
+        permissions: std::fs::Permissions,
+    }
+
+    impl RestoreMode {
+        fn set(path: &Path, mode: u32) -> Self {
+            let permissions = std::fs::metadata(path).unwrap().permissions();
+            std::fs::set_permissions(path, std::fs::Permissions::from_mode(mode)).unwrap();
+            Self { path: path.to_path_buf(), permissions }
+        }
+    }
+
+    impl Drop for RestoreMode {
+        fn drop(&mut self) {
+            let _ = std::fs::set_permissions(&self.path, self.permissions.clone());
+        }
+    }
+
+    #[test]
+    fn directory_save_without_previews_does_not_require_composite_read_permission() {
+        let root = temp_dir("composite-search-only");
+        let bundle = root.join("project.pcraft");
+        let mut writer = PcraftWriter::new();
+        writer.save_dir(&empty_doc(), &bundle, &SaveOptions::default()).unwrap();
+        let restricted = RestoreMode::set(&bundle.join("composite"), 0o300);
+        let mut changed = empty_doc();
+        changed.name = "saved without previews".into();
+        let result = writer.save_dir(&changed, &bundle, &SaveOptions::default());
+        drop(restricted);
+        result.expect("saving without previews does not read composite");
+        assert_eq!(load_path(&bundle).unwrap(), changed);
+        std::fs::remove_dir_all(root).unwrap();
+    }
+
+    #[test]
+    fn directory_save_cleans_stale_previews_through_search_only_root_and_composite() {
+        let root = temp_dir("bundle-search-only");
+        let bundle = root.join("project.pcraft");
+        let link = root.join("linked.pcraft");
+        let mut writer = PcraftWriter::new();
+        writer.save_dir(&empty_doc(), &bundle, &SaveOptions::default()).unwrap();
+        symlink(&bundle, &link).unwrap();
+        std::fs::write(bundle.join("thumb.png"), b"stale thumbnail").unwrap();
+        std::fs::write(bundle.join("composite/preview.png"), b"stale composite").unwrap();
+        let orphan = bundle.join("blobs").join(format!("{}.zst", "a".repeat(64)));
+        std::fs::write(&orphan, b"unreferenced object").unwrap();
+        let composite_mode = RestoreMode::set(&bundle.join("composite"), 0o300);
+        let root_mode = RestoreMode::set(&bundle, 0o300);
+        let mut changed = empty_doc();
+        changed.name = "saved through linked root".into();
+        let result = writer.save_dir(&changed, &link, &SaveOptions::default());
+        drop(root_mode);
+        drop(composite_mode);
+        let stats = result.expect("cleanup needs directory traversal and write permission, not read");
+        assert_eq!(stats.objects_removed, 1);
+        assert!(!orphan.exists());
+        assert!(!bundle.join("thumb.png").exists());
+        assert!(!bundle.join("composite/preview.png").exists());
+        assert_eq!(load_path(&bundle).unwrap(), changed);
+        std::fs::remove_dir_all(root).unwrap();
+    }
+
+    #[test]
+    fn directory_save_writes_current_preview_without_composite_read_permission() {
+        let root = temp_dir("preview-search-only");
+        let bundle = root.join("project.pcraft");
+        let mut writer = PcraftWriter::new();
+        writer.save_dir(&empty_doc(), &bundle, &SaveOptions::default()).unwrap();
+        let restricted = RestoreMode::set(&bundle.join("composite"), 0o300);
+        let mut changed = empty_doc();
+        changed.name = "saved with preview".into();
+        let opts = SaveOptions { composite: Some(Rgba8Image::new(2, 2)), ..Default::default() };
+        let result = writer.save_dir(&changed, &bundle, &opts);
+        drop(restricted);
+        result.expect("writing a preview does not require listing its directory");
+        assert!(std::fs::read(bundle.join("composite/preview.png")).unwrap().starts_with(b"\x89PNG\r\n\x1a\n"));
+        assert_eq!(load_path(&bundle).unwrap(), changed);
+        std::fs::remove_dir_all(root).unwrap();
+    }
+
+    #[test]
+    fn denied_stale_preview_cleanup_preserves_the_previous_manifest() {
+        let root = temp_dir("denied-preview-cleanup");
+        let bundle = root.join("project.pcraft");
+        let mut writer = PcraftWriter::new();
+        let original = empty_doc();
+        writer.save_dir(&original, &bundle, &SaveOptions::default()).unwrap();
+        let preview = bundle.join("composite/preview.png");
+        std::fs::write(&preview, b"stale composite").unwrap();
+        let restricted = RestoreMode::set(&bundle.join("composite"), 0o500);
+        let permission_probe = bundle.join("composite/permission-probe");
+        if std::fs::write(&permission_probe, b"probe").is_ok() {
+            // An elevated test process may bypass directory permissions.
+            std::fs::remove_file(permission_probe).unwrap();
+            drop(restricted);
+            std::fs::remove_dir_all(root).unwrap();
+            return;
+        }
+        let mut changed = empty_doc();
+        changed.name = "must not commit".into();
+        let result = writer.save_dir(&changed, &bundle, &SaveOptions::default());
+        drop(restricted);
+        assert!(result.is_err(), "a required pre-commit cleanup failure is reported");
+        assert_eq!(std::fs::read(preview).unwrap(), b"stale composite");
+        assert_eq!(load_path(&bundle).unwrap(), original);
+        std::fs::remove_dir_all(root).unwrap();
+    }
+
+    #[test]
+    fn denied_object_cleanup_remains_best_effort_after_manifest_commit() {
+        let root = temp_dir("denied-object-cleanup");
+        let bundle = root.join("project.pcraft");
+        let mut writer = PcraftWriter::new();
+        writer.save_dir(&empty_doc(), &bundle, &SaveOptions::default()).unwrap();
+        let orphan = bundle.join("tiles").join(format!("{}.zst", "d".repeat(64)));
+        std::fs::write(&orphan, b"unreferenced object").unwrap();
+        let restricted = RestoreMode::set(&bundle.join("tiles"), 0o500);
+        let permission_probe = bundle.join("tiles/permission-probe");
+        if std::fs::write(&permission_probe, b"probe").is_ok() {
+            std::fs::remove_file(permission_probe).unwrap();
+            drop(restricted);
+            std::fs::remove_dir_all(root).unwrap();
+            return;
+        }
+        let mut changed = empty_doc();
+        changed.name = "committed despite housekeeping failure".into();
+        let result = writer.save_dir(&changed, &bundle, &SaveOptions::default());
+        drop(restricted);
+        assert_eq!(result.unwrap().objects_removed, 0);
+        assert!(orphan.exists());
+        assert_eq!(load_path(&bundle).unwrap(), changed);
+        std::fs::remove_dir_all(root).unwrap();
+    }
+}