From 7ebbb17a8252291229386a4e1d31214467a10c10 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sun, 25 Oct 2026 15:31:09 -0700 Subject: [PATCH 1/5] Originals: on Windows a hard link to an original counts as that original safe_file::same_file compared length and canonical paths on every platform other than Unix. On Windows canonicalize returns a hard link's own path, so a hard link to a library original under another name was not the same file, and pin_write_target accepted it as a screenshot destination. For this reason the engine test write_targets_pin_absent_paths_and_recheck_the_current_library failed on Windows. On Windows same_file now first compares the volume serial number and file index through the same-file crate (already in Cargo.lock through walkdir), then falls back to the resolved-path comparison. The catalog crate takes same-file as a Windows-only dependency. --- Cargo.lock | 1 + Cargo.toml | 1 + crates/catalog/Cargo.toml | 3 +++ crates/catalog/src/safe_file.rs | 26 +++++++++++++++++++++++++- 4 files changed, 30 insertions(+), 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index b69533958..455758fb0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3220,6 +3220,7 @@ dependencies = [ "lightcraft-raw", "log", "proptest", + "same-file", "serde", "serde_json", "thiserror 2.0.21", diff --git a/Cargo.toml b/Cargo.toml index 5c0418d30..fc2d90d2e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -53,6 +53,7 @@ log = "0.4" # All date/time conversion goes through chrono (no hand-rolled calendar code beyond the 400-year range extension; AGENTS.md). chrono = { version = "0.4.45", default-features = false, features = ["std"] } rayon = "1" +same-file = "1" # lightcraft-catalog reads a file's volume serial number and file index with it on Windows proptest = "1" bytemuck = { version = "1", features = ["derive"] } crc32fast = "1" diff --git a/crates/catalog/Cargo.toml b/crates/catalog/Cargo.toml index 27273d5c4..e9df63166 100644 --- a/crates/catalog/Cargo.toml +++ b/crates/catalog/Cargo.toml @@ -20,6 +20,9 @@ log = { workspace = true } chrono = { workspace = true } web-time = { workspace = true } +[target.'cfg(windows)'.dependencies] +same-file = { workspace = true } + [dev-dependencies] lightcraft-geom = { workspace = true } proptest = { workspace = true } diff --git a/crates/catalog/src/safe_file.rs b/crates/catalog/src/safe_file.rs index fa067de0f..83139756c 100644 --- a/crates/catalog/src/safe_file.rs +++ b/crates/catalog/src/safe_file.rs @@ -23,6 +23,7 @@ //! replacing anything** (a hard link, which fails on a taken name; where the volume has no hard //! links, an exclusive create that claims the name, then a rename over that placeholder). //! - [`same_file`] tells whether two paths name the same file on disk (device + inode on Unix, +//! volume serial number + file index on Windows, //! resolved paths elsewhere), for "is this target an original?" checks. use std::cell::Cell; @@ -267,7 +268,8 @@ fn same_content(path: &Path, data: &[u8]) -> io::Result { } } -/// Do `a` and `b` name the same existing file (through links, `..`, case-insensitive names)? +/// Do `a` and `b` name the same existing file (through hard links, symlinks, `..`, +/// case-insensitive names)? pub fn same_file(a: &Path, b: &Path) -> bool { let (Ok(ma), Ok(mb)) = (fs::metadata(a), fs::metadata(b)) else { return false }; #[cfg(unix)] @@ -277,6 +279,14 @@ pub fn same_file(a: &Path, b: &Path) -> bool { } #[cfg(not(unix))] { + // A hard link has its own path but shares the volume serial number and file index with + // its original. A volume that does not report them is compared by resolved path below. + #[cfg(windows)] + { + if ::same_file::is_same_file(a, b).unwrap_or(false) { + return true; + } + } if ma.len() != mb.len() { return false; } @@ -397,6 +407,20 @@ mod tests { let _ = fs::remove_dir_all(&d); } + #[test] + fn same_file_sees_a_hard_link_under_another_name() { + let d = temp("same-link"); + let p = d.join("IMG_1.jpg"); + fs::write(&p, b"original").unwrap(); + let linked = d.join("linked.png"); + fs::hard_link(&p, &linked).unwrap(); + fs::write(d.join("copy.png"), b"original").unwrap(); + assert!(same_file(&p, &linked), "a hard link is the same file"); + assert!(same_file(&linked, &p)); + assert!(!same_file(&p, &d.join("copy.png")), "a copy with the same content and length is another file"); + let _ = fs::remove_dir_all(&d); + } + #[test] fn same_file_sees_through_other_spellings() { let d = temp("same"); From a97f19605c9fbfa9e52379faf5ff339742b71e41 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sun, 25 Oct 2026 22:32:37 -0700 Subject: [PATCH 2/5] Originals: compare lengths before the Windows file identity On ReFS two files can share the 64-bit file index that same_file compares, so same_file now rejects files of different lengths before it checks the volume serial number and file index. The hard-link test skips with a recorded reason when the temporary folder's volume has no hard links (FAT/exFAT, some shares), the same errors publish_new treats as a volume without hard links. --- Cargo.lock | 1 + crates/catalog/Cargo.toml | 1 + crates/catalog/src/safe_file.rs | 18 ++++++++++++++---- 3 files changed, 16 insertions(+), 4 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 455758fb0..3e9acf98c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3218,6 +3218,7 @@ dependencies = [ "lightcraft-geom", "lightcraft-meta", "lightcraft-raw", + "lightcraft-testkit", "log", "proptest", "same-file", diff --git a/crates/catalog/Cargo.toml b/crates/catalog/Cargo.toml index e9df63166..7a9c80439 100644 --- a/crates/catalog/Cargo.toml +++ b/crates/catalog/Cargo.toml @@ -25,4 +25,5 @@ same-file = { workspace = true } [dev-dependencies] lightcraft-geom = { workspace = true } +lightcraft-testkit = { workspace = true } proptest = { workspace = true } diff --git a/crates/catalog/src/safe_file.rs b/crates/catalog/src/safe_file.rs index 83139756c..f7737bf78 100644 --- a/crates/catalog/src/safe_file.rs +++ b/crates/catalog/src/safe_file.rs @@ -279,17 +279,18 @@ pub fn same_file(a: &Path, b: &Path) -> bool { } #[cfg(not(unix))] { + if ma.len() != mb.len() { + return false; + } // A hard link has its own path but shares the volume serial number and file index with // its original. A volume that does not report them is compared by resolved path below. + // On ReFS two files can share the 64-bit file index, so the lengths are compared first. #[cfg(windows)] { if ::same_file::is_same_file(a, b).unwrap_or(false) { return true; } } - if ma.len() != mb.len() { - return false; - } match (fs::canonicalize(a), fs::canonicalize(b)) { (Ok(x), Ok(y)) => x == y || x.to_string_lossy().to_lowercase() == y.to_string_lossy().to_lowercase(), _ => false, @@ -413,7 +414,16 @@ mod tests { let p = d.join("IMG_1.jpg"); fs::write(&p, b"original").unwrap(); let linked = d.join("linked.png"); - fs::hard_link(&p, &linked).unwrap(); + match fs::hard_link(&p, &linked) { + Ok(()) => {} + Err(e) if e.kind() == io::ErrorKind::AlreadyExists => panic!("{}: {e}", linked.display()), + // no hard links on this volume (FAT/exFAT, some shares), as in `publish_new` + Err(e) => { + let _ = fs::remove_dir_all(&d); + lightcraft_testkit::skip!(System, "no hard links in {}: {e}", d.display()); + return; + } + } fs::write(d.join("copy.png"), b"original").unwrap(); assert!(same_file(&p, &linked), "a hard link is the same file"); assert!(same_file(&linked, &p)); From 545ed92d6a1b28ed29117b43b7b8f7f1e0094cd1 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Mon, 26 Oct 2026 07:16:41 -0700 Subject: [PATCH 3/5] Originals: refuse a screenshot target whose file identity cannot be read On Windows the identity check opens both files to read the volume serial number and file index. When that open failed (another process holds the file without sharing it, or the file cannot be read), the hard link was treated as an ordinary file. The screenshot target check now refuses the target in that case. A missing original is still not a match, and other callers of same_file keep the current comparison. --- crates/catalog/src/safe_file.rs | 30 ++++++++++++---- crates/engine/src/originals.rs | 63 ++++++++++++++++++++++++++------- 2 files changed, 73 insertions(+), 20 deletions(-) diff --git a/crates/catalog/src/safe_file.rs b/crates/catalog/src/safe_file.rs index f7737bf78..b0974961f 100644 --- a/crates/catalog/src/safe_file.rs +++ b/crates/catalog/src/safe_file.rs @@ -271,30 +271,46 @@ fn same_content(path: &Path, data: &[u8]) -> io::Result { /// Do `a` and `b` name the same existing file (through hard links, symlinks, `..`, /// case-insensitive names)? pub fn same_file(a: &Path, b: &Path) -> bool { - let (Ok(ma), Ok(mb)) = (fs::metadata(a), fs::metadata(b)) else { return false }; + compare_files(a, b, false).unwrap_or(false) +} + +/// [`same_file`] for a check that must not let an unknown identity pass: `Err` when both paths +/// exist with the same length but the file identity of either cannot be read (on Windows, when +/// either file cannot be opened). A path that does not exist is still `Ok(false)`. +pub fn same_file_strict(a: &Path, b: &Path) -> io::Result { + compare_files(a, b, true) +} + +fn compare_files(a: &Path, b: &Path, strict: bool) -> io::Result { + #[cfg(not(windows))] + let _ = strict; + let (Ok(ma), Ok(mb)) = (fs::metadata(a), fs::metadata(b)) else { return Ok(false) }; #[cfg(unix)] { use std::os::unix::fs::MetadataExt; - ma.dev() == mb.dev() && ma.ino() == mb.ino() + Ok(ma.dev() == mb.dev() && ma.ino() == mb.ino()) } #[cfg(not(unix))] { if ma.len() != mb.len() { - return false; + return Ok(false); } // A hard link has its own path but shares the volume serial number and file index with // its original. A volume that does not report them is compared by resolved path below. // On ReFS two files can share the 64-bit file index, so the lengths are compared first. #[cfg(windows)] { - if ::same_file::is_same_file(a, b).unwrap_or(false) { - return true; + match ::same_file::is_same_file(a, b) { + Ok(true) => return Ok(true), + Ok(false) => {} + Err(e) if strict => return Err(e), + Err(_) => {} } } - match (fs::canonicalize(a), fs::canonicalize(b)) { + Ok(match (fs::canonicalize(a), fs::canonicalize(b)) { (Ok(x), Ok(y)) => x == y || x.to_string_lossy().to_lowercase() == y.to_string_lossy().to_lowercase(), _ => false, - } + }) } } diff --git a/crates/engine/src/originals.rs b/crates/engine/src/originals.rs index 890e98f81..e20a9378b 100644 --- a/crates/engine/src/originals.rs +++ b/crates/engine/src/originals.rs @@ -9,7 +9,7 @@ use std::path::{Path, PathBuf}; #[cfg(not(any(unix, windows)))] use std::time::SystemTime; -use lightcraft_catalog::safe_file::same_file; +use lightcraft_catalog::safe_file::{same_file, same_file_strict}; use lightcraft_catalog::{Catalog, Source}; /// The directory entry a screenshot is about to write, pinned at request admission. @@ -147,14 +147,30 @@ impl OriginalGuard { if let Some(n) = std::fs::canonicalize(target).ok().and_then(|c| c.file_name().map(|n| n.to_string_lossy().to_lowercase())) { names.push(n); } + // The identity check refuses a target whose file identity cannot be read, so an unreadable + // hard link to an original is not taken for an ordinary file. + let refuse_if_same = |p: &Path, what: &str| -> Result<(), String> { + let same = if hard_links { + same_file_strict(p, target).map_err(|e| { + format!( + "{}: LightCraft cannot tell whether this is {what} in the library ({e}) and never writes over an original (choose another folder or file name)", + target.display() + ) + })? + } else { + same_file(p, target) + }; + if same { + return Err(format!( + "{} is {what} in the library: LightCraft never writes over an original (choose another folder or file name)", + target.display() + )); + } + Ok(()) + }; for n in &names { for (p, what) in self.by_name.get(n).into_iter().flatten() { - if same_file(p, target) { - return Err(format!( - "{} is {what} in the library: LightCraft never writes over an original (choose another folder or file name)", - target.display() - )); - } + refuse_if_same(p, what)?; } } // An imported hard link or symlink can have a different name from its original. A write @@ -166,12 +182,7 @@ impl OriginalGuard { if !hard_links && std::fs::read_link(p).is_err() { continue; } - if same_file(p, target) { - return Err(format!( - "{} is {what} in the library: LightCraft never writes over an original (choose another folder or file name)", - target.display() - )); - } + refuse_if_same(p, what)?; } Ok(()) } @@ -339,4 +350,30 @@ mod tests { } let _ = std::fs::remove_dir_all(&d); } + + #[cfg(windows)] + #[test] + fn a_hard_link_whose_identity_cannot_be_read_is_refused() { + use std::os::windows::fs::OpenOptionsExt; + let d = std::env::temp_dir().join(format!("lc-unreadable-link-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&d); + std::fs::create_dir_all(&d).unwrap(); + let mut s = crate::Session::new(); + let original = d.join("IMG_1.jpg"); + std::fs::write(&original, b"original").unwrap(); + let photo = Photo::new(PhotoId(1), Source::File { path: original.to_string_lossy().to_string() }, "IMG_1.jpg", "JPEG", 10, 10, "2026-01-01"); + s.catalog.apply(Op::AddPhoto { photo: Box::new(photo) }).unwrap(); + let linked = d.join("shot.png"); + std::fs::hard_link(&original, &linked).unwrap(); + + // A handle that shares nothing makes every other open of the file fail, so the file + // identity of either name cannot be read while it is held. + let held = std::fs::OpenOptions::new().read(true).share_mode(0).open(&original).unwrap(); + let refused = s.pin_write_target(&linked.to_string_lossy()).unwrap_err(); + assert!(refused.contains("original"), "{refused}"); + drop(held); + + assert!(s.pin_write_target(&linked.to_string_lossy()).unwrap_err().contains("original")); + let _ = std::fs::remove_dir_all(&d); + } } From 86f2ba81eb5fe5fec35606dca597ce116ad77927 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Mon, 26 Oct 2026 11:01:51 -0700 Subject: [PATCH 4/5] Originals: refuse a screenshot target when an original cannot be queried same_file_strict returned false when the metadata of either path could not be read, so a hard link to an original in a folder the process cannot enter was accepted as a screenshot target. It now returns the error unless the path does not exist. --- crates/catalog/src/safe_file.rs | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/crates/catalog/src/safe_file.rs b/crates/catalog/src/safe_file.rs index b0974961f..9b8bf5bcd 100644 --- a/crates/catalog/src/safe_file.rs +++ b/crates/catalog/src/safe_file.rs @@ -274,17 +274,22 @@ pub fn same_file(a: &Path, b: &Path) -> bool { compare_files(a, b, false).unwrap_or(false) } -/// [`same_file`] for a check that must not let an unknown identity pass: `Err` when both paths -/// exist with the same length but the file identity of either cannot be read (on Windows, when -/// either file cannot be opened). A path that does not exist is still `Ok(false)`. +/// [`same_file`] for a check that must not let an unknown identity pass: `Err` when the metadata +/// of either path cannot be read for a reason other than the path not existing (for example a +/// folder it cannot enter), or when both paths exist with the same length but the file identity +/// of either cannot be read (on Windows, when either file cannot be opened). A path that does not +/// exist is still `Ok(false)`. pub fn same_file_strict(a: &Path, b: &Path) -> io::Result { compare_files(a, b, true) } fn compare_files(a: &Path, b: &Path, strict: bool) -> io::Result { - #[cfg(not(windows))] - let _ = strict; - let (Ok(ma), Ok(mb)) = (fs::metadata(a), fs::metadata(b)) else { return Ok(false) }; + let missing = |e: &io::Error| matches!(e.kind(), io::ErrorKind::NotFound | io::ErrorKind::NotADirectory); + let (ma, mb) = match (fs::metadata(a), fs::metadata(b)) { + (Ok(ma), Ok(mb)) => (ma, mb), + (Err(e), _) | (_, Err(e)) if strict && !missing(&e) => return Err(e), + _ => return Ok(false), + }; #[cfg(unix)] { use std::os::unix::fs::MetadataExt; @@ -459,4 +464,20 @@ mod tests { assert!(!same_file(&p, &d.join("missing.jpg"))); let _ = fs::remove_dir_all(&d); } + + #[test] + fn strict_comparison_reports_a_path_it_cannot_query() { + let d = temp("strict-query"); + let p = d.join("x.jpg"); + fs::write(&p, b"x").unwrap(); + // A NUL byte makes the metadata lookup fail with an error other than "not found" on + // every platform, as a folder the process cannot enter does. + let unreadable = Path::new("IMG_1\0.jpg"); + assert!(same_file_strict(unreadable, &p).is_err()); + assert!(same_file_strict(&p, unreadable).is_err()); + assert!(!same_file(unreadable, &p), "the lenient comparison still answers false"); + assert!(!same_file_strict(&d.join("missing.jpg"), &p).unwrap(), "a missing original is another file"); + assert!(!same_file_strict(&p.join("under-a-file.jpg"), &p).unwrap()); + let _ = fs::remove_dir_all(&d); + } } From 4826fff7346f831d5bd7d17ef3218ac3dd7e1012 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Mon, 26 Oct 2026 11:34:06 -0700 Subject: [PATCH 5/5] Originals: a Windows file index of 0 does not identify a file same_file::is_same_file treats two files with the same volume serial number and file index as one file, also when both report index 0. The Windows comparison now reads both identities through winapi-util and accepts a match only when both file indexes are nonzero; otherwise the resolved paths are compared, as for a volume without file indexes. --- Cargo.lock | 2 +- Cargo.toml | 2 +- crates/catalog/Cargo.toml | 2 +- crates/catalog/src/safe_file.rs | 33 +++++++++++++++++++++++++++++---- 4 files changed, 32 insertions(+), 7 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3e9acf98c..0fcf8a35c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3221,11 +3221,11 @@ dependencies = [ "lightcraft-testkit", "log", "proptest", - "same-file", "serde", "serde_json", "thiserror 2.0.21", "web-time", + "winapi-util", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index fc2d90d2e..4e122eb24 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -53,7 +53,7 @@ log = "0.4" # All date/time conversion goes through chrono (no hand-rolled calendar code beyond the 400-year range extension; AGENTS.md). chrono = { version = "0.4.45", default-features = false, features = ["std"] } rayon = "1" -same-file = "1" # lightcraft-catalog reads a file's volume serial number and file index with it on Windows +winapi-util = "0.1" # lightcraft-catalog reads a file's volume serial number and file index with it on Windows proptest = "1" bytemuck = { version = "1", features = ["derive"] } crc32fast = "1" diff --git a/crates/catalog/Cargo.toml b/crates/catalog/Cargo.toml index 7a9c80439..b12e18803 100644 --- a/crates/catalog/Cargo.toml +++ b/crates/catalog/Cargo.toml @@ -21,7 +21,7 @@ chrono = { workspace = true } web-time = { workspace = true } [target.'cfg(windows)'.dependencies] -same-file = { workspace = true } +winapi-util = { workspace = true } [dev-dependencies] lightcraft-geom = { workspace = true } diff --git a/crates/catalog/src/safe_file.rs b/crates/catalog/src/safe_file.rs index 9b8bf5bcd..bc05d4139 100644 --- a/crates/catalog/src/safe_file.rs +++ b/crates/catalog/src/safe_file.rs @@ -301,13 +301,13 @@ fn compare_files(a: &Path, b: &Path, strict: bool) -> io::Result { return Ok(false); } // A hard link has its own path but shares the volume serial number and file index with - // its original. A volume that does not report them is compared by resolved path below. + // its original. A volume that reports file index 0 is compared by resolved path below. // On ReFS two files can share the 64-bit file index, so the lengths are compared first. #[cfg(windows)] { - match ::same_file::is_same_file(a, b) { - Ok(true) => return Ok(true), - Ok(false) => {} + match file_ids(a, b) { + Ok((ia, ib)) if same_file_id(ia, ib) => return Ok(true), + Ok(_) => {} Err(e) if strict => return Err(e), Err(_) => {} } @@ -319,6 +319,22 @@ fn compare_files(a: &Path, b: &Path, strict: bool) -> io::Result { } } +/// The volume serial number and file index of `a` and `b`, read while both files are open. +#[cfg(windows)] +fn file_ids(a: &Path, b: &Path) -> io::Result<((u64, u64), (u64, u64))> { + use winapi_util::{Handle, file::information}; + let (ha, hb) = (Handle::from_path_any(a)?, Handle::from_path_any(b)?); + let (ia, ib) = (information(&ha)?, information(&hb)?); + Ok(((ia.volume_serial_number(), ia.file_index()), (ib.volume_serial_number(), ib.file_index()))) +} + +/// Whether two (volume serial number, file index) pairs name one file. A file index of 0 is not +/// an identity and matches nothing. +#[cfg(any(windows, test))] +fn same_file_id(a: (u64, u64), b: (u64, u64)) -> bool { + a.1 != 0 && b.1 != 0 && a == b +} + #[cfg(test)] mod tests { use super::*; @@ -480,4 +496,13 @@ mod tests { assert!(!same_file_strict(&p.join("under-a-file.jpg"), &p).unwrap()); let _ = fs::remove_dir_all(&d); } + + #[test] + fn a_zero_file_index_is_not_a_file_identity() { + assert!(same_file_id((7, 42), (7, 42))); + assert!(!same_file_id((7, 42), (7, 43))); + assert!(!same_file_id((7, 42), (8, 42))); + assert!(!same_file_id((7, 0), (7, 0)), "two files on a volume that reports index 0 are not the same file"); + assert!(!same_file_id((0, 0), (0, 0))); + } }