Skip to content

Verify the ExecSession pid before killing it. #25906

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 8, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 35 additions & 4 deletions libpod/container_exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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()
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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()
Expand Down
44 changes: 27 additions & 17 deletions libpod/oci_conmon_exec_common.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}
Expand All @@ -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
}
Expand All @@ -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.
Expand Down
140 changes: 140 additions & 0 deletions pkg/pidhandle/pidhandle.go
Original file line number Diff line number Diff line change
@@ -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
}
Loading