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 Container.generatePIDData(pid) which generates the PIDData
  string using the unix.NameToHandleAt if available. If not available,
  it fallbacks to start-time read from /proc/pid/stat file.
- Adds Container.isSessionAlive(pid, pidData) to use this additional
  PIDData context to verify whether the pid is created by podman or not.

Fixes: #25104

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

Author: Jan Kaluza

libpod/container_exec.go

@@ -101,6 +101,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 generatePIDData and used by isSessionAlive.
+	PIDData string `json:"pidData,omitempty"`
 }
 
 // ID returns the ID of an exec session.
@@ -295,6 +299,10 @@ func (c *Container) ExecStart(sessionID string) error {
 
 	// Update and save session to reflect PID/running
 	session.PID = pid
+	session.PIDData, err = c.generatePIDData(pid)
+	if err != nil {
+		logrus.Debugf("generatePidData(%d) failed: %v", pid, err)
+	}
 	session.State = define.ExecStateRunning
 
 	return c.save()
@@ -353,6 +361,10 @@ func (c *Container) execStartAndAttach(sessionID string, streams *define.AttachS
 
 	// Update and save session to reflect PID/running
 	session.PID = pid
+	session.PIDData, err = c.generatePIDData(pid)
+	if err != nil {
+		logrus.Debugf("generatePidData(%d) failed: %v", pid, err)
+	}
 	session.State = define.ExecStateRunning
 
 	if err := c.save(); err != nil {
@@ -534,6 +546,10 @@ func (c *Container) ExecHTTPStartAndAttach(sessionID string, r *http.Request, w
 	var lastErr error
 
 	session.PID = pid
+	session.PIDData, err = c.generatePIDData(pid)
+	if err != nil {
+		logrus.Debugf("generatePidData(%d) failed: %v", pid, err)
+	}
 	session.State = define.ExecStateRunning
 
 	if err := c.save(); err != nil {
@@ -1057,17 +1073,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 +1144,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 +1279,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/container_internal_freebsd.go

@@ -435,3 +435,61 @@ func containerPathIsFile(unsafeRoot string, containerPath string) (bool, error)
 	}
 	return false, err
 }
+
+// Generates the PID data uniquely identifying the process
+// defined by the pid.
+func (c *Container) generatePIDData(pid int) (string, error) {
+	// fallback to process start-time in case PidfdOpen or NameToHandleAt
+	// is not supported.
+	data, err := os.ReadFile(fmt.Sprintf("/proc/%d/stat", 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
+}
+
+// Verifies that the PID is still alive and matches the exactly
+// same process as described by the pidData.
+func (c *Container) isSessionAlive(pid int, pidData string) (bool, error) {
+	prefix := "start-time:"
+	if strings.HasPrefix(pidData, prefix) {
+		startTime := strings.TrimPrefix(pidData, prefix)
+
+		// Read the stat file.
+		data, err := os.ReadFile(fmt.Sprintf("/proc/%d/stat", pid))
+		if err != nil {
+			return false, nil
+		}
+
+		// Split stat line into fields.
+		fields := strings.Fields(string(data))
+		if len(fields) < 22 {
+			return false, fmt.Errorf("not enough fields in stat file")
+		}
+
+		// Field 22 is start-time.
+		if fields[21] == startTime {
+			return true, nil
+		}
+		return false, nil
+	}
+
+	// Fallback to unix.Kill to verify if the PID is still alive.
+	// 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 PID %d with signal 0: %w", pid, err)
+	}
+
+	return true, nil
+}

libpod/container_internal_linux.go

@@ -3,12 +3,14 @@
 package libpod
 
 import (
+	"encoding/hex"
 	"errors"
 	"fmt"
 	"io/fs"
 	"os"
 	"path"
 	"path/filepath"
+	"strconv"
 	"strings"
 	"sync"
 	"syscall"
@@ -846,3 +848,132 @@ func containerPathIsFile(unsafeRoot string, containerPath string) (bool, error)
 	}
 	return false, err
 }
+
+// Generates the PID data uniquely identifying the process
+// defined by the pid.
+func (c *Container) generatePIDData(pid int) (string, error) {
+	// Try to use the PidFdOpen followed by theNameToHandleAt and
+	// encode it into string.
+	fd, err := unix.PidfdOpen(pid, 0)
+	if err != nil {
+		// Do not fail if PidFdOpen is not supported, we will
+		// fallback to process start-time later.
+		if err != unix.ENOSYS {
+			logrus.Debugf("PidfdOpen(%d) failed: %v", pid, err)
+		} else {
+			return "", err
+		}
+	}
+
+	if fd > -1 {
+		defer unix.Close(fd)
+		fh, _, err := unix.NameToHandleAt(fd, "", unix.AT_EMPTY_PATH)
+		if err != nil {
+			// Do not fail if NameToHandleAt is not supported, we will
+			// fallback to process start-time later.
+			if err == unix.ENOTSUP {
+				logrus.Debugf("NameToHandleAt(%d) failed: %v", fd, err)
+			} else {
+				return "", err
+			}
+		} else {
+			hexStr := hex.EncodeToString(fh.Bytes())
+			return "name-to-handle:" + strconv.Itoa(int(fh.Type())) + " " + hexStr, nil
+		}
+	}
+
+	// fallback to process start-time in case PidfdOpen or NameToHandleAt
+	// is not supported.
+	data, err := os.ReadFile(fmt.Sprintf("/proc/%d/stat", 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
+}
+
+// Verifies that the PID is still alive and matches the exactly
+// same process as described by the pidData.
+func (c *Container) isSessionAlive(pid int, pidData string) (bool, error) {
+	prefix := "name-to-handle:"
+	if strings.HasPrefix(pidData, prefix) {
+		// Strip the prefix
+		data := strings.TrimPrefix(pidData, prefix)
+		parts := strings.SplitN(data, " ", 2)
+		if len(parts) != 2 {
+			return false, fmt.Errorf("invalid format, expected 2 parts")
+		}
+
+		// Parse fhType
+		fhTypeInt, err := strconv.Atoi(parts[0])
+		if err != nil {
+			return false, err
+		}
+		fhType := int32(fhTypeInt)
+
+		// Decode hex string to bytes
+		bytes, err := hex.DecodeString(parts[1])
+		if err != nil {
+			return false, err
+		}
+
+		// Create FileHandle and open it.
+		fh := unix.NewFileHandle(fhType, bytes)
+		pidfd, err := unix.PidfdOpen(os.Getpid(), 0)
+		if err != nil {
+			return false, err
+		}
+		defer unix.Close(pidfd)
+		fd, err := unix.OpenByHandleAt(pidfd, fh, 0)
+		if err != nil {
+			// ESTALE means the process exited already.
+			if err == unix.ESTALE {
+				return false, nil
+			}
+			return false, err
+		}
+		defer unix.Close(fd)
+		return true, nil
+	}
+
+	prefix = "start-time:"
+	if strings.HasPrefix(pidData, prefix) {
+		startTime := strings.TrimPrefix(pidData, prefix)
+
+		// Read the stat file.
+		data, err := os.ReadFile(fmt.Sprintf("/proc/%d/stat", pid))
+		if err != nil {
+			return false, nil
+		}
+
+		// Split stat line into fields.
+		fields := strings.Fields(string(data))
+		if len(fields) < 22 {
+			return false, fmt.Errorf("not enough fields in stat file")
+		}
+
+		// Field 22 is start-time.
+		if fields[21] == startTime {
+			return true, nil
+		}
+		return false, nil
+	}
+
+	// Fallback to unix.Kill to verify if the PID is still alive.
+	// 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 PID %d with signal 0: %w", pid, err)
+	}
+
+	return true, nil
+}

libpod/oci_conmon_exec_common.go

@@ -214,20 +214,20 @@ 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)
 
 	// 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 := ctr.isSessionAlive(pid, pidData)
+	if err != nil {
+		return err
+	}
+	if !sessionAlive {
+		return nil
 	}
 
 	if timeout > 0 {
@@ -268,23 +268,20 @@ 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)
 
 	// 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 := ctr.isSessionAlive(pid, pidData)
+	if err != nil {
+		return false, err
 	}
 
-	return true, nil
+	return sessionAlive, nil
 }
 
 // ExecAttachSocketPath is the path to a container's exec session attach socket.