From fa348c29d7fc96f4f083a971f6e926d5b8763a06 Mon Sep 17 00:00:00 2001 From: Aero Date: Wed, 29 Jul 2026 01:49:10 +0800 Subject: [PATCH 01/15] feat: implement cleanup on daemon exit and enhance file path handling with user suffix --- src/daemon.rs | 113 +++++++++++++++++++++++++++++++++++++++--------- src/protocol.rs | 19 ++++++-- 2 files changed, 108 insertions(+), 24 deletions(-) diff --git a/src/daemon.rs b/src/daemon.rs index fd56b75..f87ba4e 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -29,32 +29,106 @@ enum ConnectionOutcome { Fatal, } +/// Best-effort removal of the daemon's socket/address and PID files. +/// +/// Only cleans up when the PID file still names this process: the paths are +/// shared, so a stale-but-alive old daemon exiting must not delete the files +/// of a newer daemon that has since rebound them. +fn cleanup() { + let owns_files = std::fs::read_to_string(pid_path()) + .ok() + .and_then(|s| s.trim().parse::().ok()) + == Some(std::process::id()); + if !owns_files { + return; + } + #[cfg(unix)] + let _ = std::fs::remove_file(socket_path()); + #[cfg(windows)] + let _ = std::fs::remove_file(addr_path()); + let _ = std::fs::remove_file(pid_path()); +} + +/// Removes the daemon's on-disk files when `run_daemon`'s frame is left for +/// any reason: normal return, early `?` error, or an unwinding panic. +/// (SIGKILL and other non-catchable terminations are inherently not covered.) +struct CleanupGuard; + +impl Drop for CleanupGuard { + fn drop(&mut self) { + cleanup(); + } +} + macro_rules! run_accept_loop_body { - ($accept:expr, $client:expr, $ws_url:expr) => { + ($accept:expr, $client:expr, $ws_url:expr, $shutdown:expr) => { loop { - let accept = tokio::time::timeout(idle_timeout(), $accept).await; - - match accept { - Ok(Ok((stream, _))) => match handle_connection(stream, $client, $ws_url).await { - ConnectionOutcome::Continue => {} - ConnectionOutcome::Fatal => break, - }, - Ok(Err(e)) => { - eprintln!("daemon: accept error: {e}"); - } - Err(_) => { - // Idle timeout — exit + tokio::select! { + _ = &mut $shutdown => { + // SIGTERM/SIGINT (or Ctrl-C on Windows) — exit cleanly. + // Only observed between requests: an in-flight command + // finishes and its response is written before shutdown. break; } + accept = tokio::time::timeout(idle_timeout(), $accept) => match accept { + Ok(Ok((stream, _))) => match handle_connection(stream, $client, $ws_url).await { + ConnectionOutcome::Continue => {} + ConnectionOutcome::Fatal => break, + }, + Ok(Err(e)) => { + eprintln!("daemon: accept error: {e}"); + } + Err(_) => { + // Idle timeout — exit + break; + } + } } } }; } +/// Resolves when the daemon should shut down due to a signal. +#[cfg(unix)] +async fn shutdown_signal() { + use tokio::signal::unix::{signal, SignalKind}; + // If a signal stream can't be registered, fall back to never resolving — + // the daemon then behaves as before this handler existed (default + // disposition still terminates the process; only file cleanup is lost). + let mut sigterm = match signal(SignalKind::terminate()) { + Ok(s) => s, + Err(_) => return std::future::pending().await, + }; + let mut sigint = match signal(SignalKind::interrupt()) { + Ok(s) => s, + Err(_) => return std::future::pending().await, + }; + tokio::select! { + _ = sigterm.recv() => {} + _ = sigint.recv() => {} + } +} + +/// Best-effort on Windows: the daemon is spawned with CREATE_NO_WINDOW (no +/// console), so SetConsoleCtrlHandler-based Ctrl-C delivery typically never +/// fires for a backgrounded daemon. It does work when `__daemon__` is run +/// manually in a foreground console for debugging. +#[cfg(windows)] +async fn shutdown_signal() { + if tokio::signal::ctrl_c().await.is_err() { + std::future::pending::<()>().await; + } +} + pub async fn run_daemon(ws_url: &str) -> Result<()> { // Write PID std::fs::write(pid_path(), std::process::id().to_string())?; + // From here on, socket/addr/pid files are removed whenever this frame is + // left — including early `?` returns (e.g. a failed bind below) and + // unwinding panics, which previously leaked them. + let _guard = CleanupGuard; + #[cfg(unix)] let listener = { // Clean up stale socket @@ -85,16 +159,13 @@ pub async fn run_daemon(ws_url: &str) -> Result<()> { // while the daemon handles the potentially slow macOS/Chrome network permission prompt. let mut client: Option = None; - // Signal readiness by socket/address existence (it's already bound) - run_accept_loop_body!(listener.accept(), &mut client, ws_url); - - #[cfg(unix)] - let _ = std::fs::remove_file(socket_path()); + let shutdown = shutdown_signal(); + tokio::pin!(shutdown); - #[cfg(windows)] - let _ = std::fs::remove_file(addr_path()); + // Signal readiness by socket/address existence (it's already bound) + run_accept_loop_body!(listener.accept(), &mut client, ws_url, shutdown); - let _ = std::fs::remove_file(pid_path()); + // File cleanup is handled by `_guard` (also covers signal/panic exits). // Shut down telemetry before exiting so the background thread // flushes pending entries and exits cleanly. diff --git a/src/protocol.rs b/src/protocol.rs index 293865f..17d18e4 100644 --- a/src/protocol.rs +++ b/src/protocol.rs @@ -48,21 +48,34 @@ pub struct DaemonResponse { pub error_code: Option, } +/// Per-user filename suffix. `temp_dir()` is per-user on macOS ($TMPDIR) and +/// Windows (%TEMP%), but shared /tmp on Linux — a fixed name there lets users +/// collide on (or squat) each other's daemon files. +#[cfg(unix)] +fn user_suffix() -> String { + format!("-{}", unsafe { libc::getuid() }) +} + +#[cfg(windows)] +fn user_suffix() -> String { + String::new() +} + /// Path to the Unix domain socket for daemon communication. #[cfg(unix)] pub fn socket_path() -> PathBuf { - std::env::temp_dir().join("chrome-devtools-daemon.sock") + std::env::temp_dir().join(format!("chrome-devtools-daemon{}.sock", user_suffix())) } /// Path to the named-pipe address file for daemon communication (Windows). #[cfg(windows)] pub fn addr_path() -> PathBuf { - std::env::temp_dir().join("chrome-devtools-daemon.addr") + std::env::temp_dir().join(format!("chrome-devtools-daemon{}.addr", user_suffix())) } /// Path to the daemon PID file. pub fn pid_path() -> PathBuf { - std::env::temp_dir().join("chrome-devtools-daemon.pid") + std::env::temp_dir().join(format!("chrome-devtools-daemon{}.pid", user_suffix())) } /// Write a length-prefixed message to a stream. From 421eca559384668124eaf59e90f3116c3005851c Mon Sep 17 00:00:00 2001 From: Aero Date: Wed, 29 Jul 2026 07:29:29 +0800 Subject: [PATCH 02/15] fix: address PR review findings on daemon cleanup - Serialize daemon startup and cleanup with a lock file so an exiting daemon can't delete files a replacement just rebound (TOCTOU race) - Sweep pre-uid-suffix legacy files in kill-daemon so daemons started by older binaries can still be stopped after an upgrade - Reword shutdown_signal doc comment to capture rationale - Update README/AGENTS.md to the uid-suffixed daemon paths Co-Authored-By: Claude Fable 5 --- AGENTS.md | 7 +++--- README.md | 7 +++--- src/daemon.rs | 66 +++++++++++++++++++++++++++++++++++++++++++------ src/lib.rs | 37 +++++++++++++++++++++++++++ src/protocol.rs | 21 ++++++++++++++++ 5 files changed, 125 insertions(+), 13 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 5e40418..f1c1709 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -62,9 +62,10 @@ directly. `skill/chrome-devtools/CUSTOM_SCRIPTING.md` documents `run-script` and ### Daemon Architecture -A background daemon (`/tmp/chrome-devtools-daemon.sock`) keeps a persistent CDP -WebSocket connection. First CLI invocation spawns it; subsequent commands reuse -it. 5-minute idle timeout. +A background daemon (`$TMPDIR/chrome-devtools-daemon-.sock` — uid-suffixed +to isolate users sharing /tmp) keeps a persistent CDP WebSocket connection. +First CLI invocation spawns it; subsequent commands reuse it. 5-minute idle +timeout; socket/PID files are cleaned up on signals and panics too. `CdpClient::connect` (`cdp.rs`) bounds the WebSocket handshake with a timeout (`CHROME_CONNECT_TIMEOUT_SECS`, default 10s). Without it, a pending Chrome diff --git a/README.md b/README.md index ec7ea2d..2647721 100644 --- a/README.md +++ b/README.md @@ -41,7 +41,7 @@ This is a lightweight Rust binary that talks directly to Chrome's DevTools Proto ``` chrome-devtools navigate https://example.com │ - ├─ Try daemon (Unix socket /tmp/chrome-devtools-daemon.sock) + ├─ Try daemon (Unix socket $TMPDIR/chrome-devtools-daemon-.sock) │ └─ If running → send command → get result │ ├─ If no daemon → spawn one (background process) @@ -215,9 +215,10 @@ Global `--block-url` and `--unblock-url` update the **active tab's** block list ## Daemon details -- **Socket**: `/tmp/chrome-devtools-daemon.sock` -- **PID file**: `/tmp/chrome-devtools-daemon.pid` +- **Socket**: `$TMPDIR/chrome-devtools-daemon-.sock` (uid-suffixed so users on a shared machine don't collide) +- **PID file**: `$TMPDIR/chrome-devtools-daemon-.pid` - **Idle timeout**: 5 minutes (auto-exits, cleans up socket) +- **Cleanup**: socket + PID files are also removed on SIGTERM/SIGINT and panics, not just normal exit - **Protocol**: Length-prefixed JSON over Unix socket - **Spawned by**: First CLI invocation (transparent to user) - **Kill**: `chrome-devtools kill-daemon` (or delete the socket + PID file) diff --git a/src/daemon.rs b/src/daemon.rs index f87ba4e..3067cf9 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -29,12 +29,50 @@ enum ConnectionOutcome { Fatal, } +/// Acquire the cross-process lock serializing the daemon-file critical +/// sections: startup's pid-write/rebind and cleanup's check-then-remove. +/// Blocks until the lock is free; the OS releases it when the handle drops. +/// `None` means the lock file couldn't be created/locked — callers proceed +/// unlocked (degrading to pre-lock behavior) rather than refusing to run. +fn lock_daemon_files() -> Option { + let f = std::fs::OpenOptions::new() + .create(true) + .write(true) + .truncate(false) + .open(lock_path()) + .ok()?; + f.lock().ok()?; + Some(f) +} + +/// Non-blocking variant for `cleanup()`: if the lock is held, another daemon +/// is inside its own critical section — exactly when deleting the shared +/// files is guaranteed wrong — so contention means "don't touch anything". +fn try_lock_daemon_files() -> Option { + let f = std::fs::OpenOptions::new() + .create(true) + .write(true) + .truncate(false) + .open(lock_path()) + .ok()?; + f.try_lock().ok()?; + Some(f) +} + /// Best-effort removal of the daemon's socket/address and PID files. /// /// Only cleans up when the PID file still names this process: the paths are /// shared, so a stale-but-alive old daemon exiting must not delete the files -/// of a newer daemon that has since rebound them. +/// of a newer daemon that has since rebound them. The ownership check and the +/// removals happen under the daemon-file lock so a replacement can't write +/// its pid and rebind in between (it would keep running but be unreachable, +/// and every later CLI call would spawn yet another daemon). fn cleanup() { + let Some(_lock) = try_lock_daemon_files() else { + // Contended: a replacement is mid-startup. Its rebind supersedes our + // files, and any leftovers self-heal on the next daemon start. + return; + }; let owns_files = std::fs::read_to_string(pid_path()) .ok() .and_then(|s| s.trim().parse::().ok()) @@ -88,7 +126,11 @@ macro_rules! run_accept_loop_body { }; } -/// Resolves when the daemon should shut down due to a signal. +/// SIGTERM/SIGINT must be turned into a normal accept-loop exit: their +/// default disposition kills the process without unwinding, so `CleanupGuard` +/// would never run and the socket/PID files would go stale. SIGHUP is +/// deliberately left unhandled — convention reserves it for config reload, +/// not shutdown. #[cfg(unix)] async fn shutdown_signal() { use tokio::signal::unix::{signal, SignalKind}; @@ -122,13 +164,21 @@ async fn shutdown_signal() { pub async fn run_daemon(ws_url: &str) -> Result<()> { // Write PID - std::fs::write(pid_path(), std::process::id().to_string())?; - - // From here on, socket/addr/pid files are removed whenever this frame is - // left — including early `?` returns (e.g. a failed bind below) and - // unwinding panics, which previously leaked them. + // Armed before anything is written: cleanup() verifies pid-file ownership + // first, so firing "too early" is a no-op, and this declaration order + // means the startup lock below is released (locals drop in reverse order) + // before the guard's cleanup() tries to take it — no self-deadlock on an + // early `?` return or panic. Covers every way this frame is left, + // including unwinding panics, which previously leaked the files. let _guard = CleanupGuard; + // Startup critical section: the pid write and endpoint (re)bind must not + // interleave with a predecessor's cleanup() check-then-remove, or the + // predecessor can delete files this daemon just claimed. + let startup_lock = lock_daemon_files(); + + std::fs::write(pid_path(), std::process::id().to_string())?; + #[cfg(unix)] let listener = { // Clean up stale socket @@ -154,6 +204,8 @@ pub async fn run_daemon(ws_url: &str) -> Result<()> { listener }; + drop(startup_lock); + // We don't connect immediately. We wait for the first connection from the CLI. // This ensures the CLI wait_for_daemon() succeeds, and the CLI blocks on read_msg() // while the daemon handles the potentially slow macOS/Chrome network permission prompt. diff --git a/src/lib.rs b/src/lib.rs index cef203b..379574d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -989,6 +989,43 @@ pub async fn run() -> Result<()> { )); } } + + // Best-effort sweep of the pre-uid-suffix file names: a daemon + // started by an older binary is invisible to the paths above, so + // without this it could only be stopped by waiting out its idle + // timeout. Silent when no legacy files exist (the common case). + #[cfg(unix)] + { + let legacy_pid_path = protocol::legacy_pid_path(); + if let Ok(pid_str) = std::fs::read_to_string(&legacy_pid_path) { + // Same corrupted-PID-file guards as above: never signal pid 0 + // (whole process group) or a value that wraps negative. + let legacy_pid = pid_str + .trim() + .parse::() + .ok() + .filter(|&p| p != 0) + .and_then(|p| i32::try_from(p).ok()); + if let Some(pid) = legacy_pid { + let ret = unsafe { libc::kill(pid as libc::pid_t, libc::SIGTERM) }; + let gone = ret == 0 + || std::io::Error::last_os_error().raw_os_error() == Some(libc::ESRCH); + if gone { + // Old binaries have no SIGTERM handler and exit + // without cleanup, so remove their files here. + let _ = std::fs::remove_file(protocol::legacy_socket_path()); + let _ = std::fs::remove_file(&legacy_pid_path); + if ret == 0 { + println!("Also stopped legacy daemon (PID {pid})."); + } + } + } else { + // Unusable PID content — the files are junk; remove them. + let _ = std::fs::remove_file(protocol::legacy_socket_path()); + let _ = std::fs::remove_file(&legacy_pid_path); + } + } + } return Ok(()); } diff --git a/src/protocol.rs b/src/protocol.rs index 17d18e4..d4d228f 100644 --- a/src/protocol.rs +++ b/src/protocol.rs @@ -78,6 +78,27 @@ pub fn pid_path() -> PathBuf { std::env::temp_dir().join(format!("chrome-devtools-daemon{}.pid", user_suffix())) } +/// Path to the lock file serializing daemon startup and cleanup. +/// +/// The lock file is never removed once created: deleting it while another +/// process may be about to lock it would reintroduce the race it prevents. +pub fn lock_path() -> PathBuf { + std::env::temp_dir().join(format!("chrome-devtools-daemon{}.lock", user_suffix())) +} + +/// Pre-uid-suffix PID file name, so `kill-daemon` can still stop a daemon +/// left running by an older binary after an upgrade. +#[cfg(unix)] +pub fn legacy_pid_path() -> PathBuf { + std::env::temp_dir().join("chrome-devtools-daemon.pid") +} + +/// Pre-uid-suffix socket name (see [`legacy_pid_path`]). +#[cfg(unix)] +pub fn legacy_socket_path() -> PathBuf { + std::env::temp_dir().join("chrome-devtools-daemon.sock") +} + /// Write a length-prefixed message to a stream. pub async fn write_msg(w: &mut W, data: &[u8]) -> anyhow::Result<()> { let len = (data.len() as u32).to_be_bytes(); From 298faa95aad586e92cb5b54393651a1ad1360e75 Mon Sep 17 00:00:00 2001 From: Aero Date: Wed, 29 Jul 2026 10:00:49 +0800 Subject: [PATCH 03/15] fix: harden daemon lock error handling and legacy kill safety - Surface lock-file failures: daemon startup now fails explicitly when the daemon-file lock cannot be created or acquired, and cleanup distinguishes WouldBlock contention (silent skip by design) from I/O errors (reported to stderr) - Extract PID-file parsing into a pure parse_pid_file_contents helper - Probe the legacy socket before signaling: kill-daemon only SIGTERMs a legacy PID after a daemon answers on the legacy socket, so a recycled PID can never hit an unrelated process; dead sockets get their stale files removed without any signal Co-Authored-By: Claude Fable 5 --- src/daemon.rs | 73 +++++++++++++++++++++++++---------------- src/lib.rs | 91 ++++++++++++++++++++++++++++++++++++++------------- 2 files changed, 114 insertions(+), 50 deletions(-) diff --git a/src/daemon.rs b/src/daemon.rs index 3067cf9..2a8ffc7 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -1,4 +1,4 @@ -use anyhow::Result; +use anyhow::{Context, Result}; use std::time::Duration; use tokio::io::{AsyncReadExt, AsyncWriteExt}; #[cfg(windows)] @@ -29,34 +29,27 @@ enum ConnectionOutcome { Fatal, } -/// Acquire the cross-process lock serializing the daemon-file critical -/// sections: startup's pid-write/rebind and cleanup's check-then-remove. -/// Blocks until the lock is free; the OS releases it when the handle drops. -/// `None` means the lock file couldn't be created/locked — callers proceed -/// unlocked (degrading to pre-lock behavior) rather than refusing to run. -fn lock_daemon_files() -> Option { - let f = std::fs::OpenOptions::new() +fn open_lock_file() -> std::io::Result { + std::fs::OpenOptions::new() .create(true) .write(true) .truncate(false) .open(lock_path()) - .ok()?; - f.lock().ok()?; - Some(f) } -/// Non-blocking variant for `cleanup()`: if the lock is held, another daemon -/// is inside its own critical section — exactly when deleting the shared -/// files is guaranteed wrong — so contention means "don't touch anything". -fn try_lock_daemon_files() -> Option { - let f = std::fs::OpenOptions::new() - .create(true) - .write(true) - .truncate(false) - .open(lock_path()) - .ok()?; - f.try_lock().ok()?; - Some(f) +/// Acquire the cross-process lock serializing the daemon-file critical +/// sections: startup's pid-write/rebind and cleanup's check-then-remove. +/// Blocks until the lock is free; the OS releases it when the handle drops. +/// +/// Errors instead of degrading: the lock is what makes ownership handoff +/// correct, and a daemon that can't create a file in temp_dir couldn't write +/// its PID file either — failing here just surfaces the cause sooner. +fn lock_daemon_files() -> Result { + let f = open_lock_file() + .with_context(|| format!("Failed to open daemon lock file {}", lock_path().display()))?; + f.lock() + .with_context(|| format!("Failed to lock daemon lock file {}", lock_path().display()))?; + Ok(f) } /// Best-effort removal of the daemon's socket/address and PID files. @@ -68,11 +61,35 @@ fn try_lock_daemon_files() -> Option { /// its pid and rebind in between (it would keep running but be unreachable, /// and every later CLI call would spawn yet another daemon). fn cleanup() { - let Some(_lock) = try_lock_daemon_files() else { - // Contended: a replacement is mid-startup. Its rebind supersedes our - // files, and any leftovers self-heal on the next daemon start. - return; + let lock_file = match open_lock_file() { + Ok(f) => f, + Err(e) => { + // Without the lock, removal could race a replacement's startup — + // leaving the files is the safe side (they self-heal on the next + // daemon start), but say why so the cause isn't swallowed. + eprintln!( + "daemon: leaving socket/PID files in place: cannot open lock file {}: {e}", + lock_path().display() + ); + return; + } }; + match lock_file.try_lock() { + Ok(()) => {} + Err(std::fs::TryLockError::WouldBlock) => { + // Contended: a replacement is mid-startup. Its rebind supersedes + // our files, and any leftovers self-heal on the next start. + return; + } + Err(std::fs::TryLockError::Error(e)) => { + eprintln!( + "daemon: leaving socket/PID files in place: cannot lock {}: {e}", + lock_path().display() + ); + return; + } + } + let _lock = lock_file; let owns_files = std::fs::read_to_string(pid_path()) .ok() .and_then(|s| s.trim().parse::().ok()) @@ -175,7 +192,7 @@ pub async fn run_daemon(ws_url: &str) -> Result<()> { // Startup critical section: the pid write and endpoint (re)bind must not // interleave with a predecessor's cleanup() check-then-remove, or the // predecessor can delete files this daemon just claimed. - let startup_lock = lock_daemon_files(); + let startup_lock = lock_daemon_files()?; std::fs::write(pid_path(), std::process::id().to_string())?; diff --git a/src/lib.rs b/src/lib.rs index 379574d..228e6b8 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -485,6 +485,46 @@ enum KillDaemonDecision { RefuseNonInteractive, } +/// Parse PID-file contents into a value safe to pass to `libc::kill`. +/// +/// Rejects pid 0 — `kill(0, sig)` signals the caller's whole process group — +/// and values that don't fit `libc::pid_t`, where the `as` cast would wrap +/// negative and `kill` would signal process group `-pid` instead. +#[cfg(unix)] +fn parse_pid_file_contents(s: &str) -> Option { + s.trim() + .parse::() + .ok() + .filter(|&p| p != 0) + .and_then(|p| i32::try_from(p).ok()) +} + +/// Confirm a live daemon is listening on the legacy (pre-uid-suffix) socket +/// before trusting the legacy PID file. The OS recycles PIDs, so a stale +/// file alone is not evidence the process it names is still our daemon — but +/// something answering length-prefixed JSON on the daemon's socket is. The +/// probe payload is deliberately not a valid `DaemonRequest`: old daemons +/// answer "Invalid request" without ever dialing Chrome. +#[cfg(unix)] +async fn probe_legacy_daemon() -> bool { + let Ok(mut stream) = + tokio::net::UnixStream::connect(protocol::legacy_socket_path()).await + else { + return false; + }; + if protocol::write_msg(&mut stream, b"\"probe\"").await.is_err() { + return false; + } + matches!( + tokio::time::timeout( + std::time::Duration::from_secs(2), + protocol::read_msg(&mut stream) + ) + .await, + Ok(Ok(_)) + ) +} + fn kill_daemon_decision(force: bool, can_prompt: bool) -> KillDaemonDecision { if force { KillDaemonDecision::Proceed @@ -998,31 +1038,38 @@ pub async fn run() -> Result<()> { { let legacy_pid_path = protocol::legacy_pid_path(); if let Ok(pid_str) = std::fs::read_to_string(&legacy_pid_path) { - // Same corrupted-PID-file guards as above: never signal pid 0 - // (whole process group) or a value that wraps negative. - let legacy_pid = pid_str - .trim() - .parse::() - .ok() - .filter(|&p| p != 0) - .and_then(|p| i32::try_from(p).ok()); - if let Some(pid) = legacy_pid { - let ret = unsafe { libc::kill(pid as libc::pid_t, libc::SIGTERM) }; - let gone = ret == 0 - || std::io::Error::last_os_error().raw_os_error() == Some(libc::ESRCH); - if gone { - // Old binaries have no SIGTERM handler and exit - // without cleanup, so remove their files here. + match (parse_pid_file_contents(&pid_str), probe_legacy_daemon().await) { + (Some(pid), true) => { + // A daemon answered on the legacy socket, so the PID + // file is live — not a recycled PID from a dead run. + let ret = unsafe { libc::kill(pid as libc::pid_t, libc::SIGTERM) }; + if ret == 0 || std::io::Error::last_os_error().raw_os_error() + == Some(libc::ESRCH) + { + // Old binaries have no SIGTERM handler and exit + // without cleanup, so remove their files here. + let _ = std::fs::remove_file(protocol::legacy_socket_path()); + let _ = std::fs::remove_file(&legacy_pid_path); + if ret == 0 { + println!("Also stopped legacy daemon (PID {pid})."); + } + } + } + (_, false) => { + // Nothing answering on the legacy socket: the files + // are leftovers. Never signal the PID — the OS may + // have recycled it for an unrelated process. let _ = std::fs::remove_file(protocol::legacy_socket_path()); let _ = std::fs::remove_file(&legacy_pid_path); - if ret == 0 { - println!("Also stopped legacy daemon (PID {pid})."); - } } - } else { - // Unusable PID content — the files are junk; remove them. - let _ = std::fs::remove_file(protocol::legacy_socket_path()); - let _ = std::fs::remove_file(&legacy_pid_path); + (None, true) => { + // Live daemon but unusable PID contents: can't signal + // it safely. Leave its files; it idles out on its own. + println!( + "Note: a legacy daemon is listening on {} but its PID file is unreadable; it will exit after its idle timeout.", + protocol::legacy_socket_path().display() + ); + } } } } From 836f93e500e9bef6e0f197baefbcbfec6fe7ba7b Mon Sep 17 00:00:00 2001 From: Aero Date: Wed, 29 Jul 2026 10:48:55 +0800 Subject: [PATCH 04/15] docs: qualify daemon transport and cleanup guarantees per platform MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Document the Windows loopback TCP listener and unsuffixed %TEMP% addr file alongside the uid-suffixed Unix socket, and mark signal cleanup as Unix-specific (Windows Ctrl-C is best-effort — no console when backgrounded) in README and AGENTS.md. Co-Authored-By: Claude Fable 5 --- AGENTS.md | 11 +++++++---- README.md | 14 ++++++++------ 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index f1c1709..6551ead 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -62,10 +62,13 @@ directly. `skill/chrome-devtools/CUSTOM_SCRIPTING.md` documents `run-script` and ### Daemon Architecture -A background daemon (`$TMPDIR/chrome-devtools-daemon-.sock` — uid-suffixed -to isolate users sharing /tmp) keeps a persistent CDP WebSocket connection. -First CLI invocation spawns it; subsequent commands reuse it. 5-minute idle -timeout; socket/PID files are cleaned up on signals and panics too. +A background daemon keeps a persistent CDP WebSocket connection. On Unix it +listens on `$TMPDIR/chrome-devtools-daemon-.sock` (uid-suffixed to isolate +users sharing /tmp); on Windows, on a loopback TCP port published via an +unsuffixed `%TEMP%` addr file. First CLI invocation spawns it; subsequent +commands reuse it. 5-minute idle timeout; endpoint/PID files are cleaned up on +panics too, and on Unix on SIGTERM/SIGINT (Windows Ctrl-C cleanup is +best-effort — a background daemon has no console). `CdpClient::connect` (`cdp.rs`) bounds the WebSocket handshake with a timeout (`CHROME_CONNECT_TIMEOUT_SECS`, default 10s). Without it, a pending Chrome diff --git a/README.md b/README.md index 2647721..500f9af 100644 --- a/README.md +++ b/README.md @@ -41,7 +41,8 @@ This is a lightweight Rust binary that talks directly to Chrome's DevTools Proto ``` chrome-devtools navigate https://example.com │ - ├─ Try daemon (Unix socket $TMPDIR/chrome-devtools-daemon-.sock) + ├─ Try daemon (Unix socket $TMPDIR/chrome-devtools-daemon-.sock; + │ loopback TCP on Windows) │ └─ If running → send command → get result │ ├─ If no daemon → spawn one (background process) @@ -215,11 +216,12 @@ Global `--block-url` and `--unblock-url` update the **active tab's** block list ## Daemon details -- **Socket**: `$TMPDIR/chrome-devtools-daemon-.sock` (uid-suffixed so users on a shared machine don't collide) -- **PID file**: `$TMPDIR/chrome-devtools-daemon-.pid` -- **Idle timeout**: 5 minutes (auto-exits, cleans up socket) -- **Cleanup**: socket + PID files are also removed on SIGTERM/SIGINT and panics, not just normal exit -- **Protocol**: Length-prefixed JSON over Unix socket +- **Endpoint (Unix)**: socket at `$TMPDIR/chrome-devtools-daemon-.sock` (uid-suffixed so users on a shared machine don't collide) +- **Endpoint (Windows)**: loopback TCP listener; its address is written to `%TEMP%\chrome-devtools-daemon.addr` (`%TEMP%` is already per-user, so no suffix) +- **PID file**: `$TMPDIR/chrome-devtools-daemon-.pid` (Windows: `%TEMP%\chrome-devtools-daemon.pid`) +- **Idle timeout**: 5 minutes (auto-exits, cleans up its files) +- **Cleanup**: endpoint + PID files are also removed on panics, and on Unix on SIGTERM/SIGINT; Windows Ctrl-C cleanup is best-effort only (a background daemon has no console to receive it) +- **Protocol**: Length-prefixed JSON over the Unix socket / loopback TCP - **Spawned by**: First CLI invocation (transparent to user) - **Kill**: `chrome-devtools kill-daemon` (or delete the socket + PID file) From 42f60b9598dec86f8c590a79030ef64c041ec262 Mon Sep 17 00:00:00 2001 From: Aero Date: Wed, 29 Jul 2026 11:05:06 +0800 Subject: [PATCH 05/15] feat: add support for headless Chrome with isolated profiles and automated cleanup --- skill/chrome-devtools/SKILL.md | 47 ++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/skill/chrome-devtools/SKILL.md b/skill/chrome-devtools/SKILL.md index 3b850ea..44018bb 100644 --- a/skill/chrome-devtools/SKILL.md +++ b/skill/chrome-devtools/SKILL.md @@ -367,6 +367,53 @@ profiles/machines) produces a meaningless result where nearly everything is reported as both added and removed — the CLI prints a warning on stderr when it detects this. +### Pattern 16: Headless Chrome (No Login, No Human Approval) + +When the flow under test doesn't need the user's cookies/credentials, spawn a +throwaway headless Chrome instead of attaching to the user's browser. Because +the instance is launched with remote debugging already enabled, **no consent +prompt ever appears** — the whole flow runs unattended. + +```bash +PROFILE=$(mktemp -d) + +# 1. If a daemon is already attached to the user's real Chrome, stop it first +# (the daemon is per-user and sticks to whichever Chrome it first connected to) +chrome-devtools kill-daemon --force + +# 2. Spawn headless Chrome with an isolated profile; port 0 = pick a free port +"/Applications/Google Chrome.app/Contents/MacOS/Google Chrome" \ + --headless=new --remote-debugging-port=0 \ + --user-data-dir="$PROFILE" \ + --no-first-run --no-default-browser-check \ + about:blank & +CHROME_PID=$! + +# 3. Wait for Chrome to publish its debug port +while [ ! -f "$PROFILE/DevToolsActivePort" ]; do sleep 0.5; done + +# 4. Every command needs --user-data-dir pointing at the headless profile; +# the CLI auto-connects by reading its DevToolsActivePort +chrome-devtools --user-data-dir "$PROFILE" navigate https://example.com +chrome-devtools --user-data-dir "$PROFILE" evaluate 'document.title' +chrome-devtools --user-data-dir "$PROFILE" screenshot --output /tmp/shot.png + +# 5. Cleanup — REQUIRED: the daemon is now bound to the headless instance and +# would otherwise hijack later commands aimed at the user's real Chrome +chrome-devtools kill-daemon --force +kill $CHROME_PID +rm -rf "$PROFILE" +``` + +Linux path: `google-chrome` or `chromium` on `$PATH` replaces the macOS +`.app` binary path. + +**⚠️ One daemon per user, bound to one Chrome.** The daemon connects to +whichever Chrome the first command resolved, and later commands reuse it even +if their flags point elsewhere. Always `kill-daemon --force` when switching +between the user's Chrome and a headless instance — in both directions +(steps 1 and 5 above). + ## Complete Command Reference ### Navigation From 517354b9884be0b4bd84dc569792116c099c8cbc Mon Sep 17 00:00:00 2001 From: Aero Date: Wed, 29 Jul 2026 11:25:07 +0800 Subject: [PATCH 06/15] fix: harden legacy probe and daemon lock against shared-tmp adversaries - probe_legacy_daemon returns anyhow::Result: connect-refused and missing socket stay Ok(false), unexpected connect/write/read failures carry operation-specific context, and kill-daemon handles Err by warning and leaving files in place (a probe timeout no longer counts as "stale" and deletes files under a live listener) - probe timeout now spans the whole connect/write/read sequence so a hostile listener on the predictable socket name can't stall any step - lock file opens with O_NOFOLLOW + mode 0600 and validates regular file + current-uid ownership before locking; startup lock wait is bounded (5s) instead of blocking indefinitely on a squatted lock - SKILL.md: explain why DevToolsActivePort existence is the readiness signal instead of restating the polling loop Co-Authored-By: Claude Fable 5 --- skill/chrome-devtools/SKILL.md | 4 +- src/daemon.rs | 65 +++++++++++++++++++++++++++----- src/lib.rs | 69 ++++++++++++++++++++++++---------- 3 files changed, 108 insertions(+), 30 deletions(-) diff --git a/skill/chrome-devtools/SKILL.md b/skill/chrome-devtools/SKILL.md index 44018bb..e9b1d99 100644 --- a/skill/chrome-devtools/SKILL.md +++ b/skill/chrome-devtools/SKILL.md @@ -389,7 +389,9 @@ chrome-devtools kill-daemon --force about:blank & CHROME_PID=$! -# 3. Wait for Chrome to publish its debug port +# 3. DevToolsActivePort is written only after the DevTools server is +# actually listening, so the file's existence — not the process being +# alive — is the readiness signal; connecting earlier races startup while [ ! -f "$PROFILE/DevToolsActivePort" ]; do sleep 0.5; done # 4. Every command needs --user-data-dir pointing at the headless profile; diff --git a/src/daemon.rs b/src/daemon.rs index 2a8ffc7..ed7a5da 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -29,27 +29,72 @@ enum ConnectionOutcome { Fatal, } +/// The lock path has a predictable name and, on Linux, lives in shared /tmp, +/// so treat a pre-existing file as potentially hostile: refuse to follow a +/// planted symlink (O_NOFOLLOW) and only lock something that is a regular +/// file owned by this uid. macOS $TMPDIR and Windows %TEMP% are per-user, so +/// there the checks are inert. fn open_lock_file() -> std::io::Result { - std::fs::OpenOptions::new() - .create(true) - .write(true) - .truncate(false) - .open(lock_path()) + let mut opts = std::fs::OpenOptions::new(); + opts.create(true).write(true).truncate(false); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + opts.custom_flags(libc::O_NOFOLLOW); + opts.mode(0o600); + } + let f = opts.open(lock_path())?; + #[cfg(unix)] + { + use std::os::unix::fs::MetadataExt; + let md = f.metadata()?; + if !md.is_file() || md.uid() != unsafe { libc::getuid() } { + return Err(std::io::Error::new( + std::io::ErrorKind::PermissionDenied, + "lock path is not a regular file owned by the current user", + )); + } + } + Ok(f) } +/// How long startup will wait for the daemon-file lock. Legitimate holders +/// (a predecessor's cleanup, another daemon's startup) finish in +/// milliseconds; anything longer means the lock is wedged or squatted. +const LOCK_WAIT_TIMEOUT: Duration = Duration::from_secs(5); + /// Acquire the cross-process lock serializing the daemon-file critical /// sections: startup's pid-write/rebind and cleanup's check-then-remove. -/// Blocks until the lock is free; the OS releases it when the handle drops. +/// The OS releases the lock when the handle drops. /// /// Errors instead of degrading: the lock is what makes ownership handoff /// correct, and a daemon that can't create a file in temp_dir couldn't write -/// its PID file either — failing here just surfaces the cause sooner. +/// its PID file either — failing here just surfaces the cause sooner. The +/// wait is bounded because an indefinite `lock()` would let any process that +/// pre-acquired the predictable lock path park daemon startup forever. fn lock_daemon_files() -> Result { let f = open_lock_file() .with_context(|| format!("Failed to open daemon lock file {}", lock_path().display()))?; - f.lock() - .with_context(|| format!("Failed to lock daemon lock file {}", lock_path().display()))?; - Ok(f) + let deadline = std::time::Instant::now() + LOCK_WAIT_TIMEOUT; + loop { + match f.try_lock() { + Ok(()) => return Ok(f), + Err(std::fs::TryLockError::WouldBlock) => { + if std::time::Instant::now() >= deadline { + anyhow::bail!( + "Timed out after {LOCK_WAIT_TIMEOUT:?} waiting for daemon lock file {} (held by another process)", + lock_path().display() + ); + } + std::thread::sleep(Duration::from_millis(50)); + } + Err(std::fs::TryLockError::Error(e)) => { + return Err(e).with_context(|| { + format!("Failed to lock daemon lock file {}", lock_path().display()) + }); + } + } + } } /// Best-effort removal of the daemon's socket/address and PID files. diff --git a/src/lib.rs b/src/lib.rs index 228e6b8..b122e1a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -505,24 +505,45 @@ fn parse_pid_file_contents(s: &str) -> Option { /// something answering length-prefixed JSON on the daemon's socket is. The /// probe payload is deliberately not a valid `DaemonRequest`: old daemons /// answer "Invalid request" without ever dialing Chrome. +/// +/// `Ok(false)` = nothing listening (missing socket / connection refused), so +/// the files are provably stale. `Err` = something accepted the connection +/// but the handshake didn't complete — the caller must treat the daemon's +/// liveness as unknown and leave its files alone. The timeout spans the whole +/// connect/write/read sequence: the socket name is predictable on shared +/// /tmp, so a hostile listener must not be able to stall `kill-daemon` at +/// any step. #[cfg(unix)] -async fn probe_legacy_daemon() -> bool { - let Ok(mut stream) = - tokio::net::UnixStream::connect(protocol::legacy_socket_path()).await - else { - return false; +async fn probe_legacy_daemon() -> Result { + use anyhow::Context as _; + let sock = protocol::legacy_socket_path(); + let sequence = async { + let mut stream = match tokio::net::UnixStream::connect(&sock).await { + Ok(s) => s, + Err(e) + if matches!( + e.kind(), + std::io::ErrorKind::NotFound | std::io::ErrorKind::ConnectionRefused + ) => + { + return Ok(false); + } + Err(e) => { + return Err(e) + .with_context(|| format!("Failed to connect to {}", sock.display())); + } + }; + protocol::write_msg(&mut stream, b"\"probe\"") + .await + .with_context(|| format!("Failed to write probe to {}", sock.display()))?; + protocol::read_msg(&mut stream) + .await + .with_context(|| format!("Failed to read probe response from {}", sock.display()))?; + Ok(true) }; - if protocol::write_msg(&mut stream, b"\"probe\"").await.is_err() { - return false; - } - matches!( - tokio::time::timeout( - std::time::Duration::from_secs(2), - protocol::read_msg(&mut stream) - ) - .await, - Ok(Ok(_)) - ) + tokio::time::timeout(std::time::Duration::from_secs(2), sequence) + .await + .with_context(|| format!("Probe of legacy daemon socket {} timed out", sock.display()))? } fn kill_daemon_decision(force: bool, can_prompt: bool) -> KillDaemonDecision { @@ -1039,7 +1060,7 @@ pub async fn run() -> Result<()> { let legacy_pid_path = protocol::legacy_pid_path(); if let Ok(pid_str) = std::fs::read_to_string(&legacy_pid_path) { match (parse_pid_file_contents(&pid_str), probe_legacy_daemon().await) { - (Some(pid), true) => { + (Some(pid), Ok(true)) => { // A daemon answered on the legacy socket, so the PID // file is live — not a recycled PID from a dead run. let ret = unsafe { libc::kill(pid as libc::pid_t, libc::SIGTERM) }; @@ -1055,14 +1076,14 @@ pub async fn run() -> Result<()> { } } } - (_, false) => { + (_, Ok(false)) => { // Nothing answering on the legacy socket: the files // are leftovers. Never signal the PID — the OS may // have recycled it for an unrelated process. let _ = std::fs::remove_file(protocol::legacy_socket_path()); let _ = std::fs::remove_file(&legacy_pid_path); } - (None, true) => { + (None, Ok(true)) => { // Live daemon but unusable PID contents: can't signal // it safely. Leave its files; it idles out on its own. println!( @@ -1070,6 +1091,16 @@ pub async fn run() -> Result<()> { protocol::legacy_socket_path().display() ); } + (_, Err(e)) => { + // Something accepted the connection but the probe + // couldn't confirm it's our daemon — signaling or + // deleting on a guess is worse than leaving the files + // for manual inspection. + println!( + "Warning: could not verify the legacy daemon on {}: {e:#}. Leaving its files in place.", + protocol::legacy_socket_path().display() + ); + } } } } From 84ed0a8813e4001742ca8c3049cca4fce069207b Mon Sep 17 00:00:00 2001 From: Aero Date: Wed, 29 Jul 2026 11:41:43 +0800 Subject: [PATCH 07/15] fix: declare MSRV, surface legacy kill failures, dedupe PID validation - Declare rust-version 1.89 (std File::lock/try_lock/TryLockError floor) - Legacy kill-daemon branch reports non-ESRCH signal failures (e.g. EPERM) instead of silently leaving a live daemon unmentioned - Main kill-daemon path reuses parse_pid_file_contents instead of duplicating the pid-0 and pid_t-overflow guards; add unit tests for the parser - Document the persistent daemon lock file in README Co-Authored-By: Claude Fable 5 --- Cargo.toml | 3 +++ README.md | 3 ++- src/lib.rs | 75 +++++++++++++++++++++++++++++++++++++----------------- 3 files changed, 56 insertions(+), 25 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index e2985d2..4ff6be4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -2,6 +2,9 @@ name = "chrome-devtools-cli" version = "1.5.0" edition = "2021" +# std::fs::File::{lock, try_lock} and std::fs::TryLockError (daemon lock path) +# stabilized in 1.89 +rust-version = "1.89" description = "Chrome DevTools Protocol CLI — auto-connects to existing Chrome" authors = ["Aero "] license = "MIT" diff --git a/README.md b/README.md index 500f9af..edf502b 100644 --- a/README.md +++ b/README.md @@ -219,11 +219,12 @@ Global `--block-url` and `--unblock-url` update the **active tab's** block list - **Endpoint (Unix)**: socket at `$TMPDIR/chrome-devtools-daemon-.sock` (uid-suffixed so users on a shared machine don't collide) - **Endpoint (Windows)**: loopback TCP listener; its address is written to `%TEMP%\chrome-devtools-daemon.addr` (`%TEMP%` is already per-user, so no suffix) - **PID file**: `$TMPDIR/chrome-devtools-daemon-.pid` (Windows: `%TEMP%\chrome-devtools-daemon.pid`) +- **Lock file**: `$TMPDIR/chrome-devtools-daemon-.lock` (Windows: `%TEMP%\chrome-devtools-daemon.lock`) — serializes daemon startup/cleanup; intentionally never removed automatically (deleting a lock file another process may be about to take reintroduces the race it prevents). Safe to delete manually when no daemon is running. - **Idle timeout**: 5 minutes (auto-exits, cleans up its files) - **Cleanup**: endpoint + PID files are also removed on panics, and on Unix on SIGTERM/SIGINT; Windows Ctrl-C cleanup is best-effort only (a background daemon has no console to receive it) - **Protocol**: Length-prefixed JSON over the Unix socket / loopback TCP - **Spawned by**: First CLI invocation (transparent to user) -- **Kill**: `chrome-devtools kill-daemon` (or delete the socket + PID file) +- **Kill**: `chrome-devtools kill-daemon` (or delete the socket + PID file; the lock file may also be deleted) The daemon keeps a persistent CDP session on the current page to: - Continuously collect `Network.*` and `Runtime.consoleAPICalled`/`exceptionThrown` events for `console` and `network` drains. diff --git a/src/lib.rs b/src/lib.rs index b122e1a..3923e9e 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -980,28 +980,15 @@ pub async fn run() -> Result<()> { let sock_path = protocol::socket_path(); match std::fs::read_to_string(&pid_path) { Ok(pid_str) => { - let pid: u32 = pid_str - .trim() - .parse() - .map_err(|_| anyhow::anyhow!("Invalid PID in {}", pid_path.display()))?; #[cfg(unix)] { - // Refuse PID 0: kill(0, ...) signals every process in the - // caller's process group — it would take down this CLI and - // its siblings. A corrupted/truncated PID file could read "0". - if pid == 0 { - anyhow::bail!("PID in {} is 0; refusing to signal", pid_path.display()); - } - // Guard against a PID that doesn't fit in libc::pid_t (a - // signed 32-bit integer on POSIX). The OS never produces such - // PIDs, but a corrupted PID file could, and the cast below - // would silently wrap to a negative number — which kill() - // interprets as "signal all processes in process group -pid", - // potentially killing unrelated processes. - let pid_i32: i32 = i32::try_from(pid).map_err(|_| { + // parse_pid_file_contents enforces the safety rules for + // what may be passed to kill() (no pid 0, no pid_t + // overflow — see its doc comment). + let pid = parse_pid_file_contents(&pid_str).ok_or_else(|| { anyhow::anyhow!( - "PID {} in {} exceeds libc::pid_t; refusing to signal", - pid, + "PID {:?} in {} is not a positive integer that fits libc::pid_t; refusing to signal", + pid_str.trim(), pid_path.display() ) })?; @@ -1009,7 +996,7 @@ pub async fn run() -> Result<()> { // to /usr/bin/kill. A return of 0 means the signal was // delivered; -1 with errno ESRCH means the process is gone // (and the PID file was stale). - let ret = unsafe { libc::kill(pid_i32 as libc::pid_t, libc::SIGTERM) }; + let ret = unsafe { libc::kill(pid as libc::pid_t, libc::SIGTERM) }; if ret == 0 { // Signal delivered — daemon is shutting down; clean up. let _ = std::fs::remove_file(&sock_path); @@ -1033,7 +1020,7 @@ pub async fn run() -> Result<()> { } #[cfg(not(unix))] { - let _ = pid; + let _ = pid_str; println!("kill-daemon is only supported on Unix systems."); } } @@ -1064,9 +1051,8 @@ pub async fn run() -> Result<()> { // A daemon answered on the legacy socket, so the PID // file is live — not a recycled PID from a dead run. let ret = unsafe { libc::kill(pid as libc::pid_t, libc::SIGTERM) }; - if ret == 0 || std::io::Error::last_os_error().raw_os_error() - == Some(libc::ESRCH) - { + let err = std::io::Error::last_os_error(); + if ret == 0 || err.raw_os_error() == Some(libc::ESRCH) { // Old binaries have no SIGTERM handler and exit // without cleanup, so remove their files here. let _ = std::fs::remove_file(protocol::legacy_socket_path()); @@ -1074,6 +1060,12 @@ pub async fn run() -> Result<()> { if ret == 0 { println!("Also stopped legacy daemon (PID {pid})."); } + } else { + // e.g. EPERM: it answered the probe, so it's + // alive — say so instead of silently leaving it. + println!( + "Warning: could not signal legacy daemon (PID {pid}): {err}. It may still be running; its files were left in place." + ); } } (_, Ok(false)) => { @@ -1565,6 +1557,41 @@ async fn run_direct(cli: &Cli, ws_url: &str) -> Result { mod tests { use super::*; + #[cfg(unix)] + #[test] + fn test_parse_pid_file_contents_valid_with_whitespace() { + assert_eq!(parse_pid_file_contents("12345"), Some(12345)); + assert_eq!(parse_pid_file_contents(" 12345\n"), Some(12345)); + assert_eq!(parse_pid_file_contents("1"), Some(1)); + assert_eq!(parse_pid_file_contents(&i32::MAX.to_string()), Some(i32::MAX)); + } + + #[cfg(unix)] + #[test] + fn test_parse_pid_file_contents_rejects_zero() { + assert_eq!(parse_pid_file_contents("0"), None); + assert_eq!(parse_pid_file_contents(" 0 "), None); + } + + #[cfg(unix)] + #[test] + fn test_parse_pid_file_contents_rejects_pid_t_overflow() { + // Would wrap negative through `as libc::pid_t` and signal group -pid. + assert_eq!(parse_pid_file_contents(&(i32::MAX as u32 + 1).to_string()), None); + assert_eq!(parse_pid_file_contents(&u32::MAX.to_string()), None); + assert_eq!(parse_pid_file_contents(&u64::MAX.to_string()), None); + } + + #[cfg(unix)] + #[test] + fn test_parse_pid_file_contents_rejects_non_numeric() { + assert_eq!(parse_pid_file_contents("-5"), None); + assert_eq!(parse_pid_file_contents("abc"), None); + assert_eq!(parse_pid_file_contents("12 34"), None); + assert_eq!(parse_pid_file_contents(""), None); + assert_eq!(parse_pid_file_contents(" \n"), None); + } + #[test] #[allow(clippy::approx_constant)] fn test_parse_args() { From d892cfe12e224c122b2f97a50cf6a0cf0dd5a973 Mon Sep 17 00:00:00 2001 From: Aero Date: Wed, 29 Jul 2026 11:47:32 +0800 Subject: [PATCH 08/15] fix: accurate lock-file error context and legacy-sweep liveness gate - open_lock_file returns anyhow::Result with per-operation context (open vs metadata vs ownership validation) so callers no longer mislabel metadata failures as open failures - Legacy kill-daemon sweep only removes files when the named process is verifiably gone: old binaries write their PID before binding and don't take the startup lock, so an unanswered socket alone could be a daemon mid-startup Co-Authored-By: Claude Fable 5 --- src/daemon.rs | 28 +++++++++++++++------------- src/lib.rs | 28 ++++++++++++++++++++++------ 2 files changed, 37 insertions(+), 19 deletions(-) diff --git a/src/daemon.rs b/src/daemon.rs index ed7a5da..fb47192 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -34,7 +34,8 @@ enum ConnectionOutcome { /// planted symlink (O_NOFOLLOW) and only lock something that is a regular /// file owned by this uid. macOS $TMPDIR and Windows %TEMP% are per-user, so /// there the checks are inert. -fn open_lock_file() -> std::io::Result { +fn open_lock_file() -> Result { + let path = lock_path(); let mut opts = std::fs::OpenOptions::new(); opts.create(true).write(true).truncate(false); #[cfg(unix)] @@ -43,16 +44,20 @@ fn open_lock_file() -> std::io::Result { opts.custom_flags(libc::O_NOFOLLOW); opts.mode(0o600); } - let f = opts.open(lock_path())?; + let f = opts + .open(&path) + .with_context(|| format!("Failed to open daemon lock file {}", path.display()))?; #[cfg(unix)] { use std::os::unix::fs::MetadataExt; - let md = f.metadata()?; + let md = f.metadata().with_context(|| { + format!("Failed to read metadata of daemon lock file {}", path.display()) + })?; if !md.is_file() || md.uid() != unsafe { libc::getuid() } { - return Err(std::io::Error::new( - std::io::ErrorKind::PermissionDenied, - "lock path is not a regular file owned by the current user", - )); + anyhow::bail!( + "Daemon lock path {} is not a regular file owned by the current user; refusing to lock it", + path.display() + ); } } Ok(f) @@ -73,8 +78,7 @@ const LOCK_WAIT_TIMEOUT: Duration = Duration::from_secs(5); /// wait is bounded because an indefinite `lock()` would let any process that /// pre-acquired the predictable lock path park daemon startup forever. fn lock_daemon_files() -> Result { - let f = open_lock_file() - .with_context(|| format!("Failed to open daemon lock file {}", lock_path().display()))?; + let f = open_lock_file()?; let deadline = std::time::Instant::now() + LOCK_WAIT_TIMEOUT; loop { match f.try_lock() { @@ -112,10 +116,8 @@ fn cleanup() { // Without the lock, removal could race a replacement's startup — // leaving the files is the safe side (they self-heal on the next // daemon start), but say why so the cause isn't swallowed. - eprintln!( - "daemon: leaving socket/PID files in place: cannot open lock file {}: {e}", - lock_path().display() - ); + // open_lock_file's error already names the operation and path. + eprintln!("daemon: leaving socket/PID files in place: {e:#}"); return; } }; diff --git a/src/lib.rs b/src/lib.rs index 3923e9e..4e289c9 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1068,12 +1068,28 @@ pub async fn run() -> Result<()> { ); } } - (_, Ok(false)) => { - // Nothing answering on the legacy socket: the files - // are leftovers. Never signal the PID — the OS may - // have recycled it for an unrelated process. - let _ = std::fs::remove_file(protocol::legacy_socket_path()); - let _ = std::fs::remove_file(&legacy_pid_path); + (pid, Ok(false)) => { + // Nothing answering on the legacy socket. That alone + // doesn't prove staleness: old binaries write their + // PID before binding, and they don't take the new + // startup lock, so this may be a daemon mid-startup. + // Only remove when the named process is verifiably + // gone (or the PID is junk). Never signal here — the + // OS may have recycled the PID. + let alive = pid.is_some_and(|p| { + (unsafe { libc::kill(p as libc::pid_t, 0) }) == 0 + || std::io::Error::last_os_error().raw_os_error() + == Some(libc::EPERM) + }); + if alive { + println!( + "Note: legacy PID file {} names a live process but nothing answered on the legacy socket; leaving its files in place (it may be mid-startup, and it exits after its idle timeout anyway).", + legacy_pid_path.display() + ); + } else { + let _ = std::fs::remove_file(protocol::legacy_socket_path()); + let _ = std::fs::remove_file(&legacy_pid_path); + } } (None, Ok(true)) => { // Live daemon but unusable PID contents: can't signal From 79270e260f1440cebd27c470cccca00b01f46346 Mon Sep 17 00:00:00 2001 From: Aero Date: Wed, 29 Jul 2026 11:59:49 +0800 Subject: [PATCH 09/15] fix: async lock wait, SAFETY comments, and lifecycle doc clarifications - lock_daemon_files polls with tokio::time::sleep instead of thread::sleep so a contended lock never blocks a runtime worker - Add SAFETY comments to all unsafe getuid()/kill() call sites - Document cleanup() stderr visibility (foreground debugging only), the accept-loop macro's contract (re-evaluated accept future, pinned shutdown future, why it isn't a generic fn), the legacy probe's reply-to-malformed-request assumption and its safe failure mode, and parse_pid_file_contents' kill-daemon-only scope Co-Authored-By: Claude Fable 5 --- src/daemon.rs | 27 ++++++++++++++++++++++----- src/lib.rs | 18 +++++++++++++++++- src/protocol.rs | 2 ++ 3 files changed, 41 insertions(+), 6 deletions(-) diff --git a/src/daemon.rs b/src/daemon.rs index fb47192..653b7ea 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -53,6 +53,8 @@ fn open_lock_file() -> Result { let md = f.metadata().with_context(|| { format!("Failed to read metadata of daemon lock file {}", path.display()) })?; + // SAFETY: getuid() is a pure kernel query with no preconditions; it + // is thread-safe and cannot fail. if !md.is_file() || md.uid() != unsafe { libc::getuid() } { anyhow::bail!( "Daemon lock path {} is not a regular file owned by the current user; refusing to lock it", @@ -77,20 +79,22 @@ const LOCK_WAIT_TIMEOUT: Duration = Duration::from_secs(5); /// its PID file either — failing here just surfaces the cause sooner. The /// wait is bounded because an indefinite `lock()` would let any process that /// pre-acquired the predictable lock path park daemon startup forever. -fn lock_daemon_files() -> Result { +/// Async (poll + `tokio::time::sleep`) so a contended lock never blocks the +/// runtime's worker thread. +async fn lock_daemon_files() -> Result { let f = open_lock_file()?; - let deadline = std::time::Instant::now() + LOCK_WAIT_TIMEOUT; + let deadline = tokio::time::Instant::now() + LOCK_WAIT_TIMEOUT; loop { match f.try_lock() { Ok(()) => return Ok(f), Err(std::fs::TryLockError::WouldBlock) => { - if std::time::Instant::now() >= deadline { + if tokio::time::Instant::now() >= deadline { anyhow::bail!( "Timed out after {LOCK_WAIT_TIMEOUT:?} waiting for daemon lock file {} (held by another process)", lock_path().display() ); } - std::thread::sleep(Duration::from_millis(50)); + tokio::time::sleep(Duration::from_millis(50)).await; } Err(std::fs::TryLockError::Error(e)) => { return Err(e).with_context(|| { @@ -109,6 +113,12 @@ fn lock_daemon_files() -> Result { /// removals happen under the daemon-file lock so a replacement can't write /// its pid and rebind in between (it would keep running but be unreachable, /// and every later CLI call would spawn yet another daemon). +/// +/// The `eprintln!` diagnostics here are only visible when `__daemon__` is run +/// in a foreground terminal: `spawn_daemon` detaches with stderr to null. +/// That's acceptable — every branch below fails safe (files are left in +/// place and self-heal on the next daemon start), so the messages exist for +/// interactive debugging, not operational monitoring. fn cleanup() { let lock_file = match open_lock_file() { Ok(f) => f, @@ -162,6 +172,13 @@ impl Drop for CleanupGuard { } } +/// A macro (not a generic fn) because the Unix `UnixListener` and Windows +/// `TcpListener` have no common accept trait; the cfg-gated call site passes +/// whichever exists. Contract for callers: +/// - `$accept` is re-evaluated every iteration (pass `listener.accept()`, +/// which creates a fresh accept future each time around the loop). +/// - `$shutdown` is polled by `&mut` reference, so the caller must pin it +/// first (`tokio::pin!`); passing an unpinned future fails to compile. macro_rules! run_accept_loop_body { ($accept:expr, $client:expr, $ws_url:expr, $shutdown:expr) => { loop { @@ -239,7 +256,7 @@ pub async fn run_daemon(ws_url: &str) -> Result<()> { // Startup critical section: the pid write and endpoint (re)bind must not // interleave with a predecessor's cleanup() check-then-remove, or the // predecessor can delete files this daemon just claimed. - let startup_lock = lock_daemon_files()?; + let startup_lock = lock_daemon_files().await?; std::fs::write(pid_path(), std::process::id().to_string())?; diff --git a/src/lib.rs b/src/lib.rs index 4e289c9..e6a331e 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -490,6 +490,10 @@ enum KillDaemonDecision { /// Rejects pid 0 — `kill(0, sig)` signals the caller's whole process group — /// and values that don't fit `libc::pid_t`, where the `as` cast would wrap /// negative and `kill` would signal process group `-pid` instead. +/// +/// Used only by `kill-daemon`'s PID-file handling (current and legacy +/// paths); the daemon itself never parses a PID — it writes its own via +/// `std::process::id()`. #[cfg(unix)] fn parse_pid_file_contents(s: &str) -> Option { s.trim() @@ -504,7 +508,10 @@ fn parse_pid_file_contents(s: &str) -> Option { /// file alone is not evidence the process it names is still our daemon — but /// something answering length-prefixed JSON on the daemon's socket is. The /// probe payload is deliberately not a valid `DaemonRequest`: old daemons -/// answer "Invalid request" without ever dialing Chrome. +/// answer "Invalid request" without ever dialing Chrome. This assumes legacy +/// daemons reply to malformed requests rather than dropping them; if that +/// ever stops holding, the probe times out and lands in the `Err` branch — +/// the safe outcome (files left in place, nothing signaled). /// /// `Ok(false)` = nothing listening (missing socket / connection refused), so /// the files are provably stale. `Err` = something accepted the connection @@ -996,6 +1003,9 @@ pub async fn run() -> Result<()> { // to /usr/bin/kill. A return of 0 means the signal was // delivered; -1 with errno ESRCH means the process is gone // (and the PID file was stale). + // SAFETY: kill() has no memory-safety preconditions; the + // pid was validated positive and in pid_t range above, so + // it cannot alias a process group. let ret = unsafe { libc::kill(pid as libc::pid_t, libc::SIGTERM) }; if ret == 0 { // Signal delivered — daemon is shutting down; clean up. @@ -1050,6 +1060,9 @@ pub async fn run() -> Result<()> { (Some(pid), Ok(true)) => { // A daemon answered on the legacy socket, so the PID // file is live — not a recycled PID from a dead run. + // SAFETY: kill() has no memory-safety preconditions; + // parse_pid_file_contents guarantees a positive, + // pid_t-range pid (no process-group aliasing). let ret = unsafe { libc::kill(pid as libc::pid_t, libc::SIGTERM) }; let err = std::io::Error::last_os_error(); if ret == 0 || err.raw_os_error() == Some(libc::ESRCH) { @@ -1077,6 +1090,9 @@ pub async fn run() -> Result<()> { // gone (or the PID is junk). Never signal here — the // OS may have recycled the PID. let alive = pid.is_some_and(|p| { + // SAFETY: signal 0 performs only an existence + // check — no signal is sent; the pid was + // validated by parse_pid_file_contents. (unsafe { libc::kill(p as libc::pid_t, 0) }) == 0 || std::io::Error::last_os_error().raw_os_error() == Some(libc::EPERM) diff --git a/src/protocol.rs b/src/protocol.rs index d4d228f..c206a0f 100644 --- a/src/protocol.rs +++ b/src/protocol.rs @@ -53,6 +53,8 @@ pub struct DaemonResponse { /// collide on (or squat) each other's daemon files. #[cfg(unix)] fn user_suffix() -> String { + // SAFETY: getuid() is a pure kernel query with no preconditions; it is + // thread-safe and cannot fail. format!("-{}", unsafe { libc::getuid() }) } From 5e8ed269e5045c20a8f58803148adec50311753c Mon Sep 17 00:00:00 2001 From: Aero Date: Wed, 29 Jul 2026 12:05:55 +0800 Subject: [PATCH 10/15] fix: clarify lock file behavior and deletion safety in README --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index edf502b..3233de7 100644 --- a/README.md +++ b/README.md @@ -219,12 +219,12 @@ Global `--block-url` and `--unblock-url` update the **active tab's** block list - **Endpoint (Unix)**: socket at `$TMPDIR/chrome-devtools-daemon-.sock` (uid-suffixed so users on a shared machine don't collide) - **Endpoint (Windows)**: loopback TCP listener; its address is written to `%TEMP%\chrome-devtools-daemon.addr` (`%TEMP%` is already per-user, so no suffix) - **PID file**: `$TMPDIR/chrome-devtools-daemon-.pid` (Windows: `%TEMP%\chrome-devtools-daemon.pid`) -- **Lock file**: `$TMPDIR/chrome-devtools-daemon-.lock` (Windows: `%TEMP%\chrome-devtools-daemon.lock`) — serializes daemon startup/cleanup; intentionally never removed automatically (deleting a lock file another process may be about to take reintroduces the race it prevents). Safe to delete manually when no daemon is running. +- **Lock file**: `$TMPDIR/chrome-devtools-daemon-.lock` (Windows: `%TEMP%\chrome-devtools-daemon.lock`) — serializes daemon startup/cleanup; intentionally never removed automatically. Locks bind to the inode, not the name: deleting the file while any daemon process is still starting, running, or shutting down lets a new process lock a fresh replacement inode and bypass the serialization entirely. Only delete it once no daemon process exists at all — and there's rarely a reason to, since a leftover lock file is harmless. - **Idle timeout**: 5 minutes (auto-exits, cleans up its files) - **Cleanup**: endpoint + PID files are also removed on panics, and on Unix on SIGTERM/SIGINT; Windows Ctrl-C cleanup is best-effort only (a background daemon has no console to receive it) - **Protocol**: Length-prefixed JSON over the Unix socket / loopback TCP - **Spawned by**: First CLI invocation (transparent to user) -- **Kill**: `chrome-devtools kill-daemon` (or delete the socket + PID file; the lock file may also be deleted) +- **Kill**: `chrome-devtools kill-daemon` (or delete the socket + PID file; leave the lock file — see above) The daemon keeps a persistent CDP session on the current page to: - Continuously collect `Network.*` and `Runtime.consoleAPICalled`/`exceptionThrown` events for `console` and `network` drains. From 420f17b2ac94f7c789e408467c27c9edd1f9215f Mon Sep 17 00:00:00 2001 From: Aero Date: Wed, 29 Jul 2026 12:27:22 +0800 Subject: [PATCH 11/15] fix: harden pid-file trust, verify legacy probe peer, tighten lock wait Review fixes for the daemon-cleanup changes: - kill-daemon no longer trusts pid files by path alone: reads go through read_pid_file_checked (O_NOFOLLOW|O_NONBLOCK, regular-file + owner-uid check, 64-byte cap), closing a confused-deputy SIGTERM via a planted pid file at the predictable (especially legacy, unsuffixed) names in shared /tmp. - probe_legacy_daemon checks peer credentials: a listener on the legacy socket running as another uid is treated as a squatter (Err -> files left alone), not as our daemon. - open_lock_file gains O_NONBLOCK so a planted FIFO can't wedge open(). - LOCK_WAIT_TIMEOUT 5s -> 2s so a lock-delayed daemon still binds inside the client's 5s wait_for_daemon budget instead of losing the race. - Signal streams register eagerly (before lock/bind), closing the startup window where SIGTERM skipped CleanupGuard; a stream that registered while its sibling failed is now still drained instead of swallowing that signal. - Tests for the invariants the design rests on: lock-path symlink/FIFO rejection, cleanup foreign-pid no-op, lock-never-removed, and contention back-off (via path-parameterized open_lock_file_at / cleanup_at). - SKILL.md Pattern 16: cleanup runs via an EXIT trap on every failure path; readiness wait bounded at 30s with a Chrome liveness check. - Minor: drop no-op _lock rebind, user_suffix returns Cow, rustfmt. Co-Authored-By: Claude Opus 5 (1M context) --- skill/chrome-devtools/SKILL.md | 35 ++++-- src/daemon.rs | 221 +++++++++++++++++++++++++++------ src/lib.rs | 93 ++++++++++++-- src/protocol.rs | 8 +- 4 files changed, 295 insertions(+), 62 deletions(-) diff --git a/skill/chrome-devtools/SKILL.md b/skill/chrome-devtools/SKILL.md index e9b1d99..cf01f5b 100644 --- a/skill/chrome-devtools/SKILL.md +++ b/skill/chrome-devtools/SKILL.md @@ -389,22 +389,33 @@ chrome-devtools kill-daemon --force about:blank & CHROME_PID=$! -# 3. DevToolsActivePort is written only after the DevTools server is +# 3. Cleanup — REQUIRED even if a later step fails: the daemon is bound to the +# headless instance and would otherwise hijack later commands aimed at the +# user's real Chrome. A trap runs it on every exit path, not just success. +cleanup() { + chrome-devtools kill-daemon --force + kill "$CHROME_PID" 2>/dev/null + rm -rf "$PROFILE" +} +trap cleanup EXIT + +# 4. DevToolsActivePort is written only after the DevTools server is # actually listening, so the file's existence — not the process being -# alive — is the readiness signal; connecting earlier races startup -while [ ! -f "$PROFILE/DevToolsActivePort" ]; do sleep 0.5; done - -# 4. Every command needs --user-data-dir pointing at the headless profile; +# alive — is the readiness signal; connecting earlier races startup. +# Bounded (30s) and watching the PID so a Chrome that crashes or never +# starts fails the script instead of hanging it forever. +for _ in $(seq 1 60); do + [ -f "$PROFILE/DevToolsActivePort" ] && break + kill -0 "$CHROME_PID" 2>/dev/null || { echo "Chrome exited during startup" >&2; exit 1; } + sleep 0.5 +done +[ -f "$PROFILE/DevToolsActivePort" ] || { echo "Chrome not ready after 30s" >&2; exit 1; } + +# 5. Every command needs --user-data-dir pointing at the headless profile; # the CLI auto-connects by reading its DevToolsActivePort chrome-devtools --user-data-dir "$PROFILE" navigate https://example.com chrome-devtools --user-data-dir "$PROFILE" evaluate 'document.title' chrome-devtools --user-data-dir "$PROFILE" screenshot --output /tmp/shot.png - -# 5. Cleanup — REQUIRED: the daemon is now bound to the headless instance and -# would otherwise hijack later commands aimed at the user's real Chrome -chrome-devtools kill-daemon --force -kill $CHROME_PID -rm -rf "$PROFILE" ``` Linux path: `google-chrome` or `chromium` on `$PATH` replaces the macOS @@ -414,7 +425,7 @@ Linux path: `google-chrome` or `chromium` on `$PATH` replaces the macOS whichever Chrome the first command resolved, and later commands reuse it even if their flags point elsewhere. Always `kill-daemon --force` when switching between the user's Chrome and a headless instance — in both directions -(steps 1 and 5 above). +(step 1 and the EXIT trap above). ## Complete Command Reference diff --git a/src/daemon.rs b/src/daemon.rs index 653b7ea..e236086 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -32,26 +32,37 @@ enum ConnectionOutcome { /// The lock path has a predictable name and, on Linux, lives in shared /tmp, /// so treat a pre-existing file as potentially hostile: refuse to follow a /// planted symlink (O_NOFOLLOW) and only lock something that is a regular -/// file owned by this uid. macOS $TMPDIR and Windows %TEMP% are per-user, so -/// there the checks are inert. +/// file owned by this uid. O_NONBLOCK keeps a planted FIFO from wedging the +/// open() itself (it has no effect on regular files); the `is_file` check +/// then rejects it. macOS $TMPDIR and Windows %TEMP% are per-user, so there +/// the checks are inert. fn open_lock_file() -> Result { - let path = lock_path(); + open_lock_file_at(&lock_path()) +} + +/// Path-parameterized body of [`open_lock_file`], so tests can exercise the +/// hostile-path checks against a scratch directory instead of the real +/// `temp_dir()`. +fn open_lock_file_at(path: &std::path::Path) -> Result { let mut opts = std::fs::OpenOptions::new(); opts.create(true).write(true).truncate(false); #[cfg(unix)] { use std::os::unix::fs::OpenOptionsExt; - opts.custom_flags(libc::O_NOFOLLOW); + opts.custom_flags(libc::O_NOFOLLOW | libc::O_NONBLOCK); opts.mode(0o600); } let f = opts - .open(&path) + .open(path) .with_context(|| format!("Failed to open daemon lock file {}", path.display()))?; #[cfg(unix)] { use std::os::unix::fs::MetadataExt; let md = f.metadata().with_context(|| { - format!("Failed to read metadata of daemon lock file {}", path.display()) + format!( + "Failed to read metadata of daemon lock file {}", + path.display() + ) })?; // SAFETY: getuid() is a pure kernel query with no preconditions; it // is thread-safe and cannot fail. @@ -68,7 +79,11 @@ fn open_lock_file() -> Result { /// How long startup will wait for the daemon-file lock. Legitimate holders /// (a predecessor's cleanup, another daemon's startup) finish in /// milliseconds; anything longer means the lock is wedged or squatted. -const LOCK_WAIT_TIMEOUT: Duration = Duration::from_secs(5); +/// Must stay well below the CLI's `DAEMON_WAIT_TIMEOUT_SECS` (default 5s, +/// `client.rs`): if the two were equal, a daemon that spent its whole budget +/// waiting here would bind at the exact moment `wait_for_daemon` gives up, +/// and the CLI would fall back to direct execution despite a usable daemon. +const LOCK_WAIT_TIMEOUT: Duration = Duration::from_secs(2); /// Acquire the cross-process lock serializing the daemon-file critical /// sections: startup's pid-write/rebind and cleanup's check-then-remove. @@ -120,13 +135,26 @@ async fn lock_daemon_files() -> Result { /// place and self-heal on the next daemon start), so the messages exist for /// interactive debugging, not operational monitoring. fn cleanup() { - let lock_file = match open_lock_file() { + #[cfg(unix)] + let endpoint = socket_path(); + #[cfg(windows)] + let endpoint = addr_path(); + cleanup_at(&lock_path(), &pid_path(), &endpoint); +} + +/// Path-parameterized body of [`cleanup`] (see its doc for the locking and +/// ownership rules), so tests can drive it against a scratch directory +/// instead of the real `temp_dir()` files. +fn cleanup_at(lock: &std::path::Path, pid: &std::path::Path, endpoint: &std::path::Path) { + // `lock_file` holds the OS lock until it drops at the end of this scope, + // covering the ownership check and removals below. + let lock_file = match open_lock_file_at(lock) { Ok(f) => f, Err(e) => { // Without the lock, removal could race a replacement's startup — // leaving the files is the safe side (they self-heal on the next // daemon start), but say why so the cause isn't swallowed. - // open_lock_file's error already names the operation and path. + // open_lock_file_at's error already names the operation and path. eprintln!("daemon: leaving socket/PID files in place: {e:#}"); return; } @@ -141,24 +169,20 @@ fn cleanup() { Err(std::fs::TryLockError::Error(e)) => { eprintln!( "daemon: leaving socket/PID files in place: cannot lock {}: {e}", - lock_path().display() + lock.display() ); return; } } - let _lock = lock_file; - let owns_files = std::fs::read_to_string(pid_path()) + let owns_files = std::fs::read_to_string(pid) .ok() .and_then(|s| s.trim().parse::().ok()) == Some(std::process::id()); if !owns_files { return; } - #[cfg(unix)] - let _ = std::fs::remove_file(socket_path()); - #[cfg(windows)] - let _ = std::fs::remove_file(addr_path()); - let _ = std::fs::remove_file(pid_path()); + let _ = std::fs::remove_file(endpoint); + let _ = std::fs::remove_file(pid); } /// Removes the daemon's on-disk files when `run_daemon`'s frame is left for @@ -213,22 +237,36 @@ macro_rules! run_accept_loop_body { /// deliberately left unhandled — convention reserves it for config reload, /// not shutdown. #[cfg(unix)] -async fn shutdown_signal() { +fn shutdown_signal() -> impl std::future::Future { use tokio::signal::unix::{signal, SignalKind}; - // If a signal stream can't be registered, fall back to never resolving — - // the daemon then behaves as before this handler existed (default - // disposition still terminates the process; only file cleanup is lost). - let mut sigterm = match signal(SignalKind::terminate()) { - Ok(s) => s, - Err(_) => return std::future::pending().await, - }; - let mut sigint = match signal(SignalKind::interrupt()) { - Ok(s) => s, - Err(_) => return std::future::pending().await, - }; - tokio::select! { - _ = sigterm.recv() => {} - _ = sigint.recv() => {} + // A plain fn returning a future (not an async fn) so the signal streams + // are registered right here, when the caller sets up shutdown handling — + // an async fn would defer registration to the first poll, leaving a + // window during startup where a signal still hits the default + // disposition and skips CleanupGuard. + let sigterm = signal(SignalKind::terminate()).ok(); + let sigint = signal(SignalKind::interrupt()).ok(); + async move { + // A stream that failed to register keeps its default disposition + // (terminate without cleanup — as before this handler existed). One + // that registered MUST still be drained here: registration replaces + // the default handler, so ignoring it would make that signal a no-op + // and the daemon unkillable by it. + match (sigterm, sigint) { + (Some(mut term), Some(mut int)) => { + tokio::select! { + _ = term.recv() => {} + _ = int.recv() => {} + } + } + (Some(mut term), None) => { + term.recv().await; + } + (None, Some(mut int)) => { + int.recv().await; + } + (None, None) => std::future::pending().await, + } } } @@ -237,14 +275,20 @@ async fn shutdown_signal() { /// fires for a backgrounded daemon. It does work when `__daemon__` is run /// manually in a foreground console for debugging. #[cfg(windows)] -async fn shutdown_signal() { - if tokio::signal::ctrl_c().await.is_err() { - std::future::pending::<()>().await; +fn shutdown_signal() -> impl std::future::Future { + // Registered eagerly for the same reason as the Unix version. + let ctrl_c = tokio::signal::windows::ctrl_c().ok(); + async move { + match ctrl_c { + Some(mut c) => { + c.recv().await; + } + None => std::future::pending().await, + } } } pub async fn run_daemon(ws_url: &str) -> Result<()> { - // Write PID // Armed before anything is written: cleanup() verifies pid-file ownership // first, so firing "too early" is a no-op, and this declaration order // means the startup lock below is released (locals drop in reverse order) @@ -253,6 +297,12 @@ pub async fn run_daemon(ws_url: &str) -> Result<()> { // including unwinding panics, which previously leaked the files. let _guard = CleanupGuard; + // Signal streams are registered here — before the lock wait and bind — + // so a SIGTERM during startup is buffered for the accept loop instead of + // killing the process without cleanup. + let shutdown = shutdown_signal(); + tokio::pin!(shutdown); + // Startup critical section: the pid write and endpoint (re)bind must not // interleave with a predecessor's cleanup() check-then-remove, or the // predecessor can delete files this daemon just claimed. @@ -292,9 +342,6 @@ pub async fn run_daemon(ws_url: &str) -> Result<()> { // while the daemon handles the potentially slow macOS/Chrome network permission prompt. let mut client: Option = None; - let shutdown = shutdown_signal(); - tokio::pin!(shutdown); - // Signal readiness by socket/address existence (it's already bound) run_accept_loop_body!(listener.accept(), &mut client, ws_url, shutdown); @@ -421,3 +468,99 @@ async fn handle_request(client: &mut CdpClient, req: &DaemonRequest) -> DaemonRe } } } + +#[cfg(test)] +mod tests { + use super::*; + + #[cfg(unix)] + #[test] + fn test_open_lock_file_at_rejects_symlink() { + let dir = tempfile::tempdir().unwrap(); + let target = dir.path().join("target"); + std::fs::write(&target, "").unwrap(); + let link = dir.path().join("daemon.lock"); + std::os::unix::fs::symlink(&target, &link).unwrap(); + // O_NOFOLLOW must refuse a planted symlink even though its target is + // a perfectly ordinary file owned by us. + assert!(open_lock_file_at(&link).is_err()); + // And the symlink must not have been replaced or followed-through. + assert!(link.symlink_metadata().unwrap().file_type().is_symlink()); + } + + #[cfg(unix)] + #[test] + fn test_open_lock_file_at_rejects_fifo() { + use std::os::unix::ffi::OsStrExt; + let dir = tempfile::tempdir().unwrap(); + let fifo = dir.path().join("daemon.lock"); + let c_path = std::ffi::CString::new(fifo.as_os_str().as_bytes()).unwrap(); + // SAFETY: mkfifo only reads the NUL-terminated path we just built. + assert_eq!(unsafe { libc::mkfifo(c_path.as_ptr(), 0o600) }, 0); + // O_NONBLOCK makes the write-only open of a reader-less FIFO fail + // with ENXIO instead of blocking forever. + assert!(open_lock_file_at(&fifo).is_err()); + } + + #[test] + fn test_open_lock_file_at_accepts_regular_file() { + let dir = tempfile::tempdir().unwrap(); + let lock = dir.path().join("daemon.lock"); + assert!(open_lock_file_at(&lock).is_ok(), "creates when absent"); + assert!(open_lock_file_at(&lock).is_ok(), "reopens when present"); + } + + /// The invariant the whole ownership handoff rests on: a daemon whose PID + /// file has been rebound to another process must not remove the files. + #[test] + fn test_cleanup_at_leaves_files_of_foreign_pid() { + let dir = tempfile::tempdir().unwrap(); + let lock = dir.path().join("daemon.lock"); + let pid = dir.path().join("daemon.pid"); + let endpoint = dir.path().join("daemon.sock"); + std::fs::write(&pid, std::process::id().wrapping_add(1).to_string()).unwrap(); + std::fs::write(&endpoint, "").unwrap(); + cleanup_at(&lock, &pid, &endpoint); + assert!(pid.exists(), "foreign PID file must survive cleanup"); + assert!( + endpoint.exists(), + "foreign endpoint file must survive cleanup" + ); + } + + #[test] + fn test_cleanup_at_removes_own_files_but_never_the_lock() { + let dir = tempfile::tempdir().unwrap(); + let lock = dir.path().join("daemon.lock"); + let pid = dir.path().join("daemon.pid"); + let endpoint = dir.path().join("daemon.sock"); + std::fs::write(&pid, std::process::id().to_string()).unwrap(); + std::fs::write(&endpoint, "").unwrap(); + cleanup_at(&lock, &pid, &endpoint); + assert!(!pid.exists()); + assert!(!endpoint.exists()); + assert!( + lock.exists(), + "the lock file is intentionally never removed" + ); + } + + /// flock is per open-file-description, so a second handle in this same + /// process contends like another process would. + #[test] + fn test_cleanup_at_backs_off_when_lock_contended() { + let dir = tempfile::tempdir().unwrap(); + let lock = dir.path().join("daemon.lock"); + let pid = dir.path().join("daemon.pid"); + let endpoint = dir.path().join("daemon.sock"); + std::fs::write(&pid, std::process::id().to_string()).unwrap(); + std::fs::write(&endpoint, "").unwrap(); + let holder = open_lock_file_at(&lock).unwrap(); + holder.try_lock().unwrap(); + // A replacement holds the lock (mid-startup): even our own files must + // be left alone, since the replacement may be about to rebind them. + cleanup_at(&lock, &pid, &endpoint); + assert!(pid.exists()); + assert!(endpoint.exists()); + } +} diff --git a/src/lib.rs b/src/lib.rs index e6a331e..f647e9f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -503,6 +503,39 @@ fn parse_pid_file_contents(s: &str) -> Option { .and_then(|p| i32::try_from(p).ok()) } +/// Read a PID file without trusting the path it lives at. PID-file names are +/// predictable and, on Linux, live in shared /tmp — the same threat model as +/// the daemon lock file — so refuse to follow a planted symlink (O_NOFOLLOW) +/// and only accept a regular file owned by this uid. O_NONBLOCK stops a +/// planted FIFO from wedging the open() itself (it has no effect on regular +/// files); the `is_file` check then rejects the FIFO. The read is capped +/// because a PID is at most a few bytes — a large file at this path is +/// hostile by definition and must not be slurped into memory. +#[cfg(unix)] +fn read_pid_file_checked(path: &std::path::Path) -> std::io::Result { + use std::io::Read; + use std::os::unix::fs::{MetadataExt, OpenOptionsExt}; + let f = std::fs::OpenOptions::new() + .read(true) + .custom_flags(libc::O_NOFOLLOW | libc::O_NONBLOCK) + .open(path)?; + let md = f.metadata()?; + // SAFETY: getuid() is a pure kernel query with no preconditions; it is + // thread-safe and cannot fail. + if !md.is_file() || md.uid() != unsafe { libc::getuid() } { + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidData, + format!( + "{} is not a regular file owned by the current user; refusing to trust it", + path.display() + ), + )); + } + let mut s = String::new(); + f.take(64).read_to_string(&mut s)?; + Ok(s) +} + /// Confirm a live daemon is listening on the legacy (pre-uid-suffix) socket /// before trusting the legacy PID file. The OS recycles PIDs, so a stale /// file alone is not evidence the process it names is still our daemon — but @@ -536,10 +569,26 @@ async fn probe_legacy_daemon() -> Result { return Ok(false); } Err(e) => { - return Err(e) - .with_context(|| format!("Failed to connect to {}", sock.display())); + return Err(e).with_context(|| format!("Failed to connect to {}", sock.display())); } }; + // The socket path is predictable, so before treating the listener as + // our daemon, check who is actually on the other end: a legacy daemon + // was started by this user, so a peer running as any other uid is a + // squatter. Err (not Ok(false)) so the caller lands in the + // leave-everything-alone branch rather than the provably-stale one. + let cred = stream + .peer_cred() + .with_context(|| format!("Failed to read peer credentials of {}", sock.display()))?; + // SAFETY: getuid() is a pure kernel query with no preconditions; it + // is thread-safe and cannot fail. + if cred.uid() != unsafe { libc::getuid() } { + anyhow::bail!( + "Listener on {} runs as uid {}, not the current user; refusing to treat it as the legacy daemon", + sock.display(), + cred.uid() + ); + } protocol::write_msg(&mut stream, b"\"probe\"") .await .with_context(|| format!("Failed to write probe to {}", sock.display()))?; @@ -985,7 +1034,13 @@ pub async fn run() -> Result<()> { let pid_path = protocol::pid_path(); #[cfg(unix)] let sock_path = protocol::socket_path(); - match std::fs::read_to_string(&pid_path) { + // Ownership-checked read: the path is predictable in shared /tmp, so + // never act on a PID file that was planted there by another user. + #[cfg(unix)] + let read_result = read_pid_file_checked(&pid_path); + #[cfg(not(unix))] + let read_result = std::fs::read_to_string(&pid_path); + match read_result { Ok(pid_str) => { #[cfg(unix)] { @@ -1055,8 +1110,26 @@ pub async fn run() -> Result<()> { #[cfg(unix)] { let legacy_pid_path = protocol::legacy_pid_path(); - if let Ok(pid_str) = std::fs::read_to_string(&legacy_pid_path) { - match (parse_pid_file_contents(&pid_str), probe_legacy_daemon().await) { + // Ownership-checked read (same threat model as the current pid + // path above, but worse: the legacy name has no uid suffix, so on + // shared /tmp any user could have planted it). + let pid_str = match read_pid_file_checked(&legacy_pid_path) { + Ok(s) => Some(s), + // No legacy files — the common case; stay silent. + Err(e) if e.kind() == std::io::ErrorKind::NotFound => None, + Err(e) => { + println!( + "Note: skipping legacy daemon sweep: cannot trust {}: {e}", + legacy_pid_path.display() + ); + None + } + }; + if let Some(pid_str) = pid_str { + match ( + parse_pid_file_contents(&pid_str), + probe_legacy_daemon().await, + ) { (Some(pid), Ok(true)) => { // A daemon answered on the legacy socket, so the PID // file is live — not a recycled PID from a dead run. @@ -1595,7 +1668,10 @@ mod tests { assert_eq!(parse_pid_file_contents("12345"), Some(12345)); assert_eq!(parse_pid_file_contents(" 12345\n"), Some(12345)); assert_eq!(parse_pid_file_contents("1"), Some(1)); - assert_eq!(parse_pid_file_contents(&i32::MAX.to_string()), Some(i32::MAX)); + assert_eq!( + parse_pid_file_contents(&i32::MAX.to_string()), + Some(i32::MAX) + ); } #[cfg(unix)] @@ -1609,7 +1685,10 @@ mod tests { #[test] fn test_parse_pid_file_contents_rejects_pid_t_overflow() { // Would wrap negative through `as libc::pid_t` and signal group -pid. - assert_eq!(parse_pid_file_contents(&(i32::MAX as u32 + 1).to_string()), None); + assert_eq!( + parse_pid_file_contents(&(i32::MAX as u32 + 1).to_string()), + None + ); assert_eq!(parse_pid_file_contents(&u32::MAX.to_string()), None); assert_eq!(parse_pid_file_contents(&u64::MAX.to_string()), None); } diff --git a/src/protocol.rs b/src/protocol.rs index c206a0f..3f4f818 100644 --- a/src/protocol.rs +++ b/src/protocol.rs @@ -52,15 +52,15 @@ pub struct DaemonResponse { /// Windows (%TEMP%), but shared /tmp on Linux — a fixed name there lets users /// collide on (or squat) each other's daemon files. #[cfg(unix)] -fn user_suffix() -> String { +fn user_suffix() -> std::borrow::Cow<'static, str> { // SAFETY: getuid() is a pure kernel query with no preconditions; it is // thread-safe and cannot fail. - format!("-{}", unsafe { libc::getuid() }) + std::borrow::Cow::Owned(format!("-{}", unsafe { libc::getuid() })) } #[cfg(windows)] -fn user_suffix() -> String { - String::new() +fn user_suffix() -> std::borrow::Cow<'static, str> { + std::borrow::Cow::Borrowed("") } /// Path to the Unix domain socket for daemon communication. From 48dba364702b100e30240ae37adacb8db453e3ab Mon Sep 17 00:00:00 2001 From: Aero Date: Thu, 30 Jul 2026 22:05:50 +0800 Subject: [PATCH 12/15] fix: reject oversized PID files, add read context, derive lock deadline - read_pid_file_checked returns anyhow::Result with path-aware context on open/metadata/read; callers recover the absence case by downcasting the chain via the new pid_file_missing() helper instead of matching io kinds. - Reject PID files larger than 64 bytes instead of silently truncating: the reader takes one byte past the cap so an oversized file can be detected, since a truncated prefix can still parse as a PID that reaches kill(). - Derive the daemon startup lock wait from the client's own budget (min(DAEMON_WAIT_TIMEOUT_SECS / 2, 2s)) so it stays strictly shorter than every accepted value, not just the 5s default. A daemon that spent the whole budget on the lock would bind exactly as wait_for_daemon gave up. - SKILL.md headless recipe: wait for CHROME_PID to exit before removing the profile, bounded at 5s with SIGKILL escalation, so cleanup can neither race Chrome's async teardown nor hang on a Chrome that ignores SIGTERM. Co-Authored-By: Claude Opus 5 --- skill/chrome-devtools/SKILL.md | 12 +++ src/client.rs | 7 +- src/daemon.rs | 56 +++++++++++--- src/lib.rs | 131 ++++++++++++++++++++++++++------- 4 files changed, 168 insertions(+), 38 deletions(-) diff --git a/skill/chrome-devtools/SKILL.md b/skill/chrome-devtools/SKILL.md index cf01f5b..a6d52bb 100644 --- a/skill/chrome-devtools/SKILL.md +++ b/skill/chrome-devtools/SKILL.md @@ -395,6 +395,18 @@ CHROME_PID=$! cleanup() { chrome-devtools kill-daemon --force kill "$CHROME_PID" 2>/dev/null + # Chrome shuts down asynchronously, so deleting the profile right after + # SIGTERM races its teardown and can leave it running against a directory + # that no longer exists. Wait for the process to actually go — but bounded + # (5s), then SIGKILL, so a Chrome that ignores SIGTERM can't hang the trap. + for _ in $(seq 1 20); do + kill -0 "$CHROME_PID" 2>/dev/null || break + sleep 0.25 + done + if kill -0 "$CHROME_PID" 2>/dev/null; then + kill -9 "$CHROME_PID" 2>/dev/null + fi + wait "$CHROME_PID" 2>/dev/null rm -rf "$PROFILE" } trap cleanup EXIT diff --git a/src/client.rs b/src/client.rs index d8e80d5..aa812b3 100644 --- a/src/client.rs +++ b/src/client.rs @@ -20,8 +20,11 @@ async fn connect_daemon() -> Result { Ok(TcpStream::connect(addr.trim()).await?) } -/// Read the daemon wait timeout from `DAEMON_WAIT_TIMEOUT_SECS`, defaulting to 5. -fn daemon_wait_timeout() -> Duration { +/// Read the daemon wait timeout from `DAEMON_WAIT_TIMEOUT_SECS`, defaulting to +/// 5. This is the whole budget a spawned daemon has to become reachable, so the +/// daemon derives its own startup lock wait from it (`daemon::lock_wait_timeout`) +/// — the spawned process inherits this environment. +pub(crate) fn daemon_wait_timeout() -> Duration { std::env::var("DAEMON_WAIT_TIMEOUT_SECS") .ok() .and_then(|v| v.parse::().ok()) diff --git a/src/daemon.rs b/src/daemon.rs index e236086..cf25f74 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -76,14 +76,32 @@ fn open_lock_file_at(path: &std::path::Path) -> Result { Ok(f) } -/// How long startup will wait for the daemon-file lock. Legitimate holders -/// (a predecessor's cleanup, another daemon's startup) finish in -/// milliseconds; anything longer means the lock is wedged or squatted. -/// Must stay well below the CLI's `DAEMON_WAIT_TIMEOUT_SECS` (default 5s, -/// `client.rs`): if the two were equal, a daemon that spent its whole budget -/// waiting here would bind at the exact moment `wait_for_daemon` gives up, -/// and the CLI would fall back to direct execution despite a usable daemon. -const LOCK_WAIT_TIMEOUT: Duration = Duration::from_secs(2); +/// Ceiling on the lock wait. Legitimate holders (a predecessor's cleanup, +/// another daemon's startup) finish in milliseconds; anything longer means the +/// lock is wedged or squatted, so there is no reason to wait past this even +/// when the client is willing to. +const LOCK_WAIT_CEILING: Duration = Duration::from_secs(2); + +/// How long startup will wait for the daemon-file lock, given the CLI's own +/// wait budget. Half the budget, capped at [`LOCK_WAIT_CEILING`]: the lock +/// wait must stay strictly shorter than the client's deadline, or a daemon +/// that spent the whole budget waiting here would bind at the exact moment +/// `wait_for_daemon` gives up, and the CLI would fall back to direct execution +/// despite a usable daemon. Halving (rather than subtracting a fixed margin) +/// keeps that true for every value `DAEMON_WAIT_TIMEOUT_SECS` accepts, and +/// leaves the other half for the pid write and bind that follow. +/// +/// Kept pure so the relationship is unit-testable without setting env vars. +fn derive_lock_wait_timeout(client_timeout: Duration) -> Duration { + (client_timeout / 2).min(LOCK_WAIT_CEILING) +} + +/// The lock wait for this process. The daemon is spawned by the CLI and +/// inherits its environment, so both sides read the same +/// `DAEMON_WAIT_TIMEOUT_SECS` (default 5s, see `client.rs`). +fn lock_wait_timeout() -> Duration { + derive_lock_wait_timeout(crate::client::daemon_wait_timeout()) +} /// Acquire the cross-process lock serializing the daemon-file critical /// sections: startup's pid-write/rebind and cleanup's check-then-remove. @@ -98,14 +116,15 @@ const LOCK_WAIT_TIMEOUT: Duration = Duration::from_secs(2); /// runtime's worker thread. async fn lock_daemon_files() -> Result { let f = open_lock_file()?; - let deadline = tokio::time::Instant::now() + LOCK_WAIT_TIMEOUT; + let timeout = lock_wait_timeout(); + let deadline = tokio::time::Instant::now() + timeout; loop { match f.try_lock() { Ok(()) => return Ok(f), Err(std::fs::TryLockError::WouldBlock) => { if tokio::time::Instant::now() >= deadline { anyhow::bail!( - "Timed out after {LOCK_WAIT_TIMEOUT:?} waiting for daemon lock file {} (held by another process)", + "Timed out after {timeout:?} waiting for daemon lock file {} (held by another process)", lock_path().display() ); } @@ -473,6 +492,23 @@ async fn handle_request(client: &mut CdpClient, req: &DaemonRequest) -> DaemonRe mod tests { use super::*; + #[test] + fn test_lock_wait_stays_shorter_than_every_client_deadline() { + // Any value DAEMON_WAIT_TIMEOUT_SECS accepts, not just the default: + // an equal deadline lets the daemon bind exactly as the CLI gives up. + for secs in [1_u64, 2, 3, 5, 10, 60, 3600] { + let client = Duration::from_secs(secs); + let lock = derive_lock_wait_timeout(client); + assert!( + lock < client, + "lock wait {lock:?} must be shorter than client deadline {client:?}" + ); + assert!(lock <= LOCK_WAIT_CEILING); + } + // A zero budget can't be beaten, only matched: one try_lock, no wait. + assert_eq!(derive_lock_wait_timeout(Duration::ZERO), Duration::ZERO); + } + #[cfg(unix)] #[test] fn test_open_lock_file_at_rejects_symlink() { diff --git a/src/lib.rs b/src/lib.rs index f647e9f..5c3d41f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -503,36 +503,69 @@ fn parse_pid_file_contents(s: &str) -> Option { .and_then(|p| i32::try_from(p).ok()) } +/// Largest PID file we will trust. A PID is at most a few bytes plus +/// surrounding whitespace, so anything bigger is hostile by definition. +#[cfg(unix)] +const MAX_PID_FILE_LEN: u64 = 64; + +/// Whether a PID-file read failed only because the file isn't there. That's +/// the ordinary "no daemon running" signal, which callers handle rather than +/// report, so it has to stay distinguishable from every other failure — the +/// reader adds `anyhow` context, so recover the kind from the error chain. +fn pid_file_missing(e: &anyhow::Error) -> bool { + e.downcast_ref::() + .is_some_and(|io| io.kind() == std::io::ErrorKind::NotFound) +} + /// Read a PID file without trusting the path it lives at. PID-file names are /// predictable and, on Linux, live in shared /tmp — the same threat model as /// the daemon lock file — so refuse to follow a planted symlink (O_NOFOLLOW) /// and only accept a regular file owned by this uid. O_NONBLOCK stops a /// planted FIFO from wedging the open() itself (it has no effect on regular -/// files); the `is_file` check then rejects the FIFO. The read is capped -/// because a PID is at most a few bytes — a large file at this path is -/// hostile by definition and must not be slurped into memory. +/// files); the `is_file` check then rejects the FIFO. Oversized files are +/// rejected outright rather than truncated: the parsed PID goes to `kill`, so +/// a hostile file must not be able to smuggle one through in a valid-looking +/// prefix. +/// +/// Callers distinguish "no such file" — the common, silent case — with +/// [`pid_file_missing`]; every other error carries path context and is worth +/// reporting. #[cfg(unix)] -fn read_pid_file_checked(path: &std::path::Path) -> std::io::Result { +fn read_pid_file_checked(path: &std::path::Path) -> Result { + use anyhow::Context as _; use std::io::Read; use std::os::unix::fs::{MetadataExt, OpenOptionsExt}; let f = std::fs::OpenOptions::new() .read(true) .custom_flags(libc::O_NOFOLLOW | libc::O_NONBLOCK) - .open(path)?; - let md = f.metadata()?; + .open(path) + .with_context(|| format!("Failed to open PID file {}", path.display()))?; + let md = f + .metadata() + .with_context(|| format!("Failed to read metadata of PID file {}", path.display()))?; // SAFETY: getuid() is a pure kernel query with no preconditions; it is // thread-safe and cannot fail. if !md.is_file() || md.uid() != unsafe { libc::getuid() } { - return Err(std::io::Error::new( - std::io::ErrorKind::InvalidData, - format!( - "{} is not a regular file owned by the current user; refusing to trust it", - path.display() - ), - )); + anyhow::bail!( + "{} is not a regular file owned by the current user; refusing to trust it", + path.display() + ); } let mut s = String::new(); - f.take(64).read_to_string(&mut s)?; + // Read one byte past the cap: stopping *at* the cap can't tell a file that + // fits from one that was truncated, and a truncated prefix ("123\n" plus + // 60 spaces, then megabytes of anything) still parses as a usable PID. + // Seeing the extra byte is what makes rejection possible. The cap is also + // what keeps a large file at this path out of memory. + f.take(MAX_PID_FILE_LEN + 1) + .read_to_string(&mut s) + .with_context(|| format!("Failed to read PID file {}", path.display()))?; + if s.len() as u64 > MAX_PID_FILE_LEN { + anyhow::bail!( + "PID file {} is larger than {MAX_PID_FILE_LEN} bytes; refusing to trust it", + path.display() + ); + } Ok(s) } @@ -1039,7 +1072,11 @@ pub async fn run() -> Result<()> { #[cfg(unix)] let read_result = read_pid_file_checked(&pid_path); #[cfg(not(unix))] - let read_result = std::fs::read_to_string(&pid_path); + let read_result = { + use anyhow::Context as _; + std::fs::read_to_string(&pid_path) + .with_context(|| format!("Failed to read PID file {}", pid_path.display())) + }; match read_result { Ok(pid_str) => { #[cfg(unix)] @@ -1089,17 +1126,15 @@ pub async fn run() -> Result<()> { println!("kill-daemon is only supported on Unix systems."); } } - Err(e) if e.kind() == std::io::ErrorKind::NotFound => { + Err(e) if pid_file_missing(&e) => { println!("No daemon running (PID file not found)."); } Err(e) => { // Surface via the standard error path (uniform formatting, // telemetry flush, typed exit code) — matching the EPERM - // signal-failure case above rather than exiting directly. - return Err(anyhow::anyhow!( - "Failed to read PID file {}: {e}", - pid_path.display() - )); + // signal-failure case above rather than exiting directly. The + // error already names the failing operation and the path. + return Err(e); } } @@ -1116,12 +1151,10 @@ pub async fn run() -> Result<()> { let pid_str = match read_pid_file_checked(&legacy_pid_path) { Ok(s) => Some(s), // No legacy files — the common case; stay silent. - Err(e) if e.kind() == std::io::ErrorKind::NotFound => None, + Err(e) if pid_file_missing(&e) => None, Err(e) => { - println!( - "Note: skipping legacy daemon sweep: cannot trust {}: {e}", - legacy_pid_path.display() - ); + // The error chain already names the operation and the path. + println!("Note: skipping legacy daemon sweep: {e:#}"); None } }; @@ -1703,6 +1736,52 @@ mod tests { assert_eq!(parse_pid_file_contents(" \n"), None); } + #[cfg(unix)] + #[test] + fn test_read_pid_file_checked_rejects_oversized_file() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("oversized.pid"); + // A parseable PID padded with whitespace to fill the first 64 bytes, + // then more content. Truncating at the cap would yield "12345" plus + // whitespace — a PID that parses cleanly and would reach kill(). + let mut contents = "12345".to_string(); + contents.push_str(&" ".repeat(MAX_PID_FILE_LEN as usize - contents.len())); + assert_eq!(contents.len() as u64, MAX_PID_FILE_LEN); + assert_eq!(parse_pid_file_contents(&contents), Some(12345)); + contents.push_str("99999\n"); + std::fs::write(&path, &contents).unwrap(); + + let err = read_pid_file_checked(&path).unwrap_err(); + assert!(!pid_file_missing(&err)); + assert!( + format!("{err:#}").contains("larger than"), + "unexpected error: {err:#}" + ); + } + + #[cfg(unix)] + #[test] + fn test_read_pid_file_checked_accepts_file_at_size_limit() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("padded.pid"); + // Exactly at the cap must still be accepted — the extra byte the + // reader requests is only there to detect overflow. + let contents = format!("12345{}", " ".repeat(MAX_PID_FILE_LEN as usize - 5)); + std::fs::write(&path, &contents).unwrap(); + + let read = read_pid_file_checked(&path).unwrap(); + assert_eq!(read, contents); + assert_eq!(parse_pid_file_contents(&read), Some(12345)); + } + + #[cfg(unix)] + #[test] + fn test_read_pid_file_checked_missing_file_is_distinguishable() { + let dir = tempfile::tempdir().unwrap(); + let err = read_pid_file_checked(&dir.path().join("absent.pid")).unwrap_err(); + assert!(pid_file_missing(&err), "unexpected error: {err:#}"); + } + #[test] #[allow(clippy::approx_constant)] fn test_parse_args() { From 00926c80f593123f16420c82b7744455562335bc Mon Sep 17 00:00:00 2001 From: Aero Date: Thu, 30 Jul 2026 22:18:07 +0800 Subject: [PATCH 13/15] docs: document MSRV in README, note legacy probe degradation at payload The 1.89 floor (File::{lock, try_lock}) only lived in a Cargo.toml comment, so a `cargo install` on an older toolchain surprised users; there is no changelog, so the README's Installation section is where it belongs. Also move the "legacy daemons might stop answering malformed requests" caveat from the fn doc down to the probe payload itself, where the assumption is actually encoded. Co-Authored-By: Claude Opus 5 --- README.md | 8 ++++++++ src/lib.rs | 4 ++++ 2 files changed, 12 insertions(+) diff --git a/README.md b/README.md index 3233de7..6a0cf88 100644 --- a/README.md +++ b/README.md @@ -28,6 +28,14 @@ cargo build --release # Binary: ./target/release/chrome-devtools ``` +### Rust version + +Building from source — including `cargo install` — requires **Rust 1.89 or +newer**. The daemon serializes its startup and cleanup with +`std::fs::File::{lock, try_lock}`, stabilized in 1.89; `rust-version` in +`Cargo.toml` makes Cargo say so instead of failing with a type error. The +Homebrew bottle is prebuilt and carries no toolchain requirement. + ## Why this exists Inspired by [chrome-devtools-mcp](https://github.com/ChromeDevTools/chrome-devtools-mcp) — the official MCP server for Chrome DevTools. It works well, but MCP-based browser tools consume a lot of token context: every interaction sends and receives large protocol payloads through the MCP layer. diff --git a/src/lib.rs b/src/lib.rs index 5c3d41f..b33e5c7 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -622,6 +622,10 @@ async fn probe_legacy_daemon() -> Result { cred.uid() ); } + // Deliberately not a valid `DaemonRequest`, so no legacy daemon dials + // Chrome to answer it. If a future legacy binary ever drops malformed + // requests instead of replying, this read times out and degrades to + // the safe branch (Err — files left in place, nothing signaled). protocol::write_msg(&mut stream, b"\"probe\"") .await .with_context(|| format!("Failed to write probe to {}", sock.display()))?; From 7f8e85a549ba84ef7d226bcbc411a93935be6860 Mon Sep 17 00:00:00 2001 From: Aero Date: Thu, 30 Jul 2026 22:24:10 +0800 Subject: [PATCH 14/15] fix: harden the daemon PID-file write against /tmp symlink squatting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit std::fs::write opens O_CREAT|O_TRUNC and follows symlinks, so it was the one hostile-path hole the read side didn't cover: on Linux's shared /tmp another user could pre-create chrome-devtools-daemon-.pid as a symlink to any file the victim can write, and the first CLI invocation would truncate it and write a PID in. The startup lock is no defense — the squatter never takes it and it guards a different path. write_pid_file_checked mirrors open_lock_file_at: O_NOFOLLOW, O_NONBLOCK, mode 0o600, regular-file/owner-uid validation against the open fd, and the truncation deferred until after those checks pass. Tested for the symlink attack (target contents untouched), stale-content replacement, and mode. Also from review: - cleanup() reads the PID file through read_pid_file_checked instead of a bare read_to_string, so the trust rule lives in one place. - `biased;` in the accept loop's select! so a ready shutdown signal always beats a ready accept rather than being chosen at random. - Document that SIGQUIT/SIGHUP/SIGKILL skip cleanup by design, that kill-daemon returns on signal delivery rather than exit, and how to stop a daemon on Windows. Co-Authored-By: Claude Opus 5 --- README.md | 5 +- src/daemon.rs | 131 +++++++++++++++++++++++++++++++++++++++++++++++--- src/lib.rs | 2 +- 3 files changed, 128 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index 6a0cf88..eec27cd 100644 --- a/README.md +++ b/README.md @@ -229,10 +229,11 @@ Global `--block-url` and `--unblock-url` update the **active tab's** block list - **PID file**: `$TMPDIR/chrome-devtools-daemon-.pid` (Windows: `%TEMP%\chrome-devtools-daemon.pid`) - **Lock file**: `$TMPDIR/chrome-devtools-daemon-.lock` (Windows: `%TEMP%\chrome-devtools-daemon.lock`) — serializes daemon startup/cleanup; intentionally never removed automatically. Locks bind to the inode, not the name: deleting the file while any daemon process is still starting, running, or shutting down lets a new process lock a fresh replacement inode and bypass the serialization entirely. Only delete it once no daemon process exists at all — and there's rarely a reason to, since a leftover lock file is harmless. - **Idle timeout**: 5 minutes (auto-exits, cleans up its files) -- **Cleanup**: endpoint + PID files are also removed on panics, and on Unix on SIGTERM/SIGINT; Windows Ctrl-C cleanup is best-effort only (a background daemon has no console to receive it) +- **Cleanup**: endpoint + PID files are also removed on panics, and on Unix on SIGTERM/SIGINT; Windows Ctrl-C cleanup is best-effort only (a background daemon has no console to receive it). SIGQUIT, SIGHUP and SIGKILL skip cleanup by design — the leftover files are harmless and are reclaimed by the next daemon start. - **Protocol**: Length-prefixed JSON over the Unix socket / loopback TCP - **Spawned by**: First CLI invocation (transparent to user) -- **Kill**: `chrome-devtools kill-daemon` (or delete the socket + PID file; leave the lock file — see above) +- **Kill**: `chrome-devtools kill-daemon` (or delete the socket + PID file; leave the lock file — see above). It sends SIGTERM and returns once the signal is delivered, not once the process is gone: the daemon exits *between* requests, so one that is mid-command finishes it and answers that client first. Expect up to one command's latency, and note that a daemon wedged inside a CDP call outlives the command that stopped it. +- **Kill (Windows)**: not supported — `kill-daemon` says so and exits, and a backgrounded daemon has no console for Ctrl-C. Use `taskkill /PID ` with the PID from `%TEMP%\chrome-devtools-daemon.pid`, or wait out the idle timeout. The daemon keeps a persistent CDP session on the current page to: - Continuously collect `Network.*` and `Runtime.consoleAPICalled`/`exceptionThrown` events for `console` and `network` drains. diff --git a/src/daemon.rs b/src/daemon.rs index cf25f74..adf15d0 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -76,6 +76,65 @@ fn open_lock_file_at(path: &std::path::Path) -> Result { Ok(f) } +/// Write this process's PID to `path`, without trusting the path it lives at. +/// +/// `std::fs::write` opens with `O_CREAT | O_TRUNC` and **follows symlinks**, +/// which is the one hostile-path hole the read side doesn't cover: on Linux's +/// shared /tmp, another user can pre-create +/// `chrome-devtools-daemon-.pid` as a symlink to any file the +/// victim can write, and the first `chrome-devtools` invocation would then +/// truncate that file and write a PID into it. The startup lock is no defense +/// — the squatter never takes it, and it guards a different path. +/// +/// So this mirrors [`open_lock_file_at`]: O_NOFOLLOW refuses the symlink, +/// O_NONBLOCK keeps a planted FIFO from wedging the open (the `is_file` check +/// then rejects it), and mode 0o600 creates it unreadable by others. The +/// truncation is deliberately deferred until after the checks pass, and the +/// checks run against the already-open fd rather than the path, so there is no +/// window to swap the file in between. A hardlink to someone else's file would +/// pass the uid check, but planting one requires owning the target under +/// Linux's default `protected_hardlinks`. +#[cfg(unix)] +fn write_pid_file_checked(path: &std::path::Path) -> Result<()> { + use std::io::Write; + use std::os::unix::fs::{MetadataExt, OpenOptionsExt}; + let mut f = std::fs::OpenOptions::new() + .write(true) + .create(true) + // Not O_TRUNC: truncating is destructive, so it waits for the checks. + .truncate(false) + .custom_flags(libc::O_NOFOLLOW | libc::O_NONBLOCK) + .mode(0o600) + .open(path) + .with_context(|| format!("Failed to open daemon PID file {}", path.display()))?; + let md = f.metadata().with_context(|| { + format!( + "Failed to read metadata of daemon PID file {}", + path.display() + ) + })?; + // SAFETY: getuid() is a pure kernel query with no preconditions; it is + // thread-safe and cannot fail. + if !md.is_file() || md.uid() != unsafe { libc::getuid() } { + anyhow::bail!( + "Daemon PID path {} is not a regular file owned by the current user; refusing to write it", + path.display() + ); + } + f.set_len(0) + .with_context(|| format!("Failed to truncate daemon PID file {}", path.display()))?; + f.write_all(std::process::id().to_string().as_bytes()) + .with_context(|| format!("Failed to write daemon PID file {}", path.display())) +} + +/// Windows has no O_NOFOLLOW, and `%TEMP%` is already per-user, so the +/// shared-/tmp squatting the Unix version defends against doesn't apply. +#[cfg(not(unix))] +fn write_pid_file_checked(path: &std::path::Path) -> Result<()> { + std::fs::write(path, std::process::id().to_string()) + .with_context(|| format!("Failed to write daemon PID file {}", path.display())) +} + /// Ceiling on the lock wait. Legitimate holders (a predecessor's cleanup, /// another daemon's startup) finish in milliseconds; anything longer means the /// lock is wedged or squatted, so there is no reason to wait past this even @@ -193,10 +252,16 @@ fn cleanup_at(lock: &std::path::Path, pid: &std::path::Path, endpoint: &std::pat return; } } - let owns_files = std::fs::read_to_string(pid) - .ok() - .and_then(|s| s.trim().parse::().ok()) - == Some(std::process::id()); + // Same trust rule as `kill-daemon`'s read, in one place: a PID file is + // only believable if it's a regular file we own and small enough to be a + // PID. Any failure means "not provably ours", which leaves the files in + // place — the safe side, and where an unreadable file already landed. + #[cfg(unix)] + let contents = crate::read_pid_file_checked(pid).ok(); + #[cfg(not(unix))] + let contents = std::fs::read_to_string(pid).ok(); + let owns_files = + contents.and_then(|s| s.trim().parse::().ok()) == Some(std::process::id()); if !owns_files { return; } @@ -226,10 +291,16 @@ macro_rules! run_accept_loop_body { ($accept:expr, $client:expr, $ws_url:expr, $shutdown:expr) => { loop { tokio::select! { + // Shutdown first, and `biased` so a ready signal always wins + // over a ready accept instead of being picked at random — + // otherwise a steady stream of requests can defer the exit by + // an iteration at a time. + biased; _ = &mut $shutdown => { // SIGTERM/SIGINT (or Ctrl-C on Windows) — exit cleanly. // Only observed between requests: an in-flight command - // finishes and its response is written before shutdown. + // finishes and its response is written before shutdown, so + // a daemon busy with a slow CDP call exits late, not now. break; } accept = tokio::time::timeout(idle_timeout(), $accept) => match accept { @@ -254,7 +325,10 @@ macro_rules! run_accept_loop_body { /// default disposition kills the process without unwinding, so `CleanupGuard` /// would never run and the socket/PID files would go stale. SIGHUP is /// deliberately left unhandled — convention reserves it for config reload, -/// not shutdown. +/// not shutdown. SIGQUIT is left unhandled too, for the opposite reason: it +/// means "stop now and dump core", so honoring it as a graceful exit would +/// defeat its purpose. Both therefore skip cleanup, as does SIGKILL; the +/// leftover files are harmless and self-heal on the next daemon start. #[cfg(unix)] fn shutdown_signal() -> impl std::future::Future { use tokio::signal::unix::{signal, SignalKind}; @@ -327,7 +401,7 @@ pub async fn run_daemon(ws_url: &str) -> Result<()> { // predecessor can delete files this daemon just claimed. let startup_lock = lock_daemon_files().await?; - std::fs::write(pid_path(), std::process::id().to_string())?; + write_pid_file_checked(&pid_path())?; #[cfg(unix)] let listener = { @@ -524,6 +598,49 @@ mod tests { assert!(link.symlink_metadata().unwrap().file_type().is_symlink()); } + #[cfg(unix)] + #[test] + fn test_write_pid_file_checked_refuses_symlink_without_touching_target() { + let dir = tempfile::tempdir().unwrap(); + let victim = dir.path().join("bashrc"); + std::fs::write(&victim, "precious\n").unwrap(); + let planted = dir.path().join("daemon.pid"); + std::os::unix::fs::symlink(&victim, &planted).unwrap(); + + // The squatting attack: writing through the symlink would truncate the + // victim's file and leave a PID in it. + assert!(write_pid_file_checked(&planted).is_err()); + assert_eq!(std::fs::read_to_string(&victim).unwrap(), "precious\n"); + assert!(planted.symlink_metadata().unwrap().file_type().is_symlink()); + } + + #[cfg(unix)] + #[test] + fn test_write_pid_file_checked_replaces_longer_stale_contents() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("daemon.pid"); + // A stale file from a previous daemon, longer than the new PID: the + // deferred truncation has to clear it rather than leave a tail. + std::fs::write(&path, "4294967295 stale trailing bytes\n").unwrap(); + + write_pid_file_checked(&path).unwrap(); + assert_eq!( + std::fs::read_to_string(&path).unwrap(), + std::process::id().to_string() + ); + } + + #[cfg(unix)] + #[test] + fn test_write_pid_file_checked_creates_private_file() { + use std::os::unix::fs::PermissionsExt; + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("daemon.pid"); + write_pid_file_checked(&path).unwrap(); + let mode = std::fs::metadata(&path).unwrap().permissions().mode(); + assert_eq!(mode & 0o777, 0o600, "unexpected mode {:o}", mode & 0o777); + } + #[cfg(unix)] #[test] fn test_open_lock_file_at_rejects_fifo() { diff --git a/src/lib.rs b/src/lib.rs index b33e5c7..878f8da 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -531,7 +531,7 @@ fn pid_file_missing(e: &anyhow::Error) -> bool { /// [`pid_file_missing`]; every other error carries path context and is worth /// reporting. #[cfg(unix)] -fn read_pid_file_checked(path: &std::path::Path) -> Result { +pub(crate) fn read_pid_file_checked(path: &std::path::Path) -> Result { use anyhow::Context as _; use std::io::Read; use std::os::unix::fs::{MetadataExt, OpenOptionsExt}; From 93adb70e460d39eb292751c2aa7cce2facba4564 Mon Sep 17 00:00:00 2001 From: Aero Date: Thu, 30 Jul 2026 22:41:28 +0800 Subject: [PATCH 15/15] fix: key daemon file identity to the effective uid, not the real uid MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The kernel stamps a new file with the creating process's effective uid, so the ownership checks had to compare against geteuid(): under a setuid binary or a credential-transitioning launcher (uid != euid), the daemon would create its PID/lock file and then immediately reject it as another user's, failing startup and skipping cleanup. The per-user filename suffix moves to geteuid() for the same reason — the identity the paths are keyed to must be the identity the checks enforce, or the files land at a path derived from one uid and get judged against another. Where uid == euid (all ordinary runs) the paths are byte-identical, so this is not a compatibility break. Also covers the legacy-socket peer check, where both sides were already effective uids: peer_cred reports the peer's euid. Co-Authored-By: Claude Opus 5 --- src/daemon.rs | 8 ++++---- src/lib.rs | 10 ++++++---- src/protocol.rs | 11 +++++++++-- 3 files changed, 19 insertions(+), 10 deletions(-) diff --git a/src/daemon.rs b/src/daemon.rs index adf15d0..2d0b1f8 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -64,9 +64,9 @@ fn open_lock_file_at(path: &std::path::Path) -> Result { path.display() ) })?; - // SAFETY: getuid() is a pure kernel query with no preconditions; it + // SAFETY: geteuid() is a pure kernel query with no preconditions; it // is thread-safe and cannot fail. - if !md.is_file() || md.uid() != unsafe { libc::getuid() } { + if !md.is_file() || md.uid() != unsafe { libc::geteuid() } { anyhow::bail!( "Daemon lock path {} is not a regular file owned by the current user; refusing to lock it", path.display() @@ -113,9 +113,9 @@ fn write_pid_file_checked(path: &std::path::Path) -> Result<()> { path.display() ) })?; - // SAFETY: getuid() is a pure kernel query with no preconditions; it is + // SAFETY: geteuid() is a pure kernel query with no preconditions; it is // thread-safe and cannot fail. - if !md.is_file() || md.uid() != unsafe { libc::getuid() } { + if !md.is_file() || md.uid() != unsafe { libc::geteuid() } { anyhow::bail!( "Daemon PID path {} is not a regular file owned by the current user; refusing to write it", path.display() diff --git a/src/lib.rs b/src/lib.rs index 878f8da..10d63cd 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -543,9 +543,9 @@ pub(crate) fn read_pid_file_checked(path: &std::path::Path) -> Result { let md = f .metadata() .with_context(|| format!("Failed to read metadata of PID file {}", path.display()))?; - // SAFETY: getuid() is a pure kernel query with no preconditions; it is + // SAFETY: geteuid() is a pure kernel query with no preconditions; it is // thread-safe and cannot fail. - if !md.is_file() || md.uid() != unsafe { libc::getuid() } { + if !md.is_file() || md.uid() != unsafe { libc::geteuid() } { anyhow::bail!( "{} is not a regular file owned by the current user; refusing to trust it", path.display() @@ -610,12 +610,14 @@ async fn probe_legacy_daemon() -> Result { // was started by this user, so a peer running as any other uid is a // squatter. Err (not Ok(false)) so the caller lands in the // leave-everything-alone branch rather than the provably-stale one. + // Both sides of the comparison are effective uids — `peer_cred` reports + // the peer's euid — matching the identity the file checks use. let cred = stream .peer_cred() .with_context(|| format!("Failed to read peer credentials of {}", sock.display()))?; - // SAFETY: getuid() is a pure kernel query with no preconditions; it + // SAFETY: geteuid() is a pure kernel query with no preconditions; it // is thread-safe and cannot fail. - if cred.uid() != unsafe { libc::getuid() } { + if cred.uid() != unsafe { libc::geteuid() } { anyhow::bail!( "Listener on {} runs as uid {}, not the current user; refusing to treat it as the legacy daemon", sock.display(), diff --git a/src/protocol.rs b/src/protocol.rs index 3f4f818..9bf4a6e 100644 --- a/src/protocol.rs +++ b/src/protocol.rs @@ -51,11 +51,18 @@ pub struct DaemonResponse { /// Per-user filename suffix. `temp_dir()` is per-user on macOS ($TMPDIR) and /// Windows (%TEMP%), but shared /tmp on Linux — a fixed name there lets users /// collide on (or squat) each other's daemon files. +/// +/// Effective uid, not real: the kernel stamps a new file with the creating +/// process's *effective* uid, and that is what the daemon's ownership checks +/// compare against. Keying the name to the real uid instead would make the two +/// disagree wherever they differ (a setuid binary, a credential-transitioning +/// launcher): the daemon would create its files at a path derived from one +/// identity and then refuse them as belonging to another user. #[cfg(unix)] fn user_suffix() -> std::borrow::Cow<'static, str> { - // SAFETY: getuid() is a pure kernel query with no preconditions; it is + // SAFETY: geteuid() is a pure kernel query with no preconditions; it is // thread-safe and cannot fail. - std::borrow::Cow::Owned(format!("-{}", unsafe { libc::getuid() })) + std::borrow::Cow::Owned(format!("-{}", unsafe { libc::geteuid() })) } #[cfg(windows)]