google / google/capsem

[triage:service-03-pid-reuse-signal-unrelated-process] Instances are tracked and signalled by bare PID with no identity re-check (PID-reuse hazard)

Open
#142 0 comments 0 reactions 0 assignees View on GitHub
component:service type:bug
Dominant language
Rust
Stars
72
Forks
13
Avg merge
1d 2h
Merged PRs (30d)
5

Description

Imported from Capsem triage report `service-03-pid-reuse-signal-unrelated-process.md`.

- Severity: `medium`
- Category: `bug`
- Area: `capsem-service`
- Location: `crates/capsem-service/src/main.rs:936` (drain probe), `:12128`/`:12153` (kill_all_vm_processes), `:10711`/`:10730` (suspend timeout kill)
- Confidence: `verified`

## Summary
`InstanceInfo` stores a raw `pid: u32`, and the service both liveness-probes and signals that PID by
number alone, never re-verifying that the PID still refers to the `capsem-process` it spawned. On a
long-running daemon, the OS can recycle a dead child's PID for an unrelated process; the service will
then either (a) treat a stale instance as alive forever (`kill(pid,0)==0`), or (b) SIGTERM/SIGKILL a
process it does not own.

## Evidence
Liveness probe keys purely on PID:
```rust
// main.rs:936 drain_dead_instances
.filter(|(_, info)| unsafe { nix::libc::kill(info.pid as i32, 0) } != 0)
```
If the PID was reused, `kill(pid,0)` returns 0 → the dead instance is considered alive and is never
evicted by `cleanup_stale_instances`.

Shutdown signals the stored PID with no ownership check:
```rust
// main.rs:12128-12131 kill_all_vm_processes
let _ = nix::sys::signal::kill(
nix::unistd::Pid::from_raw(pid as i32),
nix::sys::signal::Signal::SIGTERM,
);
```
Same in the suspend-timeout path (main.rs:10711-10715, 10734-10737) and the SIGKILL escalation
(main.rs:12166-12169). None of these confirm the PID still maps to the spawned `capsem-process`
(e.g. by start-time, by the `.ready` socket being connectable, or by matching `--session-dir` argv as
`reap_orphan_capsem_processes` does for the cross-run case).

Note the orphan-reaper (`find_orphan_capsem_pids`, main.rs:12013) *does* the right thing — it matches
on the `--session-dir ` argv before signalling. The in-process tracking path does not apply
the same rigor.

## Impact
- Stale-instance records that never get reaped (resource accounting drift, `max_concurrent_vms`
slot leak, `/list` shows phantom VMs).
- Worst case: the daemon sends SIGTERM/SIGKILL to an unrelated same-uid process during shutdown or a
suspend timeout. Requires PID wraparound, which is uncommon on macOS but not impossible on a
long-lived daemon under heavy churn.

## Suggested fix
Record process identity alongside the PID (child start time via `proc_pidinfo`/`ps -o lstart`, or
keep the `tokio::process::Child` handle as the authority and signal through it). Before any
`kill(pid, …)`, verify the PID still belongs to the tracked instance (argv `--session-dir` match,
mirroring `find_orphan_capsem_pids`). At minimum, gate signalling on the `.ready`/uds socket being the
one this instance owns.

## Triage
Confirmed from the local reviewed report in `/Users/elie/git/capsem/tmp/bugs/service-03-pid-reuse-signal-unrelated-process.md`. Track implementation in the triage sprint; add regression coverage before fixing.

Contributor guide

Open the contributing guide

Research direction

Start in crates/capsem-service/src/main.rs at drain_dead_instances, kill_all_vm_processes, and the suspend-timeout kill paths. Compare their PID handling with find_orphan_capsem_pids and reap_orphan_capsem_processes, then review how InstanceInfo is populated. Done means liveness checks and signals verify process identity, with regression coverage for PID reuse.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
backend, operating-systems
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.