From 64171043ac3298f874ddec26d1be339251790092 Mon Sep 17 00:00:00 2001 From: Giuseppe Scrivano Date: Wed, 18 Oct 2023 10:46:15 +0200 Subject: [PATCH 1/2] test/system: ignore 127 if it is the expected rc Signed-off-by: Giuseppe Scrivano --- test/system/helpers.bash | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/test/system/helpers.bash b/test/system/helpers.bash index 3fcd69a60a..d2d1414139 100644 --- a/test/system/helpers.bash +++ b/test/system/helpers.bash @@ -339,11 +339,18 @@ function run_podman() { # Remember command args, for possible use in later diagnostic messages MOST_RECENT_PODMAN_COMMAND="podman $*" + # BATS treats 127 as a special case, so we need to silence it when 127 is the + # expected rc: https://bats-core.readthedocs.io/en/stable/warnings/BW01.html + silence127="" + if [ $expected_rc -eq 127 ]; then + silence127="-127" + fi + # stdout is only emitted upon error; this printf is to help in debugging printf "\n%s %s %s %s\n" "$(timestamp)" "$_LOG_PROMPT" "$PODMAN" "$*" # BATS hangs if a subprocess remains and keeps FD 3 open; this happens # if podman crashes unexpectedly without cleaning up subprocesses. - run timeout --foreground -v --kill=10 $PODMAN_TIMEOUT $PODMAN $_PODMAN_TEST_OPTS "$@" 3>/dev/null + run $silence127 timeout --foreground -v --kill=10 $PODMAN_TIMEOUT $PODMAN $_PODMAN_TEST_OPTS "$@" 3>/dev/null # without "quotes", multiple lines are glommed together into one if [ -n "$output" ]; then echo "$(timestamp) $output" From fa19e1baa27024f8e0078e27254a8cfb6586f9f4 Mon Sep 17 00:00:00 2001 From: Giuseppe Scrivano Date: Wed, 18 Oct 2023 10:09:56 +0200 Subject: [PATCH 2/2] exec: do not leak session IDs on errors always cleanup the exec session when the command specified to the "exec" is not found. Closes: https://github.com/containers/podman/issues/20392 Signed-off-by: Giuseppe Scrivano --- libpod/container_exec.go | 20 ++++++++++---------- pkg/domain/infra/abi/containers.go | 1 + test/system/075-exec.bats | 13 +++++++++++++ 3 files changed, 24 insertions(+), 10 deletions(-) diff --git a/libpod/container_exec.go b/libpod/container_exec.go index 9dea0216f5..c7b1a93338 100644 --- a/libpod/container_exec.go +++ b/libpod/container_exec.go @@ -759,11 +759,19 @@ func (c *Container) Exec(config *ExecConfig, streams *define.AttachStreams, resi // Exec emulates the old Libpod exec API, providing a single call to create, // run, and remove an exec session. Returns exit code and error. Exit code is // not guaranteed to be set sanely if error is not nil. -func (c *Container) exec(config *ExecConfig, streams *define.AttachStreams, resizeChan <-chan resize.TerminalSize, isHealthcheck bool) (int, error) { +func (c *Container) exec(config *ExecConfig, streams *define.AttachStreams, resizeChan <-chan resize.TerminalSize, isHealthcheck bool) (exitCode int, retErr error) { sessionID, err := c.ExecCreate(config) if err != nil { return -1, err } + defer func() { + if err := c.ExecRemove(sessionID, false); err != nil { + if retErr == nil && !errors.Is(err, define.ErrNoSuchExecSession) { + exitCode = -1 + retErr = err + } + } + }() // Start resizing if we have a resize channel. // This goroutine may likely leak, given that we cannot close it here. @@ -813,15 +821,7 @@ func (c *Container) exec(config *ExecConfig, streams *define.AttachStreams, resi } return -1, err } - exitCode := session.ExitCode - if err := c.ExecRemove(sessionID, false); err != nil { - if errors.Is(err, define.ErrNoSuchExecSession) { - return exitCode, nil - } - return -1, err - } - - return exitCode, nil + return session.ExitCode, nil } // cleanupExecBundle cleanups an exec session after its done diff --git a/pkg/domain/infra/abi/containers.go b/pkg/domain/infra/abi/containers.go index e92825432c..2d14981671 100644 --- a/pkg/domain/infra/abi/containers.go +++ b/pkg/domain/infra/abi/containers.go @@ -904,6 +904,7 @@ func (ic *ContainerEngine) ContainerExecDetached(ctx context.Context, nameOrID s // TODO: we should try and retrieve exit code if this fails. if err := ctr.ExecStart(id); err != nil { + _ = ctr.ExecRemove(id, true) return "", err } return id, nil diff --git a/test/system/075-exec.bats b/test/system/075-exec.bats index 7e573924ac..c53759fb5c 100644 --- a/test/system/075-exec.bats +++ b/test/system/075-exec.bats @@ -166,4 +166,17 @@ load helpers run_podman rm -f -t0 $cid } +@test "podman exec - does not leak session IDs on invalid command" { + run_podman run -d $IMAGE top + cid="$output" + + for i in {1..3}; do + run_podman 127 exec $cid blahblah + run_podman 125 exec -d $cid blahblah + done + + run_podman inspect --format "{{len .ExecIDs}}" $cid + assert "$output" = "0" ".ExecIDs must be empty" +} + # vim: filetype=sh