vm: record the firecracker PID, not sudo's

[DarkaMaul]

Firecracker is spawned via Cmd::sudo(), so child.id() is the sudo wrapper: sudo forks instead of exec'ing when use_pty is on. The PID file therefore named the wrapper, and stop()'s SIGKILL fallback killed sudo while leaving the VM running, holding the TAP device and API socket.

Verified in a Linux container (sudo 1.9.15p5).

[hbrodin]

Thanks for digging this out — real bug, and a genuinely non-obvious one. I reproduced it before reviewing: on sudo 1.9.15p5, sudo sleep 7 gives a wrapper (comm=sudo) plus a forked child, and SIGKILL to the wrapper leaves the child running. The use_pty-since-1.9.14 note checks out against sudo's release notes too.

One thing that makes the case stronger than the description: recording the wrapper also breaks crash detection. An exited-but-unreaped sudo wrapper is a zombie, and kill -0 succeeds on a zombie, so check_alive() and Instance::is_running() today report ALIVE for a Firecracker that already died. The reason nobody noticed is that is_firecracker_process substring-matches the cmdline, and the wrapper's is sudo firecracker …. Probably worth a line in the commit message.

Suggested alternative

The current approach discovers the PID. We could have the spawn establish it, which removes the sleep, the lsof dependency, pid_on_socket, the fallback and the race together:

sudo sh -c 'echo $$ > "$1"; shift; exec "$@"' sh <pidfile> <firecracker-bin> <args…>

Because of exec, the shell's $$ is the Firecracker PID, written before exec. I verified it: the pidfile named the exec'd process rather than the wrapper, and SIGKILL of that PID killed the real process. Passing each path as a positional parameter keeps quoting/injection at zero, and a root-written pidfile still unlinks from the user-owned instance dir, so stop()'s remove_file and the stale-PID cleanup keep working.

Two dead ends I checked so you don't have to: Defaults !use_pty is not a fix (PAM setup and I/O logging also make sudo fork a monitor), and process-group kill doesn't help since use_pty puts the child in its own session.

Worth adding either way (both outside this diff)

• stop() never calls the existing kill_process_on_socket — one line after the SIGKILL branch would catch the orphan even when the pidfile was never written.
• test_stop only asserts exit 0; nothing checks the VMM is actually gone. The socket isn't a usable proxy, since stop() rm -fs it either way.

For the record: cargo fmt, clippy -D warnings and a security pass over the new privileged lsof call are all clean at c183cd4; .cargo/mutants.toml already covers src/vm.rs, so nothing is owed there; and I confirmed the new assertion really would have failed pre-fix (comm=sudo). The deny failure is the ambient h2 RUSTSEC-2026-0258, unrelated to this PR.

The rest is inline and mostly minor. Glad to help rework it to the exec form if you'd prefer.

[hbrodin]

One fixed sleep and a single probe: if the socket isn't bound within 1s, the wrong PID is recorded silently. sudo does PAM/NSS work and pty allocation before it execs Firecracker, so >1s is plausible on a loaded host — and CI is where that bites, since the new integration assertion would then fail as a flake rather than diagnose a slow boot.

wait_for_exit (line 584) is the polling idiom already in this file; a bounded re-probe every ~100–200ms would make this deterministic.

[DarkaMaul]

No socket probe left, so there is no timing dependency to get wrong. The sleep(1) is back where it was — crash detection before check_alive — and the PID is read from the file the shell wrote.

[hbrodin]

Thanks for the rework — the exec form is much better, and the PID file genuinely names the VMM now. I think this thread is still live though, for a different reason than before.

The dependency moved rather than disappeared: it's no longer "socket bound within 1s", it's "sudo + sh reach the echo within 1s". The sleep is now load-bearing for acquiring the PID, not just for crash detection — if the file isn't there yet, start() fails with Failed to read PID file, which base couldn't do since child.id() was synchronous.

In fairness the window got much wider: I measured the pidfile appearing ~23ms after spawn (sudo 1.9.15p5), so ~40x headroom. Not alarming, and it self-heals on re-run. But it's a new failure mode and the fix is small.

The wrinkle is that the sleep is doing two jobs now. A bounded poll fixes the pidfile wait, but crash detection still wants a real delay — checking early always passes. So I'd split them and put the sleep back where it was:

let pid = wait_for_pid_file(&pid_path, Duration::from_secs(5))?;
// …
// Give Firecracker a moment to start, then check it didn't crash
std::thread::sleep(Duration::from_secs(1));
running.check_alive()?;

wait_for_exit (line 566) is the shape to mirror. Worth polling for "parses as a u32" rather than exists() — > creates the file before echo writes it.

Also flagging: the comment above the sleep is now stale, since check_alive() is 17 lines further down.

Aside, entirely your call: echo $$ to stdout and a blocking read_line removes the race instead of bounding it, and coop goes back to writing the pidfile itself. Needs >/dev/null inside the shell so Firecracker doesn't inherit the pipe on exec. Bigger change than this deserves — just noting it.

[DarkaMaul]

Reworked to drop lsof from the start path entirely — thanks, the review pushed this to a smaller and more deterministic fix than what I pushed first.

Firecracker is now spawned as sudo sh -c 'echo $$ > "$1" || exit 1; shift; exec "$@"' sh <pid-file> <firecracker> …. The shell records its own PID and execs, so the recorded PID is the VMM. No probe, no timing window, no fallback, no new dependency. Args are passed positionally, so no path is interpolated into the shell string.

Re-verified in a fresh container (rust:slim, sudo 1.9.16p2, non-root user with NOPASSWD), with a harness mirroring start()/stop()'s PID handling and /bin/sleep standing in for firecracker. 12 assertions, all passing with and without a controlling terminal:

• pre-fix baseline reproduces — child.id() is comm=sudo, cmdline=sudo /bin/sleep 30
• recorded PID's comm is the exec'd process and differs from the spawned sudo child
• cmdline survives the shell hop intact, including a pid-file path containing a space
• PID file is root-owned (uid 0) but readable by fs::read_to_string and removable by fs::remove_file as the unprivileged user — i.e. stop()'s cleanup and start()'s sudo rm -f both still work, and a restart records a fresh PID in the same instance dir
• a failed exec leaves a dead PID, so check_alive fails and start() bails

One thing worth recording: the orphan is tty-dependent, though the wrong PID was recorded either way. With no controlling terminal, SIGKILL on the wrapper leaves the VMM in state=S. With one, the pty teardown SIGHUPs it and it dies. So the old code often got away with it interactively; CI, systemd, and nohup are where it orphans. Caveat: sleep takes the default SIGHUP action — I have not checked firecracker's disposition, and if it handles SIGHUP the orphan would occur in the tty case too.

Still to run before merge: the integration suite against real Firecracker on a KVM host. The container can't exercise VM boot or stop()'s CtrlAltDel path.

[hbrodin]

Small hardening idea while you're in here — not a vulnerability, since the instance dir is 0755 owner-only and that owner already has sudo.

If firecracker.pid is a dangling symlink, the stale-file loop above skips it: stale.exists() follows the link and returns false for a broken target, so sudo rm -f never runs. Then root's > follows the link and creates a root-owned file wherever it points.

Cheapest fix is a leading rm -f inside the one-liner, so the redirect always lands on a fresh regular file:

rm -f "$1"; echo $$ > "$1" || exit 1; shift; exec "$@"

One token, and it makes the trampoline self-contained rather than depending on the caller's cleanup having worked.

[hbrodin]

Tiny consistency nit: status() (455) and check_alive() (523) both use the pid_str / pid shadowing pair, which keeps the inline capture working:

let pid_str = fs::read_to_string(&pid_path).context("Failed to read PID file")?;
let pid = pid_str.trim();
tracing::info!("Firecracker started with PID {pid} (instance '{}')", self.inst.name);

Worth a parse::<u32>() too — a truncated or empty pidfile currently logs blank and only surfaces later in check_alive().

Also pid_path is already bound at 233, so this and line 260 don't need to re-derive it.

[hbrodin]

Nice — I checked this would actually have failed on base: pre-fix the recorded PID is the sudo wrapper, so comm reads sudo and the assertion trips. Real regression coverage, not a tautology.

One robustness thought: comm is truncated to 15 chars and could be firecracker-v1.x on some installs, and /proc mounted with hidepid= makes it unreadable. [[ "$comm" == *firecracker* ]] would absorb both. Only false failures either way, so low stakes.

Worth noting for anyone who touches this later: don't be tempted to match /proc/<pid>/cmdline the way is_firecracker_process does. The sudo wrapper's cmdline contains "firecracker" via the socket path, so that check passes on the broken PID too — which is exactly why the production predicate missed this bug.