diff --git a/AGENTS.md b/AGENTS.md index 5e40418..6551ead 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -62,9 +62,13 @@ 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 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/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 ec7ea2d..eec27cd 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. @@ -41,7 +49,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 /tmp/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,12 +224,16 @@ 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` -- **Idle timeout**: 5 minutes (auto-exits, cleans up socket) -- **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`) +- **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). 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) +- **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/skill/chrome-devtools/SKILL.md b/skill/chrome-devtools/SKILL.md index 3b850ea..a6d52bb 100644 --- a/skill/chrome-devtools/SKILL.md +++ b/skill/chrome-devtools/SKILL.md @@ -367,6 +367,78 @@ 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. 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 + # 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 + +# 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. +# 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 +``` + +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 +(step 1 and the EXIT trap above). + ## Complete Command Reference ### Navigation 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 fd56b75..2d0b1f8 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,31 +29,379 @@ 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. 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 { + 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 | libc::O_NONBLOCK); + opts.mode(0o600); + } + 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().with_context(|| { + format!( + "Failed to read metadata of daemon lock file {}", + path.display() + ) + })?; + // 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::geteuid() } { + anyhow::bail!( + "Daemon lock path {} is not a regular file owned by the current user; refusing to lock it", + path.display() + ); + } + } + 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: geteuid() is a pure kernel query with no preconditions; it is + // thread-safe and cannot fail. + 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() + ); + } + 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 +/// 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. +/// 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. The +/// wait is bounded because an indefinite `lock()` would let any process that +/// pre-acquired the predictable lock path park daemon startup forever. +/// 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 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 {timeout:?} waiting for daemon lock file {} (held by another process)", + lock_path().display() + ); + } + tokio::time::sleep(Duration::from_millis(50)).await; + } + 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. +/// +/// 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. 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). +/// +/// 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() { + #[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_at's error already names the operation and path. + eprintln!("daemon: leaving socket/PID files in place: {e:#}"); + 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.display() + ); + return; + } + } + // 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; + } + 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 +/// 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(); + } +} + +/// 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) => { + ($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! { + // 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, so + // a daemon busy with a slow CDP call exits late, not now. 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; + } + } } } }; } +/// 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. 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}; + // 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, + } + } +} + +/// 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)] +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 - std::fs::write(pid_path(), std::process::id().to_string())?; + // 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; + + // 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. + let startup_lock = lock_daemon_files().await?; + + write_pid_file_checked(&pid_path())?; #[cfg(unix)] let listener = { @@ -80,21 +428,17 @@ 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. 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()); - - #[cfg(windows)] - let _ = std::fs::remove_file(addr_path()); + 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. @@ -217,3 +561,159 @@ async fn handle_request(client: &mut CdpClient, req: &DaemonRequest) -> DaemonRe } } } + +#[cfg(test)] +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() { + 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_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() { + 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 cef203b..10d63cd 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -485,6 +485,162 @@ 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. +/// +/// 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() + .parse::() + .ok() + .filter(|&p| p != 0) + .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. 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)] +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}; + let f = std::fs::OpenOptions::new() + .read(true) + .custom_flags(libc::O_NOFOLLOW | libc::O_NONBLOCK) + .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: geteuid() is a pure kernel query with no preconditions; it is + // thread-safe and cannot fail. + 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() + ); + } + let mut s = String::new(); + // 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) +} + +/// 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. 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 +/// 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() -> 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())); + } + }; + // 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. + // 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: geteuid() is a pure kernel query with no preconditions; it + // is thread-safe and cannot fail. + 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(), + 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()))?; + protocol::read_msg(&mut stream) + .await + .with_context(|| format!("Failed to read probe response from {}", sock.display()))?; + Ok(true) + }; + 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 { if force { KillDaemonDecision::Proceed @@ -917,30 +1073,27 @@ 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 = { + 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) => { - 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() ) })?; @@ -948,7 +1101,10 @@ 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) }; + // 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. let _ = std::fs::remove_file(&sock_path); @@ -972,21 +1128,116 @@ pub async fn run() -> Result<()> { } #[cfg(not(unix))] { - let _ = pid; + let _ = pid_str; 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); + } + } + + // 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(); + // 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 pid_file_missing(&e) => None, + Err(e) => { + // The error chain already names the operation and the path. + println!("Note: skipping legacy daemon sweep: {e:#}"); + 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. + // 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) { + // 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 { + // 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." + ); + } + } + (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| { + // 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) + }); + 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 + // 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() + ); + } + (_, 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() + ); + } + } } } return Ok(()); @@ -1450,6 +1701,93 @@ 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); + } + + #[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() { diff --git a/src/protocol.rs b/src/protocol.rs index 293865f..9bf4a6e 100644 --- a/src/protocol.rs +++ b/src/protocol.rs @@ -48,23 +48,66 @@ 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. +/// +/// 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: geteuid() is a pure kernel query with no preconditions; it is + // thread-safe and cannot fail. + std::borrow::Cow::Owned(format!("-{}", unsafe { libc::geteuid() })) +} + +#[cfg(windows)] +fn user_suffix() -> std::borrow::Cow<'static, str> { + std::borrow::Cow::Borrowed("") +} + /// 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(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();