From ffb57d1a9bd5e9fa5343006c1da8a13b039c90f3 Mon Sep 17 00:00:00 2001 From: Matej Vrzala Date: Thu, 22 Oct 2026 18:39:33 +0200 Subject: [PATCH] Library lock: Don't write through a link at the owner note's name LibraryLock::acquire wrote catalog.lock.owner with std::fs::write, which follows a symlink at that name. A library folder someone else prepared (a shared drive, an unpacked archive, a synced folder) could carry catalog.lock.owner -> ~/.bashrc or any other file the user can write, and opening the library replaced that file's content with the owner JSON. The catalog's staging files had the same problem and were fixed in #1502; the owner note was missed. The note is now written through store::create_staging (made pub(crate)): whatever is at the name is removed (a link itself, never its target) and the file is created with create_new. Drop already removed the note with remove_file, which never follows a link. Test (Unix): a link at catalog.lock.owner pointing at a file outside the library leaves that file's bytes unchanged through acquire and drop, and the note lands in the library as a regular file naming this process. Reported-by: BS.ehlas.cz --- crates/catalog/src/lock.rs | 6 ++++-- crates/catalog/src/store.rs | 4 ++-- crates/catalog/src/tests_lock.rs | 22 ++++++++++++++++++++++ docs/whats-new.md | 7 +++++++ 4 files changed, 35 insertions(+), 4 deletions(-) diff --git a/crates/catalog/src/lock.rs b/crates/catalog/src/lock.rs index d699da049..f701087a6 100644 --- a/crates/catalog/src/lock.rs +++ b/crates/catalog/src/lock.rs @@ -25,7 +25,7 @@ //! - Browser storage (OPFS) has no file locks: see the web host for its own guard. use std::fs::{File, OpenOptions, TryLockError}; -use std::io; +use std::io::{self, Write}; use std::path::{Path, PathBuf}; /// The lock file in the library directory. @@ -116,9 +116,11 @@ impl LibraryLock { } Err(TryLockError::Error(e)) => return Ok(unlocked(e)), } + // A new file in place of whatever is at the name: a link planted there in a library folder + // someone else prepared is removed, never written through (as for the staging files, #1502). let owner = LockOwner::this_process(program); if let Ok(json) = serde_json::to_vec_pretty(&owner) - && let Err(e) = std::fs::write(dir.join(OWNER), json) + && let Err(e) = crate::store::create_staging(&dir.join(OWNER)).and_then(|mut f| f.write_all(&json)) { log::warn!("library {}: can't write {OWNER}: {e}", dir.display()); } diff --git a/crates/catalog/src/store.rs b/crates/catalog/src/store.rs index 553eb241f..ad008cf0c 100644 --- a/crates/catalog/src/store.rs +++ b/crates/catalog/src/store.rs @@ -75,8 +75,8 @@ impl FsStore { /// A new staging file at `tmp`: whatever is already there (a leftover from a crashed save, or a link) is removed first /// (a link itself, never what it points to), then the file is created exclusively, so a write never follows a link -/// planted at that name into another file (issue #1502). -fn create_staging(tmp: &std::path::Path) -> io::Result { +/// planted at that name into another file (issue #1502). Also writes the lock's owner note. +pub(crate) fn create_staging(tmp: &std::path::Path) -> io::Result { if std::fs::symlink_metadata(tmp).is_ok() { std::fs::remove_file(tmp)?; } diff --git a/crates/catalog/src/tests_lock.rs b/crates/catalog/src/tests_lock.rs index baa8da659..2472ba92a 100644 --- a/crates/catalog/src/tests_lock.rs +++ b/crates/catalog/src/tests_lock.rs @@ -62,6 +62,28 @@ fn leftover_lock_files_are_not_a_lock() { let _ = std::fs::remove_dir_all(&dir); } +/// A link planted at the owner note's name (`catalog.lock.owner`) in a library folder someone else +/// prepared is removed, never written through: the file it points to keeps its bytes, and the note +/// still lands in the library (as for the catalog's staging files, issue #1502). +#[cfg(unix)] +#[test] +fn a_link_at_the_owner_note_is_never_written_through() { + let _serial = serial(); + let dir = temp_dir("owner-link"); + let victim = dir.join("victim.txt"); + std::fs::write(&victim, b"keep").unwrap(); + std::os::unix::fs::symlink(&victim, dir.join(OWNER)).unwrap(); + let l = LibraryLock::acquire(&dir, "LightCraft").unwrap(); + assert!(l.held()); + assert_eq!(std::fs::read(&victim).unwrap(), b"keep"); + assert!(!std::fs::symlink_metadata(dir.join(OWNER)).unwrap().file_type().is_symlink(), "the link was replaced by the note"); + let owner: LockOwner = serde_json::from_slice(&std::fs::read(dir.join(OWNER)).unwrap()).unwrap(); + assert_eq!(owner.pid, std::process::id()); + drop(l); + assert_eq!(std::fs::read(&victim).unwrap(), b"keep"); + let _ = std::fs::remove_dir_all(&dir); +} + /// Another process holds the lock; when it is killed (a crash), the OS releases it. #[test] fn lock_held_by_another_process_is_released_when_it_dies() { diff --git a/docs/whats-new.md b/docs/whats-new.md index cffff83e2..cea298611 100644 --- a/docs/whats-new.md +++ b/docs/whats-new.md @@ -1,5 +1,12 @@ # What's new in LightCraft +## 2026-10-22 + +### Reliability +- Opening a library no longer writes through a link left at the lock's owner note (`catalog.lock.owner`): the link + is removed and a new note made in its place, so the file it pointed to is never changed (as for the catalog's + temporary files, issue #1502). + ## 2026-10-21 ### Sharper resampling in HDR and panorama merges