From 24c05b71fa4fff0bf92c65240c7e22807717b3a4 Mon Sep 17 00:00:00 2001
From: Kir Kolyshkin <kolyshkin@gmail.com>
Date: Thu, 7 Jan 2021 09:11:59 -0800
Subject: [PATCH] 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 <kolyshkin@gmail.com>
---
 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