Merge pull request #25906 from jankaluza/25104-pidfs

Verify the ExecSession pid before killing it.

Author: openshift-merge-bot[bot]

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