Merge pull request #9537 from icecrime/9476-tty_stdin_redirect

Forbid client piping to tty enabled container

api/client/cli.go

@@ -3,6 +3,7 @@ package client
 import (
 	"crypto/tls"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"net"
@@ -104,6 +105,16 @@ func (cli *DockerCli) LoadConfigFile() (err error) {
 	return err
 }
 
+func (cli *DockerCli) CheckTtyInput(attachStdin, ttyMode bool) error {
+	// In order to attach to a container tty, input stream for the client must
+	// be a tty itself: redirecting or piping the client standard input is
+	// incompatible with `docker run -t`, `docker exec -t` or `docker attach`.
+	if ttyMode && attachStdin && !cli.isTerminalIn {
+		return errors.New("cannot enable tty mode on non tty input")
+	}
+	return nil
+}
+
 func NewDockerCli(in io.ReadCloser, out, err io.Writer, key libtrust.PrivateKey, proto, addr string, tlsConfig *tls.Config) *DockerCli {
 	var (
 		inFd          uintptr

api/client/commands.go

@@ -1974,6 +1974,10 @@ func (cli *DockerCli) CmdAttach(args ...string) error {
 		tty    = config.GetBool("Tty")
 	)
 
+	if err := cli.CheckTtyInput(!*noStdin, tty); err != nil {
+		return err
+	}
+
 	if tty && cli.isTerminalOut {
 		if err := cli.monitorTtySize(cmd.Arg(0), false); err != nil {
 			log.Debugf("Error monitoring TTY size: %s", err)
@@ -2288,7 +2292,11 @@ func (cli *DockerCli) CmdRun(args ...string) error {
 		return nil
 	}
 
-	if *flDetach {
+	if !*flDetach {
+		if err := cli.CheckTtyInput(config.AttachStdin, config.Tty); err != nil {
+			return err
+		}
+	} else {
 		if fl := cmd.Lookup("attach"); fl != nil {
 			flAttach = fl.Value.(*opts.ListOpts)
 			if flAttach.Len() != 0 {
@@ -2600,7 +2608,11 @@ func (cli *DockerCli) CmdExec(args ...string) error {
 		return nil
 	}
 
-	if execConfig.Detach {
+	if !execConfig.Detach {
+		if err := cli.CheckTtyInput(execConfig.AttachStdin, execConfig.Tty); err != nil {
+			return err
+		}
+	} else {
 		if _, _, err := readBody(cli.call("POST", "/exec/"+execID+"/start", execConfig, false)); err != nil {
 			return err
 		}

docs/man/docker-attach.1.md

@@ -20,6 +20,9 @@ container, or `CTRL-\` to get a stacktrace of the Docker client when it quits.
 When you detach from a container the exit code will be returned to
 the client.
 
+It is forbidden to redirect the standard input of a docker attach command while
+attaching to a tty-enabled container (i.e.: launched with -t`).
+
 # OPTIONS
 **--no-stdin**=*true*|*false*
    Do not attach STDIN. The default is *false*.

docs/man/docker-exec.1.md

@@ -31,5 +31,8 @@ container is unpaused, and then run
 **-t**, **--tty**=*true*|*false*
    Allocate a pseudo-TTY. The default is *false*.
 
+The **-t** option is incompatible with a redirection of the docker client
+standard input.
+
 # HISTORY
 November 2014, updated by Sven Dowideit <SvenDowideit@home.org.au>

docs/man/docker-run.1.md

@@ -267,6 +267,9 @@ outside of a container on the host.
 input of any container. This can be used, for example, to run a throwaway
 interactive shell. The default is value is false.
 
+The **-t** option is incompatible with a redirection of the docker client
+standard input.
+
 **-u**, **--user**=""
    Username or UID
 

docs/sources/reference/run.md

@@ -94,9 +94,10 @@ specify to which of the three standard streams (`STDIN`, `STDOUT`,
 
     $ sudo docker run -a stdin -a stdout -i -t ubuntu /bin/bash
 
-For interactive processes (like a shell) you will typically want a tty
-as well as persistent standard input (`STDIN`), so you'll use `-i -t`
-together in most interactive cases.
+For interactive processes (like a shell), you must use `-i -t` together in
+order to allocate a tty for the container process. Specifying `-t` is however
+forbidden when the client standard output is redirected or pipe, such as in:
+`echo test | docker run -i busybox cat`.
 
 ## Container identification
 

integration-cli/docker_cli_attach_test.go

@@ -87,3 +87,50 @@ func TestAttachMultipleAndRestart(t *testing.T) {
 
 	logDone("attach - multiple attach")
 }
+
+func TestAttachTtyWithoutStdin(t *testing.T) {
+	defer deleteAllContainers()
+
+	cmd := exec.Command(dockerBinary, "run", "-d", "-ti", "busybox")
+	out, _, err := runCommandWithOutput(cmd)
+	if err != nil {
+		t.Fatalf("failed to start container: %v (%v)", out, err)
+	}
+
+	id := strings.TrimSpace(out)
+	if err := waitRun(id); err != nil {
+		t.Fatal(err)
+	}
+
+	defer func() {
+		cmd := exec.Command(dockerBinary, "kill", id)
+		if out, _, err := runCommandWithOutput(cmd); err != nil {
+			t.Fatalf("failed to kill container: %v (%v)", out, err)
+		}
+	}()
+
+	done := make(chan struct{})
+	go func() {
+		defer close(done)
+
+		cmd := exec.Command(dockerBinary, "attach", id)
+		if _, err := cmd.StdinPipe(); err != nil {
+			t.Fatal(err)
+		}
+
+		expected := "cannot enable tty mode"
+		if out, _, err := runCommandWithOutput(cmd); err == nil {
+			t.Fatal("attach should have failed")
+		} else if !strings.Contains(out, expected) {
+			t.Fatal("attach failed with error %q: expected %q", out, expected)
+		}
+	}()
+
+	select {
+	case <-done:
+	case <-time.After(attachWait):
+		t.Fatal("attach is running but should have failed")
+	}
+
+	logDone("attach - forbid piped stdin to tty enabled container")
+}

integration-cli/docker_cli_exec_test.go

@@ -263,3 +263,91 @@ func TestExecPausedContainer(t *testing.T) {
 
 	logDone("exec - exec should not exec a pause container")
 }
+
+// regression test for #9476
+func TestExecTtyCloseStdin(t *testing.T) {
+	defer deleteAllContainers()
+
+	cmd := exec.Command(dockerBinary, "run", "-d", "-it", "--name", "exec_tty_stdin", "busybox")
+	if out, _, err := runCommandWithOutput(cmd); err != nil {
+		t.Fatal(out, err)
+	}
+
+	cmd = exec.Command(dockerBinary, "exec", "-i", "exec_tty_stdin", "cat")
+	stdinRw, err := cmd.StdinPipe()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	stdinRw.Write([]byte("test"))
+	stdinRw.Close()
+
+	if out, _, err := runCommandWithOutput(cmd); err != nil {
+		t.Fatal(out, err)
+	}
+
+	cmd = exec.Command(dockerBinary, "top", "exec_tty_stdin")
+	out, _, err := runCommandWithOutput(cmd)
+	if err != nil {
+		t.Fatal(out, err)
+	}
+
+	outArr := strings.Split(out, "\n")
+	if len(outArr) > 3 || strings.Contains(out, "nsenter-exec") {
+		// This is the really bad part
+		if out, _, err := runCommandWithOutput(exec.Command(dockerBinary, "rm", "-f", "exec_tty_stdin")); err != nil {
+			t.Fatal(out, err)
+		}
+
+		t.Fatalf("exec process left running\n\t %s", out)
+	}
+
+	logDone("exec - stdin is closed properly with tty enabled")
+}
+
+func TestExecTtyWithoutStdin(t *testing.T) {
+	defer deleteAllContainers()
+
+	cmd := exec.Command(dockerBinary, "run", "-d", "-ti", "busybox")
+	out, _, err := runCommandWithOutput(cmd)
+	if err != nil {
+		t.Fatalf("failed to start container: %v (%v)", out, err)
+	}
+
+	id := strings.TrimSpace(out)
+	if err := waitRun(id); err != nil {
+		t.Fatal(err)
+	}
+
+	defer func() {
+		cmd := exec.Command(dockerBinary, "kill", id)
+		if out, _, err := runCommandWithOutput(cmd); err != nil {
+			t.Fatalf("failed to kill container: %v (%v)", out, err)
+		}
+	}()
+
+	done := make(chan struct{})
+	go func() {
+		defer close(done)
+
+		cmd := exec.Command(dockerBinary, "exec", "-ti", id, "true")
+		if _, err := cmd.StdinPipe(); err != nil {
+			t.Fatal(err)
+		}
+
+		expected := "cannot enable tty mode"
+		if out, _, err := runCommandWithOutput(cmd); err == nil {
+			t.Fatal("exec should have failed")
+		} else if !strings.Contains(out, expected) {
+			t.Fatal("exec failed with error %q: expected %q", out, expected)
+		}
+	}()
+
+	select {
+	case <-done:
+	case <-time.After(3 * time.Second):
+		t.Fatal("exec is running but should have failed")
+	}
+
+	logDone("exec - forbid piped stdin to tty enabled container")
+}

integration-cli/docker_cli_run_test.go

@@ -2742,3 +2742,32 @@ func TestRunPortFromDockerRangeInUse(t *testing.T) {
 
 	logDone("run - find another port if port from autorange already bound")
 }
+
+func TestRunTtyWithPipe(t *testing.T) {
+	defer deleteAllContainers()
+
+	done := make(chan struct{})
+	go func() {
+		defer close(done)
+
+		cmd := exec.Command(dockerBinary, "run", "-ti", "busybox", "true")
+		if _, err := cmd.StdinPipe(); err != nil {
+			t.Fatal(err)
+		}
+
+		expected := "cannot enable tty mode"
+		if out, _, err := runCommandWithOutput(cmd); err == nil {
+			t.Fatal("run should have failed")
+		} else if !strings.Contains(out, expected) {
+			t.Fatal("run failed with error %q: expected %q", out, expected)
+		}
+	}()
+
+	select {
+	case <-done:
+	case <-time.After(3 * time.Second):
+		t.Fatal("container is running but should have failed")
+	}
+
+	logDone("run - forbid piped stdin with tty")
+}

integration/commands_test.go

@@ -15,6 +15,7 @@ import (
 	"github.com/docker/docker/pkg/term"
 	"github.com/docker/docker/utils"
 	"github.com/docker/libtrust"
+	"github.com/kr/pty"
 )
 
 func closeWrap(args ...io.Closer) error {
@@ -162,72 +163,20 @@ func TestRunDisconnect(t *testing.T) {
 	})
 }
 
-// Expected behaviour: the process stay alive when the client disconnects
-// but the client detaches.
-func TestRunDisconnectTty(t *testing.T) {
-
-	stdin, stdinPipe := io.Pipe()
+// TestRunDetach checks attaching and detaching with the escape sequence.
+func TestRunDetach(t *testing.T) {
 	stdout, stdoutPipe := io.Pipe()
-	key, err := libtrust.GenerateECP256PrivateKey()
+	cpty, tty, err := pty.Open()
 	if err != nil {
 		t.Fatal(err)
 	}
 
-	cli := client.NewDockerCli(stdin, stdoutPipe, ioutil.Discard, key, testDaemonProto, testDaemonAddr, nil)
-	defer cleanup(globalEngine, t)
-
-	c1 := make(chan struct{})
-	go func() {
-		defer close(c1)
-		// We're simulating a disconnect so the return value doesn't matter. What matters is the
-		// fact that CmdRun returns.
-		if err := cli.CmdRun("-i", "-t", unitTestImageID, "/bin/cat"); err != nil {
-			log.Debugf("Error CmdRun: %s", err)
-		}
-	}()
-
-	container := waitContainerStart(t, 10*time.Second)
-
-	state := setRaw(t, container)
-	defer unsetRaw(t, container, state)
-
-	// Client disconnect after run -i should keep stdin out in TTY mode
-	setTimeout(t, "Read/Write assertion timed out", 2*time.Second, func() {
-		if err := assertPipe("hello\n", "hello", stdout, stdinPipe, 150); err != nil {
-			t.Fatal(err)
-		}
-	})
-
-	// Close pipes (simulate disconnect)
-	if err := closeWrap(stdin, stdinPipe, stdout, stdoutPipe); err != nil {
-		t.Fatal(err)
-	}
-
-	// wait for CmdRun to return
-	setTimeout(t, "Waiting for CmdRun timed out", 5*time.Second, func() {
-		<-c1
-	})
-
-	// In tty mode, we expect the process to stay alive even after client's stdin closes.
-
-	// Give some time to monitor to do his thing
-	container.WaitStop(500 * time.Millisecond)
-	if !container.IsRunning() {
-		t.Fatalf("/bin/cat should  still be running after closing stdin (tty mode)")
-	}
-}
-
-// TestRunDetach checks attaching and detaching with the escape sequence.
-func TestRunDetach(t *testing.T) {
-
-	stdin, stdinPipe := io.Pipe()
-	stdout, stdoutPipe := io.Pipe()
 	key, err := libtrust.GenerateECP256PrivateKey()
 	if err != nil {
 		t.Fatal(err)
 	}
 
-	cli := client.NewDockerCli(stdin, stdoutPipe, ioutil.Discard, key, testDaemonProto, testDaemonAddr, nil)
+	cli := client.NewDockerCli(tty, stdoutPipe, ioutil.Discard, key, testDaemonProto, testDaemonAddr, nil)
 	defer cleanup(globalEngine, t)
 
 	ch := make(chan struct{})
@@ -242,22 +191,22 @@ func TestRunDetach(t *testing.T) {
 	defer unsetRaw(t, container, state)
 
 	setTimeout(t, "First read/write assertion timed out", 2*time.Second, func() {
-		if err := assertPipe("hello\n", "hello", stdout, stdinPipe, 150); err != nil {
+		if err := assertPipe("hello\n", "hello", stdout, cpty, 150); err != nil {
 			t.Fatal(err)
 		}
 	})
 
 	setTimeout(t, "Escape sequence timeout", 5*time.Second, func() {
-		stdinPipe.Write([]byte{16})
+		cpty.Write([]byte{16})
 		time.Sleep(100 * time.Millisecond)
-		stdinPipe.Write([]byte{17})
+		cpty.Write([]byte{17})
 	})
 
 	// wait for CmdRun to return
 	setTimeout(t, "Waiting for CmdRun timed out", 15*time.Second, func() {
 		<-ch
 	})
-	closeWrap(stdin, stdinPipe, stdout, stdoutPipe)
+	closeWrap(cpty, stdout, stdoutPipe)
 
 	time.Sleep(500 * time.Millisecond)
 	if !container.IsRunning() {
@@ -271,14 +220,18 @@ func TestRunDetach(t *testing.T) {
 
 // TestAttachDetach checks that attach in tty mode can be detached using the long container ID
 func TestAttachDetach(t *testing.T) {
-	stdin, stdinPipe := io.Pipe()
 	stdout, stdoutPipe := io.Pipe()
+	cpty, tty, err := pty.Open()
+	if err != nil {
+		t.Fatal(err)
+	}
+
 	key, err := libtrust.GenerateECP256PrivateKey()
 	if err != nil {
 		t.Fatal(err)
 	}
 
-	cli := client.NewDockerCli(stdin, stdoutPipe, ioutil.Discard, key, testDaemonProto, testDaemonAddr, nil)
+	cli := client.NewDockerCli(tty, stdoutPipe, ioutil.Discard, key, testDaemonProto, testDaemonAddr, nil)
 	defer cleanup(globalEngine, t)
 
 	ch := make(chan struct{})
@@ -309,9 +262,13 @@ func TestAttachDetach(t *testing.T) {
 	state := setRaw(t, container)
 	defer unsetRaw(t, container, state)
 
-	stdin, stdinPipe = io.Pipe()
 	stdout, stdoutPipe = io.Pipe()
-	cli = client.NewDockerCli(stdin, stdoutPipe, ioutil.Discard, key, testDaemonProto, testDaemonAddr, nil)
+	cpty, tty, err = pty.Open()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	cli = client.NewDockerCli(tty, stdoutPipe, ioutil.Discard, key, testDaemonProto, testDaemonAddr, nil)
 
 	ch = make(chan struct{})
 	go func() {
@@ -324,7 +281,7 @@ func TestAttachDetach(t *testing.T) {
 	}()
 
 	setTimeout(t, "First read/write assertion timed out", 2*time.Second, func() {
-		if err := assertPipe("hello\n", "hello", stdout, stdinPipe, 150); err != nil {
+		if err := assertPipe("hello\n", "hello", stdout, cpty, 150); err != nil {
 			if err != io.ErrClosedPipe {
 				t.Fatal(err)
 			}
@@ -332,9 +289,9 @@ func TestAttachDetach(t *testing.T) {
 	})
 
 	setTimeout(t, "Escape sequence timeout", 5*time.Second, func() {
-		stdinPipe.Write([]byte{16})
+		cpty.Write([]byte{16})
 		time.Sleep(100 * time.Millisecond)
-		stdinPipe.Write([]byte{17})
+		cpty.Write([]byte{17})
 	})
 
 	// wait for CmdRun to return
@@ -342,7 +299,7 @@ func TestAttachDetach(t *testing.T) {
 		<-ch
 	})
 
-	closeWrap(stdin, stdinPipe, stdout, stdoutPipe)
+	closeWrap(cpty, stdout, stdoutPipe)
 
 	time.Sleep(500 * time.Millisecond)
 	if !container.IsRunning() {
@@ -356,14 +313,18 @@ func TestAttachDetach(t *testing.T) {
 
 // TestAttachDetachTruncatedID checks that attach in tty mode can be detached
 func TestAttachDetachTruncatedID(t *testing.T) {
-	stdin, stdinPipe := io.Pipe()
 	stdout, stdoutPipe := io.Pipe()
+	cpty, tty, err := pty.Open()
+	if err != nil {
+		t.Fatal(err)
+	}
+
 	key, err := libtrust.GenerateECP256PrivateKey()
 	if err != nil {
 		t.Fatal(err)
 	}
 
-	cli := client.NewDockerCli(stdin, stdoutPipe, ioutil.Discard, key, testDaemonProto, testDaemonAddr, nil)
+	cli := client.NewDockerCli(tty, stdoutPipe, ioutil.Discard, key, testDaemonProto, testDaemonAddr, nil)
 	defer cleanup(globalEngine, t)
 
 	// Discard the CmdRun output
@@ -379,9 +340,13 @@ func TestAttachDetachTruncatedID(t *testing.T) {
 	state := setRaw(t, container)
 	defer unsetRaw(t, container, state)
 
-	stdin, stdinPipe = io.Pipe()
 	stdout, stdoutPipe = io.Pipe()
-	cli = client.NewDockerCli(stdin, stdoutPipe, ioutil.Discard, key, testDaemonProto, testDaemonAddr, nil)
+	cpty, tty, err = pty.Open()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	cli = client.NewDockerCli(tty, stdoutPipe, ioutil.Discard, key, testDaemonProto, testDaemonAddr, nil)
 
 	ch := make(chan struct{})
 	go func() {
@@ -394,7 +359,7 @@ func TestAttachDetachTruncatedID(t *testing.T) {
 	}()
 
 	setTimeout(t, "First read/write assertion timed out", 2*time.Second, func() {
-		if err := assertPipe("hello\n", "hello", stdout, stdinPipe, 150); err != nil {
+		if err := assertPipe("hello\n", "hello", stdout, cpty, 150); err != nil {
 			if err != io.ErrClosedPipe {
 				t.Fatal(err)
 			}
@@ -402,16 +367,16 @@ func TestAttachDetachTruncatedID(t *testing.T) {
 	})
 
 	setTimeout(t, "Escape sequence timeout", 5*time.Second, func() {
-		stdinPipe.Write([]byte{16})
+		cpty.Write([]byte{16})
 		time.Sleep(100 * time.Millisecond)
-		stdinPipe.Write([]byte{17})
+		cpty.Write([]byte{17})
 	})
 
 	// wait for CmdRun to return
 	setTimeout(t, "Waiting for CmdAttach timed out", 15*time.Second, func() {
 		<-ch
 	})
-	closeWrap(stdin, stdinPipe, stdout, stdoutPipe)
+	closeWrap(cpty, stdout, stdoutPipe)
 
 	time.Sleep(500 * time.Millisecond)
 	if !container.IsRunning() {
@@ -425,14 +390,18 @@ func TestAttachDetachTruncatedID(t *testing.T) {
 
 // Expected behaviour, the process stays alive when the client disconnects
 func TestAttachDisconnect(t *testing.T) {
-	stdin, stdinPipe := io.Pipe()
 	stdout, stdoutPipe := io.Pipe()
+	cpty, tty, err := pty.Open()
+	if err != nil {
+		t.Fatal(err)
+	}
+
 	key, err := libtrust.GenerateECP256PrivateKey()
 	if err != nil {
 		t.Fatal(err)
 	}
 
-	cli := client.NewDockerCli(stdin, stdoutPipe, ioutil.Discard, key, testDaemonProto, testDaemonAddr, nil)
+	cli := client.NewDockerCli(tty, stdoutPipe, ioutil.Discard, key, testDaemonProto, testDaemonAddr, nil)
 	defer cleanup(globalEngine, t)
 
 	go func() {
@@ -470,12 +439,12 @@ func TestAttachDisconnect(t *testing.T) {
 	}()
 
 	setTimeout(t, "First read/write assertion timed out", 2*time.Second, func() {
-		if err := assertPipe("hello\n", "hello", stdout, stdinPipe, 150); err != nil {
+		if err := assertPipe("hello\n", "hello", stdout, cpty, 150); err != nil {
 			t.Fatal(err)
 		}
 	})
 	// Close pipes (client disconnects)
-	if err := closeWrap(stdin, stdinPipe, stdout, stdoutPipe); err != nil {
+	if err := closeWrap(cpty, stdout, stdoutPipe); err != nil {
 		t.Fatal(err)
 	}