Merge pull request #20394 from giuseppe/cleanup-exec-session-on-errors

exec: do not leak session IDs on errors

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
+	return session.ExitCode, nil
-	if err := c.ExecRemove(sessionID, false); err != nil {
-		if errors.Is(err, define.ErrNoSuchExecSession) {
-			return exitCode, nil
-		}
-		return -1, err
-	}
-
-	return exitCode, nil
 }
 
 // cleanupExecBundle cleanups an exec session after its done

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

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

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"