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 fallback to start-time read from /proc/pid/stat file otherwise.

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.
@@ -297,6 +302,16 @@ func (c *Container) ExecStart(sessionID string) error {
 	session.PID = pid
 	session.State = define.ExecStateRunning
 
+	pidHandle, err := pidhandle.NewPIDHandle(pid)
+	if err != nil {
+		return err
+	}
+	defer pidHandle.Close()
+	session.PIDData, err = pidHandle.String()
+	if err != nil {
+		logrus.Debugf("generating PIDData (%d) failed: %v", pid, err)
+	}
+
 	return c.save()
 }
 
@@ -354,6 +369,15 @@ func (c *Container) execStartAndAttach(sessionID string, streams *define.AttachS
 	// Update and save session to reflect PID/running
 	session.PID = pid
 	session.State = define.ExecStateRunning
+	pidHandle, err := pidhandle.NewPIDHandle(pid)
+	if err != nil {
+		return err
+	}
+	defer pidHandle.Close()
+	session.PIDData, err = pidHandle.String()
+	if err != nil {
+		logrus.Debugf("generating PIDData (%d) failed: %v", pid, err)
+	}
 
 	if err := c.save(); err != nil {
 		lastErr = err
@@ -535,6 +559,15 @@ func (c *Container) ExecHTTPStartAndAttach(sessionID string, r *http.Request, w
 
 	session.PID = pid
 	session.State = define.ExecStateRunning
+	pidHandle, err := pidhandle.NewPIDHandle(pid)
+	if err != nil {
+		return err
+	}
+	defer pidHandle.Close()
+	session.PIDData, err = pidHandle.String()
+	if err != nil {
+		logrus.Debugf("generating PIDData (%d) failed: %v", pid, err)
+	}
 
 	if err := c.save(); err != nil {
 		lastErr = err
@@ -1057,17 +1090,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 +1161,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 +1296,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 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 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, 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, 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,105 @@
+//go:build !linux && !windows
+
+// Package for handling processes and PIDs.
+package pidhandle
+
+import (
+	"fmt"
+	"os"
+	"strings"
+
+	"golang.org/x/sys/unix"
+)
+
+type PIDHandle struct {
+	pid     int
+	pidfd   int
+	pidData string
+}
+
+func NewPIDHandle(pid int) (PIDHandle, error) {
+	h := PIDHandle{pid: pid, pidfd: -1, pidData: ""}
+
+	isAlive, err := h.IsAlive()
+	if err != nil {
+		return h, err
+	}
+	if !isAlive {
+		h.pidData = "no-such-process"
+	}
+	return h, err
+}
+
+func NewPIDHandleFromString(pid int, pidData string) (PIDHandle, error) {
+	h := PIDHandle{pid: pid, pidfd: -1, pidData: pidData}
+	return h, nil
+}
+
+// Close releases the pidfd resource.
+func (h *PIDHandle) Close() error {
+	if h.pidfd != 0 {
+		err := unix.Close(h.pidfd)
+		if err != nil {
+			return fmt.Errorf("failed to close pidfd: %w", err)
+		}
+		h.pidfd = 0
+	}
+	return nil
+}
+
+func (h *PIDHandle) Kill(signal unix.Signal) error {
+	startTime, found := strings.CutPrefix(h.pidData, "start-time:")
+	if found {
+		// Read the stat file.
+		data, err := os.ReadFile(fmt.Sprintf("/proc/%d/stat", h.pid))
+		if err != nil {
+			return unix.ESRCH
+		}
+
+		// Split stat line into fields.
+		fields := strings.Fields(string(data))
+		if len(fields) < 22 {
+			return fmt.Errorf("not enough fields in stat file")
+		}
+
+		// Field 22 is start-time.
+		if fields[21] != startTime {
+			return unix.ESRCH
+		}
+	}
+	if h.pidData == "no-such-process" {
+		return unix.ESRCH
+	}
+	return unix.Kill(h.pid, signal)
+}
+
+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
+}
+
+// Generates the PID data uniquely identifying the process
+// defined by the pid.
+func (h *PIDHandle) String() (string, error) {
+	// fallback to process start-time in case PidfdOpen or NameToHandleAt
+	// is not supported.
+	data, err := os.ReadFile(fmt.Sprintf("/proc/%d/stat", h.pid))
+	if err != nil {
+		return "", fmt.Errorf("could not read stat: %w", err)
+	}
+
+	// Split stat line into fields
+	fields := strings.Fields(string(data))
+	if len(fields) < 22 {
+		return "", fmt.Errorf("not enough fields in stat file")
+	}
+
+	// Field 22 is the start-time.
+	return "start-time:" + fields[21], nil
+}