diff --git a/go/internal/runtime/microvm/boot_microvm_test.go b/go/internal/runtime/microvm/boot_microvm_test.go index e334c4b4..c3f9a8da 100644 --- a/go/internal/runtime/microvm/boot_microvm_test.go +++ b/go/internal/runtime/microvm/boot_microvm_test.go @@ -20,6 +20,7 @@ package microvm import ( "context" "os" + "path/filepath" "strings" "testing" "time" @@ -117,7 +118,7 @@ func TestNetOnlyBootSmoke(t *testing.T) { // 1024 MB), complete the Health handshake with net_provisioned && // workspace_mounted inside the 60 s deadline, record boot latency and // per-process PSS, and verify Shutdown leaves no orphan process and removes the -// unix sockets. +// unix sockets and the three pidfiles. func TestFullBoot(t *testing.T) { env := microvmtest.Require(t) cfg := bootConfig(t, env, 2, 1024) @@ -143,6 +144,14 @@ func TestFullBoot(t *testing.T) { t.Errorf("socket %s not removed by Shutdown", s) } } + // And the three pidfiles: a record must not outlive the session it + // names, or the reaper would find a dir full of dead records for a + // session that shut down cleanly. + for _, p := range vm.pidfiles { + if fileExists(p) { + t.Errorf("pidfile %s not removed by Shutdown", p) + } + } }) // Poll Health until the guest reports both flags true, bounded by the boot @@ -185,6 +194,70 @@ func TestFullBoot(t *testing.T) { } } +// TestFullBootRecordsSettledPidfiles is W1's KVM assertion (§(a)): after a real +// Launch the runtime dir must hold a SETTLED record for each of the three +// children — no intent line left behind — and each record must name a process +// that is actually live with a matching starttime. That is exactly the input +// the orphan reaper acts on, so the hermetic tier's coverage of the primitives +// is not enough: this pins that launch wires them to the REAL pids of the REAL +// children, under the real spawn ordering (virtiofsd, passt, then the VMM). +func TestFullBootRecordsSettledPidfiles(t *testing.T) { + env := microvmtest.Require(t) + cfg := bootConfig(t, env, 2, 1024) + + vm, err := Launch(t.Context(), cfg) + if err != nil { + t.Fatalf("Launch failed: %v", err) + } + + dir := filepath.Dir(cfg.Net.VhostUserSocket) + // The pid each name is expected to carry, from the child handles launch + // built — so a record naming SOME live process (or another child's pid) + // cannot pass. + wantPIDs := map[string]int{ + "vmm.pid": vm.vmm.cmd.Process.Pid, + "virtiofsd.pid": vm.virtiofsd.cmd.Process.Pid, + "passt.pid": vm.passt.cmd.Process.Pid, + } + paths := make([]string, 0, len(wantPIDs)) + for name, wantPID := range wantPIDs { + path := filepath.Join(dir, name) + paths = append(paths, path) + + rec, readErr := readPidfile(path) + if readErr != nil { + t.Errorf("readPidfile(%s): %v", name, readErr) + continue + } + if rec.Intent { + t.Errorf("%s still holds an intent record after Launch; the settled write did not land", name) + continue + } + if rec.PID != wantPID { + t.Errorf("%s records pid %d, want the child's %d", name, rec.PID, wantPID) + } + // Liveness via the recorded identity, not a bare signal: this is the + // starttime comparison the reaper makes before it kills anything. + alive, aliveErr := rec.alive() + if aliveErr != nil { + t.Errorf("%s alive(): %v", name, aliveErr) + continue + } + if !alive { + t.Errorf("%s does not name a live process with a matching starttime (%+v)", name, rec) + } + } + + if shutErr := vm.Shutdown(context.WithoutCancel(t.Context())); shutErr != nil { + t.Fatalf("Shutdown: %v", shutErr) + } + for _, path := range paths { + if fileExists(path) { + t.Errorf("pidfile %s survived Shutdown", path) + } + } +} + // armRuleset is a minimal but representative in-guest egress arm: it creates the // inet table + a conntrack-stateful output rule, forcing a representative slice // of the netfilter autoload chain — the NETLINK_NETFILTER socket (nfnetlink), diff --git a/go/internal/runtime/microvm/launch.go b/go/internal/runtime/microvm/launch.go index e09bcf31..111d32bc 100644 --- a/go/internal/runtime/microvm/launch.go +++ b/go/internal/runtime/microvm/launch.go @@ -108,7 +108,7 @@ func (c *child) hasExited() bool { // VM is a running (or partially-started, on the Launch error path) guest and // its two supporting host daemons. It owns their process handles, the captured -// serial console + per-daemon logs, and the AF_UNIX sockets/pidfile that must +// serial console + per-daemon logs, and the AF_UNIX sockets/pidfiles that must // be removed on teardown. The zero devices (virtiofsd nil under the net-only // smoke) are tolerated by Shutdown and PSS. type VM struct { @@ -127,10 +127,13 @@ type VM struct { vsockSocket string // host end of the hybrid vsock (empty under the net-only smoke) vsockPort uint32 - // Cleanup targets: the AF_UNIX sockets the daemons/VMM serve and passt's - // pidfile. Removed by Shutdown after the processes are reaped. - sockets []string - pidfile string + // Cleanup targets: the AF_UNIX sockets the daemons/VMM serve and the three + // host-written pidfiles (§(a)). Removed by Shutdown after the processes are + // reaped. A pidfile path is appended by startRecordedChild BEFORE its first + // write, so a boot that fails between the intent record and the settled one + // still has its record cleaned up by the deferred Shutdown. + sockets []string + pidfiles []string shutdownOnce sync.Once shutdownErr error @@ -212,8 +215,8 @@ func launch(ctx context.Context, cfg BootConfig, opts launchOptions) (_ *VM, err //nolint:gosec // G204: the microVM harness seam — virtiofsdPath is LookPath-resolved and the argv is harness-built from BootConfig, neither user-controlled cmd: exec.CommandContext(ctx, virtiofsdPath, virtiofsdArgs(cfg, subUIDBase, subGIDBase)...), } - if startErr := startChild(vm.virtiofsd); startErr != nil { - return nil, fmt.Errorf("microvm: starting virtiofsd: %w", startErr) + if startErr := vm.startRecordedChild(vm.virtiofsd, dir, "virtiofsd.pid"); startErr != nil { + return nil, startErr } vm.sockets = append(vm.sockets, cfg.FSSocket) } @@ -222,7 +225,6 @@ func launch(ctx context.Context, cfg BootConfig, opts launchOptions) (_ *VM, err if lookErr != nil { return nil, fmt.Errorf("microvm: resolving passt on PATH: %w", lookErr) } - vm.pidfile = filepath.Join(dir, "passt.pid") vm.passt = &child{ name: "passt", logPath: filepath.Join(dir, "passt.log"), @@ -230,19 +232,24 @@ func launch(ctx context.Context, cfg BootConfig, opts launchOptions) (_ *VM, err // process (default is to daemonize, which would orphan it and make the // Cmd exit immediately). The -a/-g/-n/-D flags fix the host-controlled // address plan passt serves over DHCP (§(c)). + // + // NO --pid: passt's self-written pidfile is retired (§(a)). Because -f + // makes this Cmd the passt process, the host knows passt's pid exactly + // as it knows the other two, and a host-written record can carry the + // starttime+boot-id reuse defense that a daemon's own bare-pid file + // cannot. One writer, one format, three files. //nolint:gosec // G204: the microVM harness seam — passtPath is LookPath-resolved and the argv is harness-built (fixed flags + BootConfig socket), neither user-controlled cmd: exec.CommandContext(ctx, passtPath, "--vhost-user", "--socket", cfg.Net.VhostUserSocket, - "--pid", vm.pidfile, "-f", "-a", guestAddr, "-g", guestGW, "-n", guestPrefix, "-D", guestDNS), } - if startErr := startChild(vm.passt); startErr != nil { - return nil, fmt.Errorf("microvm: starting passt: %w", startErr) + if startErr := vm.startRecordedChild(vm.passt, dir, "passt.pid"); startErr != nil { + return nil, startErr } vm.sockets = append(vm.sockets, cfg.Net.VhostUserSocket) @@ -282,8 +289,8 @@ func launch(ctx context.Context, cfg BootConfig, opts launchOptions) (_ *VM, err //nolint:gosec // G204: the microVM harness seam — vmmPath is LookPath-resolved and vmmArgs is harness-built from BootConfig, neither user-controlled cmd: exec.CommandContext(ctx, vmmPath, vmmArgs(cfg, vm.consolePath, opts)...), } - if startErr := startChild(vm.vmm); startErr != nil { - return nil, fmt.Errorf("microvm: starting cloud-hypervisor: %w", startErr) + if startErr := vm.startRecordedChild(vm.vmm, dir, "vmm.pid"); startErr != nil { + return nil, startErr } // startChild installed the sole reaper that owns cloud-hypervisor's single // cmd.Wait; hoist its channel onto the VM so WaitVMMExit observes a guest @@ -557,20 +564,37 @@ func (vm *VM) Health(ctx context.Context) (*compassv1.HealthResponse, error) { // Shutdown tears the guest and its daemons down: the VMM is killed first (a VM // gets no graceful drain), then virtiofsd and passt are reaped (SIGTERM, a // bounded wait, then SIGKILL), each Wait'd to avoid zombies, and finally the -// AF_UNIX sockets and passt's pidfile are removed. It runs at most once (guarded -// by sync.Once) so it is safe to call explicitly AND from t.Cleanup. The serial -// console log is deliberately NOT removed — the test reads it after teardown. +// AF_UNIX sockets and the three pidfiles are removed. It runs at most once +// (guarded by sync.Once) so it is safe to call explicitly AND from t.Cleanup. +// The serial console log is deliberately NOT removed — the test reads it after +// teardown. func (vm *VM) Shutdown(ctx context.Context) error { vm.shutdownOnce.Do(func() { var errs []error // VMM first: kill outright, then let the sole reaper's single Wait // complete via vmmExited (Shutdown must not Wait the VMM itself — that // would be a second Wait on the same process). + // + // The RECEIVE carries its own nil guard, because vmmExited is hoisted + // onto the VM only AFTER startRecordedChild returns: a + // startRecordedChild that fails once startChild has already spawned + // the VMM leaves a live process handle beside a NIL channel, and + // launch's deferred Shutdown runs in exactly that state. Receiving on + // the nil channel there blocks FOREVER — a hang with no stack and no + // diagnostic, replacing the launch error it was cleaning up after. It + // is the same nil-channel condition WaitVMMExit guards on. + // + // The guard is on the receive alone and NOT on the arm: such a process + // is started, live, and unrecorded, so skipping the Kill would orphan + // precisely the child the deferred Shutdown exists to clean up. Its + // own reaper still owns the Wait, so nothing is left unreaped. if vm.vmm != nil && vm.vmm.cmd.Process != nil { if killErr := vm.vmm.cmd.Process.Kill(); killErr != nil && !errors.Is(killErr, os.ErrProcessDone) { errs = append(errs, fmt.Errorf("killing cloud-hypervisor: %w", killErr)) } - <-vm.vmmExited + if vm.vmmExited != nil { + <-vm.vmmExited + } } // Then the auxiliary daemons: SIGTERM, bounded wait, SIGKILL. for _, c := range []*child{vm.virtiofsd, vm.passt} { @@ -581,15 +605,15 @@ func (vm *VM) Shutdown(ctx context.Context) error { errs = append(errs, reapErr) } } - // Remove the sockets and pidfile now that nothing is serving them. + // Remove the sockets and pidfiles now that nothing is serving them. for _, s := range vm.sockets { if rmErr := os.Remove(s); rmErr != nil && !errors.Is(rmErr, os.ErrNotExist) { errs = append(errs, fmt.Errorf("removing socket %s: %w", s, rmErr)) } } - if vm.pidfile != "" { - if rmErr := os.Remove(vm.pidfile); rmErr != nil && !errors.Is(rmErr, os.ErrNotExist) { - errs = append(errs, fmt.Errorf("removing pidfile %s: %w", vm.pidfile, rmErr)) + for _, p := range vm.pidfiles { + if rmErr := os.Remove(p); rmErr != nil && !errors.Is(rmErr, os.ErrNotExist) { + errs = append(errs, fmt.Errorf("removing pidfile %s: %w", p, rmErr)) } } vm.shutdownErr = errors.Join(errs...) @@ -763,3 +787,56 @@ func (vm *VM) childByName(name string) *child { return nil } } + +// startRecordedChild records c's on-disk identity around its spawn, per §(a)'s +// two-step write: the pre-spawn INTENT record, then startChild, then the +// SETTLED record naming the pid the spawn returned. pidfileName is the file's +// name within the session runtime dir (vmm.pid/virtiofsd.pid/passt.pid). +// +// Either write failing fails the boot: orphan reapability is load-bearing, so a +// session that cannot be recorded must not run. launch's deferred vm.Shutdown +// tears down whatever already started and removes the paths registered here. +func (vm *VM) startRecordedChild(c *child, dir, pidfileName string) error { + path := filepath.Join(dir, pidfileName) + // Registered before the first write, not after the last: the intent record + // exists precisely to survive a crash between the two writes, so Shutdown + // must already know to remove it if the spawn in between fails. + vm.pidfiles = append(vm.pidfiles, path) + if err := writePidIntent(path); err != nil { + return fmt.Errorf("microvm: recording %s pid intent: %w", c.name, err) + } + if err := startChild(c); err != nil { + return fmt.Errorf("microvm: starting %s: %w", c.name, err) + } + if err := writePidfile(path, c.cmd.Process.Pid); err != nil { + return pidfileWriteError(c, err) + } + return nil +} + +// pidfileWriteError shapes a writePidfile failure from startRecordedChild by +// its actual cause. writePidfile's second step reads /proc//stat, and a +// child that exits fast can be reaped between the spawn and that read — so the +// read fails for a reason that is NOT a pidfile fault: the child is simply +// dead. Reporting a /proc path there buries the real cause, which is in the +// daemon's own log, so a confirmed-dead child gets the SAME error shape launch +// uses for a daemon that died before the VMM was started (name the daemon, +// wrap waitResult, carry the log tail). +// +// BOTH conditions are required: procReadMeansGone alone could describe a /proc +// that vanished under a still-live pid (it cannot, but the pairing makes the +// claim "this child is dead" one the reaper's channel actually proves), and +// hasExited alone would swallow a genuine write fault on a child that merely +// happens to have exited. Every other writePidfile failure stays fatal as a +// pidfile error, because orphan reapability is load-bearing. +// +// The on-disk record is deliberately NOT touched on the dead-child branch: the +// intent record stands and the path stays registered, so Shutdown removes it +// and no window exists where the dir under-names a child that may have run. +func pidfileWriteError(c *child, err error) error { + if procReadMeansGone(err) && c.hasExited() { + return fmt.Errorf("microvm: %s exited before its pidfile could be recorded: %w; log tail:\n%s", + c.name, waitResult(c.name, c.waitErr), tailFile(c.logPath)) + } + return fmt.Errorf("microvm: recording %s pidfile: %w", c.name, err) +} diff --git a/go/internal/runtime/microvm/launch_teardown_test.go b/go/internal/runtime/microvm/launch_teardown_test.go index 35e7e824..10b434f5 100644 --- a/go/internal/runtime/microvm/launch_teardown_test.go +++ b/go/internal/runtime/microvm/launch_teardown_test.go @@ -43,14 +43,25 @@ func TestLaunchFailClosedTeardown(t *testing.T) { vfsPidFile := filepath.Join(run, "vfs.pid") passtPidFile := filepath.Join(run, "passt.pid") + // The fakes must OUTLIVE launch, and the only absence this test wants is + // cloud-hypervisor's — so `sleep` is resolved on the real PATH now, while it + // is still intact, and burned into the stubs as an absolute path. Naming it + // bare would make the stubs die on `sleep: command not found` the moment + // PATH is narrowed below (there is no `sleep` builtin in /bin/sh), so both + // daemons would exit on their own and "teardown reaped them" would be + // trivially true — the RIG-3480 defect. + sleepBin, err := exec.LookPath("sleep") + if err != nil { + t.Fatalf("resolving sleep on PATH (the fakes need it to stay alive): %v", err) + } // virtiofsd: record pid, touch its --socket-path=, then stay alive. writeFake(t, bin, "virtiofsd", `echo $$ > `+vfsPidFile+` for a in "$@"; do case "$a" in --socket-path=*) : > "${a#--socket-path=}";; esac; done -sleep 30`) +`+sleepBin+` 30`) // passt: record pid, touch the path following --socket, then stay alive. writeFake(t, bin, "passt", `echo $$ > `+passtPidFile+` p=""; for a in "$@"; do [ "$p" = --socket ] && : > "$a"; p="$a"; done -sleep 30`) +`+sleepBin+` 30`) // cloud-hypervisor deliberately absent → LookPath fails after aux are up. t.Setenv("PATH", bin) @@ -71,8 +82,20 @@ sleep 30`) t.Errorf("expected a nil VM on the error path, got %v", vm) } + // The failure must be cloud-hypervisor's LookPath, which is the ONLY absence + // this test arranges. Anything earlier — notably a fake that exited before + // its socket was served — means the daemons never really came up, so the + // no-orphan assertion below would pass vacuously. This control is what + // caught RIG-3480: it fired on 39/40 runs of the merge result while the + // assertion it guards stayed green. + if !strings.Contains(err.Error(), "cloud-hypervisor") { + t.Fatalf("launch failed before the cloud-hypervisor lookup (%v); the aux daemons never came up, "+ + "so the no-orphan assertion below would be vacuous", err) + } + // The daemons launch already started must have been reaped by the deferred - // cleanup — no orphan left sleeping. + // cleanup — no orphan left sleeping. Shutdown's reap Waits each child, so by + // the time Launch has returned this is a settled fact, not a race to poll. for name, pidFile := range map[string]string{"virtiofsd": vfsPidFile, "passt": passtPidFile} { pid := readPidFile(t, pidFile) if pidAlive(pid) { @@ -209,21 +232,21 @@ func TestWaitForSocketsSucceedsForALiveDaemon(t *testing.T) { } } -// readPidFile reads a pid a fake wrote, retrying briefly since the fake writes -// it asynchronously after exec. +// readPidFile reads the pid a fake recorded. It does NOT poll: each fake writes +// its pid BEFORE it touches its socket, and the caller has already confirmed +// launch got past the socket wait, which orders the write before this read. So a +// missing or malformed pidfile here is a real defect in that ordering rather +// than a race to wait out, and it fails immediately instead of behind a +// wall-clock budget that can only ever expire (RIG-3480). func readPidFile(t *testing.T, path string) int { t.Helper() - deadline := time.Now().Add(2 * time.Second) - for { - raw, err := os.ReadFile(path) - if err == nil { - if pid, convErr := strconv.Atoi(strings.TrimSpace(string(raw))); convErr == nil { - return pid - } - } - if time.Now().After(deadline) { - t.Fatalf("fake never wrote its pid to %s", path) - } - time.Sleep(10 * time.Millisecond) //nolint:forbidigo // bounded poll tick; event-gated on the fake writing its pidfile above with a deadline (rule://go-no-sleep-in-test poll-until exemption) + raw, err := os.ReadFile(path) + if err != nil { + t.Fatalf("reading the pid the fake recorded at %s: %v", path, err) + } + pid, err := strconv.Atoi(strings.TrimSpace(string(raw))) + if err != nil { + t.Fatalf("pidfile %s holds %q, not a pid: %v", path, string(raw), err) } + return pid } diff --git a/go/internal/runtime/microvm/pidfile.go b/go/internal/runtime/microvm/pidfile.go new file mode 100644 index 00000000..64d1213c --- /dev/null +++ b/go/internal/runtime/microvm/pidfile.go @@ -0,0 +1,353 @@ +//go:build unix + +// pidfile.go is the host-written per-session process record (V7 §(a)): the +// three pidfiles under a session's runtime dir that make the dir the durable +// record of the session's process set, so an orphan left by a Runner crash can +// be identified and killed WITHOUT the risk of killing an innocent process that +// inherited a recycled pid. +// +// The record is (pid, starttime, bootid), not a bare pid, and it is written in +// TWO atomic steps around each spawn. Both choices are load-bearing and each is +// argued at its own declaration below: writePidIntent for the pre-spawn step, +// pidRecord.alive for the identity comparison. + +package microvm + +import ( + "errors" + "fmt" + "io" + "os" + "path/filepath" + "strconv" + "strings" + "sync" + "syscall" +) + +// bootIDPath is the kernel's per-boot UUID. It changes on every boot and is +// stable for the boot's life, which is exactly the property the cross-reboot +// arm of the identity check needs. +const bootIDPath = "/proc/sys/kernel/random/boot_id" + +// pidIntentToken is the literal standing in place of a pid in the pre-spawn +// INTENT record. It cannot collide with a settled record: a settled record's +// first field always parses as a positive integer. +const pidIntentToken = "intent" + +// procStartTimeField is the 1-based field number of starttime in +// /proc//stat (proc(5)): the process start time in clock ticks since boot. +const procStartTimeField = 22 + +// errPidUnknown is returned by pidRecord.alive for a SAME-BOOT intent record: a +// child that was about to be spawned when the writer died, so it may or may not +// be running and there is NO pid with which to find out. It is deliberately +// neither "alive" nor "dead" — the reaper (V7 §(b)) must route it to its +// possibly-live arm (warn, keep the dir) rather than to a kill or a removal, +// and a bool return cannot express that third verdict. +var errPidUnknown = errors.New("microvm: pidfile records a pre-spawn intent from this boot: liveness unknowable") + +// procBootID reads the host boot id ONCE per process and caches it: it cannot +// change while this process lives (a new boot id means a new kernel, which +// means this process is gone), so re-reading it per liveness check would be a +// syscall per pidfile for a value that is constant by construction. +var procBootID = sync.OnceValues(func() (string, error) { + raw, err := os.ReadFile(bootIDPath) + if err != nil { + return "", fmt.Errorf("reading boot id from %s: %w", bootIDPath, err) + } + id := strings.TrimSpace(string(raw)) + if id == "" { + return "", fmt.Errorf("empty boot id in %s", bootIDPath) + } + // The boot id is one field of a space-separated record, so whitespace in it + // would make a written record unparseable. A kernel UUID never contains + // any; fail loudly rather than emit a file that cannot be read back. + if strings.ContainsAny(id, " \t") { + return "", fmt.Errorf("boot id %q from %s contains whitespace", id, bootIDPath) + } + return id, nil +}) + +// pidRecord is one parsed pidfile: either the pre-spawn intent record (Intent +// true, PID/StartTime zero) or the settled record naming a spawned child. +type pidRecord struct { + Intent bool + PID int + StartTime uint64 + BootID string +} + +// writePidIntent writes the pre-spawn INTENT record `intent ` to path +// at 0600. It is called BEFORE the child's startChild so the runtime dir names +// every child that MAY become live before it can be live (§(a)). +// +// Atomicity alone does not close the spawn→write window: the pid does not exist +// until the spawn returns, so a writer dying between a successful spawn and the +// settled write would leave a dir naming only the children it already recorded +// and NO record at all of the live child it just started — whereupon the reaper +// reads "everything recorded is dead", removes the dir, and erases the only +// evidence of the leak. The intent record makes the on-disk set CONSERVATIVE by +// construction: it may over-name a child that never spawned (which the reaper +// tolerates — there is no pid to signal), but it can never under-name a live +// one, which is the failure the reaper cannot recover from. +func writePidIntent(path string) error { + bootID, err := procBootID() + if err != nil { + return fmt.Errorf("pidfile %s: %w", path, err) + } + return writePidRecordLine(path, pidIntentToken+" "+bootID+"\n") +} + +// writePidfile writes the settled record ` ` to path +// at 0600, superseding any intent record in ONE atomic rename — so a concurrent +// reader sees the intent record or the settled record, never a torn prefix and +// never nothing. It is called after startChild succeeds, with the pid the spawn +// returned. +// +// starttime is read here, from the live process, rather than trusted from +// anywhere else: it is the second half of the identity pair pidRecord.alive +// compares, and a record carrying a starttime that was not actually read off +// this pid would defeat the whole reuse defense. +func writePidfile(path string, pid int) error { + bootID, err := procBootID() + if err != nil { + return fmt.Errorf("pidfile %s: %w", path, err) + } + startTime, err := readProcStartTime(pid) + if err != nil { + return fmt.Errorf("pidfile %s: %w", path, err) + } + line := strconv.Itoa(pid) + " " + strconv.FormatUint(startTime, 10) + " " + bootID + "\n" + return writePidRecordLine(path, line) +} + +// writePidRecordLine writes line to path atomically: a temp file in the SAME +// directory, then os.Rename. A same-directory rename is atomic against a +// RUNNER PROCESS crash — a concurrent reader, and any reader after the Runner +// dies, sees the complete record or the previous one, never a torn prefix. That +// is exactly the window the reaper exists for: a half-written pidfile would +// demote a recorded live child to the no-pidfile arm and leak it, which the +// torn-write window a plain os.WriteFile leaves open. +// +// It is deliberately NOT durable against a host crash or power loss: tmp.Close +// does not flush to stable storage and os.Rename is not durable without an +// fsync of the parent directory, so after a host crash the record may be absent +// or stale. That durability is not bought on purpose — a host crash changes the +// boot id, so EVERY surviving record short-circuits dead at pidRecord.alive's +// boot-id check whether or not the rename landed, and the reaper's verdict is +// identical either way. Adding an fsync would buy a durability barrier per +// spawn, on the boot path, for a guarantee the boot-id check already supplies. +// +// The temp file is removed on every failure path: the runtime dir is scanned +// per-file by the reaper, so a stray temp file left behind would be an +// unparseable extra entry it has to reason about. +func writePidRecordLine(path, line string) (err error) { + dir := filepath.Dir(path) + // The pattern's trailing '*' puts os.CreateTemp's random suffix where a + // reader expects it (".vmm.pid.tmp1234" reads as a temp of vmm.pid), and + // the leading dot keeps the transient file out of a non-dotfile scan. + tmp, err := os.CreateTemp(dir, "."+filepath.Base(path)+".tmp.*") + if err != nil { + return fmt.Errorf("creating temp pidfile in %s: %w", dir, err) + } + tmpName := tmp.Name() + closed := false + defer func() { + if err == nil { + return + } + if !closed { + _ = tmp.Close() // the write already failed; a close error on the doomed temp adds nothing actionable + } + _ = os.Remove(tmpName) // best-effort cleanup of our own temp file on an already-failing write + }() + + // os.CreateTemp already opens at 0600, but that mode is umask-masked; + // Chmod makes the record's 0600 a guarantee rather than an environment + // dependency. + if err = tmp.Chmod(0o600); err != nil { + return fmt.Errorf("setting mode on temp pidfile %s: %w", tmpName, err) + } + if _, err = tmp.WriteString(line); err != nil { + return fmt.Errorf("writing temp pidfile %s: %w", tmpName, err) + } + if err = tmp.Close(); err != nil { + return fmt.Errorf("closing temp pidfile %s: %w", tmpName, err) + } + closed = true + if err = os.Rename(tmpName, path); err != nil { + return fmt.Errorf("renaming temp pidfile into %s: %w", path, err) + } + return nil +} + +// maxPidfileRecordBytes bounds readPidfile's read. A record is one line of +// about 60 bytes, so anything longer is not a record at all: bounding the read +// keeps a pidfile that was replaced by something huge from being pulled into +// memory whole, and routes it to the malformed arm below instead. +const maxPidfileRecordBytes = 256 + +// readPidfile parses either record form from path. Anything else is an error +// rather than a zero record: an unreadable pidfile is a distinct state from a +// dead one, and the reaper must not treat "I cannot tell what this says" as +// "nothing here is alive". +func readPidfile(path string) (pidRecord, error) { + f, err := os.Open(path) //nolint:gosec // G304: path is a host-built pidfile path in the session runtime dir (/microvm//*.pid), not user input + if err != nil { + return pidRecord{}, fmt.Errorf("reading pidfile %s: %w", path, err) + } + defer func() { + _ = f.Close() // read-only handle: a close error cannot affect the bytes already read, and the record is returned by value + }() + // One byte PAST the bound, so an over-long file is detected rather than + // silently truncated into a prefix that happens to parse as a record. + raw, err := io.ReadAll(io.LimitReader(f, maxPidfileRecordBytes+1)) + if err != nil { + return pidRecord{}, fmt.Errorf("reading pidfile %s: %w", path, err) + } + if len(raw) > maxPidfileRecordBytes { + return pidRecord{}, fmt.Errorf("pidfile %s: record exceeds %d bytes", path, maxPidfileRecordBytes) + } + text, rest, _ := strings.Cut(string(raw), "\n") + if strings.TrimSpace(rest) != "" { + return pidRecord{}, fmt.Errorf("pidfile %s: unexpected content after the record line", path) + } + fields := strings.Fields(text) + if len(fields) > 0 && fields[0] == pidIntentToken { + if len(fields) != 2 { + return pidRecord{}, fmt.Errorf("pidfile %s: malformed intent record %q", path, text) + } + return pidRecord{Intent: true, BootID: fields[1]}, nil + } + if len(fields) != 3 { + return pidRecord{}, fmt.Errorf("pidfile %s: malformed record %q", path, text) + } + pid, err := strconv.Atoi(fields[0]) + if err != nil { + return pidRecord{}, fmt.Errorf("pidfile %s: parsing pid %q: %w", path, fields[0], err) + } + if pid < 1 { + return pidRecord{}, fmt.Errorf("pidfile %s: non-positive pid %d", path, pid) + } + startTime, err := strconv.ParseUint(fields[1], 10, 64) + if err != nil { + return pidRecord{}, fmt.Errorf("pidfile %s: parsing starttime %q: %w", path, fields[1], err) + } + return pidRecord{PID: pid, StartTime: startTime, BootID: fields[2]}, nil +} + +// alive reports whether the process this record names is still running. +// +// The boot id is compared FIRST, and the short-circuit is not belt-and-braces: +// starttime is measured in clock ticks SINCE BOOT, so after a reboot a +// long-uptime host re-issues low pids and a stale record's (pid, starttime) pair +// can legitimately MATCH an unrelated process on the new boot. Since a match is +// what authorizes the reaper to signal, that would be a plausible wrong kill; a +// boot-id mismatch means the record predates a reboot and nothing it names can +// exist, so it is "gone" before any proc read or signal. +// +// Within the same boot it re-reads starttime and compares: a mismatch, or a +// /proc read that says the process is gone (procReadMeansGone), means the +// recorded process is gone and the pid, if live at all, now belongs to an +// unrelated process that must NOT be killed. A bare-pid file can make neither +// distinction. +// +// A same-boot intent record is the third verdict: false with errPidUnknown (see +// errPidUnknown). The bool is meaningless when that error is returned. +// +// So a non-nil error here is one of TWO kinds and callers MUST discriminate +// with errors.Is before acting on the bool: +// +// - errPidUnknown is a VERDICT, not a fault: route the dir to the +// possibly-live arm — warn, KEEP the dir, never signal and never remove. +// - anything else is a genuine fault (the boot id could not be read, or the +// /proc read failed for a reason other than the process being gone): +// surface it as a named per-dir error and leave the dir alone. +// +// The natural-but-wrong handling — `if err != nil { return err }`, or +// `if !alive { remove }` — converts the possibly-live verdict into exactly the +// removal it exists to prevent, because the bool is false in that case too. +func (r pidRecord) alive() (bool, error) { + bootID, err := procBootID() + if err != nil { + return false, err + } + if r.BootID != bootID { + return false, nil + } + if r.Intent { + return false, errPidUnknown + } + startTime, err := readProcStartTime(r.PID) + if err != nil { + // The process is gone: its /proc entry was already absent, or the task + // was reaped between the open and the read. Any OTHER read failure is a + // genuine fault and propagates, because the reaper must not read "I + // could not look" as "dead". + if procReadMeansGone(err) { + return false, nil + } + return false, err + } + return startTime == r.StartTime, nil +} + +// readProcStartTime reads field 22 (starttime) of /proc//stat: the +// kernel's process start time in clock ticks since boot, immutable for the +// process's life. Together with the pid it is unique across pid reuse within a +// boot for any wrap slower than one clock tick — the same identity pair systemd +// and every pidfd-less supervisor relies on. +func readProcStartTime(pid int) (uint64, error) { + statPath := "/proc/" + strconv.Itoa(pid) + "/stat" + raw, err := os.ReadFile(statPath) //nolint:gosec // G304: statPath is a /proc path built from an integer pid, not user input + if err != nil { + return 0, fmt.Errorf("reading %s: %w", statPath, err) + } + startTime, err := parseProcStartTime(string(raw)) + if err != nil { + return 0, fmt.Errorf("%s: %w", statPath, err) + } + return startTime, nil +} + +// procReadMeansGone reports whether err from a /proc/ read means the +// process is gone rather than that the read itself failed. Both kinds are +// reachable: ENOENT when /proc/ was already absent at open time, and +// ESRCH when the open succeeded but the task was reaped before the read. +// ESRCH does NOT satisfy errors.Is(err, os.ErrNotExist), so matching only +// that predicate misses about half the occurrences. +func procReadMeansGone(err error) bool { + return errors.Is(err, syscall.ENOENT) || errors.Is(err, syscall.ESRCH) +} + +// parseProcStartTime extracts field 22 from one /proc//stat line. It is +// separate from the read above so it can be tested against lines whose field 22 +// is KNOWN: a round-trip through the live /proc cannot detect a wrong offset, +// because the writer and the liveness check would misparse identically and +// still agree. +// +// The parse starts from the LAST ')' in the line, not from a whole-line field +// split: field 2 is the executable name, parenthesized, and it may itself +// contain both spaces and parentheses (it is the first 15 bytes of a +// caller-chosen comm), so a naive split misaligns every field after it — and +// yields a wrong-but-plausible number rather than an error. +func parseProcStartTime(line string) (uint64, error) { + commEnd := strings.LastIndexByte(line, ')') + if commEnd < 0 { + return 0, errors.New("malformed stat: no comm terminator") + } + // The first field after comm is field 3 (state), so field N sits at index + // N-3 of this slice. + fields := strings.Fields(line[commEnd+1:]) + const startTimeIndex = procStartTimeField - 3 + if len(fields) <= startTimeIndex { + return 0, fmt.Errorf("malformed stat: %d fields after comm, need %d", + len(fields), startTimeIndex+1) + } + startTime, err := strconv.ParseUint(fields[startTimeIndex], 10, 64) + if err != nil { + return 0, fmt.Errorf("parsing starttime %q: %w", fields[startTimeIndex], err) + } + return startTime, nil +} diff --git a/go/internal/runtime/microvm/pidfile_test.go b/go/internal/runtime/microvm/pidfile_test.go new file mode 100644 index 00000000..9cae3735 --- /dev/null +++ b/go/internal/runtime/microvm/pidfile_test.go @@ -0,0 +1,847 @@ +//go:build unix + +// pidfile_test.go is the hermetic tier for §(a)'s pidfile primitives. It needs +// no KVM and no guest: it reads /proc/self/stat and the boot id, exactly as the +// production path does — the same Linux-hermetic posture readPSS already has in +// this unix-tagged package. +// +// Every assertion here is a contract the reaper (§(b)) will act on: it kills on +// a starttime match, refuses to kill on a mismatch or a stale boot id, and +// keeps a dir on errPidUnknown. A wrong verdict from any of these is a killed +// innocent process or a leaked orphan, so each is pinned rather than assumed. + +package microvm + +import ( + "errors" + "os" + "os/exec" + "path/filepath" + "slices" + "strconv" + "strings" + "syscall" + "testing" + "time" +) + +// deadPID returns a pid that provably names no process: pid_max itself, which +// the kernel never allocates (pids are drawn from [1, pid_max)). That makes the +// ENOENT arm deterministic — spawning and reaping a real process would leave a +// pid the kernel is free to recycle before the assertion runs. +func deadPID(t *testing.T) int { + t.Helper() + raw, err := os.ReadFile("/proc/sys/kernel/pid_max") + if err != nil { + t.Fatalf("reading pid_max: %v", err) + } + pidMax, err := strconv.Atoi(strings.TrimSpace(string(raw))) + if err != nil { + t.Fatalf("parsing pid_max %q: %v", raw, err) + } + return pidMax +} + +// liveBootID is the boot id the production path writes and compares against. +func liveBootID(t *testing.T) string { + t.Helper() + id, err := procBootID() + if err != nil { + t.Fatalf("procBootID: %v", err) + } + return id +} + +// dirEntries lists dir's entry names, sorted — the surface the "no temp file +// left behind" assertions compare against. +func dirEntries(t *testing.T, dir string) []string { + t.Helper() + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("reading dir %s: %v", dir, err) + } + names := make([]string, 0, len(entries)) + for _, e := range entries { + names = append(names, e.Name()) + } + slices.Sort(names) + return names +} + +// TestWritePidfileRoundTrip is the settled record's contract: what +// writePidfile puts on disk must read back as the LIVE identity of the pid it +// names — this process's own starttime from /proc and this boot's id. If the +// starttime written were anything other than the one a later readProcStartTime +// yields, alive() would report a false mismatch and the reaper would refuse to +// kill every real orphan. +func TestWritePidfileRoundTrip(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "vmm.pid") + pid := os.Getpid() + + if err := writePidfile(path, pid); err != nil { + t.Fatalf("writePidfile: %v", err) + } + + rec, err := readPidfile(path) + if err != nil { + t.Fatalf("readPidfile: %v", err) + } + if rec.Intent { + t.Errorf("settled record read back with Intent=true") + } + if rec.PID != pid { + t.Errorf("record PID = %d, want %d", rec.PID, pid) + } + wantStart, err := readProcStartTime(pid) + if err != nil { + t.Fatalf("readProcStartTime(self): %v", err) + } + if rec.StartTime != wantStart { + t.Errorf("record StartTime = %d, want the live proc value %d", rec.StartTime, wantStart) + } + if rec.BootID != liveBootID(t) { + t.Errorf("record BootID = %q, want the live boot id %q", rec.BootID, liveBootID(t)) + } + + // The whole point of the round-trip: this record must read as ALIVE, since + // it names a running process on this boot. + alive, err := rec.alive() + if err != nil { + t.Fatalf("alive() on a record naming this live process: %v", err) + } + if !alive { + t.Error("alive() = false for a record naming this live process, want true") + } + + // Mode 0600: the record is host-private. + info, err := os.Stat(path) + if err != nil { + t.Fatalf("stat %s: %v", path, err) + } + if perm := info.Mode().Perm(); perm != 0o600 { + t.Errorf("pidfile mode = %04o, want 0600", perm) + } +} + +// TestWritePidIntentRoundTrip pins the pre-spawn record's THREE-way verdict, +// which is the reason errPidUnknown exists at all. A same-boot intent record +// names a child that may or may not be running with no pid to check, so the +// reaper must neither kill nor remove — it needs a verdict distinct from both +// alive and dead. A record from a PREVIOUS boot has no such ambiguity: nothing +// it names can exist, so it is plainly dead by the boot-id short-circuit. +func TestWritePidIntentRoundTrip(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "vmm.pid") + + if err := writePidIntent(path); err != nil { + t.Fatalf("writePidIntent: %v", err) + } + + rec, err := readPidfile(path) + if err != nil { + t.Fatalf("readPidfile: %v", err) + } + if !rec.Intent { + t.Errorf("intent record read back with Intent=false (%+v)", rec) + } + if rec.BootID != liveBootID(t) { + t.Errorf("intent record BootID = %q, want the live boot id %q", rec.BootID, liveBootID(t)) + } + if rec.PID != 0 || rec.StartTime != 0 { + t.Errorf("intent record carries a pid/starttime (%+v); it names no spawned process", rec) + } + + // Same boot: unknowable, and that must be a distinct sentinel rather than a + // bool the reaper would read as a kill authorization or a removal. + alive, err := rec.alive() + if !errors.Is(err, errPidUnknown) { + t.Errorf("alive() on a same-boot intent record: err = %v, want errPidUnknown", err) + } + if alive { + t.Error("alive() = true alongside errPidUnknown; the bool must not claim liveness") + } + + // A previous boot: dead, with no error — the boot-id short-circuit runs + // BEFORE the intent check, so a stale intent record is reapable. + stale := rec + stale.BootID = perturb(rec.BootID) + alive, err = stale.alive() + if err != nil { + t.Errorf("alive() on a previous-boot intent record: err = %v, want nil", err) + } + if alive { + t.Error("alive() = true for an intent record from a previous boot, want false") + } +} + +// TestWritePidfileSupersedesIntent is the atomic-supersede contract: step 2's +// rename must REPLACE step 1's record in one operation, leaving the dir holding +// exactly one file with exactly the settled record. A scheme that wrote a +// second file, or left the intent record beside the settled one, would hand the +// reaper two contradictory records for one child. +func TestWritePidfileSupersedesIntent(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "vmm.pid") + + if err := writePidIntent(path); err != nil { + t.Fatalf("writePidIntent: %v", err) + } + if err := writePidfile(path, os.Getpid()); err != nil { + t.Fatalf("writePidfile over an intent record: %v", err) + } + + if got := dirEntries(t, dir); !slices.Equal(got, []string{"vmm.pid"}) { + t.Errorf("dir entries after both writes = %v, want exactly [vmm.pid]", got) + } + raw, err := os.ReadFile(path) + if err != nil { + t.Fatalf("reading %s: %v", path, err) + } + if strings.Contains(string(raw), pidIntentToken) { + t.Errorf("pidfile still carries the intent token after the settled write: %q", raw) + } + rec, err := readPidfile(path) + if err != nil { + t.Fatalf("readPidfile: %v", err) + } + if rec.Intent || rec.PID != os.Getpid() { + t.Errorf("record after the settled write = %+v, want the settled record for pid %d", rec, os.Getpid()) + } +} + +// TestPidRecordAliveRefusesUnverifiedIdentities is the kill-an-innocent guard, +// stated as the three ways a record can fail to name its process. Each must be +// (false, nil) — a plain "gone", so the reaper skips it — and NOT a signal. +func TestPidRecordAliveRefusesUnverifiedIdentities(t *testing.T) { + bootID := liveBootID(t) + selfStart, err := readProcStartTime(os.Getpid()) + if err != nil { + t.Fatalf("readProcStartTime(self): %v", err) + } + + cases := map[string]pidRecord{ + // ENOENT: /proc/ does not exist, so there is nothing to compare. + "enoent": {PID: deadPID(t), StartTime: selfStart, BootID: bootID}, + // Starttime mismatch on a LIVE pid — the reuse case. This pid is very + // much alive (it is the test process), so a bare-pid reaper would kill + // it; the starttime says it is not the recorded process. + "starttime mismatch": {PID: os.Getpid(), StartTime: selfStart + 1, BootID: bootID}, + // Stale boot id: the record predates a reboot. Load-bearing on its own, + // because after a reboot a (pid, starttime) pair from the previous boot + // can legitimately MATCH an unrelated process — and a match authorizes + // a kill. + "stale boot id": {PID: os.Getpid(), StartTime: selfStart, BootID: perturb(bootID)}, + } + for name, rec := range cases { + t.Run(name, func(t *testing.T) { + alive, aliveErr := rec.alive() + if aliveErr != nil { + t.Fatalf("alive() = _, %v; want a nil error (an unverified identity is plainly gone)", aliveErr) + } + if alive { + t.Errorf("alive() = true for %+v; the reaper would signal a process this record does not name", rec) + } + }) + } +} + +// TestPidfileWritesLeaveNoTempFile pins the atomic write's hygiene. The reaper +// scans the runtime dir per file, so a leftover `*.tmp*` would be an extra +// unparseable entry it has to reason about — and a temp file that outlived its +// rename would mean the rename was not the only thing that landed. +func TestPidfileWritesLeaveNoTempFile(t *testing.T) { + dir := t.TempDir() + names := []string{"vmm.pid", "virtiofsd.pid", "passt.pid"} + for _, name := range names { + path := filepath.Join(dir, name) + if err := writePidIntent(path); err != nil { + t.Fatalf("writePidIntent(%s): %v", name, err) + } + if err := writePidfile(path, os.Getpid()); err != nil { + t.Fatalf("writePidfile(%s): %v", name, err) + } + } + want := slices.Clone(names) + slices.Sort(want) + if got := dirEntries(t, dir); !slices.Equal(got, want) { + t.Errorf("dir entries = %v, want exactly %v (a temp file survived a write)", got, want) + } +} + +// TestReadPidfileRejectsUnparseableContent is the "I cannot tell what this +// says" contract: every malformed form must ERROR rather than read back as a +// zero record, because the reaper must never mistake an unreadable pidfile for +// one naming nothing alive — that path removes the dir and erases the record. +func TestReadPidfileRejectsUnparseableContent(t *testing.T) { + bootID := liveBootID(t) + cases := map[string]string{ + "empty": "", + "garbage": "not a pid record at all\n", + "intent missing boot id": pidIntentToken + "\n", + "intent with extra field": pidIntentToken + " " + bootID + " extra\n", + "settled missing boot id": "1234 5678\n", + "settled extra field": "1234 5678 " + bootID + " extra\n", + "non-numeric pid": "abc 5678 " + bootID + "\n", + "non-numeric starttime": "1234 abc " + bootID + "\n", + "non-positive pid": "0 5678 " + bootID + "\n", + "negative pid": "-1 5678 " + bootID + "\n", + "second record line": "1234 5678 " + bootID + "\n9999 1111 " + bootID + "\n", + "starttime out of uint64": "1234 99999999999999999999999 " + bootID + "\n", + } + for name, content := range cases { + t.Run(name, func(t *testing.T) { + path := filepath.Join(t.TempDir(), "vmm.pid") + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatal(err) + } + rec, err := readPidfile(path) + if err == nil { + t.Fatalf("readPidfile(%q) = %+v, nil; want an error", content, rec) + } + }) + } +} + +// TestReadPidfileErrorsOnAbsentFile keeps the absent-file case distinct from the +// malformed one: it must surface as an os.ErrNotExist-wrapping error, since the +// reaper's "no readable pidfiles" arm (the age gate) is selected by exactly that +// distinction. +func TestReadPidfileErrorsOnAbsentFile(t *testing.T) { + _, err := readPidfile(filepath.Join(t.TempDir(), "vmm.pid")) + if !errors.Is(err, os.ErrNotExist) { + t.Errorf("readPidfile on an absent path: err = %v, want it to wrap os.ErrNotExist", err) + } +} + +// TestParseProcStartTimeFindsField22 is the field-22 parsing hazard, checked +// against lines whose field 22 is KNOWN rather than through the live /proc. +// That independence is the whole point: writePidfile and alive() both go +// through this parse, so a wrong offset makes them misparse IDENTICALLY and +// still agree — a write→read→alive round-trip passes just as happily on a +// naive whole-line strings.Fields split. Verified by mutation: swapping this +// parse for that split leaves every round-trip test in this file green. +// +// Field 2 is the comm, parenthesized, carrying the first 15 bytes of the +// executable's basename VERBATIM — spaces and parens included. A whole-line +// split therefore misaligns every later field and yields a wrong-but-plausible +// starttime, not an error: alive() reports a false mismatch and the reaper +// silently refuses to kill a real orphan. +func TestParseProcStartTimeFindsField22(t *testing.T) { + // Each line's field 22 is 987654321, and every other numeric field is a + // distinguishable decoy, so an off-by-N offset reads a different value + // rather than coincidentally matching. + const want = 987654321 + // Fields 3..21 (state + 18 numbers), then field 22, then a tail. + tail := " 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 " + + strconv.Itoa(want) + " 111 222 333 444\n" + cases := map[string]string{ + "plain comm": "4242 (sleep) S" + tail, + "comm with a space": "4242 (a b c) S" + tail, + "comm with parens": "4242 (a (b) c) S" + tail, + "comm with both": "4242 (a b) c (d) 0 0) S" + tail, + "comm that looks numeric": "4242 (0 0 0 0 0) S" + tail, + } + for name, line := range cases { + t.Run(name, func(t *testing.T) { + got, err := parseProcStartTime(line) + if err != nil { + t.Fatalf("parseProcStartTime: %v", err) + } + if got != want { + t.Errorf("field 22 = %d, want %d — the parse is offset by the comm's own spaces/parens", got, want) + } + }) + } +} + +// TestParseProcStartTimeRejectsMalformedLines keeps a truncated or garbled stat +// line an ERROR rather than a zero starttime: zero would compare unequal to +// every real record and quietly demote a live orphan to "gone". +func TestParseProcStartTimeRejectsMalformedLines(t *testing.T) { + cases := map[string]string{ + "empty": "", + "no comm terminator": "4242 (sleep S 1 2 3\n", + "too few fields": "4242 (sleep) S 1 2 3\n", + "non-numeric field 22": "4242 (sleep) S 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 nope 111\n", + } + for name, line := range cases { + t.Run(name, func(t *testing.T) { + got, err := parseProcStartTime(line) + if err == nil { + t.Errorf("parseProcStartTime(%q) = %d, nil; want an error", line, got) + } + }) + } +} + +// TestParseProcStartTimeAgreesWithProcSelf ties the parse above back to the +// real kernel format: the synthetic lines pin the OFFSET, and this pins that +// /proc/self/stat actually has that shape. Field 22 is ticks since boot, so the +// only bound available without re-deriving the clock is that it is non-zero and +// below the uptime in ticks — enough to catch a parse that landed on a pointer +// field (those are vastly larger) or on a zero. +func TestParseProcStartTimeAgreesWithProcSelf(t *testing.T) { + raw, err := os.ReadFile("/proc/self/stat") + if err != nil { + t.Fatalf("reading /proc/self/stat: %v", err) + } + startTime, err := parseProcStartTime(string(raw)) + if err != nil { + t.Fatalf("parseProcStartTime(/proc/self/stat): %v", err) + } + if startTime == 0 { + t.Fatal("this process's starttime parsed as 0; field 22 is non-zero for any process after boot") + } + uptimeRaw, err := os.ReadFile("/proc/uptime") + if err != nil { + t.Fatalf("reading /proc/uptime: %v", err) + } + uptimeSeconds, err := strconv.ParseFloat(strings.Fields(string(uptimeRaw))[0], 64) + if err != nil { + t.Fatalf("parsing uptime %q: %v", uptimeRaw, err) + } + // USER_HZ is conventionally 100 and is what /proc//stat reports in; + // an upper bound only needs it to be no smaller than that. + const userHZ = 100 + if maxTicks := uint64(uptimeSeconds * userHZ); startTime > maxTicks { + t.Errorf("starttime %d ticks exceeds the host uptime %d ticks — field 22 was read from the wrong offset", + startTime, maxTicks) + } +} + +// TestPidfileIdentifiesAProcessWithADeceptiveComm exercises the hazard +// end-to-end on a REAL process whose comm carries spaces and parens: the +// production write→read→alive path must call a live child alive. The offset +// itself is pinned by the synthetic cases above (this path cannot detect a +// wrong one); what this adds is that nothing in the live path — comm +// truncation, the log capture, the two-step write — breaks on such a name. +func TestPidfileIdentifiesAProcessWithADeceptiveComm(t *testing.T) { + dir := t.TempDir() + // A basename whose first 15 bytes contain both spaces and parens, so the + // comm the kernel reports is itself field-shaped. Copied from /bin/sh: comm + // comes from the EXECUTED binary, so a shell script under this name would + // report "sh" and prove nothing. + sh, err := exec.LookPath("sh") + if err != nil { + t.Skipf("no sh on PATH: %v", err) + } + shBytes, err := os.ReadFile(sh) + if err != nil { + t.Fatalf("reading %s: %v", sh, err) + } + bin := filepath.Join(dir, "a b) c (d) 0 0 0 0") + if err = os.WriteFile(bin, shBytes, 0o700); err != nil { + t.Fatalf("writing the deceptively-named binary: %v", err) + } + + vm := &VM{} + c := &child{ + name: "cloud-hypervisor", + logPath: filepath.Join(dir, "child.log"), + // Blocks on a stdin read rather than sleeping, so it stays alive for + // the assertions and exits when the pipe closes — no timing window. + cmd: exec.CommandContext(t.Context(), bin, "-c", "read -r _ || true"), + } + stdin, err := c.cmd.StdinPipe() + if err != nil { + t.Fatalf("StdinPipe: %v", err) + } + if err = vm.startRecordedChild(c, dir, "vmm.pid"); err != nil { + t.Fatalf("startRecordedChild over a deceptively-named binary: %v", err) + } + t.Cleanup(func() { + _ = stdin.Close() // releases the child's blocking read; a close error on teardown is not actionable + <-c.exited + }) + + // The comm really is deceptive: assert the fixture before trusting the + // verdict it produces, so a kernel that truncated the parens away cannot + // make this test pass vacuously. + stat, err := os.ReadFile("/proc/" + strconv.Itoa(c.cmd.Process.Pid) + "/stat") + if err != nil { + t.Fatalf("reading the child's stat: %v", err) + } + comm := string(stat) + comm = comm[strings.IndexByte(comm, '(')+1 : strings.LastIndexByte(comm, ')')] + if !strings.ContainsAny(comm, " )") { + t.Fatalf("comm %q carries no space or paren; the fixture no longer exercises the hazard", comm) + } + + rec, err := readPidfile(filepath.Join(dir, "vmm.pid")) + if err != nil { + t.Fatalf("readPidfile: %v", err) + } + if rec.PID != c.cmd.Process.Pid { + t.Errorf("record PID = %d, want the child's %d", rec.PID, c.cmd.Process.Pid) + } + alive, err := rec.alive() + if err != nil { + t.Fatalf("alive() on the live deceptively-named child: %v", err) + } + if !alive { + t.Errorf("alive() = false for a LIVE child with comm %q", comm) + } +} + +// TestSpawnFailureLeavesTheIntentRecord is the whole reason step 1 exists, +// exercised through the production wiring rather than the primitives alone: when +// the spawn between the two writes fails, the runtime dir must still NAME the +// child. Before the intent record, that crash window left a dir with no record +// at all of a child that may have become live, and §(b) step 3 would read +// "everything recorded is dead" and remove the evidence. +func TestSpawnFailureLeavesTheIntentRecord(t *testing.T) { + dir := t.TempDir() + vm := &VM{} + c := &child{ + name: "cloud-hypervisor", + logPath: filepath.Join(dir, "cloud-hypervisor.log"), + // An absolute path to a binary that does not exist: Start fails, so the + // settled write never runs. + cmd: exec.CommandContext(t.Context(), filepath.Join(dir, "no-such-binary")), + } + + err := vm.startRecordedChild(c, dir, "vmm.pid") + if err == nil { + t.Fatal("startRecordedChild over an unspawnable binary = nil, want an error") + } + if c.cmd.Process != nil { + t.Fatalf("the fake spawned after all (pid %d); the test no longer exercises the spawn-failure window", c.cmd.Process.Pid) + } + + // The path must be registered for cleanup even though the boot failed — + // otherwise the intent record outlives the session that wrote it. + if !slices.Contains(vm.pidfiles, filepath.Join(dir, "vmm.pid")) { + t.Errorf("vm.pidfiles = %v, want it to carry vmm.pid so Shutdown removes the intent record", vm.pidfiles) + } + + rec, readErr := readPidfile(filepath.Join(dir, "vmm.pid")) + if readErr != nil { + t.Fatalf("readPidfile after a failed spawn: %v — the dir no longer names the child", readErr) + } + if !rec.Intent { + t.Errorf("record after a failed spawn = %+v, want the intent record (no settled record can exist)", rec) + } + // And it routes to the possibly-live arm, not to a kill or a removal. + if _, aliveErr := rec.alive(); !errors.Is(aliveErr, errPidUnknown) { + t.Errorf("alive() after a failed spawn: err = %v, want errPidUnknown", aliveErr) + } +} + +// perturb returns a boot id that is well-formed but NOT this host's, by +// swapping the first hex digit for a different one. Fabricating a +// plainly-bogus string would leave the boot-id comparison passing for the +// wrong reason (an unparseable id rather than a merely different one). +func perturb(bootID string) string { + if bootID == "" { + return "0" + } + first := "0" + if bootID[0] == '0' { + first = "1" + } + return first + bootID[1:] +} + +// shutdownGuardBudget bounds the Shutdown-guard test below. It is not a poll +// budget or a settling delay: the passing path returns as fast as a Kill and a +// channel receive, so any value merely has to be longer than that. It exists so +// a regression of the nil-channel guard FAILS instead of wedging the suite +// forever, which is the exact symptom the guard prevents in production. +const shutdownGuardBudget = 10 * time.Second + +// writeShellFake writes an executable shell stub into dir under name. It is +// deliberately private to this file rather than shared with the teardown +// suite's equivalent: these tests must keep compiling and asserting on their +// own if that fixture moves. +func writeShellFake(t *testing.T, dir, name, body string) { + t.Helper() + // 0o755: the stub is exec'd by name through PATH, so it must carry its + // exec bits. + if err := os.WriteFile(filepath.Join(dir, name), []byte("#!/bin/sh\n"+body+"\n"), 0o755); err != nil { + t.Fatalf("writing the %s fake: %v", name, err) + } +} + +// startedAndReapedChild starts c through the production startChild and blocks +// until its SOLE reaper has observed the exit. The wait is the whole point: +// after a receive from c.exited the process has been Wait'd, so it is no longer +// even a zombie and its /proc/ entry is PROVABLY gone. That makes the +// "child exited before its pidfile could be recorded" window deterministic — +// the production race is narrow, but the state it lands in is exactly this one, +// reached here without a sleep, a retry or a repeat count. +func startedAndReapedChild(t *testing.T, dir, name string) *child { + t.Helper() + c := &child{ + name: name, + logPath: filepath.Join(dir, name+".log"), + // Writes a diagnostic to its captured log and exits non-zero, as a + // daemon that cannot start does — so the log tail the error carries + // has something in it to assert on. + cmd: exec.CommandContext(t.Context(), "/bin/sh", "-c", "echo 'could not setup id mappings' >&2; exit 1"), + } + if err := startChild(c); err != nil { + t.Fatalf("startChild(%s fake): %v", name, err) + } + <-c.exited + return c +} + +// TestShutdownDoesNotBlockOnAnUnrecordedVMM pins the nil-channel guard. Between +// startChild spawning cloud-hypervisor and launch hoisting the reaper's channel +// onto vm.vmmExited, a startRecordedChild failure leaves the VM holding a LIVE +// process handle and a NIL channel — and launch's deferred Shutdown runs in +// exactly that state. Guarding the VMM arm on the process handle alone then +// receives on a nil channel and blocks FOREVER: a hang with no stack and no +// diagnostic, which is strictly worse than the launch error it was cleaning up +// after. Shutdown must skip only the receive, and must still Kill. +func TestShutdownDoesNotBlockOnAnUnrecordedVMM(t *testing.T) { + dir := t.TempDir() + vm := &VM{ + vmm: &child{ + name: "cloud-hypervisor", + logPath: filepath.Join(dir, "cloud-hypervisor.log"), + // Long-lived: the process must still be running when Shutdown is + // called, so the Kill below is a real one and the nil receive is + // reached rather than short-circuited by an already-dead child. + cmd: exec.CommandContext(t.Context(), "/bin/sh", "-c", "sleep 300"), + }, + } + if err := startChild(vm.vmm); err != nil { + t.Fatalf("startChild(vmm fake): %v", err) + } + // The state under test: the child is started and reaper-backed, but the + // channel was never hoisted onto the VM — precisely what a + // startRecordedChild failure after a successful spawn leaves behind. + if vm.vmmExited != nil { + t.Fatal("vmmExited is set; the test no longer exercises the unrecorded-VMM state") + } + pid := vm.vmm.cmd.Process.Pid + + done := make(chan error, 1) + go func() { done <- vm.Shutdown(t.Context()) }() + select { + case err := <-done: + if err != nil { + t.Errorf("Shutdown on an unrecorded VMM = %v, want nil", err) + } + case <-time.After(shutdownGuardBudget): + t.Fatalf("Shutdown blocked for %s on an unrecorded VMM: it received on the nil vmmExited channel", shutdownGuardBudget) + } + + // The guard must skip the RECEIVE only. The started-but-unrecorded process + // is still an orphan-in-waiting, so Kill has to have run: a guard that + // skipped the whole arm would leak it. + <-vm.vmm.exited // the child's own reaper, which the VM never learned about + if pidAlive(pid) { + t.Errorf("cloud-hypervisor (pid %d) still alive after Shutdown: the guard skipped the Kill, not just the receive", pid) + } +} + +// TestStartRecordedChildNamesADeadChildNotAProcPath is the second half of the +// same window. writePidfile's step 2 reads /proc//stat, so a child that +// exits fast is reaped out from under it and the read fails — for a reason that +// is NOT a pidfile fault. Reporting the /proc path there buries the actual +// cause (the daemon's own diagnostic) behind a path the operator can do nothing +// with, so a CONFIRMED-dead child must get the same error shape launch uses for +// a daemon that died before the VMM was started. +// +// Both failure kinds are covered by procReadMeansGone: ENOENT when /proc/ +// is already absent at open time, and ESRCH when the open won but the read +// lost. ESRCH does NOT satisfy errors.Is(err, os.ErrNotExist), so a predicate +// keyed on that alone misses roughly half the real occurrences. +func TestStartRecordedChildNamesADeadChildNotAProcPath(t *testing.T) { + dir := t.TempDir() + c := startedAndReapedChild(t, dir, "virtiofsd") + + // The production step-2 write against a provably-reaped pid: the same call + // startRecordedChild makes, failing the same way. + writeErr := writePidfile(filepath.Join(dir, "virtiofsd.pid"), c.cmd.Process.Pid) + if writeErr == nil { + t.Fatal("writePidfile on a reaped pid = nil; the fixture no longer reaches the dead-child window") + } + if !procReadMeansGone(writeErr) { + t.Fatalf("writePidfile error %v is not classified as the process being gone", writeErr) + } + + err := pidfileWriteError(c, writeErr) + if err == nil { + t.Fatal("pidfileWriteError on a confirmed-dead child = nil, want an error (the boot must still fail)") + } + msg := err.Error() + if !strings.Contains(msg, "virtiofsd") { + t.Errorf("error %q does not name the daemon", msg) + } + if strings.Contains(msg, "/proc/") { + t.Errorf("error %q reports a /proc path; the operator needs the daemon's cause, not the failed read", msg) + } + // The daemon's own diagnostic is the thing the operator acts on. + if !strings.Contains(msg, "could not setup id mappings") { + t.Errorf("error %q does not carry the daemon's log tail", msg) + } +} + +// TestPidfileWriteErrorKeepsGenuineFaultsFatal is the other arm: only a +// read failure that means "the process is gone" AND a reaper-confirmed exit may +// be reshaped. A guard that dropped either condition would swallow a real +// pidfile fault — an unwritable runtime dir, a full filesystem — as a dead +// child, and a session whose record cannot be written must not boot, because +// orphan reapability is load-bearing. +func TestPidfileWriteErrorKeepsGenuineFaultsFatal(t *testing.T) { + dir := t.TempDir() + exited := startedAndReapedChild(t, dir, "passt") + live := &child{name: "passt", logPath: filepath.Join(dir, "live.log"), exited: make(chan struct{})} + + cases := map[string]struct { + c *child + err error + }{ + // Dead child, but the write failed for its own reason: still fatal. + "exited child, genuine write fault": {c: exited, err: syscall.EACCES}, + // The /proc read said gone, but the reaper has not confirmed it: the + // claim "this child is dead" is unproven, so it stays fatal. + "live child, proc read says gone": {c: live, err: syscall.ESRCH}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + err := pidfileWriteError(tc.c, tc.err) + if !strings.Contains(err.Error(), "recording passt pidfile") { + t.Errorf("pidfileWriteError = %q, want the fatal pidfile-error shape", err) + } + if !errors.Is(err, tc.err) { + t.Errorf("pidfileWriteError dropped the underlying %v from the chain", tc.err) + } + }) + } +} + +// TestWritePidfileErrorStaysClassifiable guards the wrapping added to +// writePidfile's error paths. The dead-child branch in startRecordedChild is +// selected by errors.Is against the raw syscall errnos, so a wrap that used +// anything but %w would silently demote every fast-exiting daemon back to the +// unhelpful /proc-path error — and nothing else would notice. +func TestWritePidfileErrorStaysClassifiable(t *testing.T) { + err := writePidfile(filepath.Join(t.TempDir(), "vmm.pid"), deadPID(t)) + if err == nil { + t.Fatal("writePidfile against a never-allocated pid = nil, want an error") + } + if !procReadMeansGone(err) { + t.Errorf("writePidfile error %v is not classified as gone; the wrap broke errors.Is matching", err) + } +} + +// TestLaunchRecordsAllThreePidfileNames pins the WIRING: that launch records +// vmm.pid, virtiofsd.pid and passt.pid in the session runtime dir. Every other +// hermetic test in this file passes its own pidfile name in, so it asserts the +// name it supplied; the real-pid check lives in the KVM-gated lane, which does +// not run on the ordinary test lane. Without this, wiring that recorded +// virtiofsd under vmm.pid — or dropped a spawn site entirely — is caught by +// NOTHING here. It needs no KVM: which strings launch passes is a property of +// launch, so all three children are shell fakes. +func TestLaunchRecordsAllThreePidfileNames(t *testing.T) { + bin := t.TempDir() + // PATH is narrowed to the fake dir below so LookPath resolves the fakes and + // nothing else, which also means a fake's own body cannot resolve a command + // by name. So the stay-alive exec is an ABSOLUTE path, found before the + // narrowing: a fake that could not resolve `sleep` would exit instantly and + // fail the boot at the liveness-aware readiness poll instead of reaching + // the VMM spawn this test is about. + sleepBin, err := exec.LookPath("sleep") + if err != nil { + t.Skipf("no sleep on PATH: %v", err) + } + stayAlive := "exec " + sleepBin + " 300" + // Each fake touches the socket path it is told to serve, then stays alive: + // launch's readiness poll is liveness-aware, so a fake that exited would + // fail the boot before the VMM spawn. + writeShellFake(t, bin, "virtiofsd", `for a in "$@"; do case "$a" in --socket-path=*) : > "${a#--socket-path=}";; esac; done +`+stayAlive) + writeShellFake(t, bin, "passt", `p=""; for a in "$@"; do [ "$p" = --socket ] && : > "$a"; p="$a"; done +`+stayAlive) + // The VMM is spawned last and launch waits on no socket of its own, so a + // fake that merely stays alive carries the boot to a successful return — + // which is what makes vm.pidfiles readable at all (the error path returns + // a nil VM by design). + writeShellFake(t, bin, "cloud-hypervisor", stayAlive) + t.Setenv("PATH", bin) + + dir := t.TempDir() + cfg := BootConfig{ + Kernel: "/nonexistent/kernel", Initrd: "/nonexistent/initrd", Rootfs: "/nonexistent/rootfs", + VsockCID: 3, VsockPort: 1024, VsockSocket: filepath.Join(dir, "vsock.sock"), + FSTag: "workspace", FSSocket: filepath.Join(dir, "virtiofsd.sock"), FSSharedDir: dir, + CPUs: 2, MemoryMB: 1024, + Net: NetConfig{VhostUserSocket: filepath.Join(dir, "net.sock"), MAC: "12:34:56:78:9a:bc"}, + } + + vm, err := Launch(t.Context(), cfg) + if err != nil { + t.Fatalf("Launch over shell fakes: %v", err) + } + t.Cleanup(func() { + _ = vm.Shutdown(t.Context()) // teardown of fakes; a reap error here is not what this test asserts + }) + + want := []string{ + filepath.Join(dir, "passt.pid"), + filepath.Join(dir, "virtiofsd.pid"), + filepath.Join(dir, "vmm.pid"), + } + got := slices.Clone(vm.pidfiles) + slices.Sort(got) + if !slices.Equal(got, want) { + t.Errorf("vm.pidfiles = %v, want exactly %v", got, want) + } + // Each must be a SETTLED record naming a live pid, not a leftover intent: + // a spawn site that recorded the intent and never settled would satisfy the + // name check above while leaving the reaper unable to identify the child. + for _, p := range want { + rec, readErr := readPidfile(p) + if readErr != nil { + t.Errorf("readPidfile(%s): %v", filepath.Base(p), readErr) + continue + } + if rec.Intent { + t.Errorf("%s holds the intent record; the settled write never ran", filepath.Base(p)) + continue + } + alive, aliveErr := rec.alive() + if aliveErr != nil { + t.Errorf("%s: alive() = %v", filepath.Base(p), aliveErr) + } + if !alive { + t.Errorf("%s names pid %d, which is not the live child", filepath.Base(p), rec.PID) + } + } +} + +// TestReadPidfileRejectsAnOverlongFile bounds the read. A pidfile is one line +// of about 60 bytes, so a file longer than the bound is not a record at all — +// it must route to the malformed arm (which the reaper treats as "I cannot tell +// what this says") rather than being pulled into memory whole, and rather than +// being truncated into a prefix that happens to parse. +func TestReadPidfileRejectsAnOverlongFile(t *testing.T) { + bootID := liveBootID(t) + path := filepath.Join(t.TempDir(), "vmm.pid") + // A VALID record followed by enough padding to exceed the bound: a read + // that truncated instead of erroring would parse the leading record and + // report a confident, wrong verdict. + line := "1234 5678 " + bootID + "\n" + content := line + strings.Repeat("x", maxPidfileRecordBytes+1) + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatal(err) + } + rec, err := readPidfile(path) + if err == nil { + t.Fatalf("readPidfile on a %d-byte file = %+v, nil; want an error", len(content), rec) + } + // And a record AT the bound still reads, so the bound is not off by one + // against the real record size. + atBound := line + strings.Repeat(" ", maxPidfileRecordBytes-len(line)) + if err = os.WriteFile(path, []byte(atBound), 0o600); err != nil { + t.Fatal(err) + } + if _, err = readPidfile(path); err != nil { + t.Errorf("readPidfile on a %d-byte file (at the bound): %v", len(atBound), err) + } +}