From 2d5a7497fd1e1891c796f1a93741b27a1d904604 Mon Sep 17 00:00:00 2001 From: shmuel biller Date: Tue, 13 Oct 2026 11:20:31 -0400 Subject: [PATCH] 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 --- Cargo.lock | 1 + crates/format/Cargo.toml | 11 +- crates/format/src/store.rs | 117 +++++++++++++- crates/format/tests/directory_symlinks.rs | 176 ++++++++++++++++++++++ 4 files changed, 296 insertions(+), 9 deletions(-) create mode 100644 crates/format/tests/directory_symlinks.rs diff --git a/Cargo.lock b/Cargo.lock index 79e7cd895e..6820d6dee5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4397,6 +4397,7 @@ dependencies = [ "blake3", "crc32fast", "flate2", + "libc", "photocraft-codecs", "photocraft-color", "photocraft-doc", diff --git a/crates/format/Cargo.toml b/crates/format/Cargo.toml index 00a41ac070..45d7a4ccf3 100644 --- a/crates/format/Cargo.toml +++ b/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 } diff --git a/crates/format/src/store.rs b/crates/format/src/store.rs index c14b488250..d047668cb0 100644 --- a/crates/format/src/store.rs +++ b/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 { + 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 { + 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)> { @@ -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> { let mut out = HashSet::new(); for sub in ["tiles", "blobs"] { @@ -878,7 +983,7 @@ fn list_objects(dir: &Path) -> Result> { 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)), diff --git a/crates/format/tests/directory_symlinks.rs b/crates/format/tests/directory_symlinks.rs new file mode 100644 index 0000000000..0302593f49 --- /dev/null +++ b/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(); + } +}