jerry/buzz
1
0
Fork 0
mirror of https://github.com/block/buzz.git synced 2026-10-31 22:41:12 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(desktop): harden blob lockfile against tmp-cleaner unlink race

Browse Source 
The service-keyed /tmp blob lockfile could be unlinked by a tmp cleaner
while a holder kept its flock; a recreate under the same pathname is a
fresh inode a second process can lock in parallel, splitting mutual
exclusion across two inodes. Recheck (dev, ino) after the lock is granted
and re-acquire against the live pathname on a mismatch, bounded by
MAX_BLOB_LOCK_REACQUIRE so a pathname churned faster than we can lock
fails loudly instead of spinning.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
This commit is contained in:
Duncan
2026-08-13 01:55:00 -04:00
parent 3f7e1b60d6
commit 1d3c27c676
2 changed files with 169 additions and 12 deletions
Download Patch File Download Diff File Expand all files Collapse all files

75
desktop/src-tauri/src/secret_store.rs
View File

@@ -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<Self, String> {
#[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)]

106
desktop/src-tauri/src/secret_store_tests.rs
View File

@@ -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]