From 14d31889e43c70be3e104a2937b4d8f2b367226d Mon Sep 17 00:00:00 2001
From: Laura Brehm <laurabrehm@hey.com>
Date: Mon, 2 Dec 2024 10:33:17 +0000
Subject: [PATCH] run: don't hang if only attaching STDIN
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

If STDOUT or STDERR are attached and the container exits, the streams
will be closed by the daemon while the container is exiting, causing
the streamer to return an error
https://github.com/docker/cli/blob/61b02e636d2afed778f2e871c3ec1c6adee69ca4/cli/command/container/hijack.go#L53
that gets sent
https://github.com/docker/cli/blob/61b02e636d2afed778f2e871c3ec1c6adee69ca4/cli/command/container/run.go#L278
and received
https://github.com/docker/cli/blob/61b02e636d2afed778f2e871c3ec1c6adee69ca4/cli/command/container/run.go#L225
on `errCh`.

However, if only STDIN is attached, it's not closed (since this is
attached to the user's TTY) when the container exits, so the streamer
doesn't exit and nothing gets sent on `errCh`, meaning the CLI execution
hangs receiving on `errCh` on L231.

Change the logic to receive on both `errCh` and `statusChan` – this way,
if the container exits, we get notified on `statusChan` (even if only
STDIN is attached), and can cancel the streamer and exit.

Signed-off-by: Laura Brehm <laurabrehm@hey.com>
---
 cli/command/container/run.go | 33 ++++++++++++++++++++++-----------
 e2e/container/run_test.go    | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 11 deletions(-)

diff --git a/cli/command/container/run.go b/cli/command/container/run.go
index ed4c96574c5f..5ef3e8b7bf7b 100644
--- a/cli/command/container/run.go
+++ b/cli/command/container/run.go
@@ -228,20 +228,31 @@ func runContainer(ctx context.Context, dockerCli command.Cli, runOpts *runOption
 		}
 	}
 
-	if err := <-errCh; err != nil {
-		if _, ok := err.(term.EscapeError); ok {
-			// The user entered the detach escape sequence.
-			return nil
-		}
+	select {
+	case err := <-errCh:
+		if err != nil {
+			if _, ok := err.(term.EscapeError); ok {
+				// The user entered the detach escape sequence.
+				return nil
+			}
 
-		logrus.Debugf("Error hijack: %s", err)
-		return err
+			logrus.Debugf("Error hijack: %s", err)
+			return err
+		}
+		status := <-statusChan
+		if status != 0 {
+			return cli.StatusError{StatusCode: status}
+		}
+	case status := <-statusChan:
+		// notify hijackedIOStreamer that we're exiting and wait
+		// so that the terminal can be restored.
+		cancelFun()
+		<-errCh
+		if status != 0 {
+			return cli.StatusError{StatusCode: status}
+		}
 	}
 
-	status := <-statusChan
-	if status != 0 {
-		return cli.StatusError{StatusCode: status}
-	}
 	return nil
 }
 
diff --git a/e2e/container/run_test.go b/e2e/container/run_test.go
index c835f1caf294..b408cc34f1c4 100644
--- a/e2e/container/run_test.go
+++ b/e2e/container/run_test.go
@@ -3,11 +3,13 @@ package container
 import (
 	"bytes"
 	"fmt"
+	"os/exec"
 	"strings"
 	"syscall"
 	"testing"
 	"time"
 
+	"github.com/creack/pty"
 	"github.com/docker/cli/e2e/internal/fixtures"
 	"github.com/docker/cli/internal/test/environment"
 	"github.com/docker/docker/api/types/versions"
@@ -38,6 +40,37 @@ func TestRunAttachedFromRemoteImageAndRemove(t *testing.T) {
 	golden.Assert(t, result.Stderr(), "run-attached-from-remote-and-remove.golden")
 }
 
+func TestRunAttach(t *testing.T) {
+	skip.If(t, environment.RemoteDaemon())
+
+	streams := []string{"stdin", "stdout", "stderr"}
+	for _, stream := range streams {
+		t.Run(stream, func(t *testing.T) {
+			c := exec.Command("docker", "run", "-a", stream, "--rm", "alpine",
+				"sh", "-c", "sleep 1 && exit 7")
+			d := bytes.Buffer{}
+			c.Stdout = &d
+			c.Stderr = &d
+			_, err := pty.Start(c)
+			assert.NilError(t, err)
+
+			done := make(chan error)
+			go func() {
+				done <- c.Wait()
+			}()
+
+			select {
+			case <-time.After(20 * time.Second):
+				t.Fatal("docker run took too long, likely hang", d.String())
+			case <-done:
+			}
+
+			assert.Equal(t, c.ProcessState.ExitCode(), 7)
+			assert.Check(t, strings.Contains(d.String(), "exit status 7"))
+		})
+	}
+}
+
 // Regression test for https://github.com/docker/cli/issues/5053
 func TestRunInvalidEntrypointWithAutoremove(t *testing.T) {
 	environment.SkipIfDaemonNotLinux(t)