podman exec: correctly support detaching

podman exec support detaching early via the detach key sequence. In that
case the podman process should exit successfully but the container exec
process keeps running.

Given that I could not find any existing test for the detach key
functionality not even for exec I added some. This seems to reveal more
issues with on podman-remote, podman-remote run detach was broken which
I fixed here as well but for podman-remote exec something bigger is
needed. While I thought I fixed most problems there there was a strange
race condition which caused the process to just hang.

Thus I skipped the remote exec test for now and filled containers#25089 to track
that.

Fixes containers#24895

Signed-off-by: Paul Holzinger <[email protected]>

Author: Luap99

libpod/container_exec.go

@@ -287,9 +287,14 @@ func (c *Container) ExecStart(sessionID string) error {
 // execStartAndAttach starts and attaches to an exec session in a container.
 // newSize resizes the tty to this size before the process is started, must be nil if the exec session has no tty
 func (c *Container) execStartAndAttach(sessionID string, streams *define.AttachStreams, newSize *resize.TerminalSize, isHealthcheck bool) error {
+	unlock := true
 	if !c.batched {
 		c.lock.Lock()
-		defer c.lock.Unlock()
+		defer func() {
+			if unlock {
+				c.lock.Unlock()
+			}
+		}()
 
 		if err := c.syncContainer(); err != nil {
 			return err
@@ -344,6 +349,12 @@ func (c *Container) execStartAndAttach(sessionID string, streams *define.AttachS
 	}
 
 	tmpErr := <-attachChan
+	// user detached
+	if errors.Is(tmpErr, define.ErrDetach) {
+		// ensure we the defer does not unlock as we are not locked here
+		unlock = false
+		return tmpErr
+	}
 	if lastErr != nil {
 		logrus.Errorf("Container %s exec session %s error: %v", c.ID(), session.ID(), lastErr)
 	}
@@ -431,9 +442,14 @@ func (c *Container) ExecHTTPStartAndAttach(sessionID string, r *http.Request, w
 		close(hijackDone)
 	}()
 
+	unlock := true
 	if !c.batched {
 		c.lock.Lock()
-		defer c.lock.Unlock()
+		defer func() {
+			if unlock {
+				c.lock.Unlock()
+			}
+		}()
 
 		if err := c.syncContainer(); err != nil {
 			return err
@@ -514,6 +530,12 @@ func (c *Container) ExecHTTPStartAndAttach(sessionID string, r *http.Request, w
 	}
 
 	tmpErr := <-attachChan
+	// user detached
+	if errors.Is(tmpErr, define.ErrDetach) {
+		// ensure we the defer does not unlock as we are not locked here
+		unlock = false
+		return tmpErr
+	}
 	if lastErr != nil {
 		logrus.Errorf("Container %s exec session %s error: %v", c.ID(), session.ID(), lastErr)
 	}
@@ -765,11 +787,14 @@ func (c *Container) exec(config *ExecConfig, streams *define.AttachStreams, resi
 	if err != nil {
 		return -1, err
 	}
+	cleanup := true
 	defer func() {
-		if err := c.ExecRemove(sessionID, false); err != nil {
-			if retErr == nil && !errors.Is(err, define.ErrNoSuchExecSession) {
-				exitCode = -1
-				retErr = err
+		if cleanup {
+			if err := c.ExecRemove(sessionID, false); err != nil {
+				if retErr == nil && !errors.Is(err, define.ErrNoSuchExecSession) {
+					exitCode = -1
+					retErr = err
+				}
 			}
 		}
 	}()
@@ -803,6 +828,11 @@ func (c *Container) exec(config *ExecConfig, streams *define.AttachStreams, resi
 	}
 
 	if err := c.execStartAndAttach(sessionID, streams, size, isHealthcheck); err != nil {
+		// user detached, there will be no exit just exit without reporting an error
+		if errors.Is(err, define.ErrDetach) {
+			cleanup = false
+			return 0, nil
+		}
 		return -1, err
 	}
 

libpod/util.go

@@ -149,7 +149,7 @@ func checkDependencyContainer(depCtr, ctr *Container) error {
 
 // hijackWriteError writes an error to a hijacked HTTP session.
 func hijackWriteError(toWrite error, cid string, terminal bool, httpBuf *bufio.ReadWriter) {
-	if toWrite != nil {
+	if toWrite != nil && !errors.Is(toWrite, define.ErrDetach) {
 		errString := []byte(fmt.Sprintf("Error: %v\n", toWrite))
 		if !terminal {
 			// We need a header.

pkg/api/handlers/compat/exec.go

@@ -200,7 +200,7 @@ func ExecStartHandler(w http.ResponseWriter, r *http.Request) {
 		t := r.Context().Value(api.IdleTrackerKey).(*idle.Tracker)
 		defer t.Close()
 
-		if err != nil {
+		if err != nil && !errors.Is(err, define.ErrDetach) {
 			// Cannot report error to client as a 500 as the Upgrade set status to 101
 			logErr(err)
 		}

pkg/domain/infra/tunnel/containers.go

@@ -666,6 +666,7 @@ func startAndAttach(ic *ContainerEngine, name string, detachKeys *string, sigPro
 	go func() {
 		err := containers.Attach(ic.ClientCtx, name, input, output, errput, attachReady, options)
 		attachErr <- err
+		close(attachErr)
 	}()
 	// Wait for the attach to actually happen before starting
 	// the container.
@@ -683,13 +684,28 @@ func startAndAttach(ic *ContainerEngine, name string, detachKeys *string, sigPro
 			return -1, err
 		}
 
-		// call wait immediately after start to avoid racing against container removal when it was created with --rm
-		exitCode, err := containers.Wait(cancelCtx, name, nil)
-		if err != nil {
-			return -1, err
-		}
-		code = int(exitCode)
+		errorChan := make(chan error)
+		go func() {
+			defer close(errorChan)
+			// call wait immediately after start to avoid racing against container removal when it was created with --rm
+			exitCode, err := containers.Wait(cancelCtx, name, nil)
+			if err != nil {
+				errorChan <- err
+				return
+			}
+			code = int(exitCode)
+		}()
 
+		select {
+		case err := <-errorChan:
+			if err != nil {
+				return -1, err
+			}
+		case err := <-attachErr:
+			if err != nil {
+				return -1, err
+			}
+		}
 	case err := <-attachErr:
 		return -1, err
 	}
@@ -904,6 +920,7 @@ func (ic *ContainerEngine) ContainerRun(ctx context.Context, opts entities.Conta
 	}
 
 	code, err := startAndAttach(ic, con.ID, &opts.DetachKeys, opts.SigProxy, opts.InputStream, opts.OutputStream, opts.ErrorStream)
+	logrus.Error(4)
 	if err != nil {
 		if err == define.ErrDetach {
 			return &report, nil

test/system/450-interactive.bats

@@ -39,7 +39,7 @@ function teardown() {
         kill $PODMAN_SOCAT_PID
         PODMAN_SOCAT_PID=
     fi
-    rm -f $PODMAN_TEST_PTY $PODMAN_DUMMY_PTY
+    rm -f $PODMAN_TEST_PTY $PODMAN_DUMMY
 
     basic_teardown
 }
@@ -124,4 +124,59 @@ function teardown() {
     is "$output" "$expected_tty" "passthrough-tty: tty matches"
 }
 
+@test "podman run detach keys" {
+    local cname1=c1-$(safename)
+    local cname2=c2-$(safename)
+
+    local msg=$(random_string)
+    # First "type" a command then send CTRL-p,CTRL-q sequence to the run command.
+    # We must send that sequence in two echo commands if I use a single echo it
+    # doesn't work, I don't know why.
+    # If the detach does not work podman run will hang and timeout.
+    # The sleep is needed to ensure the output can be printed before we detach.
+    (echo "echo $msg" > $PODMAN_DUMMY; sleep 1; echo -n $'\cp' > $PODMAN_DUMMY; echo -n $'\cq' > $PODMAN_DUMMY ) &
+    run_podman run -it --name $cname1 $IMAGE sh <$PODMAN_TEST_PTY
+    # Because we print to a terminal it appends "\r"
+    assert "$output" =~ "$msg"$'\r' "run output contains message"
+
+    # The container should still be running
+    run_podman inspect --format {{.State.Status}} $cname1
+    assert "$output" == "running" "container status"
+
+    # Now a second test with --detach-keys to make sure the cli option works
+    (echo "echo $msg" > $PODMAN_DUMMY; sleep 1; echo -n $'\cc' > $PODMAN_DUMMY; echo -n $'J' > $PODMAN_DUMMY ) &
+    run_podman run -it --name $cname2 --detach-keys ctrl-c,J $IMAGE sh <$PODMAN_TEST_PTY
+    assert "$output" =~ "$msg"$'\r' "run output with --detach-keys contains message"
+
+    run_podman rm -f -t0 $cname1 $cname2
+}
+
+@test "podman exec detach keys" {
+    skip_if_remote "FIXME #25089: podman-remote exec detach broken"
+
+    local cname=c-$(safename)
+    run_podman run -d --name $cname $IMAGE sleep inf
+
+    local msg=$(random_string)
+    # First "type" a command then send CTRL-p,CTRL-q sequence to the exec command.
+    # If the detach does not work podman exec will hang and timeout.
+    # The sleep is needed to ensure the output can be printed before we detach.
+    (echo "echo $msg" > $PODMAN_DUMMY; sleep 1; echo -n $'\cp' > $PODMAN_DUMMY; echo -n $'\cq' > $PODMAN_DUMMY ) &
+    run_podman exec -it $cname sh <$PODMAN_TEST_PTY
+    # Because we print to a terminal it appends "\r"
+    assert "$output" =~ "$msg"$'\r' "exec output contains message"
+
+    # The previous exec session/process should still be running
+    run_podman exec $cname ps aux
+    # Match PID=2 USER=root and COMMAND=sh from the ps output
+    assert "$output" =~ "2 root.*sh" "sh exec process still running"
+
+    # Now a second test with --detach-keys to make sure the cli option works
+    (echo "echo $msg" > $PODMAN_DUMMY; sleep 1; echo -n $'\ct' > $PODMAN_DUMMY; echo -n $'f' > $PODMAN_DUMMY ) &
+    run_podman exec -it --detach-keys ctrl-t,f $cname sh <$PODMAN_TEST_PTY
+    assert "$output" =~ "$msg"$'\r' "exec output with --detach-keys contains message"
+
+    run_podman rm -f -t0 $cname
+}
+
 # vim: filetype=sh