diff --git a/desktop/src-tauri/src/secret_store.rs b/desktop/src-tauri/src/secret_store.rs index df13eb144e..c0be93bfeb 100644 --- a/desktop/src-tauri/src/secret_store.rs +++ b/desktop/src-tauri/src/secret_store.rs @@ -127,6 +127,37 @@ pub fn store_txn_lock_dir(store_file: &std::path::Path) -> PathBuf { .unwrap_or_else(|| store_file.to_path_buf()) } +/// Upper bound on lockfile re-acquisition attempts when a tmp cleaner keeps +/// unlinking the blob lockfile out from under us. Reaching it means the file is +/// being churned faster than we can lock a live inode — fail loudly rather than +/// spin forever. +#[cfg(all(unix, feature = "system-keyring"))] +const MAX_BLOB_LOCK_REACQUIRE: u32 = 100; + +/// True iff `file` (an open, `flock`-held fd) is locked on the same inode the +/// pathname currently resolves to. +/// +/// Called after the lock is granted to detect a tmp-cleaner unlink/recreate: a +/// mismatch means our fd holds a lock on a now-nameless dead inode while the +/// live pathname is a *different* inode another process can lock in parallel — +/// two processes each "holding" the lock over different inodes, mutual +/// exclusion lost. A missing pathname (`stat` fails) counts as not-live so the +/// caller re-creates and re-locks the live inode. +#[cfg(all(unix, feature = "system-keyring"))] +fn locked_inode_is_live(file: &std::fs::File, path: &std::path::Path) -> bool { + use std::os::unix::fs::MetadataExt; + let Ok(locked) = file.metadata() else { + return false; + }; + // Compare (dev, ino): inode numbers are only unique within a device, and + // the open fd pins its inode number so a recreate under the same pathname + // is guaranteed a different one. + matches!( + std::fs::metadata(path), + Ok(live) if live.dev() == locked.dev() && live.ino() == locked.ino() + ) +} + /// Acquire an exclusive advisory file lock for the blob identified by `service`. /// /// Opens (or creates) the lockfile and blocks until the lock is acquired. @@ -160,20 +191,40 @@ impl BlobLockGuard { fn acquire(path: &std::path::Path) -> Result { #[cfg(unix)] { - let file = std::fs::OpenOptions::new() - .create(true) - .truncate(false) - .write(true) - .open(path) - .map_err(|e| format!("blob lock open {}: {e}", path.display()))?; use std::os::unix::io::AsRawFd; - // LOCK_EX blocks until the lock is acquired (no LOCK_NB). - let ret = unsafe { libc::flock(file.as_raw_fd(), libc::LOCK_EX) }; - if ret != 0 { - let err = std::io::Error::last_os_error(); - return Err(format!("blob lock flock: {err}")); + // Loop to survive a tmp cleaner unlinking the lockfile out from + // under us. The classic `/tmp` lock split: we `flock` an inode, the + // pathname is unlinked and recreated as a fresh inode, and a second + // process locks that fresh inode — both "hold" the lock over + // different inodes. Defeat it by rechecking, after the lock is + // granted, that the pathname still resolves to the inode we locked. + // If it does not, our lock is on a dead inode: drop it and retry so + // we contend on the live one. LOCK_EX blocks until granted, so a + // stable inode converges in one pass. + for _ in 0..MAX_BLOB_LOCK_REACQUIRE { + let file = std::fs::OpenOptions::new() + .create(true) + .truncate(false) + .write(true) + .open(path) + .map_err(|e| format!("blob lock open {}: {e}", path.display()))?; + // LOCK_EX blocks until the lock is acquired (no LOCK_NB). + let ret = unsafe { libc::flock(file.as_raw_fd(), libc::LOCK_EX) }; + if ret != 0 { + let err = std::io::Error::last_os_error(); + return Err(format!("blob lock flock: {err}")); + } + if locked_inode_is_live(&file, path) { + return Ok(BlobLockGuard { file }); + } + // Stale inode: the pathname was unlinked/recreated while we + // blocked. Drop this fd (releasing the dead-inode lock) and + // retry against the live pathname. } - return Ok(BlobLockGuard { file }); + return Err(format!( + "blob lock: pathname {} churned {MAX_BLOB_LOCK_REACQUIRE} times without a stable inode", + path.display() + )); } #[cfg(windows)] diff --git a/desktop/src-tauri/src/secret_store_tests.rs b/desktop/src-tauri/src/secret_store_tests.rs index b3de9cae8b..58746c98fd 100644 --- a/desktop/src-tauri/src/secret_store_tests.rs +++ b/desktop/src-tauri/src/secret_store_tests.rs @@ -186,6 +186,112 @@ fn test_blob_lock_acquire_and_release() { ); } +// ── W6-B: blob lockfile hardening against tmp-cleaner unlink/recreate ────── +// +// The blob lock is a service-keyed `/tmp` lockfile (distinct from the +// directory-inode transaction lock). A tmp cleaner can unlink it while a holder +// keeps its `flock`, after which a recreate under the same pathname is a fresh +// inode a second process can lock in parallel — the classic split. The fix is +// an inode recheck after the lock is granted: a holder whose locked inode no +// longer matches the live pathname re-acquires against the live one, so all +// contenders converge on a single inode. + +#[cfg(all(unix, feature = "system-keyring"))] +#[test] +fn test_locked_inode_is_live_true_for_untouched_file() { + // A freshly opened lockfile that no one has churned must read as live. + let path = std::env::temp_dir().join(format!("buzz-w6b-live-{}.lock", std::process::id())); + let _ = std::fs::remove_file(&path); + let file = std::fs::File::create(&path).expect("create lockfile"); + assert!( + locked_inode_is_live(&file, &path), + "an untouched lockfile must resolve to its own locked inode" + ); + let _ = std::fs::remove_file(&path); +} + +#[cfg(all(unix, feature = "system-keyring"))] +#[test] +fn test_locked_inode_is_live_false_after_unlink_recreate() { + // A held fd whose pathname was unlinked and recreated points at a dead + // inode while the pathname resolves to a fresh one — the split condition + // the recheck exists to catch. + let path = std::env::temp_dir().join(format!("buzz-w6b-churn-{}.lock", std::process::id())); + let _ = std::fs::remove_file(&path); + let held = std::fs::File::create(&path).expect("create original lockfile"); + std::fs::remove_file(&path).expect("unlink original"); + std::fs::File::create(&path).expect("recreate lockfile (fresh inode)"); + assert!( + !locked_inode_is_live(&held, &path), + "a recreated pathname must NOT match the dead inode a holder still owns" + ); + let _ = std::fs::remove_file(&path); +} + +#[cfg(all(unix, feature = "system-keyring"))] +#[test] +fn test_locked_inode_is_live_false_when_pathname_absent() { + // Unlinked with no recreate: the pathname does not resolve, so the holder's + // inode cannot be the live one. The acquire loop must then re-create it. + let path = std::env::temp_dir().join(format!("buzz-w6b-gone-{}.lock", std::process::id())); + let _ = std::fs::remove_file(&path); + let held = std::fs::File::create(&path).expect("create lockfile"); + std::fs::remove_file(&path).expect("unlink lockfile"); + assert!( + !locked_inode_is_live(&held, &path), + "an absent pathname must read as not-live" + ); +} + +#[cfg(all(unix, feature = "system-keyring"))] +#[test] +fn test_blob_lock_converges_on_live_inode_after_recreate() { + // End-to-end: a holder acquires the lock, a tmp cleaner unlinks+recreates + // the lockfile (leaving the first guard on a dead inode), and a second + // acquire must land on the LIVE inode — not the dead one — so a peer + // process contending on that live pathname is still excluded. + use std::os::unix::fs::MetadataExt; + use std::os::unix::io::AsRawFd; + + let service = format!("buzz-w6b-converge-{}", std::process::id()); + let path = blob_lockfile_path(&service); + let _ = std::fs::remove_file(&path); + + let first = acquire_blob_lock(&service).expect("first acquire"); + let dead_ino = first.file.metadata().expect("first meta").ino(); + + // Tmp cleaner churns the pathname out from under the first holder. + std::fs::remove_file(&path).expect("unlink lockfile"); + std::fs::File::create(&path).expect("recreate lockfile"); + let live_ino = std::fs::metadata(&path).expect("live meta").ino(); + assert_ne!(dead_ino, live_ino, "recreate must yield a fresh inode"); + + // A second acquire (the analog of process B) converges on the live inode. + let second = acquire_blob_lock(&service).expect("second acquire"); + assert_eq!( + second.file.metadata().expect("second meta").ino(), + live_ino, + "the hardened acquire must lock the LIVE inode, not the dead one" + ); + + // A peer opening the live pathname must be excluded while `second` holds. + let peer = std::fs::File::open(&path).expect("peer opens live lockfile"); + let rc = unsafe { libc::flock(peer.as_raw_fd(), libc::LOCK_EX | libc::LOCK_NB) }; + assert_eq!( + rc, -1, + "a peer must NOT acquire the live inode while the hardened guard holds it" + ); + assert_eq!( + std::io::Error::last_os_error().raw_os_error(), + Some(libc::EWOULDBLOCK), + "peer exclusion must fail with EWOULDBLOCK" + ); + + drop(second); + drop(first); + let _ = std::fs::remove_file(&path); +} + // ── F5a: cross-process transaction lock (directory-inode based) ─────── #[test]