From 24c05b71fa4fff0bf92c65240c7e22807717b3a4 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 7 Jan 2021 09:11:59 -0800 Subject: [PATCH 1/4] tty: fix ClearONLCR race The TestExecInTTY test case is sometimes failing like this: > execin_test.go:332: unexpected carriage-return in output "PID USER TIME COMMAND\r\n 1 root 0:00 cat\r\n 7 root 0:00 ps\r\n" or this: > execin_test.go:332: unexpected carriage-return in output "PID USER TIME COMMAND\r\n 1 root 0:00 cat\n 7 root 0:00 ps\n" (this is easy to repro with `go test -run TestExecInTTY -count 1000`). This is caused by a race between - an Init() (in this case it is is (*linuxSetnsInit.Init(), but (*linuxStandardInit).Init() is no different in this regard), which creates a pty pair, sends pty master to runc, and execs the container process, and - a parent runc process, which receives the pty master fd and calls ClearONLCR() on it. One way of fixing it would be to add a synchronization mechanism between these two, so Init() won't exec the process until the parent sets the flag. This seems excessive, though, as we can just move the ClearONLCR() call to Init(), putting it right after console.NewPty(). Note that bug only happens in the TestExecInTTY test case, but from looking at the code it seems like it can happen in runc run or runc exec, too. Signed-off-by: Kir Kolyshkin --- contrib/cmd/recvtty/recvtty.go | 3 --- libcontainer/init_linux.go | 4 ++++ libcontainer/integration/execin_test.go | 1 - tty.go | 4 ---- 4 files changed, 4 insertions(+), 8 deletions(-) diff --git a/contrib/cmd/recvtty/recvtty.go b/contrib/cmd/recvtty/recvtty.go index cf2fddf76..8fddd0269 100644 --- a/contrib/cmd/recvtty/recvtty.go +++ b/contrib/cmd/recvtty/recvtty.go @@ -106,9 +106,6 @@ func handleSingle(path string, noStdin bool) error { if err != nil { return err } - if err := console.ClearONLCR(c.Fd()); err != nil { - return err - } // Copy from our stdio to the master fd. var ( diff --git a/libcontainer/init_linux.go b/libcontainer/init_linux.go index bb8ff0b52..06684eb5b 100644 --- a/libcontainer/init_linux.go +++ b/libcontainer/init_linux.go @@ -186,6 +186,10 @@ func setupConsole(socket *os.File, config *initConfig, mount bool) error { if err != nil { return err } + err = console.ClearONLCR(pty.Fd()) + if err != nil { + return err + } // After we return from here, we don't need the console anymore. defer pty.Close() diff --git a/libcontainer/integration/execin_test.go b/libcontainer/integration/execin_test.go index ad2066ea9..ca0ba63f4 100644 --- a/libcontainer/integration/execin_test.go +++ b/libcontainer/integration/execin_test.go @@ -313,7 +313,6 @@ func TestExecInTTY(t *testing.T) { } return } - console.ClearONLCR(c.Fd()) dc <- &cdata{ c: c, } diff --git a/tty.go b/tty.go index 844d7f842..b15010202 100644 --- a/tty.go +++ b/tty.go @@ -112,10 +112,6 @@ func (t *tty) recvtty(process *libcontainer.Process, socket *os.File) (Err error if err != nil { return err } - err = console.ClearONLCR(cons.Fd()) - if err != nil { - return err - } epoller, err := console.NewEpoller() if err != nil { return err From 719d70d2e37d35026c6ec0165ffe6e97b074e473 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 7 Jan 2021 13:33:41 -0800 Subject: [PATCH 2/4] setupIO: simplify code There's no need to check err for nil. Signed-off-by: Kir Kolyshkin --- utils_linux.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/utils_linux.go b/utils_linux.go index f15537fa7..8241d4232 100644 --- a/utils_linux.go +++ b/utils_linux.go @@ -164,10 +164,7 @@ func setupIO(process *libcontainer.Process, rootuid, rootgid int, createTTY, det t.postStart = append(t.postStart, parent, child) t.consoleC = make(chan error, 1) go func() { - if err := t.recvtty(process, parent); err != nil { - t.consoleC <- err - } - t.consoleC <- nil + t.consoleC <- t.recvtty(process, parent) }() } else { // the caller of runc will handle receiving the console master From fedaa2abedada801fd339feca155f986eb6c4596 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 7 Jan 2021 09:45:47 -0800 Subject: [PATCH 3/4] TestExecInTTY: simplify, improve error reporting Simplify the tty code by using 1 goroutine instead of 2. Improve error reporting by wrapping the errors. Signed-off-by: Kir Kolyshkin --- libcontainer/integration/execin_test.go | 45 +++++++++---------------- 1 file changed, 15 insertions(+), 30 deletions(-) diff --git a/libcontainer/integration/execin_test.go b/libcontainer/integration/execin_test.go index ca0ba63f4..b19ecf885 100644 --- a/libcontainer/integration/execin_test.go +++ b/libcontainer/integration/execin_test.go @@ -277,7 +277,12 @@ func TestExecInTTY(t *testing.T) { } err = container.Run(process) stdinR.Close() - defer stdinW.Close() + defer func() { + stdinW.Close() + if _, err := process.Wait(); err != nil { + t.Log(err) + } + }() ok(t, err) var stdout bytes.Buffer @@ -293,53 +298,33 @@ func TestExecInTTY(t *testing.T) { defer parent.Close() defer child.Close() ps.ConsoleSocket = child - type cdata struct { - c console.Console - err error - } - dc := make(chan *cdata, 1) + done := make(chan (error)) go func() { f, err := utils.RecvFd(parent) if err != nil { - dc <- &cdata{ - err: err, - } + done <- fmt.Errorf("RecvFd: %w", err) return } c, err := console.ConsoleFromFile(f) if err != nil { - dc <- &cdata{ - err: err, - } + done <- fmt.Errorf("ConsoleFromFile: %w", err) return } - dc <- &cdata{ - c: c, - } + // An error from io.Copy is expected once the terminal + // is gone, so we deliberately ignore it. + _, _ = io.Copy(&stdout, c) + done <- nil }() err = container.Run(ps) ok(t, err) - data := <-dc - if data.err != nil { - ok(t, data.err) - } - console := data.c - copy := make(chan struct{}) - go func() { - io.Copy(&stdout, console) - close(copy) - }() - ok(t, err) select { case <-time.After(5 * time.Second): t.Fatal("Waiting for copy timed out") - case <-copy: + case err := <-done: + ok(t, err) } waitProcess(ps, t) - stdinW.Close() - waitProcess(process, t) - out := stdout.String() if !strings.Contains(out, "cat") || !strings.Contains(out, "ps") { t.Fatalf("unexpected running process, output %q", out) From 06a684d6a79371df5ee5942e2427c1a294eda1a5 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Thu, 7 Jan 2021 13:21:38 -0800 Subject: [PATCH 4/4] libct/int/TestExecInTTY: repeat the test 300 times This is to increase the chance to hit the recently fixed race. Signed-off-by: Kir Kolyshkin --- libcontainer/integration/execin_test.go | 87 ++++++++++++++----------- 1 file changed, 48 insertions(+), 39 deletions(-) diff --git a/libcontainer/integration/execin_test.go b/libcontainer/integration/execin_test.go index b19ecf885..dda716712 100644 --- a/libcontainer/integration/execin_test.go +++ b/libcontainer/integration/execin_test.go @@ -291,46 +291,55 @@ func TestExecInTTY(t *testing.T) { Args: []string{"ps"}, Env: standardEnvironment, } - parent, child, err := utils.NewSockPair("console") - if err != nil { - ok(t, err) - } - defer parent.Close() - defer child.Close() - ps.ConsoleSocket = child - done := make(chan (error)) - go func() { - f, err := utils.RecvFd(parent) - if err != nil { - done <- fmt.Errorf("RecvFd: %w", err) - return - } - c, err := console.ConsoleFromFile(f) - if err != nil { - done <- fmt.Errorf("ConsoleFromFile: %w", err) - return - } - // An error from io.Copy is expected once the terminal - // is gone, so we deliberately ignore it. - _, _ = io.Copy(&stdout, c) - done <- nil - }() - err = container.Run(ps) - ok(t, err) - select { - case <-time.After(5 * time.Second): - t.Fatal("Waiting for copy timed out") - case err := <-done: - ok(t, err) - } - waitProcess(ps, t) - out := stdout.String() - if !strings.Contains(out, "cat") || !strings.Contains(out, "ps") { - t.Fatalf("unexpected running process, output %q", out) - } - if strings.Contains(out, "\r") { - t.Fatalf("unexpected carriage-return in output %q", out) + // Repeat to increase chances to catch a race; see + // https://github.com/opencontainers/runc/issues/2425. + for i := 0; i < 300; i++ { + parent, child, err := utils.NewSockPair("console") + if err != nil { + ok(t, err) + } + ps.ConsoleSocket = child + + done := make(chan (error)) + go func() { + f, err := utils.RecvFd(parent) + if err != nil { + done <- fmt.Errorf("RecvFd: %w", err) + return + } + c, err := console.ConsoleFromFile(f) + if err != nil { + done <- fmt.Errorf("ConsoleFromFile: %w", err) + return + } + // An error from io.Copy is expected once the terminal + // is gone, so we deliberately ignore it. + _, _ = io.Copy(&stdout, c) + done <- nil + }() + + err = container.Run(ps) + ok(t, err) + + select { + case <-time.After(5 * time.Second): + t.Fatal("Waiting for copy timed out") + case err := <-done: + ok(t, err) + } + + waitProcess(ps, t) + parent.Close() + child.Close() + + out := stdout.String() + if !strings.Contains(out, "cat") || !strings.Contains(out, "ps") { + t.Fatalf("unexpected running process, output %q", out) + } + if strings.Contains(out, "\r") { + t.Fatalf("unexpected carriage-return in output %q", out) + } } }