From c6e53eea1245b2141039c1b15ada2f5b78524429 Mon Sep 17 00:00:00 2001 From: Anand-240 Date: Tue, 4 Aug 2026 18:18:11 +0530 Subject: [PATCH] fix(unikontainers): verify pid identity before signaling or joining its netns isRunning(), Signal(), Kill(), and joinSandboxNetNs() all trusted a raw stored pid without checking whether it still identified the VMM process that was originally launched. Linux recycles pid numbers as soon as a process is reaped, so after the VMM exits and its pid gets reused by an unrelated process, urunc could end up sending SIGKILL to that unrelated process and joining its network namespace instead of the sandbox's, or could refuse to ever delete an already-dead container because isRunning() saw a live but unrelated process. Record the /proc//stat starttime for the VMM pid at Create() time, since the kernel guarantees this value changes whenever a pid number is reused, and validate it before treating the pid as belonging to this container in isRunning(), Signal(), Kill(), and joinSandboxNetNs(). A mismatch is now treated the same as the process no longer existing. Fixes #899 Signed-off-by: Anand-240 --- pkg/unikontainers/unikontainers.go | 63 ++++++++++++++++++++++++- pkg/unikontainers/utils.go | 28 +++++++++++ pkg/unikontainers/utils_test.go | 76 ++++++++++++++++++++++++++++++ 3 files changed, 166 insertions(+), 1 deletion(-) diff --git a/pkg/unikontainers/unikontainers.go b/pkg/unikontainers/unikontainers.go index b218c4cea..bf3474016 100644 --- a/pkg/unikontainers/unikontainers.go +++ b/pkg/unikontainers/unikontainers.go @@ -52,6 +52,11 @@ var ErrQueueProxy = errors.New("this a queue proxy container") var ErrNotUnikernel = errors.New("this is not a unikernel container") var ErrNotExistingNS = errors.New("the namespace does not exist") +// annotPidStarttime holds the /proc//stat starttime recorded for +// State.Pid at Create() time, used to detect pid reuse after the VMM +// process has exited. See pidIsCurrentVMM. +const annotPidStarttime = "com.urunc.internal.pid.starttime" + // Unikontainer holds the data necessary to create, manage and delete unikernel containers type Unikontainer struct { State *specs.State @@ -245,6 +250,11 @@ func (u *Unikontainer) Create(pid int, pidFilePath string) error { return err } u.State.Pid = pid + starttime, err := getProcStarttime(pid) + if err != nil { + return fmt.Errorf("failed to record start time for pid %d: %w", pid, err) + } + u.State.Annotations[annotPidStarttime] = starttime u.State.Status = specs.StateCreated return u.saveContainerState() } @@ -792,6 +802,15 @@ func setupUser(user specs.User) error { // Signal sends a specified signal to container's init. func (u *Unikontainer) Signal(signal unix.Signal) error { + if !u.pidIsCurrentVMM() { + // The recorded pid no longer identifies the VMM process we + // launched, either it has already exited, or the pid number has + // been reused by an unrelated process. Report it the same way a + // signal to a dead process would be reported, instead of + // signalling whatever now holds that pid. + return unix.ESRCH + } + vmmType := u.State.Annotations[annotHypervisor] vmm, err := hypervisors.NewVMM(hypervisors.VmmType(vmmType), u.UruncCfg.Monitors) if err != nil { @@ -804,6 +823,16 @@ func (u *Unikontainer) Signal(signal unix.Signal) error { // Kill stops the VMM process, first by asking the VMM struct to stop // and consequently by killing the process described in u.State.Pid func (u *Unikontainer) Kill() error { + if !u.pidIsCurrentVMM() { + // The VMM process is already gone, or its pid has been reused by + // an unrelated process. There is nothing left to signal or to + // join the namespace of; only attempt a best-effort tap cleanup. + if err := network.CleanupAllUruncTaps(); err != nil { + uniklog.Errorf("failed to cleanup tap devices: %v", err) + } + return nil + } + // Try to join the Network namespace of the monitor before killing it. // If we kill it there might be no process inside the namespace and hence // the namespace gets destroyed. @@ -927,6 +956,14 @@ func (u Unikontainer) joinSandboxNetNs() error { // that we had to create a new one and therefore we can join it by // using the pid of the monitor process. if netNsPath == "" { + if !u.pidIsCurrentVMM() { + // The recorded pid no longer identifies the VMM process we + // launched, so /proc//ns/net would point to the + // namespace of an unrelated process, if the pid was reused, + // or would simply fail to exist. Treat it as if there was + // no sandbox namespace left to join. + return ErrNotExistingNS + } netNsPath = fmt.Sprintf("/proc/%d/ns/net", u.State.Pid) err := checkValidNsPath(netNsPath) if err != nil { @@ -1403,11 +1440,35 @@ func (u *Unikontainer) SendMessage(message IPCMessage) error { return nil } +// pidIsCurrentVMM returns true if u.State.Pid is alive and still identifies +// the same process that was recorded during Create(). Linux recycles pid +// numbers as soon as a process is reaped, so a plain liveness check (e.g. +// kill(pid, 0)) can report true for an unrelated process that happens to +// reuse the VMM's old pid. To detect that, we compare the process's +// /proc//stat starttime against the value recorded at Create() time; +// the kernel guarantees this value changes across pid reuse. +func (u *Unikontainer) pidIsCurrentVMM() bool { + if u.State.Pid <= 0 { + return false + } + recorded := u.State.Annotations[annotPidStarttime] + if recorded == "" { + // No starttime was recorded for this container (e.g. state + // predates this check); fall back to a plain liveness check. + return syscall.Kill(u.State.Pid, syscall.Signal(0)) == nil + } + current, err := getProcStarttime(u.State.Pid) + if err != nil { + return false + } + return current == recorded +} + // isRunning returns true if the PID is alive or hedge.ListVMs returns our containerID func (u *Unikontainer) isRunning() bool { vmmType := hypervisors.VmmType(u.State.Annotations[annotHypervisor]) if vmmType != hypervisors.HedgeVmm { - return syscall.Kill(u.State.Pid, syscall.Signal(0)) == nil + return u.pidIsCurrentVMM() } hedge := hypervisors.Hedge{} state := hedge.VMState(u.State.ID) diff --git a/pkg/unikontainers/utils.go b/pkg/unikontainers/utils.go index 83c9d72af..12560b40b 100644 --- a/pkg/unikontainers/utils.go +++ b/pkg/unikontainers/utils.go @@ -206,6 +206,34 @@ func remove(s []string, i int) []string { return append(s[:i], s[i+1:]...) } +// getProcStarttime returns the starttime field (field 22) of +// /proc//stat, as a string. The kernel guarantees this value changes +// whenever a pid number gets reused by a different process, so comparing +// two starttime readings for the same pid is a reliable way to detect pid +// reuse. +func getProcStarttime(pid int) (string, error) { + data, err := os.ReadFile(fmt.Sprintf("/proc/%d/stat", pid)) + if err != nil { + return "", err + } + // The comm field is surrounded by parentheses and may itself contain + // spaces or parentheses, so locate the fields that follow it by + // searching for the last ')' in the line, as recommended by proc(5). + line := string(data) + closeParen := strings.LastIndexByte(line, ')') + if closeParen == -1 || closeParen+2 > len(line) { + return "", fmt.Errorf("unexpected format in /proc/%d/stat", pid) + } + fields := strings.Fields(line[closeParen+2:]) + // After the comm field, "state" is field 3, so starttime (field 22) + // is at index 22-3=19 in the remaining fields. + const starttimeFieldIndex = 19 + if len(fields) <= starttimeFieldIndex { + return "", fmt.Errorf("unexpected number of fields in /proc/%d/stat", pid) + } + return fields[starttimeFieldIndex], nil +} + func checkValidNsPath(path string) error { // only set to join this namespace if it exists if _, err := os.Lstat(path); err != nil { diff --git a/pkg/unikontainers/utils_test.go b/pkg/unikontainers/utils_test.go index 2d23ea335..b548abe90 100644 --- a/pkg/unikontainers/utils_test.go +++ b/pkg/unikontainers/utils_test.go @@ -340,3 +340,79 @@ func TestLoadSpec(t *testing.T) { assert.Contains(t, err.Error(), "failed to parse specification json", "Expected specific error message") }) } + +func TestGetProcStarttime(t *testing.T) { + t.Parallel() + + t.Run("returns the same value for a live process across calls", func(t *testing.T) { + t.Parallel() + pid := os.Getpid() + first, err := getProcStarttime(pid) + assert.NoError(t, err) + assert.NotEmpty(t, first) + + second, err := getProcStarttime(pid) + assert.NoError(t, err) + assert.Equal(t, first, second) + }) + + t.Run("errors for a pid that does not exist", func(t *testing.T) { + t.Parallel() + // PID 1 always exists on a normal Linux system, so pick a very + // large pid that is extremely unlikely to be in use. + _, err := getProcStarttime(999999999) + assert.Error(t, err) + }) +} + +func TestPidIsCurrentVMM(t *testing.T) { + t.Parallel() + + t.Run("matches when the recorded starttime is still current", func(t *testing.T) { + t.Parallel() + pid := os.Getpid() + starttime, err := getProcStarttime(pid) + assert.NoError(t, err) + + u := &Unikontainer{ + State: &specs.State{ + Pid: pid, + Annotations: map[string]string{annotPidStarttime: starttime}, + }, + } + assert.True(t, u.pidIsCurrentVMM()) + }) + + t.Run("does not match a stale recorded starttime", func(t *testing.T) { + t.Parallel() + u := &Unikontainer{ + State: &specs.State{ + Pid: os.Getpid(), + Annotations: map[string]string{annotPidStarttime: "not-the-real-starttime"}, + }, + } + assert.False(t, u.pidIsCurrentVMM()) + }) + + t.Run("rejects non-positive pids", func(t *testing.T) { + t.Parallel() + u := &Unikontainer{ + State: &specs.State{ + Pid: -1, + Annotations: map[string]string{}, + }, + } + assert.False(t, u.pidIsCurrentVMM()) + }) + + t.Run("falls back to a liveness check when no starttime was recorded", func(t *testing.T) { + t.Parallel() + u := &Unikontainer{ + State: &specs.State{ + Pid: os.Getpid(), + Annotations: map[string]string{}, + }, + } + assert.True(t, u.pidIsCurrentVMM()) + }) +}