From 40807732d39d539aee156bd294dc5277bba75af3 Mon Sep 17 00:00:00 2001 From: mintaka Date: Mon, 7 Sep 2026 13:07:27 -0400 Subject: [PATCH 1/3] test(microvm): keep the fail-closed teardown fakes alive (RIG-3480) TestLaunchFailClosedTeardown narrows PATH to its own fake bin so that cloud-hypervisor is absent, but both fakes ended in a bare `sleep 30`. There is no `sleep` in that dir and /bin/sh has no `sleep` builtin, so each fake died on `sleep: command not found` milliseconds after recording its pid. The daemons were not orphans being reaped; they exited on their own, and the no-orphan assertion held trivially. Once waitForSockets became liveness-aware, that latent fixture defect turned into a merge-queue failure: launch now fails at the socket wait with "virtiofsd exited before its socket was serving" instead of reaching the cloud-hypervisor lookup, the second fake never records a pid, and readPidFile could only ever exhaust its 2s budget. Every observed failure landed at ~2.02s, the deadline itself. It reproduced only in the merge result, never on either head alone. Resolve `sleep` on the real PATH before narrowing it and burn the absolute path into the stubs, so the fakes outlive launch as the test always claimed. Add the control that makes this class of vacuity self-reporting: assert the error names cloud-hypervisor, the only absence this test arranges. Anything earlier means the daemons never came up. On the unfixed merge result that control fires on 39/40 runs while the assertion it guards stays green. readPidFile then needs no poll at all: each fake records its pid before it touches its socket, and the caller has confirmed launch got past the socket wait, which orders the write before the read. A missing pidfile is now a real ordering defect that fails immediately rather than a wall-clock budget that can only expire. Measured on the merge result (git merge-tree of main and the V6 head), 40 runs 5-way concurrent: 4/40 then 3/40 failures before, 0/40 after. Mutating Shutdown to skip reaping passt fails 12/12 on the real invariant in ~0.03s, so the assertion is load-bearing rather than merely green. --- .../runtime/microvm/launch_teardown_test.go | 57 +++++++++++++------ 1 file changed, 40 insertions(+), 17 deletions(-) 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 } From 0326e2b91f649252d08cbdb18e05e72265a2ce06 Mon Sep 17 00:00:00 2001 From: mintaka Date: Mon, 7 Sep 2026 11:52:08 -0400 Subject: [PATCH 2/3] feat(microvm): host-written per-session pidfiles with PID-reuse defense (RIG-2498) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit V7 W1 per the frozen record §(a)/§W1: the host writes a pidfile for each of the three children, carrying enough identity that a later reaper can tell a live child from a recycled PID. Each record is written in two steps. An `intent ` line goes down before the spawn, so a crash between fork and the settled write still leaves a removable marker; the settled ` ` line replaces it once the child is up. Both go through one atomic temp-file-plus-rename helper that chmods 0600 explicitly, because CreateTemp's mode is umask-masked and the record's permissions should not be an environment dependency. `alive()` checks the boot id first, so a record from a previous boot reads as dead rather than reaching the PID comparison at all. Within the same boot an intent record returns `errPidUnknown` — the spawn outcome is genuinely unknown, which is a third verdict and not a synonym for dead. A settled record compares `/proc/` field 22 against the recorded starttime, so a recycled PID does not read as the original child. Field 22 is parsed from the last `)` in the stat line: a process can put parentheses in its own comm, and splitting the whole line on whitespace shifts every later field. A round-trip through live `/proc` cannot catch that, since the write and the read share the parse and misparse identically, so `parseProcStartTime` is tested against synthetic lines whose field 22 is a known sentinel with decoys in the other numeric fields. passt's `--pid` argument is retired: the host now owns all three records, so there is one writer and one format rather than a daemon-specific special case. `VM.pidfiles` is registered before the intent write, not after the settled one, so teardown removes a record left behind by a failed spawn. Refs: RIG-2498 --- .../runtime/microvm/boot_microvm_test.go | 75 ++- go/internal/runtime/microvm/launch.go | 69 ++- go/internal/runtime/microvm/pidfile.go | 295 ++++++++++ go/internal/runtime/microvm/pidfile_test.go | 542 ++++++++++++++++++ 4 files changed, 962 insertions(+), 19 deletions(-) create mode 100644 go/internal/runtime/microvm/pidfile.go create mode 100644 go/internal/runtime/microvm/pidfile_test.go 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..e07a4152 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,7 +564,7 @@ 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 +// 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 { @@ -581,15 +588,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 +770,29 @@ 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 fmt.Errorf("microvm: recording %s pidfile: %w", c.name, err) + } + return nil +} diff --git a/go/internal/runtime/microvm/pidfile.go b/go/internal/runtime/microvm/pidfile.go new file mode 100644 index 00000000..6fbd3aab --- /dev/null +++ b/go/internal/runtime/microvm/pidfile.go @@ -0,0 +1,295 @@ +//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" + "os" + "path/filepath" + "strconv" + "strings" + "sync" +) + +// 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 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 err + } + startTime, err := readProcStartTime(pid) + if err != nil { + return 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, so a reader +// sees the complete record or the previous one, never a torn prefix — and the +// torn-write window a plain os.WriteFile leaves open is EXACTLY the Runner-crash +// window the reaper exists for, where a half-written pidfile would demote a +// recorded live child to the no-pidfile arm and leak it. +// +// 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) + 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 +} + +// 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) { + raw, err := os.ReadFile(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) + } + 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 ENOENT +// 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. +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 (or the whole pid) no longer + // exists. Any other read failure is a genuine fault and propagates, + // because the reaper must not read "I could not look" as "dead". + if errors.Is(err, os.ErrNotExist) { + 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 +} + +// 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..e89e0374 --- /dev/null +++ b/go/internal/runtime/microvm/pidfile_test.go @@ -0,0 +1,542 @@ +//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" + "testing" +) + +// 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:] +} From f399d7047785cf399661308ea412465bbd8e68a1 Mon Sep 17 00:00:00 2001 From: mintaka Date: Mon, 7 Sep 2026 13:20:00 -0400 Subject: [PATCH 3/3] =?UTF-8?q?fix(microvm):=20fold=20W1=20review=20round?= =?UTF-8?q?=201=20=E2=80=94=20nil-channel=20hang,=20dead-child=20pidfile?= =?UTF-8?q?=20errors=20(RIG-2498)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two HIGH findings, both in the new launch wiring rather than the pidfile primitives. A settled-write failure hung Shutdown on a nil channel. vm.vmmExited is hoisted onto the VM only after startRecordedChild returns, but startRecordedChild can fail after startChild has already spawned the VMM — leaving a live process handle beside a nil channel, which is exactly the state launch deferred Shutdown runs in. The bare receive there blocked forever, replacing the launch error it was cleaning up after with a hang carrying no stack. Guard the RECEIVE alone, not the arm: such a child is started, live and unrecorded, so skipping its Kill would orphan precisely what the deferred Shutdown exists to reap. Guarding the whole arm was measured to leave the VMM alive past Shutdown. A fast-exiting child failed the boot with a /proc path instead of its own diagnostic. writePidfile reads /proc//stat after startChild has installed the reaper, so the read can lose to cmd.Wait reaping the pid. Reshape that one case — and only when the child has provably exited — into the shape launch already uses for a dead child: name the daemon, wrap waitResult, carry the log tail. Every other write failure stays fatal, and the on-disk record is left alone so a child that may have become live is never under-named. That failure has TWO kinds, roughly half each: ENOENT when /proc/ is already gone at open time, and ESRCH when the open succeeds but the task is reaped before the read. ESRCH does not satisfy errors.Is(err, os.ErrNotExist), so the obvious predicate silently misses about half the occurrences. Add procReadMeansGone covering both and use it at the alive() site too, where the same predicate decided whether a dead process routes to reapable or surfaces as a fault. Also: scope writePidRecordLine durability doc to a process crash (host-crash durability is deliberately not bought, since a changed boot id short-circuits every surviving record dead); document alive() two-kinds-of-error contract so the natural-but-wrong W2 call is harder to write; bound readPidfile read at 256 bytes; wrap the bare error returns with the path; move the three-name wiring assertion into the hermetic tier so it no longer needs KVM; dot-prefix the temp pidfile pattern. --- go/internal/runtime/microvm/launch.go | 54 +++- go/internal/runtime/microvm/pidfile.go | 94 ++++-- go/internal/runtime/microvm/pidfile_test.go | 305 ++++++++++++++++++++ 3 files changed, 430 insertions(+), 23 deletions(-) diff --git a/go/internal/runtime/microvm/launch.go b/go/internal/runtime/microvm/launch.go index e07a4152..111d32bc 100644 --- a/go/internal/runtime/microvm/launch.go +++ b/go/internal/runtime/microvm/launch.go @@ -564,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 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. +// 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} { @@ -792,7 +809,34 @@ func (vm *VM) startRecordedChild(c *child, dir, pidfileName string) error { return fmt.Errorf("microvm: starting %s: %w", c.name, err) } if err := writePidfile(path, c.cmd.Process.Pid); err != nil { - return fmt.Errorf("microvm: recording %s pidfile: %w", c.name, err) + 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/pidfile.go b/go/internal/runtime/microvm/pidfile.go index 6fbd3aab..64d1213c 100644 --- a/go/internal/runtime/microvm/pidfile.go +++ b/go/internal/runtime/microvm/pidfile.go @@ -16,11 +16,13 @@ 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 @@ -92,7 +94,7 @@ type pidRecord struct { func writePidIntent(path string) error { bootID, err := procBootID() if err != nil { - return err + return fmt.Errorf("pidfile %s: %w", path, err) } return writePidRecordLine(path, pidIntentToken+" "+bootID+"\n") } @@ -110,29 +112,42 @@ func writePidIntent(path string) error { func writePidfile(path string, pid int) error { bootID, err := procBootID() if err != nil { - return err + return fmt.Errorf("pidfile %s: %w", path, err) } startTime, err := readProcStartTime(pid) if err != nil { - return err + 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, so a reader -// sees the complete record or the previous one, never a torn prefix — and the -// torn-write window a plain os.WriteFile leaves open is EXACTLY the Runner-crash -// window the reaper exists for, where a half-written pidfile would demote a -// recorded live child to the no-pidfile arm and leak it. +// 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) - tmp, err := os.CreateTemp(dir, filepath.Base(path)+".tmp") + // 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) } @@ -167,15 +182,33 @@ func writePidRecordLine(path, line string) (err error) { 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) { - raw, err := os.ReadFile(path) //nolint:gosec // G304: path is a host-built pidfile path in the session runtime dir (/microvm//*.pid), not user input + 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) @@ -214,13 +247,27 @@ func readPidfile(path string) (pidRecord, error) { // 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 ENOENT -// 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. +// 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 { @@ -234,10 +281,11 @@ func (r pidRecord) alive() (bool, error) { } startTime, err := readProcStartTime(r.PID) if err != nil { - // The process is gone: its /proc entry (or the whole pid) no longer - // exists. Any other read failure is a genuine fault and propagates, - // because the reaper must not read "I could not look" as "dead". - if errors.Is(err, os.ErrNotExist) { + // 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 @@ -263,6 +311,16 @@ func readProcStartTime(pid int) (uint64, error) { 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, diff --git a/go/internal/runtime/microvm/pidfile_test.go b/go/internal/runtime/microvm/pidfile_test.go index e89e0374..9cae3735 100644 --- a/go/internal/runtime/microvm/pidfile_test.go +++ b/go/internal/runtime/microvm/pidfile_test.go @@ -20,7 +20,9 @@ import ( "slices" "strconv" "strings" + "syscall" "testing" + "time" ) // deadPID returns a pid that provably names no process: pid_max itself, which @@ -540,3 +542,306 @@ func perturb(bootID string) string { } 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) + } +}