Verify the ExecSession pid before killing it.

When container is being removed, podman iterates
through its exec sessions and checks whether exec
session pid is still alive.

The problem is that the pid can be reused for other processes,
so that it may not belong to exec session.
In this scenario podman may kill another process

This commit prevents it by doing following changes:

- Adds the PIDData string to ExecSession struct. This string
  is used to store additional context for a PID to later verify
  that the PID killed by the podman is really the one started by
  it.
- Adds new package called pidhandle which implements the methods
  generating the PIDData, and killing the PID with the PIDData
  ensuring the right PID is killed by verifying the metadata.

The new code uses pidfd_open and name_to_handle_at when available.
It fallbacks to process start-time get using the gopsutil package.

Fixes: #25104

Signed-off-by: Jan Kaluza <[email protected]>

Author: Jan Kaluza

libpod/container_exec.go

@@ -16,6 +16,7 @@ import (
 	"github.com/containers/common/pkg/util"
 	"github.com/containers/podman/v5/libpod/define"
 	"github.com/containers/podman/v5/libpod/events"
+	"github.com/containers/podman/v5/pkg/pidhandle"
 	"github.com/containers/storage/pkg/stringid"
 	"github.com/sirupsen/logrus"
 	"golang.org/x/sys/unix"
@@ -101,6 +102,10 @@ type ExecSession struct {
 	// Config is the configuration of this exec session.
 	// Cannot be empty.
 	Config *ExecConfig `json:"config"`
+
+	// PIDData is a string uniquely identifying the PID. The value is platform
+	// specific. It is generated by pidhandle package and used by isSessionAlive.
+	PIDData string `json:"pidData,omitempty"`
 }
 
 // ID returns the ID of an exec session.
@@ -251,6 +256,27 @@ func (c *Container) ExecCreate(config *ExecConfig) (string, error) {
 	return session.Id, nil
 }
 
+// Returns a serialized representation of the pid using the PIDHandle package.
+// This string can be passed to NewPIDHandleFromString to recreate a PIDHandle
+// that reliably refers to the same process as the original.
+//
+// NOTE that there is a still race in generating the PIDData on start but this
+// is as good as we can do currently. Long term we could change conmon to
+// send pidfd back directly.
+func getPidData(pid int) string {
+	pidHandle, err := pidhandle.NewPIDHandle(pid)
+	if err != nil {
+		logrus.Debugf("getting the PID handle for pid %d: %v", pid, err)
+		return ""
+	}
+	defer pidHandle.Close()
+	pidData, err := pidHandle.String()
+	if err != nil {
+		logrus.Debugf("generating PIDData (%d) failed: %v", pid, err)
+	}
+	return pidData
+}
+
 // ExecStart starts an exec session in the container, but does not attach to it.
 // Returns immediately upon starting the exec session, unlike other ExecStart
 // functions, which will only return when the exec session exits.
@@ -296,6 +322,7 @@ func (c *Container) ExecStart(sessionID string) error {
 	// Update and save session to reflect PID/running
 	session.PID = pid
 	session.State = define.ExecStateRunning
+	session.PIDData = getPidData(pid)
 
 	return c.save()
 }
@@ -354,6 +381,7 @@ func (c *Container) execStartAndAttach(sessionID string, streams *define.AttachS
 	// Update and save session to reflect PID/running
 	session.PID = pid
 	session.State = define.ExecStateRunning
+	session.PIDData = getPidData(pid)
 
 	if err := c.save(); err != nil {
 		lastErr = err
@@ -535,6 +563,7 @@ func (c *Container) ExecHTTPStartAndAttach(sessionID string, r *http.Request, w
 
 	session.PID = pid
 	session.State = define.ExecStateRunning
+	session.PIDData = getPidData(pid)
 
 	if err := c.save(); err != nil {
 		lastErr = err
@@ -1057,17 +1086,17 @@ func (c *Container) readExecExitCode(sessionID string) (int, error) {
 }
 
 // getExecSessionPID gets the PID of an active exec session
-func (c *Container) getExecSessionPID(sessionID string) (int, error) {
+func (c *Container) getExecSessionPID(sessionID string) (int, string, error) {
 	session, ok := c.state.ExecSessions[sessionID]
 	if ok {
-		return session.PID, nil
+		return session.PID, session.PIDData, nil
 	}
 	oldSession, ok := c.state.LegacyExecSessions[sessionID]
 	if ok {
-		return oldSession.PID, nil
+		return oldSession.PID, "", nil
 	}
 
-	return -1, fmt.Errorf("no exec session with ID %s found in container %s: %w", sessionID, c.ID(), define.ErrNoSuchExecSession)
+	return -1, "", fmt.Errorf("no exec session with ID %s found in container %s: %w", sessionID, c.ID(), define.ErrNoSuchExecSession)
 }
 
 // getKnownExecSessions gets a list of all exec sessions we think are running,
@@ -1128,6 +1157,7 @@ func (c *Container) getActiveExecSessions() ([]string, error) {
 				}
 				session.ExitCode = exitCode
 				session.PID = 0
+				session.PIDData = ""
 				session.State = define.ExecStateStopped
 
 				c.newExecDiedEvent(session.ID(), exitCode)
@@ -1262,6 +1292,7 @@ func justWriteExecExitCode(c *Container, sessionID string, exitCode int, emitEve
 	session.State = define.ExecStateStopped
 	session.ExitCode = exitCode
 	session.PID = 0
+	session.PIDData = ""
 
 	// Finally, save our changes.
 	return c.save()

libpod/oci_conmon_exec_common.go

@@ -21,6 +21,7 @@ import (
 	"github.com/containers/podman/v5/libpod/define"
 	"github.com/containers/podman/v5/pkg/errorhandling"
 	"github.com/containers/podman/v5/pkg/lookup"
+	"github.com/containers/podman/v5/pkg/pidhandle"
 	spec "github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/sirupsen/logrus"
 	"golang.org/x/sys/unix"
@@ -214,26 +215,32 @@ func (r *ConmonOCIRuntime) ExecAttachResize(ctr *Container, sessionID string, ne
 
 // ExecStopContainer stops a given exec session in a running container.
 func (r *ConmonOCIRuntime) ExecStopContainer(ctr *Container, sessionID string, timeout uint) error {
-	pid, err := ctr.getExecSessionPID(sessionID)
+	pid, pidData, err := ctr.getExecSessionPID(sessionID)
 	if err != nil {
 		return err
 	}
 
 	logrus.Debugf("Going to stop container %s exec session %s", ctr.ID(), sessionID)
 
+	pidHandle, err := pidhandle.NewPIDHandleFromString(pid, pidData)
+	if err != nil {
+		return fmt.Errorf("getting the PID handle for pid %d from '%s': %w", pid, pidData, err)
+	}
+	defer pidHandle.Close()
+
 	// Is the session dead?
-	// Ping the PID with signal 0 to see if it still exists.
-	if err := unix.Kill(pid, 0); err != nil {
-		if err == unix.ESRCH {
-			return nil
-		}
-		return fmt.Errorf("pinging container %s exec session %s PID %d with signal 0: %w", ctr.ID(), sessionID, pid, err)
+	sessionAlive, err := pidHandle.IsAlive()
+	if err != nil {
+		return fmt.Errorf("getting the process status for pid %d: %w", pid, err)
+	}
+	if !sessionAlive {
+		return nil
 	}
 
 	if timeout > 0 {
 		// Use SIGTERM by default, then SIGSTOP after timeout.
 		logrus.Debugf("Killing exec session %s (PID %d) of container %s with SIGTERM", sessionID, pid, ctr.ID())
-		if err := unix.Kill(pid, unix.SIGTERM); err != nil {
+		if err := pidHandle.Kill(unix.SIGTERM); err != nil {
 			if err == unix.ESRCH {
 				return nil
 			}
@@ -251,7 +258,7 @@ func (r *ConmonOCIRuntime) ExecStopContainer(ctr *Container, sessionID string, t
 
 	// SIGTERM did not work. On to SIGKILL.
 	logrus.Debugf("Killing exec session %s (PID %d) of container %s with SIGKILL", sessionID, pid, ctr.ID())
-	if err := unix.Kill(pid, unix.SIGTERM); err != nil {
+	if err := pidHandle.Kill(unix.SIGTERM); err != nil {
 		if err == unix.ESRCH {
 			return nil
 		}
@@ -268,23 +275,26 @@ func (r *ConmonOCIRuntime) ExecStopContainer(ctr *Container, sessionID string, t
 
 // ExecUpdateStatus checks if the given exec session is still running.
 func (r *ConmonOCIRuntime) ExecUpdateStatus(ctr *Container, sessionID string) (bool, error) {
-	pid, err := ctr.getExecSessionPID(sessionID)
+	pid, pidData, err := ctr.getExecSessionPID(sessionID)
 	if err != nil {
 		return false, err
 	}
 
 	logrus.Debugf("Checking status of container %s exec session %s", ctr.ID(), sessionID)
 
+	pidHandle, err := pidhandle.NewPIDHandleFromString(pid, pidData)
+	if err != nil {
+		return false, fmt.Errorf("getting the PID handle for pid %d from '%s': %w", pid, pidData, err)
+	}
+	defer pidHandle.Close()
+
 	// Is the session dead?
-	// Ping the PID with signal 0 to see if it still exists.
-	if err := unix.Kill(pid, 0); err != nil {
-		if err == unix.ESRCH {
-			return false, nil
-		}
-		return false, fmt.Errorf("pinging container %s exec session %s PID %d with signal 0: %w", ctr.ID(), sessionID, pid, err)
+	sessionAlive, err := pidHandle.IsAlive()
+	if err != nil {
+		return false, fmt.Errorf("getting the process status for pid %d: %w", pid, err)
 	}
 
-	return true, nil
+	return sessionAlive, nil
 }
 
 // ExecAttachSocketPath is the path to a container's exec session attach socket.

pkg/pidhandle/pidhandle.go

@@ -0,0 +1,140 @@
+//go:build !windows
+
+// Package for handling processes and PIDs.
+package pidhandle
+
+import (
+	"strconv"
+	"strings"
+
+	"github.com/shirou/gopsutil/v4/process"
+	"github.com/sirupsen/logrus"
+	"golang.org/x/sys/unix"
+)
+
+// PIDHandle defines an interface for working with operating system processes
+// in a reliable way. OS-specific implementations include additional logic
+// to try to ensure that operations (e.g., sending signals) are performed
+// on the exact same process that was originally referenced when the PIDHandle
+// was created via NewPIDHandle or NewPIDHandleFromString.
+//
+// This prevents accidental interaction with a different process in scenarios
+// where the original process has exited and its PID has been reused by
+// the system for an unrelated process.
+type PIDHandle interface {
+	// Returns the PID associated with this PIDHandle.
+	PID() int
+	// Releases the PIDHandle resources.
+	Close() error
+	// Sends the signal to process.
+	Kill(signal unix.Signal) error
+	// Returns true in case the process is still alive.
+	IsAlive() (bool, error)
+	// Returns a serialized representation of the PIDHandle.
+	// This string can be passed to NewPIDHandleFromString to recreate
+	// a PIDHandle that reliably refers to the same process as the original.
+	String() (string, error)
+}
+
+// The pidData value used when no process with this PID exists when creating
+// the PIDHandle.
+const noSuchProcessID = "no-proc"
+
+// The pidData prefix used when only the process start time (creation time)
+// is supported when creating the PIDHandle to uniquely identify the process.
+const startTimePrefix = "start-time:"
+
+type pidHandle struct {
+	pid     int
+	pidData string
+}
+
+// Returns the PID.
+func (h *pidHandle) PID() int {
+	return h.pid
+}
+
+// Close releases the PIDHandle resource.
+func (h *pidHandle) Close() error {
+	// No resources for the default PIDHandle implementation.
+	return nil
+}
+
+// Sends the signal to process.
+func (h *pidHandle) Kill(signal unix.Signal) error {
+	if h.pidData == noSuchProcessID {
+		// The process did not exist when we created the PIDHandle, so return
+		// ESRCH error.
+		return unix.ESRCH
+	}
+
+	// Get the start-time of the process and check if it is the same as
+	// the one we store in pidData. If it is not, we know that the PID
+	// has been recycled and return ESRCH error.
+	startTime, found := strings.CutPrefix(h.pidData, startTimePrefix)
+	if found {
+		p, err := process.NewProcess(int32(h.pid))
+		if err != nil {
+			if err == process.ErrorProcessNotRunning {
+				return unix.ESRCH
+			}
+			return err
+		}
+
+		ctime, err := p.CreateTime()
+		if err != nil {
+			return err
+		}
+
+		if strconv.FormatInt(ctime, 10) != startTime {
+			return unix.ESRCH
+		}
+	}
+
+	return unix.Kill(h.pid, signal)
+}
+
+// Returns true in case the process is still alive.
+func (h *pidHandle) IsAlive() (bool, error) {
+	err := h.Kill(0)
+	if err != nil {
+		if err == unix.ESRCH {
+			return false, nil
+		}
+		return false, err
+	}
+	return true, nil
+}
+
+// Returns a serialized representation of the PIDHandle.
+// This string can be passed to NewPIDHandleFromString to recreate
+// a PIDHandle that reliably refers to the same process as the original.
+func (h *pidHandle) String() (string, error) {
+	if len(h.pidData) != 0 {
+		return h.pidData, nil
+	}
+
+	// Get the start-time of the process and return it as string.
+	p, err := process.NewProcess(int32(h.pid))
+	if err != nil {
+		if err == process.ErrorProcessNotRunning {
+			return noSuchProcessID, nil
+		}
+		return "", err
+	}
+
+	ctime, err := p.CreateTime()
+	if err != nil {
+		// The process existed, but we cannot get its start-time. There is
+		// either an issue with getting it, or the process terminated in the
+		// mean-time. We have no way to find out what actually happened, so
+		// in this case, we just fallback to an empty string. This will mean
+		// that Kill or IsAlive might kill wrong process in rare situation
+		// when CreateTime() failed for different reason than the process
+		// terminated...
+		logrus.Debugf("Getting CreateTime for process (%d) failed: %v", h.pid, err)
+		return "", nil
+	}
+
+	return startTimePrefix + strconv.FormatInt(ctime, 10), nil
+}