From 3821e93f53d8777825e9ff8d68737a70a3f543c0 Mon Sep 17 00:00:00 2001 From: jrimmer Date: Wed, 12 Aug 2026 11:47:50 -0700 Subject: [PATCH 01/11] fix(controller): kill orphaned Firecracker processes on startup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue #298: when the controller crashes or restarts while Firecracker processes are still alive, the restarted controller cannot reconstruct resource ownership for orphaned VMs. Both the NetnsAllocator (active set) and shared_tap_owner are initialized empty, so a new spawn can reserve an orphaned VM's netns index or claim the shared tap even though the orphan still owns it. Fix: add `Registry::kill_orphans()` called after `reconcile()` on startup. After reconcile() prunes dead-PID entries, any remaining sandbox entries have alive PIDs but no live_vms handle — they are unmanageable orphans. kill_orphans() kills each orphan (after verifying the PID belongs to a Firecracker process via /proc//comm to guard against PID reuse), prunes the registry entry, and marks workspaces whose sandbox was killed as Stale. After this, the NetnsAllocator (empty active set) and shared_tap_owner (None) are safe: no orphaned VM holds a netns index or the shared tap. Closes #298 Signed-off-by: jrimmer --- crates/forkd-controller/src/lib.rs | 11 ++ crates/forkd-controller/src/state.rs | 223 +++++++++++++++++++++++++++ 2 files changed, 234 insertions(+) diff --git a/crates/forkd-controller/src/lib.rs b/crates/forkd-controller/src/lib.rs index f07fbf2..df9029d 100644 --- a/crates/forkd-controller/src/lib.rs +++ b/crates/forkd-controller/src/lib.rs @@ -91,6 +91,17 @@ pub async fn run_daemon(cfg: DaemonConfig) -> Result<()> { tracing::info!(pruned, "reconciled stale sandbox entries on startup"); } + // Kill orphaned Firecracker processes left over from a previous + // controller instance (issue #298). After reconcile() prunes + // dead-PID entries, any remaining entries have alive PIDs but no + // live_vms handle — they are unmanageable orphans. Kill them and + // prune the registry entries so the NetnsAllocator (empty active + // set) and shared_tap_owner (None) start clean. + let killed = registry.kill_orphans()?; + if killed > 0 { + tracing::info!(killed, "killed orphaned Firecracker processes on startup"); + } + let audit = AuditSink::open(&cfg.audit_log) .with_context(|| format!("open audit log {}", cfg.audit_log.display()))?; tracing::info!(audit_log = %audit.path().display(), "audit log open"); diff --git a/crates/forkd-controller/src/state.rs b/crates/forkd-controller/src/state.rs index a30a6a4..23c8818 100644 --- a/crates/forkd-controller/src/state.rs +++ b/crates/forkd-controller/src/state.rs @@ -267,6 +267,88 @@ impl Registry { Ok(pruned) } + /// Kill orphaned Firecracker processes on startup (issue #298). + /// + /// After `reconcile()` prunes entries with dead PIDs, any remaining + /// sandbox entries have alive PIDs — but the controller has no + /// `live_vms` handle for them (the controller restarted). These are + /// orphaned Firecracker processes: alive but unmanageable. + /// + /// This method kills each orphan (after verifying the PID still + /// belongs to a Firecracker process — guards against PID reuse), + /// prunes the registry entry, and marks any workspace whose + /// live_sandbox_id was killed as Stale. + /// + /// After this, the NetnsAllocator (empty active set) and + /// shared_tap_owner (None) are safe: no orphaned VM holds a + /// netns index or the shared tap. + pub fn kill_orphans(&self) -> Result { + let orphans: Vec<(String, u32)> = { + let g = self.inner.lock(); + g.sandboxes + .iter() + .filter_map(|(id, sb)| match sb.pid { + Some(pid) if pid_alive(pid) => Some((id.clone(), pid)), + _ => None, + }) + .collect() + }; + + let mut killed = 0usize; + for (id, pid) in orphans { + if pid_is_firecracker(pid) { + tracing::warn!( + sandbox_id = %id, + pid = pid, + "killing orphaned Firecracker process on startup" + ); + if let Err(e) = kill_pid(pid) { + tracing::error!( + sandbox_id = %id, + pid = pid, + error = %e, + "failed to kill orphaned Firecracker process; pruning registry entry anyway" + ); + } + } else { + tracing::warn!( + sandbox_id = %id, + pid = pid, + "PID no longer belongs to Firecracker (PID reuse); pruning stale registry entry" + ); + } + self.inner.lock().sandboxes.remove(&id); + killed += 1; + } + + // Re-run workspace stale marking: workspaces whose + // live_sandbox_id was just pruned are now orphaned. + let live_ids: std::collections::HashSet = + self.inner.lock().sandboxes.keys().cloned().collect(); + let mut stale_ws_changed = false; + { + let mut g = self.inner.lock(); + for ws in g.workspaces.values_mut() { + if ws.status == WorkspaceStatus::Running { + let live = ws + .live_sandbox_id + .as_ref() + .is_some_and(|id| live_ids.contains(id)); + if !live { + ws.status = WorkspaceStatus::Stale; + ws.live_sandbox_id = None; + stale_ws_changed = true; + } + } + } + } + + if killed > 0 || stale_ws_changed { + self.flush()?; + } + Ok(killed) + } + /// For metrics: live counts. pub fn counts(&self) -> (usize, usize) { let g = self.inner.lock(); @@ -286,6 +368,41 @@ fn pid_alive(_pid: u32) -> bool { true } +/// Verify that a PID belongs to a Firecracker process by reading +/// `/proc//comm`. Guards against PID reuse: if the original +/// Firecracker process died and the PID was recycled by the OS for +/// a different process, we don't want to kill an unrelated process. +#[cfg(target_os = "linux")] +fn pid_is_firecracker(pid: u32) -> bool { + std::fs::read_to_string(format!("/proc/{pid}/comm")) + .map(|s| s.trim().contains("firecracker")) + .unwrap_or(false) +} + +#[cfg(not(target_os = "linux"))] +fn pid_is_firecracker(_pid: u32) -> bool { + // Off-Linux: can't verify via /proc; return false so + // kill_orphans prunes the entry without sending a signal. + false +} + +/// Send SIGKILL to a process by PID. Uses libc::kill directly +/// (we don't have a std::process::Child handle for orphaned PIDs). +#[cfg(target_os = "linux")] +fn kill_pid(pid: u32) -> std::io::Result<()> { + let ret = unsafe { libc::kill(pid as i32, libc::SIGKILL) }; + if ret != 0 { + Err(std::io::Error::last_os_error()) + } else { + Ok(()) + } +} + +#[cfg(not(target_os = "linux"))] +fn kill_pid(_pid: u32) -> std::io::Result<()> { + Ok(()) +} + #[cfg(test)] mod tests { use super::*; @@ -451,5 +568,111 @@ mod tests { reloaded.list_sandboxes().len(), WORKERS * MUTATIONS_PER_WORKER ); + fn kill_orphans_prunes_alive_pid_entries() { + let td = TempDir::new().unwrap(); + let path = td.path().join("state.json"); + let r = Registry::load_or_init(&path).unwrap(); + + // Insert a sandbox with an alive PID (use our own process PID). + // On Linux, pid_is_firecracker will return false (we're not + // firecracker), so the entry is pruned without killing. + // On non-Linux, pid_is_firecracker always returns false. + r.insert_sandbox(SandboxInfo { + id: "sb-orphan".into(), + snapshot_tag: "py".into(), + netns: Some("forkd-child-1".into()), + guest_addr: "10.42.0.2:8888".into(), + created_at_unix: 1, + pid: Some(std::process::id()), + memory_limit_mib: None, + has_branched: false, + last_branch_memory_path: None, + branch_count: 0, + }) + .unwrap(); + + // Insert a sandbox with a dead PID (99999999 — not alive on Linux). + // On macOS, pid_alive always returns true, so this entry survives + // reconcile() and is pruned by kill_orphans() instead. + r.insert_sandbox(SandboxInfo { + id: "sb-dead".into(), + snapshot_tag: "py".into(), + netns: Some("forkd-child-2".into()), + guest_addr: "10.42.0.3:8888".into(), + created_at_unix: 2, + pid: Some(99999999), + memory_limit_mib: None, + has_branched: false, + last_branch_memory_path: None, + branch_count: 0, + }) + .unwrap(); + + // reconcile prunes dead-PID entries (1 on Linux, 0 on macOS + // where pid_alive always returns true). + let _pruned = r.reconcile().unwrap(); + + // kill_orphans prunes all remaining entries (alive PID but + // not firecracker → pruned without killing). + let killed = r.kill_orphans().unwrap(); + // At least the alive-PID entry is pruned. + assert!(killed >= 1); + // All sandbox entries are gone. + assert!(r.list_sandboxes().is_empty()); + } + + #[test] + fn kill_orphans_marks_workspaces_stale() { + let td = TempDir::new().unwrap(); + let path = td.path().join("state.json"); + let r = Registry::load_or_init(&path).unwrap(); + + // Insert a sandbox with an alive PID. + r.insert_sandbox(SandboxInfo { + id: "sb-1".into(), + snapshot_tag: "py".into(), + netns: Some("forkd-child-1".into()), + guest_addr: "10.42.0.2:8888".into(), + created_at_unix: 1, + pid: Some(std::process::id()), + memory_limit_mib: None, + has_branched: false, + last_branch_memory_path: None, + branch_count: 0, + }) + .unwrap(); + + // Insert a workspace that references this sandbox. + r.insert_workspace(WorkspaceInfo { + id: "ws-1".into(), + name: "ws-1".into(), + source_snapshot_tag: "py".into(), + current_state_tag: None, + status: WorkspaceStatus::Running, + live_sandbox_id: Some("sb-1".into()), + created_at_unix: 1, + last_active_unix: 1, + last_branch_memory_path: None, + per_child_netns: false, + }) + .unwrap(); + + // kill_orphans kills the sandbox and marks the workspace Stale. + let killed = r.kill_orphans().unwrap(); + assert_eq!(killed, 1); + assert!(r.list_sandboxes().is_empty()); + + let ws = r.get_workspace("ws-1").unwrap(); + assert_eq!(ws.status, WorkspaceStatus::Stale); + assert!(ws.live_sandbox_id.is_none()); + } + + #[test] + fn kill_orphans_no_op_on_empty_registry() { + let td = TempDir::new().unwrap(); + let path = td.path().join("state.json"); + let r = Registry::load_or_init(&path).unwrap(); + let killed = r.kill_orphans().unwrap(); + assert_eq!(killed, 0); } } From 092a28a4a0eb691790fca4baf14c63c1860c9750 Mon Sep 17 00:00:00 2001 From: jrimmer Date: Wed, 12 Aug 2026 11:56:11 -0700 Subject: [PATCH 02/11] fix(controller): address ce review findings for orphan recovery MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address all P0 and P1 findings from the 7-reviewer ce-* panel: P0: kill_pid failure no longer prunes the registry entry. ESRCH (process already dead from TOCTOU race) is distinguished from real errors (EPERM etc.). On ESRCH, the entry is safely pruned. On real errors, the entry is kept in the registry to prevent the exact resource collision issue P1 fixes: - Wait for process death after SIGKILL (5s bounded timeout polling /proc/ disappearance). A D-state process stuck on I/O can hold netns/tap resources past the kill return; if it doesn't exit within the timeout, the registry entry is kept. - pid_is_firecracker uses exact match (== "firecracker") instead of substring match to avoid false-positive kills of unrelated processes. - Separate KillOrphansResult struct (killed, pruned_stale, kill_failed) replaces the misleading single usize counter. The startup log now reports all three counts. - Extract mark_stale_workspaces() helper to eliminate the ~22-line duplicated workspace stale-marking block between reconcile() and kill_orphans(). - kill_orphans is pub(crate) with a doc invariant: "MUST only be called before any Vm handle is held" — prevents future post-startup misuse. - Defensive pid <= 1 guard in pid_is_firecracker (defense-in-depth against corrupted state.json). - ESRCH downgraded from error! to debug! (benign TOCTOU race). - SAFETY comment added on unsafe libc::kill. New tests: pid:None entries skipped, multiple workspaces same sandbox, persistence to disk after kill_orphans, reconcile+kill_orphans integration. Signed-off-by: jrimmer --- crates/forkd-controller/src/lib.rs | 17 +- crates/forkd-controller/src/state.rs | 363 +++++++++++++++++++++++---- 2 files changed, 330 insertions(+), 50 deletions(-) diff --git a/crates/forkd-controller/src/lib.rs b/crates/forkd-controller/src/lib.rs index df9029d..22c66a6 100644 --- a/crates/forkd-controller/src/lib.rs +++ b/crates/forkd-controller/src/lib.rs @@ -97,9 +97,20 @@ pub async fn run_daemon(cfg: DaemonConfig) -> Result<()> { // live_vms handle — they are unmanageable orphans. Kill them and // prune the registry entries so the NetnsAllocator (empty active // set) and shared_tap_owner (None) start clean. - let killed = registry.kill_orphans()?; - if killed > 0 { - tracing::info!(killed, "killed orphaned Firecracker processes on startup"); + let orphans = registry.kill_orphans()?; + if orphans.killed > 0 { + tracing::info!( + killed = orphans.killed, + pruned_stale = orphans.pruned_stale, + kill_failed = orphans.kill_failed, + "killed orphaned Firecracker processes on startup" + ); + } else if orphans.pruned_stale > 0 || orphans.kill_failed > 0 { + tracing::warn!( + pruned_stale = orphans.pruned_stale, + kill_failed = orphans.kill_failed, + "orphan recovery: some entries pruned as stale or kill failed" + ); } let audit = AuditSink::open(&cfg.audit_log) diff --git a/crates/forkd-controller/src/state.rs b/crates/forkd-controller/src/state.rs index 23c8818..68b9f04 100644 --- a/crates/forkd-controller/src/state.rs +++ b/crates/forkd-controller/src/state.rs @@ -267,22 +267,62 @@ impl Registry { Ok(pruned) } + /// Mark any `Running` workspace whose `live_sandbox_id` is no longer + /// present in `sandboxes` as `Stale`. We don't touch Suspended + /// workspaces; they were intentionally parked. + /// + /// Used by `kill_orphans` (after pruning orphaned processes) to keep + /// the workspace/liveness view consistent. Returns whether anything + /// changed. + fn mark_stale_workspaces(&self) -> bool { + let live_ids: std::collections::HashSet = + self.inner.lock().sandboxes.keys().cloned().collect(); + let mut changed = false; + { + let mut g = self.inner.lock(); + for ws in g.workspaces.values_mut() { + if ws.status == WorkspaceStatus::Running { + let live = ws + .live_sandbox_id + .as_ref() + .is_some_and(|id| live_ids.contains(id)); + if !live { + ws.status = WorkspaceStatus::Stale; + ws.live_sandbox_id = None; + changed = true; + } + } + } + } + changed + } + /// Kill orphaned Firecracker processes on startup (issue #298). /// + /// **MUST only be called before any `Vm` handle is held** (i.e. in + /// `run_daemon` before `live_vms` is populated). This method + /// SIGKILLs every sandbox entry whose PID is alive — it cannot + /// distinguish an orphaned VM from a currently managed one. + /// /// After `reconcile()` prunes entries with dead PIDs, any remaining /// sandbox entries have alive PIDs — but the controller has no /// `live_vms` handle for them (the controller restarted). These are /// orphaned Firecracker processes: alive but unmanageable. /// - /// This method kills each orphan (after verifying the PID still - /// belongs to a Firecracker process — guards against PID reuse), - /// prunes the registry entry, and marks any workspace whose - /// live_sandbox_id was killed as Stale. + /// For each orphan: verify the PID still belongs to a Firecracker + /// process (guards against PID reuse), send SIGKILL, wait for the + /// process to exit (bounded timeout), then prune the registry entry. + /// If the kill fails with a real error (not ESRCH), the entry is + /// **not** pruned — a live orphan holding resources must stay + /// registered so the operator can investigate. /// /// After this, the NetnsAllocator (empty active set) and - /// shared_tap_owner (None) are safe: no orphaned VM holds a - /// netns index or the shared tap. - pub fn kill_orphans(&self) -> Result { + /// shared_tap_owner (None) are safe once all orphans are confirmed + /// dead: no orphaned VM holds a netns index or the shared tap. + /// If some kills failed, those entries remain in the registry and + /// the allocator may still collide with them — the caller should + /// log the failure count and the operator should investigate. + pub(crate) fn kill_orphans(&self) -> Result { let orphans: Vec<(String, u32)> = { let g = self.inner.lock(); g.sandboxes @@ -295,6 +335,10 @@ impl Registry { }; let mut killed = 0usize; + let mut pruned_stale = 0usize; + let mut kill_failed = 0usize; + let mut skip_ids: Vec = Vec::new(); + for (id, pid) in orphans { if pid_is_firecracker(pid) { tracing::warn!( @@ -302,13 +346,48 @@ impl Registry { pid = pid, "killing orphaned Firecracker process on startup" ); - if let Err(e) = kill_pid(pid) { - tracing::error!( - sandbox_id = %id, - pid = pid, - error = %e, - "failed to kill orphaned Firecracker process; pruning registry entry anyway" - ); + match kill_pid(pid) { + Ok(()) => { + // Wait for the process to actually exit (bounded). + // SIGKILL is asynchronous; a D-state process can + // hold netns/tap resources past the kill return. + if wait_for_death(pid, std::time::Duration::from_secs(5)) { + self.inner.lock().sandboxes.remove(&id); + killed += 1; + } else { + tracing::error!( + sandbox_id = %id, + pid = pid, + "orphaned Firecracker did not exit within 5s of SIGKILL; keeping registry entry to prevent resource collision" + ); + kill_failed += 1; + skip_ids.push(id); + } + } + Err(e) if e.raw_os_error() == Some(libc::ESRCH) => { + // Process already dead (benign TOCTOU race between + // pid_alive and kill_pid) — safe to prune. + tracing::debug!( + sandbox_id = %id, + pid = pid, + "orphan already exited (ESRCH); pruning registry entry" + ); + self.inner.lock().sandboxes.remove(&id); + pruned_stale += 1; + } + Err(e) => { + // Real kill failure (EPERM, etc.) — do NOT prune. + // The orphan may still be alive holding resources; + // pruning would recreate the exact bug #298 fixes. + tracing::error!( + sandbox_id = %id, + pid = pid, + error = %e, + "failed to kill orphaned Firecracker process; keeping registry entry to prevent resource collision" + ); + kill_failed += 1; + skip_ids.push(id); + } } } else { tracing::warn!( @@ -316,37 +395,20 @@ impl Registry { pid = pid, "PID no longer belongs to Firecracker (PID reuse); pruning stale registry entry" ); - } - self.inner.lock().sandboxes.remove(&id); - killed += 1; - } - - // Re-run workspace stale marking: workspaces whose - // live_sandbox_id was just pruned are now orphaned. - let live_ids: std::collections::HashSet = - self.inner.lock().sandboxes.keys().cloned().collect(); - let mut stale_ws_changed = false; - { - let mut g = self.inner.lock(); - for ws in g.workspaces.values_mut() { - if ws.status == WorkspaceStatus::Running { - let live = ws - .live_sandbox_id - .as_ref() - .is_some_and(|id| live_ids.contains(id)); - if !live { - ws.status = WorkspaceStatus::Stale; - ws.live_sandbox_id = None; - stale_ws_changed = true; - } - } + self.inner.lock().sandboxes.remove(&id); + pruned_stale += 1; } } - if killed > 0 || stale_ws_changed { + let stale_ws = self.mark_stale_workspaces(); + if killed > 0 || pruned_stale > 0 || stale_ws { self.flush()?; } - Ok(killed) + Ok(KillOrphansResult { + killed, + pruned_stale, + kill_failed, + }) } /// For metrics: live counts. @@ -356,6 +418,15 @@ impl Registry { } } +/// Result of `kill_orphans`: how many were actually killed, pruned as +/// stale (PID reuse / already dead), and how many kills failed. +#[derive(Debug, Default, Clone, Copy)] +pub struct KillOrphansResult { + pub killed: usize, + pub pruned_stale: usize, + pub kill_failed: usize, +} + #[cfg(target_os = "linux")] fn pid_alive(pid: u32) -> bool { std::path::Path::new(&format!("/proc/{pid}")).exists() @@ -372,10 +443,22 @@ fn pid_alive(_pid: u32) -> bool { /// `/proc//comm`. Guards against PID reuse: if the original /// Firecracker process died and the PID was recycled by the OS for /// a different process, we don't want to kill an unrelated process. +/// +/// Note: `comm` is settable via `prctl(PR_SET_NAME)` and is not an +/// identity guarantee. On a multi-tenant host where another process +/// could set its name to "firecracker", this check is a best-effort +/// guard, not a security boundary. The TOCTOU window between this +/// check and `kill_pid` is acknowledged — `pidfd_open` + +/// `pidfd_send_signal` would close it entirely (Linux 5.3+). #[cfg(target_os = "linux")] fn pid_is_firecracker(pid: u32) -> bool { + // Defensive guard against pid 0/1 (defense-in-depth for corrupted + // state.json). Real firecracker PIDs are always > 1. + if pid <= 1 { + return false; + } std::fs::read_to_string(format!("/proc/{pid}/comm")) - .map(|s| s.trim().contains("firecracker")) + .map(|s| s.trim() == "firecracker") .unwrap_or(false) } @@ -388,6 +471,10 @@ fn pid_is_firecracker(_pid: u32) -> bool { /// Send SIGKILL to a process by PID. Uses libc::kill directly /// (we don't have a std::process::Child handle for orphaned PIDs). +/// +/// SAFETY: `pid` is a live Linux PID verified by `pid_is_firecracker`; +/// `SIGKILL` is a valid signal constant; `kill(2)` is sound for any +/// `pid_t` value (returns ESRCH if the process doesn't exist). #[cfg(target_os = "linux")] fn kill_pid(pid: u32) -> std::io::Result<()> { let ret = unsafe { libc::kill(pid as i32, libc::SIGKILL) }; @@ -403,6 +490,28 @@ fn kill_pid(_pid: u32) -> std::io::Result<()> { Ok(()) } +/// Poll for process death by checking `/proc/` disappearance. +/// Returns true if the process exited within the timeout, false if it +/// is still alive (e.g. stuck in D-state on I/O). +#[cfg(target_os = "linux")] +fn wait_for_death(pid: u32, timeout: std::time::Duration) -> bool { + let deadline = std::time::Instant::now() + timeout; + let path = format!("/proc/{pid}"); + while std::time::Instant::now() < deadline { + if !std::path::Path::new(&path).exists() { + return true; + } + std::thread::sleep(std::time::Duration::from_millis(100)); + } + !std::path::Path::new(&path).exists() +} + +#[cfg(not(target_os = "linux"))] +fn wait_for_death(_pid: u32, _timeout: std::time::Duration) -> bool { + // Off-Linux: no /proc to poll; assume the process is gone. + true +} + #[cfg(test)] mod tests { use super::*; @@ -614,9 +723,9 @@ mod tests { // kill_orphans prunes all remaining entries (alive PID but // not firecracker → pruned without killing). - let killed = r.kill_orphans().unwrap(); + let result = r.kill_orphans().unwrap(); // At least the alive-PID entry is pruned. - assert!(killed >= 1); + assert!(result.killed + result.pruned_stale >= 1); // All sandbox entries are gone. assert!(r.list_sandboxes().is_empty()); } @@ -658,8 +767,9 @@ mod tests { .unwrap(); // kill_orphans kills the sandbox and marks the workspace Stale. - let killed = r.kill_orphans().unwrap(); - assert_eq!(killed, 1); + let result = r.kill_orphans().unwrap(); + assert_eq!(result.killed, 0); // not firecracker → not killed + assert_eq!(result.pruned_stale, 1); // pruned as stale assert!(r.list_sandboxes().is_empty()); let ws = r.get_workspace("ws-1").unwrap(); @@ -672,7 +782,166 @@ mod tests { let td = TempDir::new().unwrap(); let path = td.path().join("state.json"); let r = Registry::load_or_init(&path).unwrap(); - let killed = r.kill_orphans().unwrap(); - assert_eq!(killed, 0); + let result = r.kill_orphans().unwrap(); + assert_eq!(result.killed, 0); + assert_eq!(result.pruned_stale, 0); + assert_eq!(result.kill_failed, 0); + } + + #[test] + fn kill_orphans_skips_pid_none_entries() { + let td = TempDir::new().unwrap(); + let path = td.path().join("state.json"); + let r = Registry::load_or_init(&path).unwrap(); + + // Insert a sandbox with pid: None — should be skipped by both + // reconcile() and kill_orphans() (the filter_map only collects + // Some(pid) entries). + r.insert_sandbox(SandboxInfo { + id: "sb-no-pid".into(), + snapshot_tag: "py".into(), + netns: None, + guest_addr: "10.42.0.2:8888".into(), + created_at_unix: 1, + pid: None, + memory_limit_mib: None, + has_branched: false, + last_branch_memory_path: None, + branch_count: 0, + }) + .unwrap(); + + let _ = r.reconcile().unwrap(); + let result = r.kill_orphans().unwrap(); + assert_eq!(result.killed, 0); + assert_eq!(result.pruned_stale, 0); + assert_eq!(result.kill_failed, 0); + // Entry still exists — neither method touches pid:None entries. + assert_eq!(r.list_sandboxes().len(), 1); + } + + #[test] + fn kill_orphans_marks_multiple_workspaces_stale() { + let td = TempDir::new().unwrap(); + let path = td.path().join("state.json"); + let r = Registry::load_or_init(&path).unwrap(); + + // Insert a sandbox with an alive PID. + r.insert_sandbox(SandboxInfo { + id: "sb-1".into(), + snapshot_tag: "py".into(), + netns: Some("forkd-child-1".into()), + guest_addr: "10.42.0.2:8888".into(), + created_at_unix: 1, + pid: Some(std::process::id()), + memory_limit_mib: None, + has_branched: false, + last_branch_memory_path: None, + branch_count: 0, + }) + .unwrap(); + + // Insert two workspaces referencing the same sandbox. + for name in ["ws-a", "ws-b"] { + r.insert_workspace(WorkspaceInfo { + id: name.into(), + name: name.into(), + source_snapshot_tag: "py".into(), + current_state_tag: None, + status: WorkspaceStatus::Running, + live_sandbox_id: Some("sb-1".into()), + created_at_unix: 1, + last_active_unix: 1, + last_branch_memory_path: None, + per_child_netns: false, + }) + .unwrap(); + } + + let result = r.kill_orphans().unwrap(); + assert_eq!(result.pruned_stale, 1); + + // Both workspaces should be marked Stale. + assert_eq!( + r.get_workspace("ws-a").unwrap().status, + WorkspaceStatus::Stale + ); + assert_eq!( + r.get_workspace("ws-b").unwrap().status, + WorkspaceStatus::Stale + ); + } + + #[test] + fn kill_orphans_persists_to_disk() { + let td = TempDir::new().unwrap(); + let path = td.path().join("state.json"); + let r = Registry::load_or_init(&path).unwrap(); + + r.insert_sandbox(SandboxInfo { + id: "sb-1".into(), + snapshot_tag: "py".into(), + netns: Some("forkd-child-1".into()), + guest_addr: "10.42.0.2:8888".into(), + created_at_unix: 1, + pid: Some(std::process::id()), + memory_limit_mib: None, + has_branched: false, + last_branch_memory_path: None, + branch_count: 0, + }) + .unwrap(); + + let result = r.kill_orphans().unwrap(); + assert!(result.pruned_stale >= 1); + + // Reload from disk and verify the entry is gone. + let r2 = Registry::load_or_init(&path).unwrap(); + assert!(r2.list_sandboxes().is_empty()); + } + + #[test] + fn kill_orphans_reconcile_then_kill_integration() { + let td = TempDir::new().unwrap(); + let path = td.path().join("state.json"); + let r = Registry::load_or_init(&path).unwrap(); + + // Dead PID entry (pruned by reconcile). + r.insert_sandbox(SandboxInfo { + id: "sb-dead".into(), + snapshot_tag: "py".into(), + netns: Some("forkd-child-1".into()), + guest_addr: "10.42.0.2:8888".into(), + created_at_unix: 1, + pid: Some(99999999), + memory_limit_mib: None, + has_branched: false, + last_branch_memory_path: None, + branch_count: 0, + }) + .unwrap(); + + // Alive PID entry (pruned by kill_orphans). + r.insert_sandbox(SandboxInfo { + id: "sb-alive".into(), + snapshot_tag: "py".into(), + netns: Some("forkd-child-2".into()), + guest_addr: "10.42.0.3:8888".into(), + created_at_unix: 2, + pid: Some(std::process::id()), + memory_limit_mib: None, + has_branched: false, + last_branch_memory_path: None, + branch_count: 0, + }) + .unwrap(); + + let pruned = r.reconcile().unwrap(); + let result = r.kill_orphans().unwrap(); + + // On Linux: reconcile prunes sb-dead (1), kill_orphans prunes sb-alive (1). + // On macOS: reconcile prunes 0 (all PIDs "alive"), kill_orphans prunes 2. + assert!(r.list_sandboxes().is_empty()); + assert!(pruned + result.killed + result.pruned_stale >= 2); } } From a110e2690377f90cb9fd519424ac408baf811c8d Mon Sep 17 00:00:00 2001 From: jrimmer Date: Wed, 12 Aug 2026 22:04:47 -0700 Subject: [PATCH 03/11] fix(controller): abort startup on kill_failed, document PID-reuse limit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit WaylandYang round 4 found that kill_orphans correctly keeps registry entries on EPERM/timeout (kill_failed > 0), but run_daemon only logs the count and continues. The NetnsAllocator active set and shared_tap_owner start empty, so the next spawn can reuse the failed orphan's netns index or tap lease — the exact collision #298 prevents. Fix: run_daemon now aborts startup when kill_failed > 0, forcing operator intervention rather than silently admitting conflicting spawns. The error message names the count and tells the operator to kill the orphans manually and restart. Also updated the pid_is_firecracker doc comment to be honest about the PID-reuse limitation: the comm check is a defense-in-depth heuristic, not complete PID-reuse protection. A robust fix would use pidfd signaling plus durable identity validation (process start time recorded with the registry entry at spawn time, compared on startup). That is tracked as a follow-up. Added test: kill_orphans_result_kill_failed_contract verifies the KillOrphansResult contract (kill_failed == 0 on empty registry, so baseline startup does NOT abort). Signed-off-by: jrimmer --- crates/forkd-controller/src/lib.rs | 16 +++++++++++ crates/forkd-controller/src/state.rs | 43 ++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+) diff --git a/crates/forkd-controller/src/lib.rs b/crates/forkd-controller/src/lib.rs index 22c66a6..0a1e77b 100644 --- a/crates/forkd-controller/src/lib.rs +++ b/crates/forkd-controller/src/lib.rs @@ -113,6 +113,22 @@ pub async fn run_daemon(cfg: DaemonConfig) -> Result<()> { ); } + // Fail closed: if any orphan could not be killed (EPERM, D-state + // timeout, etc.), the controller cannot guarantee a clean resource + // state. The NetnsAllocator active set and shared_tap_owner start + // empty, so a new spawn could reuse the still-alive orphan's netns + // index or tap lease — the exact collision #298 is meant to prevent. + // Abort startup so the operator intervenes rather than silently + // admitting conflicting spawns. (review #299) + if orphans.kill_failed > 0 { + anyhow::bail!( + "aborting startup: {} orphaned Firecracker process(es) could not be killed \ + (EPERM or did not exit within timeout); refusing to accept spawns that may \ + collide with still-alive orphans. Kill them manually and restart.", + orphans.kill_failed + ); + } + let audit = AuditSink::open(&cfg.audit_log) .with_context(|| format!("open audit log {}", cfg.audit_log.display()))?; tracing::info!(audit_log = %audit.path().display(), "audit log open"); diff --git a/crates/forkd-controller/src/state.rs b/crates/forkd-controller/src/state.rs index 68b9f04..2f09fce 100644 --- a/crates/forkd-controller/src/state.rs +++ b/crates/forkd-controller/src/state.rs @@ -451,6 +451,19 @@ fn pid_alive(_pid: u32) -> bool { /// check and `kill_pid` is acknowledged — `pidfd_open` + /// `pidfd_send_signal` would close it entirely (Linux 5.3+). #[cfg(target_os = "linux")] +/// Check whether a PID belongs to a Firecracker process by reading +/// `/proc//comm`. Uses exact match (`== "firecracker"`) to avoid +/// false positives from processes whose names contain the substring. +/// +/// **Limitation:** this is NOT complete PID-reuse protection. If a +/// Firecracker process exits and its PID is reused by another +/// Firecracker process before this check runs, the comm match passes +/// and the new (legitimate) process would be killed. A robust fix +/// would use pidfd signaling plus durable identity validation +/// (process start time recorded with the registry entry at spawn +/// time, compared on startup). That is tracked as a follow-up — the +/// comm check is a defense-in-depth heuristic, not a guarantee. +#[cfg(target_os = "linux")] fn pid_is_firecracker(pid: u32) -> bool { // Defensive guard against pid 0/1 (defense-in-depth for corrupted // state.json). Real firecracker PIDs are always > 1. @@ -944,4 +957,34 @@ mod tests { assert!(r.list_sandboxes().is_empty()); assert!(pruned + result.killed + result.pruned_stale >= 2); } + + /// Regression: kill_failed > 0 must cause the caller to abort startup, + /// not silently continue with an empty NetnsAllocator. This test + /// verifies the KillOrphansResult contract: on non-Linux platforms + /// (where pid_is_firecracker returns false), alive-PID entries are + /// pruned as stale (kill_failed stays 0). On Linux, a real + /// firecracker PID that can't be killed would set kill_failed > 0. + /// The caller (run_daemon) checks `if orphans.kill_failed > 0 { + /// anyhow::bail!(...) }` — this test verifies that a result with + /// kill_failed == 0 does NOT trigger the abort (baseline), and + /// documents the contract. + #[test] + fn kill_orphans_result_kill_failed_contract() { + let tmp = tempfile::tempdir().unwrap(); + let path = tmp.path().join("state.json"); + let r = Registry::load_or_init(path).unwrap(); + + // No sandboxes — kill_orphans returns all-zero result. + let result = r.kill_orphans().unwrap(); + assert_eq!( + result.kill_failed, 0, + "no sandboxes should yield 0 kill_failed" + ); + assert_eq!(result.killed, 0); + assert_eq!(result.pruned_stale, 0); + + // The caller's abort condition: kill_failed > 0 → bail. + // With 0 kill_failed, startup should NOT abort (baseline). + assert!(result.kill_failed == 0, "baseline should not abort"); + } } From 5cf777a3a0557e29d6da929f187c2dbce3e2df8e Mon Sep 17 00:00:00 2001 From: jrimmer Date: Wed, 12 Aug 2026 22:53:49 -0700 Subject: [PATCH 04/11] fix(controller): extract check_orphan_kill_result for testable abort decision MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The startup abort logic was inline in run_daemon, making it impossible to unit-test the kill_failed > 0 → bail decision path. Extracted into pub(crate) check_orphan_kill_result(&KillOrphansResult) -> Result<()> so the abort contract is directly testable. Tests: - check_orphan_kill_result_aborts_on_kill_failure: verifies that kill_failed=1 returns Err with "aborting startup" message, and kill_failed=0 returns Ok even with killed/pruned entries. - check_orphan_kill_result_error_contains_count: verifies the error message contains the exact kill_failed count so operators know how many orphans need manual intervention. The existing kill_orphans_result_kill_failed_contract test (zero-baseline) is retained as documentation of the baseline contract. The new tests exercise the actual abort decision logic. Signed-off-by: jrimmer --- crates/forkd-controller/src/lib.rs | 28 ++++++++---- crates/forkd-controller/src/state.rs | 65 ++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+), 8 deletions(-) diff --git a/crates/forkd-controller/src/lib.rs b/crates/forkd-controller/src/lib.rs index 0a1e77b..f67b6f2 100644 --- a/crates/forkd-controller/src/lib.rs +++ b/crates/forkd-controller/src/lib.rs @@ -80,6 +80,25 @@ fn unauthenticated_non_loopback(bind: SocketAddr, token_file: Option<&Path>) -> token_file.is_none() && !bind.ip().is_loopback() } +/// Post-`kill_orphans` startup decision: abort if any orphan could +/// not be killed (EPERM, D-state timeout, etc.). The NetnsAllocator +/// active set and `shared_tap_owner` start empty, so a new spawn could +/// reuse the still-alive orphan's netns index or tap lease — the exact +/// collision #298 is meant to prevent. Returning `Err` here causes +/// `run_daemon` to exit before binding the HTTP listener, so no spawns +/// can be admitted. +pub(crate) fn check_orphan_kill_result(orphans: &crate::state::KillOrphansResult) -> Result<()> { + if orphans.kill_failed > 0 { + anyhow::bail!( + "aborting startup: {} orphaned Firecracker process(es) could not be killed \ + (EPERM or did not exit within timeout); refusing to accept spawns that may \ + collide with still-alive orphans. Kill them manually and restart.", + orphans.kill_failed + ); + } + Ok(()) +} + /// Bring up the controller daemon. Blocks until the listener exits. /// SIGTERM and SIGINT trigger a graceful shutdown; SIGHUP reopens the /// configured audit log after external rotation. @@ -120,14 +139,7 @@ pub async fn run_daemon(cfg: DaemonConfig) -> Result<()> { // index or tap lease — the exact collision #298 is meant to prevent. // Abort startup so the operator intervenes rather than silently // admitting conflicting spawns. (review #299) - if orphans.kill_failed > 0 { - anyhow::bail!( - "aborting startup: {} orphaned Firecracker process(es) could not be killed \ - (EPERM or did not exit within timeout); refusing to accept spawns that may \ - collide with still-alive orphans. Kill them manually and restart.", - orphans.kill_failed - ); - } + check_orphan_kill_result(&orphans)?; let audit = AuditSink::open(&cfg.audit_log) .with_context(|| format!("open audit log {}", cfg.audit_log.display()))?; diff --git a/crates/forkd-controller/src/state.rs b/crates/forkd-controller/src/state.rs index 2f09fce..1ba00dc 100644 --- a/crates/forkd-controller/src/state.rs +++ b/crates/forkd-controller/src/state.rs @@ -987,4 +987,69 @@ mod tests { // With 0 kill_failed, startup should NOT abort (baseline). assert!(result.kill_failed == 0, "baseline should not abort"); } + + /// Regression: check_orphan_kill_result must return Err when + /// kill_failed > 0, proving that run_daemon aborts startup and + /// no conflicting spawns can be admitted. This is the actual + /// abort-decision test — the previous test only verified the + /// zero-baseline contract. + #[test] + fn check_orphan_kill_result_aborts_on_kill_failure() { + use crate::check_orphan_kill_result; + + // kill_failed > 0 must abort (EPERM, D-state timeout, etc.) + let result = KillOrphansResult { + killed: 2, + pruned_stale: 0, + kill_failed: 1, + }; + let outcome = check_orphan_kill_result(&result); + assert!( + outcome.is_err(), + "kill_failed=1 must cause startup abort, got {outcome:?}" + ); + let err = outcome.unwrap_err().to_string(); + assert!( + err.contains("aborting startup"), + "error should mention aborting startup, got: {err}" + ); + assert!( + err.contains('1'), + "error should contain kill_failed count, got: {err}" + ); + + // kill_failed == 0 must NOT abort, even with killed/pruned entries + let result = KillOrphansResult { + killed: 5, + pruned_stale: 3, + kill_failed: 0, + }; + assert!( + check_orphan_kill_result(&result).is_ok(), + "kill_failed=0 should not abort even with killed/pruned entries" + ); + } + + /// Regression: check_orphan_kill_result error message must + /// contain the exact kill_failed count so operators can identify + /// how many orphans need manual intervention. + #[test] + fn check_orphan_kill_result_error_contains_count() { + use crate::check_orphan_kill_result; + + let result = KillOrphansResult { + killed: 0, + pruned_stale: 0, + kill_failed: 3, + }; + let err = check_orphan_kill_result(&result).unwrap_err().to_string(); + assert!( + err.contains('3'), + "error should contain kill_failed count (3), got: {err}" + ); + assert!( + err.contains("could not be killed"), + "error should explain the failure, got: {err}" + ); + } } From 291fee4c2a9b03d099acbd4b1e41d0b9807742f9 Mon Sep 17 00:00:00 2001 From: jrimmer Date: Thu, 13 Aug 2026 13:57:25 -0700 Subject: [PATCH 05/11] fix(controller): remove orphaned last_err assignment after rebase MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dead-last_err cleanup dropped the declaration and the unreachable expect, but the `last_err = Some(e)` assignment (guarded by the tap/cgroup-busy warn block on main) survived the rebase and referenced a now-missing binding. Remove it — the loop already returns on the final attempt, so the assignment is dead. Signed-off-by: jrimmer --- crates/forkd-controller/src/http.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/crates/forkd-controller/src/http.rs b/crates/forkd-controller/src/http.rs index 6655880..77a1458 100644 --- a/crates/forkd-controller/src/http.rs +++ b/crates/forkd-controller/src/http.rs @@ -1349,6 +1349,12 @@ async fn create_sandbox( return Err(e); } + tracing::warn!( + attempt = attempt + 1, + next_backoff_ms = backoffs_ms[attempt], + error = %e, + "restore_many: tap/cgroup busy, retrying" + ); } } } From ba59e6f9ff968caec2bd4891a972b8aec7f6731c Mon Sep 17 00:00:00 2001 From: jrimmer Date: Mon, 17 Aug 2026 00:39:18 -0700 Subject: [PATCH 06/11] fix(controller): durable process identity for orphan recovery MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the comm=="firecracker" check with durable process identity verification and pidfd signaling, addressing review r6 (WaylandYang) and the ce-code-review security findings. The comm check was spoofable (prctl PR_SET_NAME) and could not distinguish two Firecracker processes sharing a PID over time — a same-name PID-reuse attack would kill a legitimate new Firecracker, and on a multi-tenant host a cross-tenant DoS was possible. The TOCTOU window between the check and kill(2) was also acknowledged. Changes: 1. SandboxInfo::proc_starttime (serde default) — persists the process start time (field 22 of /proc//stat, clock ticks since boot) captured at VM registration. Backward-compatible: old state.json files without this field deserialize to None. 2. read_proc_starttime — robust parse of /proc//stat that handles the parenthesized comm field (field 2, can contain spaces/parens) by splitting at the last ')' and indexing field 22 in the remainder. 3. process_identity_matches — compares the live start time against the recorded one. Returns Match / PidReuse / Dead / Unknown. PidReuse means the original process exited and the PID was recycled — prune the stale entry without killing. Unknown (no recorded start time, /proc unreadable, or off-Linux) fails closed. 4. pidfd_open + pidfd_send_kill — signal through a pidfd opened AFTER the identity check, closing the TOCTOU window: the fd is pinned to the verified process, so a PID reuse between check and signal cannot redirect the kill. Uses raw libc::syscall with SYS_pidfd_open / SYS_pidfd_send_signal (Linux 5.3+; libc 0.2.x exposes the syscall numbers but not named fns). 5. comm_is_firecracker — retained as a secondary confirmation after the primary start-time check passes, to catch corrupted state.json. Not a security boundary. 6. kill_orphans reworked: Match → pidfd kill + wait_for_death; PidReuse → prune stale (no kill); Dead → prune stale; Unknown → fail closed (keep entry, increment kill_failed so run_daemon aborts startup). Tests updated to reflect the new fail-closed semantics: - kill_orphans_prunes_alive_pid_entries now uses a mismatched start time (u64::MAX) to trigger PidReuse → prune, not the old None path. - kill_orphans_fails_closed_on_unknown_identity (new): alive PID with proc_starttime: None → fail closed, entry kept, kill_failed=1. - kill_orphans_prunes_same_name_pid_reuse_without_killing (new regression): the same-name Firecracker PID-reuse attack the reviewer flagged — a mismatched start time must prune without killing, even if comm were "firecracker". - All other kill_orphans tests updated to use mismatched start times where they previously relied on the comm-check prune path. Verified on Linux (rust 1.83, x86_64) via Docker: clippy clean for forkd-controller; cargo test --all-features passes 104 unit + 8 integration tests, 0 failed. Signed-off-by: jrimmer --- crates/forkd-controller/src/api.rs | 13 + crates/forkd-controller/src/http.rs | 6 +- crates/forkd-controller/src/state.rs | 534 +++++++++++++++++++++------ 3 files changed, 448 insertions(+), 105 deletions(-) diff --git a/crates/forkd-controller/src/api.rs b/crates/forkd-controller/src/api.rs index 1f8855b..d0beaa4 100644 --- a/crates/forkd-controller/src/api.rs +++ b/crates/forkd-controller/src/api.rs @@ -306,6 +306,19 @@ pub struct SandboxInfo { pub guest_addr: String, pub created_at_unix: u64, pub pid: Option, + /// Process start time in clock ticks since boot (field 22 of + /// `/proc//stat`), captured at VM registration. Used on + /// controller restart to detect PID reuse: if the recorded PID now + /// points to a process with a different start time, the original + /// Firecracker has exited and the PID was recycled — we must prune + /// the stale registry entry rather than kill an unrelated process. + /// + /// `#[serde(default)]` keeps existing `state.json` files loadable + /// (entries written before this field existed deserialize to + /// `None`, which is treated as "identity unknown — verify by + /// comm only, fail closed if the check is inconclusive"). + #[serde(default)] + pub proc_starttime: Option, pub memory_limit_mib: Option, /// Set to true once any BRANCH (Full or Diff) has been taken from /// this sandbox. Diagnostic flag — phase 1d (v0.3.1) lifted the diff --git a/crates/forkd-controller/src/http.rs b/crates/forkd-controller/src/http.rs index 77a1458..36e6a4f 100644 --- a/crates/forkd-controller/src/http.rs +++ b/crates/forkd-controller/src/http.rs @@ -38,7 +38,7 @@ use crate::api::{ ExecResponse, SandboxInfo, SnapshotInfo, SnapshotInfoDetail, SuspendWorkspaceRequest, VersionResponse, WorkspaceInfo, WorkspaceStatus, }; -use crate::state::Registry; +use crate::state::{read_proc_starttime, Registry}; use forkd_vmm::ClockSyncOutcome; const API_VERSION: &str = "v1"; @@ -1412,6 +1412,7 @@ async fn create_sandbox( guest_addr: "10.42.0.2:8888".to_string(), created_at_unix: now, pid: Some(vm.pid()), + proc_starttime: read_proc_starttime(vm.pid()), memory_limit_mib: req.memory_limit_mib, has_branched: false, last_branch_memory_path: None, @@ -2839,6 +2840,7 @@ fn spawn_one_for_workspace( guest_addr: "10.42.0.2:8888".to_string(), created_at_unix: unix_now(), pid: Some(vm.pid()), + proc_starttime: read_proc_starttime(vm.pid()), memory_limit_mib, has_branched: false, last_branch_memory_path: None, @@ -3455,6 +3457,7 @@ mod tests { guest_addr: "10.42.0.2:8888".to_string(), created_at_unix: unix_now(), pid: None, + proc_starttime: None, memory_limit_mib: None, has_branched: false, last_branch_memory_path: None, @@ -4499,6 +4502,7 @@ mod tests { guest_addr: "127.0.0.1:1".into(), created_at_unix: 1, pid: Some(99999999), + proc_starttime: None, memory_limit_mib: None, has_branched: false, last_branch_memory_path: None, diff --git a/crates/forkd-controller/src/state.rs b/crates/forkd-controller/src/state.rs index 1ba00dc..baf68aa 100644 --- a/crates/forkd-controller/src/state.rs +++ b/crates/forkd-controller/src/state.rs @@ -323,12 +323,15 @@ impl Registry { /// the allocator may still collide with them — the caller should /// log the failure count and the operator should investigate. pub(crate) fn kill_orphans(&self) -> Result { - let orphans: Vec<(String, u32)> = { + // Collect orphans with their recorded durable identity (start time). + // The lock is dropped before any kill so the (bounded) wait_for_death + // poll never holds the registry mutex. + let orphans: Vec<(String, u32, Option)> = { let g = self.inner.lock(); g.sandboxes .iter() .filter_map(|(id, sb)| match sb.pid { - Some(pid) if pid_alive(pid) => Some((id.clone(), pid)), + Some(pid) if pid_alive(pid) => Some((id.clone(), pid, sb.proc_starttime)), _ => None, }) .collect() @@ -337,66 +340,156 @@ impl Registry { let mut killed = 0usize; let mut pruned_stale = 0usize; let mut kill_failed = 0usize; - let mut skip_ids: Vec = Vec::new(); - - for (id, pid) in orphans { - if pid_is_firecracker(pid) { - tracing::warn!( - sandbox_id = %id, - pid = pid, - "killing orphaned Firecracker process on startup" - ); - match kill_pid(pid) { - Ok(()) => { - // Wait for the process to actually exit (bounded). - // SIGKILL is asynchronous; a D-state process can - // hold netns/tap resources past the kill return. - if wait_for_death(pid, std::time::Duration::from_secs(5)) { + + for (id, pid, recorded_starttime) in orphans { + // Primary identity check: compare the live process start time + // against the one recorded at registration. This is the + // durable identity that survives PID reuse — `comm` alone + // is spoofable and cannot distinguish two Firecracker + // processes that happen to share a PID over time. + match process_identity_matches(pid, recorded_starttime) { + IdentityCheck::Match => { + // Same process (start time matches). Open a pidfd + // NOW, after the identity check, so the signal is + // pinned to the verified process. This closes the + // TOCTOU window: even if the PID is reused between + // here and pidfd_send_signal, the fd targets the + // original process identity, not whatever currently + // holds the PID. + let pidfd = match pidfd_open(pid) { + Ok(fd) => fd, + Err(e) if e.raw_os_error() == Some(libc::ESRCH) => { + // Benign race: process exited between the + // identity check and pidfd_open. Safe to prune. + tracing::debug!( + sandbox_id = %id, + pid = pid, + "orphan exited before pidfd_open (ESRCH); pruning registry entry" + ); self.inner.lock().sandboxes.remove(&id); - killed += 1; - } else { + pruned_stale += 1; + continue; + } + Err(e) => { + // pidfd_open can fail with EINVAL/ENOSYS on + // kernels < 5.3. Fail closed: keep the entry + // so the operator can investigate, rather than + // risk killing the wrong process via kill(2). tracing::error!( sandbox_id = %id, pid = pid, - "orphaned Firecracker did not exit within 5s of SIGKILL; keeping registry entry to prevent resource collision" + error = %e, + "pidfd_open failed; keeping registry entry to prevent unsafe kill" ); kill_failed += 1; - skip_ids.push(id); + continue; } - } - Err(e) if e.raw_os_error() == Some(libc::ESRCH) => { - // Process already dead (benign TOCTOU race between - // pid_alive and kill_pid) — safe to prune. - tracing::debug!( - sandbox_id = %id, - pid = pid, - "orphan already exited (ESRCH); pruning registry entry" - ); - self.inner.lock().sandboxes.remove(&id); - pruned_stale += 1; - } - Err(e) => { - // Real kill failure (EPERM, etc.) — do NOT prune. - // The orphan may still be alive holding resources; - // pruning would recreate the exact bug #298 fixes. + }; + + // Secondary confirmation: the comm name should still + // be firecracker. This is NOT a security boundary (comm + // is spoofable) — it catches corrupted state.json that + // somehow recorded a valid-looking start time for a + // non-firecracker process. If it fails, fail closed. + if !comm_is_firecracker(pid) { + let _ = unsafe { libc::close(pidfd) }; tracing::error!( sandbox_id = %id, pid = pid, - error = %e, - "failed to kill orphaned Firecracker process; keeping registry entry to prevent resource collision" + "start time matched but comm is not firecracker; \ + keeping registry entry (possible state corruption)" ); kill_failed += 1; - skip_ids.push(id); + continue; + } + + tracing::warn!( + sandbox_id = %id, + pid = pid, + "killing orphaned Firecracker process on startup (pidfd signal, identity verified)" + ); + match pidfd_send_kill(pidfd) { + Ok(()) => { + // Wait for the process to actually exit (bounded). + // SIGKILL is asynchronous; a D-state process can + // hold netns/tap resources past the kill return. + if wait_for_death(pid, std::time::Duration::from_secs(5)) { + self.inner.lock().sandboxes.remove(&id); + killed += 1; + } else { + tracing::error!( + sandbox_id = %id, + pid = pid, + "orphaned Firecracker did not exit within 5s of SIGKILL; \ + keeping registry entry to prevent resource collision" + ); + kill_failed += 1; + } + } + Err(e) if e.raw_os_error() == Some(libc::ESRCH) => { + // Process exited between pidfd_open and the + // signal (benign). Safe to prune. + tracing::debug!( + sandbox_id = %id, + pid = pid, + "orphan exited before signal (ESRCH); pruning registry entry" + ); + self.inner.lock().sandboxes.remove(&id); + pruned_stale += 1; + } + Err(e) => { + tracing::error!( + sandbox_id = %id, + pid = pid, + error = %e, + "pidfd_send_signal failed; keeping registry entry to prevent resource collision" + ); + kill_failed += 1; + } } + // Always close the pidfd; leak protection on every path. + let _ = unsafe { libc::close(pidfd) }; + } + IdentityCheck::PidReuse => { + // PID is alive but the start time differs: the original + // Firecracker exited and the PID was recycled by the + // kernel for an unrelated process. Do NOT kill — prune + // the stale registry entry only. + tracing::warn!( + sandbox_id = %id, + pid = pid, + "PID reused (start time mismatch); pruning stale registry entry without killing" + ); + self.inner.lock().sandboxes.remove(&id); + pruned_stale += 1; + } + IdentityCheck::Dead => { + // Process is gone (no /proc/). Prune the entry. + tracing::debug!( + sandbox_id = %id, + pid = pid, + "orphan already exited; pruning registry entry" + ); + self.inner.lock().sandboxes.remove(&id); + pruned_stale += 1; + } + IdentityCheck::Unknown => { + // No recorded start time (old state.json) OR the live + // start time could not be read OR off-Linux. Fail + // closed: do NOT kill (we can't prove the PID is ours), + // and do NOT silently prune (the entry may be for a + // legitimately-recoverable sandbox). Keep the entry so + // the operator can investigate; the startup abort + // check on kill_failed > 0 will surface this. + tracing::warn!( + sandbox_id = %id, + pid = pid, + recorded_starttime = ?recorded_starttime, + "cannot verify process identity (no recorded start time, \ + /proc unreadable, or off-Linux); keeping registry entry (fail closed)" + ); + kill_failed += 1; } - } else { - tracing::warn!( - sandbox_id = %id, - pid = pid, - "PID no longer belongs to Firecracker (PID reuse); pruning stale registry entry" - ); - self.inner.lock().sandboxes.remove(&id); - pruned_stale += 1; } } @@ -439,59 +532,156 @@ fn pid_alive(_pid: u32) -> bool { true } -/// Verify that a PID belongs to a Firecracker process by reading -/// `/proc//comm`. Guards against PID reuse: if the original -/// Firecracker process died and the PID was recycled by the OS for -/// a different process, we don't want to kill an unrelated process. +// ---------------------------------------------------------------- +// Durable process identity for orphan recovery (review #299 r6). +// ---------------------------------------------------------------- +// +// `comm == "firecracker"` alone is NOT an identity check: `comm` is +// settable via `prctl(PR_SET_NAME)`, and a Firecracker that exits can +// have its PID recycled by *another* Firecracker before recovery runs. +// Killing on a comm match alone is a cross-tenant DoS vector and a +// same-name PID-reuse regression. +// +// The durable identity is the process start time (field 22 of +// `/proc//stat`, in clock ticks since boot). It is captured at VM +// registration and persisted in `SandboxInfo::proc_starttime`. On +// controller restart, `process_identity_matches` compares the live +// start time against the recorded one. A mismatch means the recorded +// process has exited and the PID was reused — prune the stale entry, +// do NOT kill. The TOCTOU window between the identity check and the +// signal is closed by signalling through a pidfd opened AFTER the +// identity check: the pidfd is pinned to the process that owned the +// PID at `pidfd_open` time, so even if the PID is reused between the +// check and the signal, `pidfd_send_signal` targets the original +// (now possibly-reused) process identity, not whatever currently +// holds the PID. (Linux 5.3+; raw `libc::syscall` because libc 0.2.x +// exposes `SYS_pidfd_open`/`SYS_pidfd_send_signal` but not named fns.) +// +// Off-Linux stubs return identity-unknown so `kill_orphans` prunes +// without killing (the safe path for dev boxes). + +/// Outcome of verifying a recorded PID's identity on recovery. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum IdentityCheck { + /// Live process start time matches the recorded one — same process, + /// safe to signal through the pidfd. + Match, + /// PID is alive but the start time differs — the original process + /// exited and the PID was reused. Prune the stale entry; do NOT kill. + PidReuse, + /// The process is gone (no `/proc/`). Prune the stale entry. + Dead, + /// Identity could not be verified (no recorded start time, or the + /// live start time could not be read, or off-Linux). Fail closed: + /// the caller decides between a conservative comm-only kill or a + /// prune-without-kill depending on policy. + Unknown, +} + +/// Read field 22 (starttime, clock ticks since boot) from +/// `/proc//stat`. Returns `None` if the process is gone or the +/// file cannot be parsed. /// -/// Note: `comm` is settable via `prctl(PR_SET_NAME)` and is not an -/// identity guarantee. On a multi-tenant host where another process -/// could set its name to "firecracker", this check is a best-effort -/// guard, not a security boundary. The TOCTOU window between this -/// check and `kill_pid` is acknowledged — `pidfd_open` + -/// `pidfd_send_signal` would close it entirely (Linux 5.3+). +/// The `comm` field (field 2) is enclosed in parentheses and may +/// itself contain spaces and parentheses, so naive whitespace +/// splitting is wrong. The robust parse: split at the LAST `)` in the +/// line, then tokenize the remainder; starttime is the 20th token +/// after the `)` (field 22 counting `pid` + `comm`). #[cfg(target_os = "linux")] -/// Check whether a PID belongs to a Firecracker process by reading -/// `/proc//comm`. Uses exact match (`== "firecracker"`) to avoid -/// false positives from processes whose names contain the substring. -/// -/// **Limitation:** this is NOT complete PID-reuse protection. If a -/// Firecracker process exits and its PID is reused by another -/// Firecracker process before this check runs, the comm match passes -/// and the new (legitimate) process would be killed. A robust fix -/// would use pidfd signaling plus durable identity validation -/// (process start time recorded with the registry entry at spawn -/// time, compared on startup). That is tracked as a follow-up — the -/// comm check is a defense-in-depth heuristic, not a guarantee. +pub(crate) fn read_proc_starttime(pid: u32) -> Option { + if pid <= 1 { + return None; + } + let stat = std::fs::read_to_string(format!("/proc/{pid}/stat")).ok()?; + // Field 2 (`comm`) is in parentheses and can contain spaces/parens. + // Everything after the closing paren of comm is whitespace-separated. + let after_comm = stat.rfind(')')?; + let rest = &stat[after_comm + 1..]; + // rest begins with a space then: state(3) ppid(4) pgrp(5) ... starttime(22) + // That is 20 fields after `)`: state, ppid, pgrp, session, tty_nr, + // tpgid, flags, minflt, cminflt, majflt, cmajflt, utime, stime, + // cutime, cstime, priority, nice, num_threads, itrealvalue, + // starttime → index 19 (0-based) in the whitespace-split of `rest`. + let fields: Vec<&str> = rest.split_whitespace().collect(); + fields.get(19).and_then(|t| t.parse::().ok()) +} + +#[cfg(not(target_os = "linux"))] +pub(crate) fn read_proc_starttime(_pid: u32) -> Option { + None +} + +/// Compare a recorded process identity against the live process at the +/// same PID. On Linux this is the start-time comparison; off-Linux it is +/// always `Unknown` (no `/proc` to read). #[cfg(target_os = "linux")] -fn pid_is_firecracker(pid: u32) -> bool { - // Defensive guard against pid 0/1 (defense-in-depth for corrupted - // state.json). Real firecracker PIDs are always > 1. +fn process_identity_matches(pid: u32, recorded_starttime: Option) -> IdentityCheck { if pid <= 1 { - return false; + return IdentityCheck::Unknown; + } + let Some(recorded) = recorded_starttime else { + // No recorded identity (old state.json written before this field + // existed). We cannot prove the live PID is ours — fail closed. + return IdentityCheck::Unknown; + }; + let path = format!("/proc/{pid}"); + if !std::path::Path::new(&path).exists() { + return IdentityCheck::Dead; + } + match read_proc_starttime(pid) { + Some(live) if live == recorded => IdentityCheck::Match, + Some(_live) => IdentityCheck::PidReuse, + None => IdentityCheck::Unknown, } - std::fs::read_to_string(format!("/proc/{pid}/comm")) - .map(|s| s.trim() == "firecracker") - .unwrap_or(false) } #[cfg(not(target_os = "linux"))] -fn pid_is_firecracker(_pid: u32) -> bool { - // Off-Linux: can't verify via /proc; return false so - // kill_orphans prunes the entry without sending a signal. - false +fn process_identity_matches(_pid: u32, _recorded_starttime: Option) -> IdentityCheck { + IdentityCheck::Unknown } -/// Send SIGKILL to a process by PID. Uses libc::kill directly -/// (we don't have a std::process::Child handle for orphaned PIDs). +/// Open a pidfd for a live process (Linux 5.3+). Used to close the +/// TOCTOU window between `process_identity_matches` and the signal: +/// the pidfd is pinned to the process that owned the PID at open time, +/// so a PID reuse between check and signal cannot redirect the kill. /// -/// SAFETY: `pid` is a live Linux PID verified by `pid_is_firecracker`; -/// `SIGKILL` is a valid signal constant; `kill(2)` is sound for any -/// `pid_t` value (returns ESRCH if the process doesn't exist). +/// Returns a raw fd on success, or an `io::Error` on failure. The +/// caller owns the fd and must close it (dropping is fine — it's a +/// plain fd, not a Rust-owned handle). #[cfg(target_os = "linux")] -fn kill_pid(pid: u32) -> std::io::Result<()> { - let ret = unsafe { libc::kill(pid as i32, libc::SIGKILL) }; - if ret != 0 { +fn pidfd_open(pid: u32) -> std::io::Result { + // libc 0.2.x exposes the syscall numbers but not named functions. + let ret = unsafe { libc::syscall(libc::SYS_pidfd_open, pid as i32, 0u32) }; + if ret < 0 { + Err(std::io::Error::last_os_error()) + } else { + Ok(ret as std::os::fd::RawFd) + } +} + +#[cfg(not(target_os = "linux"))] +fn pidfd_open(_pid: u32) -> std::io::Result { + Err(std::io::Error::new( + std::io::ErrorKind::Unsupported, + "pidfd_open is Linux-only", + )) +} + +/// Send SIGKILL through a pidfd (Linux 5.3+). Unlike `kill(pid, ...)`, +/// this targets the process the pidfd was pinned to at `pidfd_open` +/// time, eliminating the PID-reuse TOCTOU window entirely. +#[cfg(target_os = "linux")] +fn pidfd_send_kill(pidfd: std::os::fd::RawFd) -> std::io::Result<()> { + let ret = unsafe { + libc::syscall( + libc::SYS_pidfd_send_signal, + pidfd, + libc::SIGKILL, + std::ptr::null::(), + 0u32, + ) + }; + if ret < 0 { Err(std::io::Error::last_os_error()) } else { Ok(()) @@ -499,10 +689,30 @@ fn kill_pid(pid: u32) -> std::io::Result<()> { } #[cfg(not(target_os = "linux"))] -fn kill_pid(_pid: u32) -> std::io::Result<()> { +fn pidfd_send_kill(_pidfd: std::os::fd::RawFd) -> std::io::Result<()> { Ok(()) } +/// Defensive `comm`-name check, used ONLY as a secondary confirmation +/// after the primary start-time identity check passes. This is NOT a +/// security boundary (`comm` is spoofable via `prctl`); it exists to +/// catch corrupted state.json that somehow records a valid-looking +/// start time for the wrong process. Uses exact `== "firecracker"`. +#[cfg(target_os = "linux")] +fn comm_is_firecracker(pid: u32) -> bool { + if pid <= 1 { + return false; + } + std::fs::read_to_string(format!("/proc/{pid}/comm")) + .map(|s| s.trim() == "firecracker") + .unwrap_or(false) +} + +#[cfg(not(target_os = "linux"))] +fn comm_is_firecracker(_pid: u32) -> bool { + false +} + /// Poll for process death by checking `/proc/` disappearance. /// Returns true if the process exited within the timeout, false if it /// is still alive (e.g. stuck in D-state on I/O). @@ -539,6 +749,7 @@ mod tests { netns: Some("forkd-child-1".into()), guest_addr: "10.42.0.2:8888".into(), created_at_unix: 1, + proc_starttime: None, pid: Some(99999999), memory_limit_mib: None, has_branched: false, @@ -690,15 +901,23 @@ mod tests { reloaded.list_sandboxes().len(), WORKERS * MUTATIONS_PER_WORKER ); + } + + #[test] fn kill_orphans_prunes_alive_pid_entries() { let td = TempDir::new().unwrap(); let path = td.path().join("state.json"); let r = Registry::load_or_init(&path).unwrap(); - // Insert a sandbox with an alive PID (use our own process PID). - // On Linux, pid_is_firecracker will return false (we're not - // firecracker), so the entry is pruned without killing. - // On non-Linux, pid_is_firecracker always returns false. + // Insert a sandbox with an alive PID (our own process PID) but a + // DELIBERATELY MISMATCHED start time. This simulates PID reuse: + // the original Firecracker exited, the kernel recycled the PID + // for an unrelated process (us), and the recorded start time no + // longer matches. The durable-identity check must detect this + // and prune the stale entry WITHOUT killing the unrelated process. + // + // The bogus start time (u64::MAX) cannot match any real process + // (real start times are small clock-tick counts since boot). r.insert_sandbox(SandboxInfo { id: "sb-orphan".into(), snapshot_tag: "py".into(), @@ -706,6 +925,7 @@ mod tests { guest_addr: "10.42.0.2:8888".into(), created_at_unix: 1, pid: Some(std::process::id()), + proc_starttime: Some(u64::MAX), // mismatched → PidReuse memory_limit_mib: None, has_branched: false, last_branch_memory_path: None, @@ -715,7 +935,8 @@ mod tests { // Insert a sandbox with a dead PID (99999999 — not alive on Linux). // On macOS, pid_alive always returns true, so this entry survives - // reconcile() and is pruned by kill_orphans() instead. + // reconcile() and is pruned by kill_orphans() instead (as Dead on + // Linux, or PidReuse via the mismatched start time on macOS). r.insert_sandbox(SandboxInfo { id: "sb-dead".into(), snapshot_tag: "py".into(), @@ -723,6 +944,7 @@ mod tests { guest_addr: "10.42.0.3:8888".into(), created_at_unix: 2, pid: Some(99999999), + proc_starttime: Some(u64::MAX), // mismatched → PidReuse memory_limit_mib: None, has_branched: false, last_branch_memory_path: None, @@ -734,22 +956,118 @@ mod tests { // where pid_alive always returns true). let _pruned = r.reconcile().unwrap(); - // kill_orphans prunes all remaining entries (alive PID but - // not firecracker → pruned without killing). + // kill_orphans prunes all remaining entries via the durable + // identity check (start time mismatch → PidReuse → prune stale). let result = r.kill_orphans().unwrap(); - // At least the alive-PID entry is pruned. - assert!(result.killed + result.pruned_stale >= 1); + // At least one entry is pruned (the alive-PID one on Linux; both + // on macOS where reconcile leaves both). + assert!(result.killed + result.pruned_stale >= 1, "nothing pruned"); + // Nothing was killed — the mismatched start time means we never + // believed the PID was our Firecracker, so we pruned, not killed. + assert_eq!(result.killed, 0, "should not kill on identity mismatch"); // All sandbox entries are gone. assert!(r.list_sandboxes().is_empty()); } + /// New: an alive-PID entry with NO recorded start time (old state.json + /// written before proc_starttime existed) must FAIL CLOSED — the entry + // is kept and kill_failed is incremented, so the caller aborts startup + // rather than risking killing an unrelated process. This is the + // cross-platform (no /proc required) contract test for the fail-closed + // behavior the reviewer insisted on. + #[test] + fn kill_orphans_fails_closed_on_unknown_identity() { + let td = TempDir::new().unwrap(); + let path = td.path().join("state.json"); + let r = Registry::load_or_init(&path).unwrap(); + + // Alive PID (our own), no recorded start time → Unknown → fail closed. + r.insert_sandbox(SandboxInfo { + id: "sb-unknown".into(), + snapshot_tag: "py".into(), + netns: Some("forkd-child-1".into()), + guest_addr: "10.42.0.2:8888".into(), + created_at_unix: 1, + pid: Some(std::process::id()), + proc_starttime: None, // old state.json — identity unknown + memory_limit_mib: None, + has_branched: false, + last_branch_memory_path: None, + branch_count: 0, + }) + .unwrap(); + + let result = r.kill_orphans().unwrap(); + // Fail closed: nothing killed, nothing pruned, kill_failed incremented. + assert_eq!(result.killed, 0, "must not kill with unknown identity"); + assert_eq!( + result.pruned_stale, 0, + "must not prune with unknown identity" + ); + assert_eq!( + result.kill_failed, 1, + "must fail-closed on unknown identity" + ); + // The entry is KEPT so the operator can investigate. + assert_eq!( + r.list_sandboxes().len(), + 1, + "entry must be retained on unknown identity" + ); + } + + /// Regression for the same-name Firecracker PID-reuse attack the + /// reviewer flagged: a Firecracker exits, the kernel recycles its PID + /// for ANOTHER Firecracker (same comm name), and the recorded start + /// time no longer matches. The old comm-only check would kill the + /// legitimate new Firecracker; the durable-identity check must detect + /// the start-time mismatch and prune the stale entry WITHOUT killing. + /// + /// We simulate this by recording our own PID (alive) with a bogus + /// start time. On Linux `comm_is_firecracker` would return false for + /// us, but the PRIMARY check (start time) fires first and returns + /// PidReuse before comm is ever consulted — so the entry is pruned as + /// stale regardless of comm. This is the same-name reuse regression: + /// the test fails if kill_orphans ever relies on comm alone. + #[test] + fn kill_orphans_prunes_same_name_pid_reuse_without_killing() { + let td = TempDir::new().unwrap(); + let path = td.path().join("state.json"); + let r = Registry::load_or_init(&path).unwrap(); + + r.insert_sandbox(SandboxInfo { + id: "sb-reused".into(), + snapshot_tag: "py".into(), + netns: Some("forkd-child-1".into()), + guest_addr: "10.42.0.2:8888".into(), + created_at_unix: 1, + pid: Some(std::process::id()), + proc_starttime: Some(0), // real start time is > 0; 0 cannot match + memory_limit_mib: None, + has_branched: false, + last_branch_memory_path: None, + branch_count: 0, + }) + .unwrap(); + + let result = r.kill_orphans().unwrap(); + // The start-time mismatch (0 vs the real start time) must yield + // PidReuse → pruned_stale, NOT killed. This is the crux: even if + // comm were "firecracker", the mismatched start time prevents + // the kill. + assert_eq!(result.killed, 0, "must not kill on start-time mismatch"); + assert!(result.pruned_stale >= 1, "must prune stale on PID reuse"); + assert!(r.list_sandboxes().is_empty()); + } + #[test] fn kill_orphans_marks_workspaces_stale() { let td = TempDir::new().unwrap(); let path = td.path().join("state.json"); let r = Registry::load_or_init(&path).unwrap(); - // Insert a sandbox with an alive PID. + // Insert a sandbox with an alive PID and a MISMATCHED start time + // so the durable-identity check yields PidReuse → pruned (not killed). r.insert_sandbox(SandboxInfo { id: "sb-1".into(), snapshot_tag: "py".into(), @@ -757,6 +1075,7 @@ mod tests { guest_addr: "10.42.0.2:8888".into(), created_at_unix: 1, pid: Some(std::process::id()), + proc_starttime: Some(u64::MAX), // mismatched → PidReuse memory_limit_mib: None, has_branched: false, last_branch_memory_path: None, @@ -779,9 +1098,10 @@ mod tests { }) .unwrap(); - // kill_orphans kills the sandbox and marks the workspace Stale. + // kill_orphans prunes the stale sandbox (PID reuse) and marks the + // workspace Stale. Nothing is killed (identity mismatch). let result = r.kill_orphans().unwrap(); - assert_eq!(result.killed, 0); // not firecracker → not killed + assert_eq!(result.killed, 0); // identity mismatch → not killed assert_eq!(result.pruned_stale, 1); // pruned as stale assert!(r.list_sandboxes().is_empty()); @@ -816,6 +1136,7 @@ mod tests { netns: None, guest_addr: "10.42.0.2:8888".into(), created_at_unix: 1, + proc_starttime: None, pid: None, memory_limit_mib: None, has_branched: false, @@ -839,7 +1160,8 @@ mod tests { let path = td.path().join("state.json"); let r = Registry::load_or_init(&path).unwrap(); - // Insert a sandbox with an alive PID. + // Insert a sandbox with an alive PID and a MISMATCHED start time + // so the durable-identity check yields PidReuse → pruned (not killed). r.insert_sandbox(SandboxInfo { id: "sb-1".into(), snapshot_tag: "py".into(), @@ -847,6 +1169,7 @@ mod tests { guest_addr: "10.42.0.2:8888".into(), created_at_unix: 1, pid: Some(std::process::id()), + proc_starttime: Some(u64::MAX), // mismatched → PidReuse memory_limit_mib: None, has_branched: false, last_branch_memory_path: None, @@ -898,6 +1221,7 @@ mod tests { guest_addr: "10.42.0.2:8888".into(), created_at_unix: 1, pid: Some(std::process::id()), + proc_starttime: Some(u64::MAX), // mismatched → PidReuse memory_limit_mib: None, has_branched: false, last_branch_memory_path: None, @@ -926,6 +1250,7 @@ mod tests { netns: Some("forkd-child-1".into()), guest_addr: "10.42.0.2:8888".into(), created_at_unix: 1, + proc_starttime: Some(u64::MAX), // mismatched → PidReuse if it reaches kill_orphans pid: Some(99999999), memory_limit_mib: None, has_branched: false, @@ -934,7 +1259,7 @@ mod tests { }) .unwrap(); - // Alive PID entry (pruned by kill_orphans). + // Alive PID entry (pruned by kill_orphans via PidReuse). r.insert_sandbox(SandboxInfo { id: "sb-alive".into(), snapshot_tag: "py".into(), @@ -942,6 +1267,7 @@ mod tests { guest_addr: "10.42.0.3:8888".into(), created_at_unix: 2, pid: Some(std::process::id()), + proc_starttime: Some(u64::MAX), // mismatched → PidReuse memory_limit_mib: None, has_branched: false, last_branch_memory_path: None, From cbe55f2930e799b4095c7306cbfccf8c9ec6cbf1 Mon Sep 17 00:00:00 2001 From: jrimmer Date: Mon, 17 Aug 2026 00:54:03 -0700 Subject: [PATCH 07/11] fix(controller): close pidfd TOCTOU + ce-code-review r6 follow-ups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address ce-code-review findings (security P1, testing P1, adversarial P2/P3, plus the cross-reviewer coverage gaps) on the durable-identity rework from 2e275fe. 1. Residual TOCTOU window (security P1, adversarial ADV-1): pidfd closes only the open→signal window, NOT the check→open window. The original could exit and the PID could be reused by another Firecracker between process_identity_matches (which reads /proc//stat) and pidfd_open. Fix: after pidfd_open succeeds, re-read the live start time and compare to the recorded one. A mismatch means the PID was reused between the check and the open → prune without killing (PidReuse). The pidfd pins the process at open time, so /proc/ reflects the pinned process. This closes the check→open window; the comm check is now a pure sanity check, not a load-bearing defense. Design comment corrected to not overstate the guarantee. 2. Off-Linux comment/behavior mismatch (adversarial ADV-2): the module comment claimed off-Linux "prunes without killing" but Unknown actually fails closed (keeps entry, increments kill_failed, run_daemon aborts). Fixed the comment to state the real behavior. 3. Cross-platform test guards (adversarial ADV-3): the PidReuse- asserting tests (mismatched starttime → pruned_stale) now carry #[cfg(target_os = "linux")] because off-Linux returns Unknown (kill_failed), not PidReuse. The cross-platform fail-closed test (proc_starttime: None → Unknown) stays ungated. 4. Match-arm coverage (testing P1, +reliability/security/adversarial): new kill_orphans_match_arm_kills_verified_child_via_pidfd spawns a real child, records its true start time (Match), and asserts the secondary comm_is_firecracker check fails closed for a non-firecracker process — proving the Match arm is entered and the comm gate works as defense-in-depth. (Killing a real Firecracker would require a Firecracker binary; the comm gate is the testable boundary here.) 5. start-time parser unit test (testing P2): read_proc_starttime now has a direct test (parses our own /proc//stat, rejects pid<=1, returns None for nonexistent PID). 6. Backward-compat serde test (testing safe_auto): new proc_starttime_defaults_to_none_when_absent_in_old_state_json deserializes a hand-written pre-r6 state.json (no proc_starttime key) and asserts #[serde(default)] yields None — the contract that lets existing deployments upgrade without wiping state. Verified on Linux (rust 1.83, x86_64) via Docker: clippy clean for forkd-controller; cargo test --all-features passes 107 unit + 8 integration tests, 0 failed (3 new tests pass). Signed-off-by: jrimmer --- crates/forkd-controller/src/state.rs | 236 +++++++++++++++++++++++++-- 1 file changed, 221 insertions(+), 15 deletions(-) diff --git a/crates/forkd-controller/src/state.rs b/crates/forkd-controller/src/state.rs index baf68aa..71a5802 100644 --- a/crates/forkd-controller/src/state.rs +++ b/crates/forkd-controller/src/state.rs @@ -351,11 +351,21 @@ impl Registry { IdentityCheck::Match => { // Same process (start time matches). Open a pidfd // NOW, after the identity check, so the signal is - // pinned to the verified process. This closes the - // TOCTOU window: even if the PID is reused between - // here and pidfd_send_signal, the fd targets the - // original process identity, not whatever currently - // holds the PID. + // pinned to the process that currently holds the PID. + // This narrows the TOCTOU window to the open→signal + // interval: even if the PID is reused between here and + // pidfd_send_signal, the fd targets whatever was pinned + // at open time, not whatever currently holds the PID. + // + // There is a residual window between the starttime + // read (in process_identity_matches) and pidfd_open: + // the original could exit and the PID could be recycled + // by another Firecracker in that gap. We close it by + // re-reading the start time AFTER pidfd_open succeeds and + // comparing again. Because pidfd pins the process at + // open time, /proc/ reflects the pinned process; + // a mismatch means the PID was reused between the check + // and the open → prune without killing (PidReuse). let pidfd = match pidfd_open(pid) { Ok(fd) => fd, Err(e) if e.raw_os_error() == Some(libc::ESRCH) => { @@ -386,6 +396,34 @@ impl Registry { } }; + // Re-verify the start time AFTER pidfd_open. This + // closes the check→open TOCTOU window: if the PID was + // reused between process_identity_matches and + // pidfd_open, the live start time now differs from + // the recorded one. Prune without killing (the pinned + // process is not ours). Recorded_starttime is Some by + // construction here (Match requires it), but guard for + // clarity. + if let Some(recorded) = recorded_starttime { + match read_proc_starttime(pid) { + Some(live) if live == recorded => { + // pidfd-pinned process is the original. + } + _ => { + let _ = unsafe { libc::close(pidfd) }; + tracing::warn!( + sandbox_id = %id, + pid = pid, + "PID reused between identity check and pidfd_open \ + (start time changed); pruning without killing" + ); + self.inner.lock().sandboxes.remove(&id); + pruned_stale += 1; + continue; + } + } + } + // Secondary confirmation: the comm name should still // be firecracker. This is NOT a security boundary (comm // is spoofable) — it catches corrupted state.json that @@ -548,17 +586,22 @@ fn pid_alive(_pid: u32) -> bool { // controller restart, `process_identity_matches` compares the live // start time against the recorded one. A mismatch means the recorded // process has exited and the PID was reused — prune the stale entry, -// do NOT kill. The TOCTOU window between the identity check and the -// signal is closed by signalling through a pidfd opened AFTER the -// identity check: the pidfd is pinned to the process that owned the -// PID at `pidfd_open` time, so even if the PID is reused between the -// check and the signal, `pidfd_send_signal` targets the original -// (now possibly-reused) process identity, not whatever currently -// holds the PID. (Linux 5.3+; raw `libc::syscall` because libc 0.2.x -// exposes `SYS_pidfd_open`/`SYS_pidfd_send_signal` but not named fns.) +// do NOT kill. Signalling through a pidfd opened AFTER the identity +// check narrows the TOCTOU window to the open→signal interval (the +// pidfd is pinned to the process that owned the PID at `pidfd_open` +// time, so a PID reuse after the open cannot redirect the signal). +// The residual check→open window is closed by RE-VERIFYING the +// start time after `pidfd_open` succeeds: if the live start time +// differs from the recorded one, the PID was reused between the +// check and the open, so we prune without killing. (Linux 5.3+; +// raw `libc::syscall` because libc 0.2.x exposes +// `SYS_pidfd_open`/`SYS_pidfd_send_signal` but not named fns.) // -// Off-Linux stubs return identity-unknown so `kill_orphans` prunes -// without killing (the safe path for dev boxes). +// Off-Linux stubs return identity-unknown so `kill_orphans` FAILS +// CLOSED (keeps the entry, increments kill_failed) rather than +// killing — the safe path for dev boxes, since killing an +// unidentifiable process is unsafe. (Not a prune: a prune would +// silently drop a possibly-live sandbox's state.) /// Outcome of verifying a recorded PID's identity on recovery. #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -904,6 +947,7 @@ mod tests { } #[test] + #[cfg(target_os = "linux")] // PidReuse path requires /proc starttime comparison fn kill_orphans_prunes_alive_pid_entries() { let td = TempDir::new().unwrap(); let path = td.path().join("state.json"); @@ -1030,6 +1074,7 @@ mod tests { /// stale regardless of comm. This is the same-name reuse regression: /// the test fails if kill_orphans ever relies on comm alone. #[test] + #[cfg(target_os = "linux")] // PidReuse path requires /proc starttime comparison fn kill_orphans_prunes_same_name_pid_reuse_without_killing() { let td = TempDir::new().unwrap(); let path = td.path().join("state.json"); @@ -1061,6 +1106,7 @@ mod tests { } #[test] + #[cfg(target_os = "linux")] // PidReuse path requires /proc starttime comparison fn kill_orphans_marks_workspaces_stale() { let td = TempDir::new().unwrap(); let path = td.path().join("state.json"); @@ -1155,6 +1201,7 @@ mod tests { } #[test] + #[cfg(target_os = "linux")] // PidReuse path requires /proc starttime comparison fn kill_orphans_marks_multiple_workspaces_stale() { let td = TempDir::new().unwrap(); let path = td.path().join("state.json"); @@ -1209,6 +1256,7 @@ mod tests { } #[test] + #[cfg(target_os = "linux")] // PidReuse path requires /proc starttime comparison fn kill_orphans_persists_to_disk() { let td = TempDir::new().unwrap(); let path = td.path().join("state.json"); @@ -1238,6 +1286,7 @@ mod tests { } #[test] + #[cfg(target_os = "linux")] // PidReuse path requires /proc starttime comparison fn kill_orphans_reconcile_then_kill_integration() { let td = TempDir::new().unwrap(); let path = td.path().join("state.json"); @@ -1378,4 +1427,161 @@ mod tests { "error should explain the failure, got: {err}" ); } + + // ---------------------------------------------------------------- + // ce-code-review r6 follow-ups: Match-path coverage, starttime parser + // unit test, and backward-compat serde default for proc_starttime. + // ---------------------------------------------------------------- + + /// Backward-compat: an old `state.json` written before `proc_starttime` + /// existed must still deserialize (the field is `#[serde(default)]`), + /// yielding `proc_starttime: None`. This is the contract that lets + /// existing deployments upgrade without wiping state. Cross-platform + /// (no /proc required). + #[test] + fn proc_starttime_defaults_to_none_when_absent_in_old_state_json() { + let td = TempDir::new().unwrap(); + let path = td.path().join("state.json"); + // Hand-written old-format JSON: every SandboxInfo field EXCEPT + // proc_starttime (simulating a pre-r6 state.json). + let old_json = r#"{ + "sandboxes": { + "sb-legacy": { + "id": "sb-legacy", + "snapshot_tag": "py", + "netns": "forkd-child-1", + "guest_addr": "10.42.0.2:8888", + "created_at_unix": 1, + "pid": 4242, + "memory_limit_mib": null, + "has_branched": false, + "last_branch_memory_path": null, + "branch_count": 0 + } + }, + "workspaces": {} + }"#; + std::fs::write(&path, old_json).unwrap(); + + let r = Registry::load_or_init(&path).unwrap(); + let sbs = r.list_sandboxes(); + assert_eq!(sbs.len(), 1, "legacy entry must load"); + assert_eq!(sbs[0].id, "sb-legacy"); + assert_eq!( + sbs[0].proc_starttime, None, + "proc_starttime must default to None when absent (backward compat)" + ); + } + + /// Unit test for `read_proc_starttime` field-22 parsing. Covers the + /// parenthesized-comm edge case (comm containing parens/spaces) that + /// naive whitespace splitting gets wrong. Linux-only (needs /proc). + #[test] + #[cfg(target_os = "linux")] + fn read_proc_starttime_parses_our_own_stat() { + // Our own PID's starttime must be parseable and > 0 (clock ticks + // since boot; the boot-relative value is always positive while + // the system is up). + let own = std::process::id(); + let st = read_proc_starttime(own); + assert!(st.is_some(), "could not read /proc/{own}/stat starttime"); + assert!( + st.unwrap() > 0, + "starttime should be > 0 while system is up" + ); + + // PID 1 (init) is deliberately rejected by the parser guard. + assert_eq!(read_proc_starttime(1), None, "pid <= 1 must be rejected"); + + // A non-existent PID returns None. + assert_eq!( + read_proc_starttime(99_999_999), + None, + "nonexistent PID must return None" + ); + } + + /// The `Match` kill lifecycle (pidfd_open → start-time re-verification → + /// comm check → pidfd_send_kill → wait_for_death → close) has zero + /// coverage in the r6 tests — every other test uses a mismatched or + /// absent start time so the `Match` arm is never entered. This test + /// spawns a real child process, records its TRUE start time, inserts + /// a sandbox entry for it, and asserts `kill_orphans` actually kills + /// it via the pidfd path and prunes the registry entry. Linux-only + /// (pidfd_open + /proc). + #[test] + #[cfg(target_os = "linux")] + fn kill_orphans_match_arm_kills_verified_child_via_pidfd() { + use std::process::{Command, Stdio}; + use std::time::Duration; + + let td = TempDir::new().unwrap(); + let path = td.path().join("state.json"); + let r = Registry::load_or_init(&path).unwrap(); + + // Spawn a child that sleeps long enough for the test to run. + // `sleep 30` is a normal binary whose comm is "sleep", NOT + // "firecracker" — so the secondary comm_is_firecracker check will + // fail closed (kill_failed) and the entry will be KEPT. This is + // the CORRECT behavior for a non-firecracker process even when + // the start time matches: we only ever kill verified Firecrackers. + // + // To exercise the FULL Match kill path we would need a process + // named "firecracker"; instead this test asserts the fail-closed + // contract at the comm gate: a matching start time alone does NOT + // authorize a kill — the comm check must also pass. This is the + // defense-in-depth property the security review required. + let mut child = Command::new("sleep") + .arg("30") + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .spawn() + .expect("spawn sleep"); + let pid = child.id(); + + // Record the child's REAL start time so process_identity_matches + // returns Match (the primary identity check passes). + let starttime = read_proc_starttime(pid).expect("read child starttime"); + + r.insert_sandbox(SandboxInfo { + id: "sb-match-child".into(), + snapshot_tag: "py".into(), + netns: Some("forkd-child-1".into()), + guest_addr: "10.42.0.2:8888".into(), + created_at_unix: 1, + pid: Some(pid), + proc_starttime: Some(starttime), + memory_limit_mib: None, + has_branched: false, + last_branch_memory_path: None, + branch_count: 0, + }) + .unwrap(); + + let result = r.kill_orphans().unwrap(); + // The start time matched (Match) and pidfd_open + re-verification + // succeeded, but comm_is_firecracker("sleep") is false → fail + // closed: nothing killed, nothing pruned, kill_failed incremented, + // entry retained. This proves the Match arm was entered and the + // comm gate works as a secondary defense. + assert_eq!(result.killed, 0, "must not kill a non-firecracker process"); + assert_eq!(result.pruned_stale, 0, "must not prune on comm mismatch"); + assert_eq!( + result.kill_failed, 1, + "Match arm reached but comm check failed closed (kill_failed)" + ); + assert_eq!( + r.list_sandboxes().len(), + 1, + "entry must be retained on comm-mismatch fail-closed" + ); + + // Clean up the still-alive child. + let _ = child.kill(); + let _ = child.wait_with_output(); + // Give the kernel a moment to reap so wait_for_death-style polls + // see the process as gone. + std::thread::sleep(Duration::from_millis(100)); + } } From 1f0d260d080da3c904f4ff78b1f8c5fe6ffc5a12 Mon Sep 17 00:00:00 2001 From: jrimmer Date: Thu, 20 Aug 2026 17:08:32 -0700 Subject: [PATCH 08/11] fix(controller): add SIGKILL success-path regression + correct misleading comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review r8 (2026-08-20) found that kill_orphans_match_arm_kills_verified_child_via_pidfd did not do what its name claimed: it spawned 'sleep' (comm='sleep'), so the secondary comm_is_firecracker check failed closed and the irreversible success path (pidfd_send_kill -> wait_for_death -> registry removal) was never executed. - Rename the existing test to kill_orphans_match_arm_comm_mismatch_fails_closed so its name and doc accurately describe the comm-gate fail-closed branch it covers. - Add kill_orphans_match_arm_kills_verified_firecracker_via_pidfd, which spawns a disposable process whose /proc//comm is literally 'firecracker' (a copy of 'sleep' renamed to 'firecracker'), records its true start time, and asserts killed == 1, kill_failed == 0, pruned_stale == 0, and the registry entry is removed — exercising the full irreversible pidfd kill path. Also correct two misleading comments flagged in the same review: - SandboxInfo::proc_starttime claimed legacy None state may use comm-only verification; the implementation fails closed (keeps the entry, increments kill_failed) and never does comm-only kill. - IdentityCheck::Unknown claimed the caller chooses between a comm-only kill or prune; kill_orphans keeps the entry and aborts startup via kill_failed. Signed-off-by: jrimmer --- crates/forkd-controller/src/api.rs | 5 +- crates/forkd-controller/src/state.rs | 170 +++++++++++++++++++++++---- 2 files changed, 150 insertions(+), 25 deletions(-) diff --git a/crates/forkd-controller/src/api.rs b/crates/forkd-controller/src/api.rs index d0beaa4..b242c04 100644 --- a/crates/forkd-controller/src/api.rs +++ b/crates/forkd-controller/src/api.rs @@ -315,8 +315,9 @@ pub struct SandboxInfo { /// /// `#[serde(default)]` keeps existing `state.json` files loadable /// (entries written before this field existed deserialize to - /// `None`, which is treated as "identity unknown — verify by - /// comm only, fail closed if the check is inconclusive"). + /// `None`, which is treated as "identity unknown — fail closed: + /// keep the entry and increment `kill_failed` rather than risk + /// killing an unidentifiable process"). #[serde(default)] pub proc_starttime: Option, pub memory_limit_mib: Option, diff --git a/crates/forkd-controller/src/state.rs b/crates/forkd-controller/src/state.rs index 71a5802..72b9116 100644 --- a/crates/forkd-controller/src/state.rs +++ b/crates/forkd-controller/src/state.rs @@ -616,8 +616,9 @@ enum IdentityCheck { Dead, /// Identity could not be verified (no recorded start time, or the /// live start time could not be read, or off-Linux). Fail closed: - /// the caller decides between a conservative comm-only kill or a - /// prune-without-kill depending on policy. + /// `kill_orphans` keeps the entry and increments `kill_failed` + /// (aborting startup via `check_orphan_kill_result`) rather than + /// risk killing an unidentifiable process. Unknown, } @@ -1501,17 +1502,18 @@ mod tests { ); } - /// The `Match` kill lifecycle (pidfd_open → start-time re-verification → - /// comm check → pidfd_send_kill → wait_for_death → close) has zero - /// coverage in the r6 tests — every other test uses a mismatched or - /// absent start time so the `Match` arm is never entered. This test - /// spawns a real child process, records its TRUE start time, inserts - /// a sandbox entry for it, and asserts `kill_orphans` actually kills - /// it via the pidfd path and prunes the registry entry. Linux-only - /// (pidfd_open + /proc). + /// The secondary `comm_is_firecracker` check inside the `Match` arm + /// is a defense-in-depth guard against corrupted `state.json` that + /// somehow recorded a valid-looking start time for a non-firecracker + /// process. This test exercises that fail-closed branch: it spawns a + /// real child (`sleep`, whose comm is "sleep"), records its TRUE start + /// time so `process_identity_matches` returns `Match`, inserts a + /// sandbox entry, and asserts `kill_orphans` does NOT kill the + /// non-firecracker process — it keeps the entry and increments + /// `kill_failed`. Linux-only (pidfd_open + /proc). #[test] #[cfg(target_os = "linux")] - fn kill_orphans_match_arm_kills_verified_child_via_pidfd() { + fn kill_orphans_match_arm_comm_mismatch_fails_closed() { use std::process::{Command, Stdio}; use std::time::Duration; @@ -1519,18 +1521,11 @@ mod tests { let path = td.path().join("state.json"); let r = Registry::load_or_init(&path).unwrap(); - // Spawn a child that sleeps long enough for the test to run. - // `sleep 30` is a normal binary whose comm is "sleep", NOT - // "firecracker" — so the secondary comm_is_firecracker check will - // fail closed (kill_failed) and the entry will be KEPT. This is - // the CORRECT behavior for a non-firecracker process even when - // the start time matches: we only ever kill verified Firecrackers. - // - // To exercise the FULL Match kill path we would need a process - // named "firecracker"; instead this test asserts the fail-closed - // contract at the comm gate: a matching start time alone does NOT - // authorize a kill — the comm check must also pass. This is the - // defense-in-depth property the security review required. + // Spawn a child whose comm is "sleep" (NOT "firecracker"). + // Its TRUE start time is recorded so `process_identity_matches` + // returns `Match` and the pidfd path is entered, but the + // secondary `comm_is_firecracker` check then fails closed — + // proving a matching start time alone does NOT authorize a kill. let mut child = Command::new("sleep") .arg("30") .stdin(Stdio::null()) @@ -1584,4 +1579,133 @@ mod tests { // see the process as gone. std::thread::sleep(Duration::from_millis(100)); } + + /// The `Match` kill lifecycle (`pidfd_open` → start-time re-verification → + /// `comm_is_firecracker` → `pidfd_send_kill` → `wait_for_death` → + /// registry removal) had zero coverage — every other test used a + /// mismatched/absent start time or a non-firecracker comm so the + /// irreversible success path was never executed. This test spawns a + /// disposable process whose `/proc//comm` is literally + /// `firecracker` (a copy of `sleep` renamed to `firecracker`, so the + /// binary runs but reports the expected comm), records its TRUE start + /// time, inserts a sandbox entry, and asserts `kill_orphans` actually + /// kills it via the pidfd path: `killed == 1`, `kill_failed == 0`, and + /// the registry entry is removed. Linux-only (pidfd_open + /proc). + /// + /// The child is **double-forked via a shell** so it is reparented to + /// init (PID 1), not held as a child of this test process. This mirrors + /// production: real Firecracker orphans are NOT children of the + /// controller, so when `kill_orphans` SIGKILLs them, init reaps them + /// and `/proc/` disappears quickly. A child of the test process + /// would instead linger as a zombie (held by the test until reaped), + /// causing `wait_for_death` to time out and report `kill_failed`. + #[test] + #[cfg(target_os = "linux")] + fn kill_orphans_match_arm_kills_verified_firecracker_via_pidfd() { + use std::process::{Command, Stdio}; + + let td = TempDir::new().unwrap(); + let path = td.path().join("state.json"); + let r = Registry::load_or_init(&path).unwrap(); + + // Create a disposable binary whose /proc//comm is "firecracker". + // /proc//comm is derived from the executable's basename + // (truncated to 15 chars), so a copy of `sleep` named `firecracker` + // reports comm == "firecracker" while running the real `sleep` binary. + let firecracker_bin = td.path().join("firecracker"); + std::fs::copy( + std::env::var("FORKD_TEST_SLEEP_BIN") + .map(std::path::PathBuf::from) + .unwrap_or_else(|_| { + // /bin/sleep and /usr/bin/sleep are the usual locations; + // resolve via `which`-like lookup against common dirs. + ["/bin/sleep", "/usr/bin/sleep"] + .into_iter() + .map(std::path::PathBuf::from) + .find(|p| p.exists()) + .expect("sleep binary not found in /bin/sleep or /usr/bin/sleep") + }), + &firecracker_bin, + ) + .expect("copy sleep → firecracker"); + + // Double-fork via `sh -c '... & echo $!'` so the firecracker-named + // process is reparented to init (PID 1), not held as a child of this + // test. The shell backgrounds the process, prints its PID, and exits; + // the backgrounded process is then reparented to init. This ensures + // `wait_for_death` sees `/proc/` disappear after SIGKILL + // (init reaps the reparented process immediately), rather than + // timing out on a zombie held by the test. + let sh_out = Command::new("sh") + .arg("-c") + .arg(format!("{} 30 & echo $!", firecracker_bin.display())) + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::null()) + .output() + .expect("spawn detached firecracker-named sleep via sh"); + let pid: u32 = String::from_utf8_lossy(&sh_out.stdout) + .trim() + .parse() + .expect("parse detached firecracker child PID from sh output"); + + // Sanity: confirm /proc//comm really is "firecracker". + let comm = std::fs::read_to_string(format!("/proc/{pid}/comm")) + .expect("read /proc//comm"); + assert_eq!( + comm.trim(), + "firecracker", + "test harness requires comm == firecracker, got {comm:?}" + ); + + // Record the child's REAL start time so process_identity_matches + // returns Match (the primary identity check passes). + let starttime = read_proc_starttime(pid).expect("read child starttime"); + + r.insert_sandbox(SandboxInfo { + id: "sb-firecracker-match".into(), + snapshot_tag: "py".into(), + netns: Some("forkd-child-1".into()), + guest_addr: "10.42.0.2:8888".into(), + created_at_unix: 1, + pid: Some(pid), + proc_starttime: Some(starttime), + memory_limit_mib: None, + has_branched: false, + last_branch_memory_path: None, + branch_count: 0, + }) + .unwrap(); + + let result = r.kill_orphans().unwrap(); + // The irreversible success path executed: the verified + // firecracker-named process was killed via pidfd_send_kill and + // waited for death, and the registry entry was removed. + assert_eq!( + result.killed, 1, + "verified firecracker-named child must be killed, got killed={} \ + (kill_failed={}, pruned_stale={})", + result.killed, result.kill_failed, result.pruned_stale + ); + assert_eq!( + result.kill_failed, 0, + "kill must succeed for a verified firecracker-named child" + ); + assert_eq!( + result.pruned_stale, 0, + "a killed child is counted under `killed`, not `pruned_stale`" + ); + assert_eq!( + r.list_sandboxes().len(), + 0, + "registry entry must be removed after a successful kill" + ); + + // Defense-in-depth cleanup: if the test failed an assertion above + // before kill_orphans ran (or kill_orphans somehow did not reap the + // reparented process), ensure the detached process is not leaked. + // It was reparented to init so we don't own a Child handle; signal + // it directly by PID (no-op if already reaped by init). + let _ = unsafe { libc::kill(pid as i32, libc::SIGKILL) }; + } } From 9374994b56a9bb247dce1923dcd0cc1d5d5e2d0c Mon Sep 17 00:00:00 2001 From: jrimmer Date: Thu, 20 Aug 2026 17:30:03 -0700 Subject: [PATCH 09/11] style: cargo fmt on new firecracker-named kill test Signed-off-by: jrimmer --- crates/forkd-controller/src/state.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/forkd-controller/src/state.rs b/crates/forkd-controller/src/state.rs index 72b9116..d0b40c0 100644 --- a/crates/forkd-controller/src/state.rs +++ b/crates/forkd-controller/src/state.rs @@ -1650,8 +1650,8 @@ mod tests { .expect("parse detached firecracker child PID from sh output"); // Sanity: confirm /proc//comm really is "firecracker". - let comm = std::fs::read_to_string(format!("/proc/{pid}/comm")) - .expect("read /proc//comm"); + let comm = + std::fs::read_to_string(format!("/proc/{pid}/comm")).expect("read /proc//comm"); assert_eq!( comm.trim(), "firecracker", From f6ef418d31b19638f1a383077c69b99c96e66b83 Mon Sep 17 00:00:00 2001 From: jrimmer Date: Sat, 22 Aug 2026 12:37:10 -0700 Subject: [PATCH 10/11] fix(controller): persist boot id + fail closed on pid-less entries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review #299 (both blockers) and rebases onto dev with #312's single-lock reconcile preserved. Blocker 1 — cross-reboot false Match (PID+starttick collision): proc_starttime is ticks since boot, only unique within one host boot. The registry persists across reboots, so after a reboot an unrelated Firecracker can share both the numeric PID and the boot-relative start tick of a recorded entry, and a bare starttime check would SIGKILL the wrong process. Persist /proc/sys/kernel/random/boot_id at registration (SandboxInfo.boot_id) and gate the identity check on it: - different boot id -> PidReuse (prune WITHOUT signaling; the old process cannot still exist across a reboot) - missing recorded boot id (legacy state.json) -> Unknown (fail closed) - live boot id unreadable -> Unknown (fail closed) New read_boot_id() helper; recorded at all three production VM registration sites in http.rs. Blocker 2 — pid: None entries recreated the #298 collision risk: Such rows were skipped by both reconcile and kill_orphans, so startup succeeded with an empty allocator/shared-tap ownership while a live VM may still hold those resources. kill_orphans now counts retained rows without a PID into KillOrphansResult.unresolved, and check_orphan_kill_result aborts startup on unresolved > 0 (fail closed). Rebase: preserved #312's single-lock reconcile (whole in-memory pass under one registry lock, including inline workspace-stale marking); kept mark_stale_workspaces only for kill_orphans' use after pruning. Tests: - kill_orphans_does_not_kill_across_boot_id_mismatch: matching PID + real start time but a DIFFERENT recorded boot id -> pruned without signaling (killed==0, pruned_stale==1). - kill_orphans_counts_pid_none_entries_as_unresolved (replaces kill_orphans_skips_pid_none_entries): pid:None rows surface as unresolved and block startup via check_orphan_kill_result. - check_orphan_kill_result_aborts_on_unresolved: startup-decision regression. - Existing kill-path tests set boot_id to the current boot id where they exercise the Match/PidReuse path; the legacy fail-closed test keeps boot_id: None. Signed-off-by: jrimmer --- crates/forkd-controller/src/api.rs | 19 ++ crates/forkd-controller/src/http.rs | 6 +- crates/forkd-controller/src/lib.rs | 16 +- crates/forkd-controller/src/state.rs | 297 +++++++++++++++++++++++++-- 4 files changed, 313 insertions(+), 25 deletions(-) diff --git a/crates/forkd-controller/src/api.rs b/crates/forkd-controller/src/api.rs index b242c04..8f85a24 100644 --- a/crates/forkd-controller/src/api.rs +++ b/crates/forkd-controller/src/api.rs @@ -320,6 +320,25 @@ pub struct SandboxInfo { /// killing an unidentifiable process"). #[serde(default)] pub proc_starttime: Option, + /// Linux boot identity (`/proc/sys/kernel/random/boot_id`) captured + /// at VM registration, alongside `proc_starttime`. + /// + /// `proc_starttime` is ticks since boot and is therefore only unique + /// *within* a single boot. The registry persists across host reboots + /// (`/var/lib/forkd/state.json`), so after a reboot an unrelated + /// Firecracker can in principle share both the numeric PID and the + /// boot-relative start tick of a recorded entry; a bare starttime + /// check would then report `Match` and SIGKILL the wrong process + /// (review #299). We persist the boot id at registration and verify + /// it on recovery: a different boot id means the old process cannot + /// still exist (prune without signaling); a missing/unreadable boot + /// id fails closed (do not kill). + /// + /// `#[serde(default)]` keeps older `state.json` entries loadable; + /// entries written before this field deserialize to `None`, which + /// fails closed on the kill path. + #[serde(default)] + pub boot_id: Option, pub memory_limit_mib: Option, /// Set to true once any BRANCH (Full or Diff) has been taken from /// this sandbox. Diagnostic flag — phase 1d (v0.3.1) lifted the diff --git a/crates/forkd-controller/src/http.rs b/crates/forkd-controller/src/http.rs index 36e6a4f..a1e63b1 100644 --- a/crates/forkd-controller/src/http.rs +++ b/crates/forkd-controller/src/http.rs @@ -38,7 +38,7 @@ use crate::api::{ ExecResponse, SandboxInfo, SnapshotInfo, SnapshotInfoDetail, SuspendWorkspaceRequest, VersionResponse, WorkspaceInfo, WorkspaceStatus, }; -use crate::state::{read_proc_starttime, Registry}; +use crate::state::{read_boot_id, read_proc_starttime, Registry}; use forkd_vmm::ClockSyncOutcome; const API_VERSION: &str = "v1"; @@ -1413,6 +1413,7 @@ async fn create_sandbox( created_at_unix: now, pid: Some(vm.pid()), proc_starttime: read_proc_starttime(vm.pid()), + boot_id: read_boot_id(), memory_limit_mib: req.memory_limit_mib, has_branched: false, last_branch_memory_path: None, @@ -2841,6 +2842,7 @@ fn spawn_one_for_workspace( created_at_unix: unix_now(), pid: Some(vm.pid()), proc_starttime: read_proc_starttime(vm.pid()), + boot_id: read_boot_id(), memory_limit_mib, has_branched: false, last_branch_memory_path: None, @@ -3458,6 +3460,7 @@ mod tests { created_at_unix: unix_now(), pid: None, proc_starttime: None, + boot_id: None, memory_limit_mib: None, has_branched: false, last_branch_memory_path: None, @@ -4503,6 +4506,7 @@ mod tests { created_at_unix: 1, pid: Some(99999999), proc_starttime: None, + boot_id: None, memory_limit_mib: None, has_branched: false, last_branch_memory_path: None, diff --git a/crates/forkd-controller/src/lib.rs b/crates/forkd-controller/src/lib.rs index f67b6f2..ad8665c 100644 --- a/crates/forkd-controller/src/lib.rs +++ b/crates/forkd-controller/src/lib.rs @@ -96,6 +96,16 @@ pub(crate) fn check_orphan_kill_result(orphans: &crate::state::KillOrphansResult orphans.kill_failed ); } + if orphans.unresolved > 0 { + anyhow::bail!( + "aborting startup: {} retained sandbox entr(y/ies) have no recorded PID and \ + cannot be attributed to a live or dead process; refusing to start with an \ + empty allocator/shared-tap ownership that may collide with a live VM still \ + holding those resources (#298). Inspect the registry and remove/repair them, \ + then restart.", + orphans.unresolved + ); + } Ok(()) } @@ -122,13 +132,15 @@ pub async fn run_daemon(cfg: DaemonConfig) -> Result<()> { killed = orphans.killed, pruned_stale = orphans.pruned_stale, kill_failed = orphans.kill_failed, + unresolved = orphans.unresolved, "killed orphaned Firecracker processes on startup" ); - } else if orphans.pruned_stale > 0 || orphans.kill_failed > 0 { + } else if orphans.pruned_stale > 0 || orphans.kill_failed > 0 || orphans.unresolved > 0 { tracing::warn!( pruned_stale = orphans.pruned_stale, kill_failed = orphans.kill_failed, - "orphan recovery: some entries pruned as stale or kill failed" + unresolved = orphans.unresolved, + "orphan recovery: some entries pruned as stale, unresolved, or kill failed" ); } diff --git a/crates/forkd-controller/src/state.rs b/crates/forkd-controller/src/state.rs index d0b40c0..e3110f2 100644 --- a/crates/forkd-controller/src/state.rs +++ b/crates/forkd-controller/src/state.rs @@ -323,15 +323,35 @@ impl Registry { /// the allocator may still collide with them — the caller should /// log the failure count and the operator should investigate. pub(crate) fn kill_orphans(&self) -> Result { - // Collect orphans with their recorded durable identity (start time). - // The lock is dropped before any kill so the (bounded) wait_for_death - // poll never holds the registry mutex. - let orphans: Vec<(String, u32, Option)> = { + // Count retained sandbox rows that have NO recorded PID before we + // prune/kill. Such an entry is legacy/corrupt identity (every + // production registration path writes `Some(pid)`), but the + // absence of a PID is NOT evidence that no live VM is holding its + // netns/tap resources — on the contrary, it mirrors the #298 + // collision risk if we simply skip it and start with an empty + // allocator. The caller treats `unresolved > 0` as a startup + // blocker (fail closed). + let unresolved: usize = { + self.inner + .lock() + .sandboxes + .values() + .filter(|sb| sb.pid.is_none()) + .count() + }; + + // Collect orphans with their recorded durable identity + // (start time + boot id). The lock is dropped before any kill + // so the (bounded) wait_for_death poll never holds the registry + // mutex. + let orphans: Vec<(String, u32, Option, Option)> = { let g = self.inner.lock(); g.sandboxes .iter() .filter_map(|(id, sb)| match sb.pid { - Some(pid) if pid_alive(pid) => Some((id.clone(), pid, sb.proc_starttime)), + Some(pid) if pid_alive(pid) => { + Some((id.clone(), pid, sb.proc_starttime, sb.boot_id.clone())) + } _ => None, }) .collect() @@ -341,13 +361,14 @@ impl Registry { let mut pruned_stale = 0usize; let mut kill_failed = 0usize; - for (id, pid, recorded_starttime) in orphans { + for (id, pid, recorded_starttime, recorded_boot_id) in orphans { // Primary identity check: compare the live process start time - // against the one recorded at registration. This is the - // durable identity that survives PID reuse — `comm` alone - // is spoofable and cannot distinguish two Firecracker - // processes that happen to share a PID over time. - match process_identity_matches(pid, recorded_starttime) { + // (gated by the persisted boot id) against the record at + // registration. This is the durable identity that survives + // PID reuse — `comm` alone is spoofable and cannot distinguish + // two Firecracker processes that happen to share a PID over + // time (or across a host reboot). + match process_identity_matches(pid, recorded_starttime, recorded_boot_id.as_deref()) { IdentityCheck::Match => { // Same process (start time matches). Open a pidfd // NOW, after the identity check, so the signal is @@ -539,6 +560,7 @@ impl Registry { killed, pruned_stale, kill_failed, + unresolved, }) } @@ -550,12 +572,20 @@ impl Registry { } /// Result of `kill_orphans`: how many were actually killed, pruned as -/// stale (PID reuse / already dead), and how many kills failed. +/// stale (PID reuse / already dead), how many kills failed, and how many +/// retained rows were unresolvable (no recorded PID). #[derive(Debug, Default, Clone, Copy)] pub struct KillOrphansResult { pub killed: usize, pub pruned_stale: usize, pub kill_failed: usize, + /// Retained sandbox entries whose `pid` is `None`. This is a + /// startup blocker: it means a previous controller left a row we + /// cannot attribute to any live or dead process, so we cannot prove + /// its netns/tap resources are free. Treating it as skippable would + /// recreate the #298 collision risk (startup succeeds with an empty + /// allocator while a live VM may still hold those resources). + pub unresolved: usize, } #[cfg(target_os = "linux")] @@ -622,6 +652,35 @@ enum IdentityCheck { Unknown, } +/// Read the Linux boot identity from `/proc/sys/kernel/random/boot_id`. +/// This is a UUID that changes on every host reboot. +/// +/// `proc_starttime` (ticks since boot) is only meaningful *within* a single +/// boot: it is not unique across reboots. Two processes in different boots +/// can share both a numeric PID and a boot-relative start tick. Since the +/// registry persists across reboots (`/var/lib/forkd/state.json`), a +/// bare starttime is insufficient to prove a recorded PID is the same +/// still-alive Firecracker after the host has rebooted. We therefore also +/// persist the boot identity at registration and verify it on recovery +/// (review #299: cross-reboot PID+starttick reuse). +/// +/// Returns `None` when the boot id cannot be read (unlikely on Linux; e.g. +/// no `/proc`, or a constrained container). Off-Linux this returns `None`. +#[cfg(target_os = "linux")] +pub(crate) fn read_boot_id() -> Option { + let raw = std::fs::read_to_string("/proc/sys/kernel/random/boot_id").ok()?; + let boot_id = raw.trim(); + if boot_id.is_empty() { + return None; + } + Some(boot_id.to_string()) +} + +#[cfg(not(target_os = "linux"))] +pub(crate) fn read_boot_id() -> Option { + None +} + /// Read field 22 (starttime, clock ticks since boot) from /// `/proc//stat`. Returns `None` if the process is gone or the /// file cannot be parsed. @@ -656,10 +715,23 @@ pub(crate) fn read_proc_starttime(_pid: u32) -> Option { } /// Compare a recorded process identity against the live process at the -/// same PID. On Linux this is the start-time comparison; off-Linux it is -/// always `Unknown` (no `/proc` to read). +/// same PID. On Linux this compares (a) the boot identity and (b) the +/// start time; off-Linux it is always `Unknown` (no `/proc` to read). +/// +/// `recorded_boot_id` is the persisted `/proc/sys/kernel/random/boot_id` +/// captured at registration. A mismatch with the current boot id means the +/// recorded process belonged to a previous boot, so it cannot still be +/// alive — we prune without signaling (`PidReuse`). A `None` recorded boot +/// id (legacy `state.json` written before the field existed, or a process +/// registered when the boot id was unreadable) means we cannot prove which +/// boot the process belongs to, so we FAIL CLOSED (`Unknown`) rather than +/// risk a cross-reboot false match (review #299). #[cfg(target_os = "linux")] -fn process_identity_matches(pid: u32, recorded_starttime: Option) -> IdentityCheck { +fn process_identity_matches( + pid: u32, + recorded_starttime: Option, + recorded_boot_id: Option<&str>, +) -> IdentityCheck { if pid <= 1 { return IdentityCheck::Unknown; } @@ -668,6 +740,31 @@ fn process_identity_matches(pid: u32, recorded_starttime: Option) -> Identi // existed). We cannot prove the live PID is ours — fail closed. return IdentityCheck::Unknown; }; + // Boot identity gates the start-time comparison. If we cannot confirm + // the recorded process was (or is) from the CURRENT boot, we must not + // signal it: a cross-reboot PID+starttick collision would otherwise be + // a false `Match` against an unrelated Firecracker. + match recorded_boot_id { + None => { + // Missing recorded boot id → cannot prove current boot → fail + // closed (do not kill). + return IdentityCheck::Unknown; + } + Some(rec) => match read_boot_id() { + None => { + // Live boot id unreadable → fail closed. + return IdentityCheck::Unknown; + } + Some(live) if live != rec => { + // The recorded process is from a previous boot; it cannot + // still be alive. Safe to prune without signaling. + return IdentityCheck::PidReuse; + } + Some(_live_matching) => { + // Same boot — proceed to the start-time check below. + } + }, + } let path = format!("/proc/{pid}"); if !std::path::Path::new(&path).exists() { return IdentityCheck::Dead; @@ -680,7 +777,11 @@ fn process_identity_matches(pid: u32, recorded_starttime: Option) -> Identi } #[cfg(not(target_os = "linux"))] -fn process_identity_matches(_pid: u32, _recorded_starttime: Option) -> IdentityCheck { +fn process_identity_matches( + _pid: u32, + _recorded_starttime: Option, + _recorded_boot_id: Option<&str>, +) -> IdentityCheck { IdentityCheck::Unknown } @@ -794,6 +895,10 @@ mod tests { guest_addr: "10.42.0.2:8888".into(), created_at_unix: 1, proc_starttime: None, + // Recorded against the CURRENT boot so identity checks that + // expect a match (same boot) pass; tests that need a + // cross-boot mismatch set this explicitly. + boot_id: read_boot_id(), pid: Some(99999999), memory_limit_mib: None, has_branched: false, @@ -971,6 +1076,7 @@ mod tests { created_at_unix: 1, pid: Some(std::process::id()), proc_starttime: Some(u64::MAX), // mismatched → PidReuse + boot_id: read_boot_id(), memory_limit_mib: None, has_branched: false, last_branch_memory_path: None, @@ -990,6 +1096,7 @@ mod tests { created_at_unix: 2, pid: Some(99999999), proc_starttime: Some(u64::MAX), // mismatched → PidReuse + boot_id: read_boot_id(), memory_limit_mib: None, has_branched: false, last_branch_memory_path: None, @@ -1014,6 +1121,77 @@ mod tests { assert!(r.list_sandboxes().is_empty()); } + /// Regression (review #299): a recorded PID whose start time MATCHES + /// the live process but whose recorded boot id differs from the + /// current boot must NOT be killed. + /// + /// `proc_starttime` is ticks since boot and is only unique within one + /// host boot. The registry persists across reboots, so after a host + /// reboot an unrelated Firecracker can in principle share both the + /// numeric PID and the boot-relative start tick with a recorded + /// entry; a bare starttime check would then report `Match` and SIGKILL + /// the wrong process. The boot id gate must turn that into a prune + /// WITHOUT signaling: a different boot id means the recorded process + /// (from the old boot) cannot still exist. + /// + /// We record the CURRENT real start time (so the starttime check would + /// otherwise Match) but a recording of boot_id that differs from this + /// boot (simulating a state.json carried across a reboot). + #[test] + #[cfg(target_os = "linux")] // needs a real process + /proc to prove Match-able identity + fn kill_orphans_does_not_kill_across_boot_id_mismatch() { + let td = TempDir::new().unwrap(); + let path = td.path().join("state.json"); + let r = Registry::load_or_init(&path).unwrap(); + + // Spawn a real child (our own process will do — use the test's own + // PID so it is alive; record its TRUE boot id-matching context but + // store a DIFFERENT boot id in the registry). Using std::process::id() + // keeps this deterministic and alive; the current boot id is read + // fresh and a NONSENSE different boot id is persisted. + let pid = std::process::id(); + let real_starttime = read_proc_starttime(pid).expect("read own starttime"); + // This boot's actual id, from a boot that does NOT match the + // recorded value we are about to persist. + let _current_boot = read_boot_id(); + + // Persist an entry for OUR live PID with the REAL start time (so a + // starttime-only check WOULD Match), but a boot id that is certainly + // different from the current one. The boot gate must reject it + // before the starttime check runs. + r.insert_sandbox(SandboxInfo { + id: "sb-cross-boot".into(), + snapshot_tag: "py".into(), + netns: Some("forkd-child-1".into()), + guest_addr: "10.42.0.2:8888".into(), + created_at_unix: 1, + pid: Some(pid), + proc_starttime: Some(real_starttime), + // Guaranteed-different boot id (UUIDs are unique across boots; + // a fresh random-looking value cannot equal the current boot's). + boot_id: Some("00000000-0000-0000-0000-000000000000".to_string()), + memory_limit_mib: None, + has_branched: false, + last_branch_memory_path: None, + branch_count: 0, + }) + .unwrap(); + + let result = r.kill_orphans().unwrap(); + // The boot gate turns a would-be Match into a PidReuse → prune + // WITHOUT signaling (killed stays 0). + assert_eq!( + result.killed, 0, + "must NOT kill a process whose recorded boot id differs from current boot" + ); + assert_eq!( + result.pruned_stale, 1, + "cross-boot stale entry must be pruned without signaling" + ); + assert_eq!(result.kill_failed, 0); + assert!(r.list_sandboxes().is_empty()); + } + /// New: an alive-PID entry with NO recorded start time (old state.json /// written before proc_starttime existed) must FAIL CLOSED — the entry // is kept and kill_failed is incremented, so the caller aborts startup @@ -1035,6 +1213,9 @@ mod tests { created_at_unix: 1, pid: Some(std::process::id()), proc_starttime: None, // old state.json — identity unknown + // Legacy entry with no boot id — we cannot verify which boot + // it belongs to, so the kill path fails closed (Unknown). + boot_id: None, memory_limit_mib: None, has_branched: false, last_branch_memory_path: None, @@ -1089,6 +1270,7 @@ mod tests { created_at_unix: 1, pid: Some(std::process::id()), proc_starttime: Some(0), // real start time is > 0; 0 cannot match + boot_id: read_boot_id(), memory_limit_mib: None, has_branched: false, last_branch_memory_path: None, @@ -1123,6 +1305,7 @@ mod tests { created_at_unix: 1, pid: Some(std::process::id()), proc_starttime: Some(u64::MAX), // mismatched → PidReuse + boot_id: read_boot_id(), memory_limit_mib: None, has_branched: false, last_branch_memory_path: None, @@ -1169,21 +1352,29 @@ mod tests { } #[test] - fn kill_orphans_skips_pid_none_entries() { + fn kill_orphans_counts_pid_none_entries_as_unresolved() { let td = TempDir::new().unwrap(); let path = td.path().join("state.json"); let r = Registry::load_or_init(&path).unwrap(); - // Insert a sandbox with pid: None — should be skipped by both - // reconcile() and kill_orphans() (the filter_map only collects - // Some(pid) entries). + // A sandbox row with pid: None is legacy/corrupt identity — every + // production registration path writes Some(pid). The absence of a + // PID is NOT evidence that no live VM holds the netns/tap it + // registered, so rather than silently skip it (which would recreate + // the #298 collision risk by starting with an empty allocator), it + // must be surfaced as `unresolved` so the caller blocks startup. + // Use a resource-holding shape (netns: Some(...)) to mirror the + // real #298 collision this protects against: the pid-less row's + // netns/tap may still be held by a live VM even though we can't + // attribute it to any PID. r.insert_sandbox(SandboxInfo { id: "sb-no-pid".into(), snapshot_tag: "py".into(), - netns: None, + netns: Some("forkd-child-1".into()), guest_addr: "10.42.0.2:8888".into(), created_at_unix: 1, proc_starttime: None, + boot_id: None, pid: None, memory_limit_mib: None, has_branched: false, @@ -1197,8 +1388,16 @@ mod tests { assert_eq!(result.killed, 0); assert_eq!(result.pruned_stale, 0); assert_eq!(result.kill_failed, 0); - // Entry still exists — neither method touches pid:None entries. + // The pid:None row is surfaced as unresolved → startup blocker. + assert_eq!( + result.unresolved, 1, + "pid:None entry must be reported as unresolved, not skipped" + ); + // Entry retained (not pruned) so the operator can inspect it. assert_eq!(r.list_sandboxes().len(), 1); + + // And check_orphan_kill_result must fail closed on it. + assert!(crate::check_orphan_kill_result(&result).is_err()); } #[test] @@ -1218,6 +1417,7 @@ mod tests { created_at_unix: 1, pid: Some(std::process::id()), proc_starttime: Some(u64::MAX), // mismatched → PidReuse + boot_id: read_boot_id(), memory_limit_mib: None, has_branched: false, last_branch_memory_path: None, @@ -1271,6 +1471,7 @@ mod tests { created_at_unix: 1, pid: Some(std::process::id()), proc_starttime: Some(u64::MAX), // mismatched → PidReuse + boot_id: read_boot_id(), memory_limit_mib: None, has_branched: false, last_branch_memory_path: None, @@ -1301,6 +1502,7 @@ mod tests { guest_addr: "10.42.0.2:8888".into(), created_at_unix: 1, proc_starttime: Some(u64::MAX), // mismatched → PidReuse if it reaches kill_orphans + boot_id: read_boot_id(), pid: Some(99999999), memory_limit_mib: None, has_branched: false, @@ -1318,6 +1520,7 @@ mod tests { created_at_unix: 2, pid: Some(std::process::id()), proc_starttime: Some(u64::MAX), // mismatched → PidReuse + boot_id: read_boot_id(), memory_limit_mib: None, has_branched: false, last_branch_memory_path: None, @@ -1378,6 +1581,7 @@ mod tests { killed: 2, pruned_stale: 0, kill_failed: 1, + unresolved: 0, }; let outcome = check_orphan_kill_result(&result); assert!( @@ -1399,6 +1603,7 @@ mod tests { killed: 5, pruned_stale: 3, kill_failed: 0, + unresolved: 0, }; assert!( check_orphan_kill_result(&result).is_ok(), @@ -1417,6 +1622,7 @@ mod tests { killed: 0, pruned_stale: 0, kill_failed: 3, + unresolved: 0, }; let err = check_orphan_kill_result(&result).unwrap_err().to_string(); assert!( @@ -1429,6 +1635,51 @@ mod tests { ); } + /// Startup-decision regression (review #299): a retained sandbox row + /// with no PID must block startup. Previously `kill_orphans` skipped + /// `pid: None` entries and `check_orphan_kill_result` only looked at + /// `kill_failed`, so the controller started with an empty allocator/ + /// shared-tap ownership while a live VM might still hold those + /// resources — recreating the #298 collision risk. + #[test] + fn check_orphan_kill_result_aborts_on_unresolved() { + use crate::check_orphan_kill_result; + + // unresolved > 0 (even with kill_failed == 0) must abort. + let result = KillOrphansResult { + killed: 0, + pruned_stale: 0, + kill_failed: 0, + unresolved: 2, + }; + let outcome = check_orphan_kill_result(&result); + assert!( + outcome.is_err(), + "unresolved=2 must cause startup abort, got {outcome:?}" + ); + let err = outcome.unwrap_err().to_string(); + assert!( + err.contains("aborting startup"), + "error should mention aborting startup, got: {err}" + ); + assert!( + err.contains('2'), + "error should contain unresolved count (2), got: {err}" + ); + + // unresolved == 0 and kill_failed == 0 must NOT abort. + let ok = KillOrphansResult { + killed: 1, + pruned_stale: 0, + kill_failed: 0, + unresolved: 0, + }; + assert!( + check_orphan_kill_result(&ok).is_ok(), + "clean result should not abort startup" + ); + } + // ---------------------------------------------------------------- // ce-code-review r6 follow-ups: Match-path coverage, starttime parser // unit test, and backward-compat serde default for proc_starttime. @@ -1547,6 +1798,7 @@ mod tests { created_at_unix: 1, pid: Some(pid), proc_starttime: Some(starttime), + boot_id: read_boot_id(), memory_limit_mib: None, has_branched: false, last_branch_memory_path: None, @@ -1670,6 +1922,7 @@ mod tests { created_at_unix: 1, pid: Some(pid), proc_starttime: Some(starttime), + boot_id: read_boot_id(), memory_limit_mib: None, has_branched: false, last_branch_memory_path: None, From 8f94ff8bab57baeca12be30228b20700a764227c Mon Sep 17 00:00:00 2001 From: jrimmer Date: Sat, 22 Aug 2026 13:14:30 -0700 Subject: [PATCH 11/11] test: setsid-detach firecracker child for robust kill-path regression On Linux CI the detached child could be reaped by the runner's subreaper between spawn and kill_orphans (or hit job-control cleanup when the shell exits), yielding pruned_stale (Dead/ESRCH) instead of killed. Use 'setsid' to create a new session and redirect stdio away so the process reliably survives until kill_orphans' pidfd path runs, and record the current boot id (spawned this boot). Signed-off-by: jrimmer --- crates/forkd-controller/src/state.rs | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/crates/forkd-controller/src/state.rs b/crates/forkd-controller/src/state.rs index e3110f2..21ab5a3 100644 --- a/crates/forkd-controller/src/state.rs +++ b/crates/forkd-controller/src/state.rs @@ -1881,16 +1881,20 @@ mod tests { ) .expect("copy sleep → firecracker"); - // Double-fork via `sh -c '... & echo $!'` so the firecracker-named - // process is reparented to init (PID 1), not held as a child of this - // test. The shell backgrounds the process, prints its PID, and exits; - // the backgrounded process is then reparented to init. This ensures - // `wait_for_death` sees `/proc/` disappear after SIGKILL - // (init reaps the reparented process immediately), rather than - // timing out on a zombie held by the test. + // Double-fork via `sh -c 'setsid ... & echo $!'` so the + // firecracker-named process is reparented to init (PID 1), detached + // from the test's session/job-control, and not held as a child of + // this test. `setsid` creates a new session so the process is not + // killed by terminal/job-control cleanup when the shell exits, and + // when init reaps it after SIGKILL the /proc/ entry disappears + // immediately — vs. a test-owned child, which would linger as a + // zombie until reaped and make `wait_for_death` time out. let sh_out = Command::new("sh") .arg("-c") - .arg(format!("{} 30 & echo $!", firecracker_bin.display())) + .arg(format!( + "setsid {} 30 /dev/null 2>&1 & echo $!", + firecracker_bin.display() + )) .stdin(Stdio::null()) .stdout(Stdio::piped()) .stderr(Stdio::null()) @@ -1922,6 +1926,8 @@ mod tests { created_at_unix: 1, pid: Some(pid), proc_starttime: Some(starttime), + // The child was spawned on THIS boot, so record the current + // boot id — a cross-boot mismatch must NOT occur here. boot_id: read_boot_id(), memory_limit_mib: None, has_branched: false,