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()) + }) +}