libcontainer: io: stop screwing with \n in console output

[cyphar]

The default terminal setting for a new pty on Linux (unix98) has +ONLCR,
resulting in '\n' writes by a container process to be converted to
'\r\n' reads by the managing process. This is quite unexpected, and
causes multiple issues with things like bats testing. To fix it, make
the terminal sane after opening it by setting -ONLCR.

This patch might need to be rewritten after the console rewrite patchset
is merged.

Fixes #1145.

Signed-off-by: Aleksa Sarai [email protected]

[cyphar]

/cc @hqhq Does this patchset help with the bats situation?

[hqhq]

My local experimental change looks like:

diff --git a/Godeps/_workspace/src/github.com/docker/docker/pkg/term/tc_linux_cgo.go b/Godeps/_workspace/src/github.com/docker/docker/pkg/term/tc_linux_cgo.go
index d47cf59..2804089 100644
--- a/Godeps/_workspace/src/github.com/docker/docker/pkg/term/tc_linux_cgo.go
+++ b/Godeps/_workspace/src/github.com/docker/docker/pkg/term/tc_linux_cgo.go
@@ -25,6 +25,8 @@ func MakeRaw(fd uintptr) (*State, error) {

        C.cfmakeraw((*C.struct_termios)(unsafe.Pointer(&newState)))
        newState.Oflag = newState.Oflag | C.OPOST
+       newState.Oflag = newState.Oflag ^ C.ONLCR
+
        if err := tcset(fd, &newState); err != 0 {
                return nil, err
        }
diff --git a/tty.go b/tty.go
index 5a76ebe..415d2e2 100644
--- a/tty.go
+++ b/tty.go
@@ -66,7 +66,8 @@ func createTty(p *libcontainer.Process, rootuid, rootgid int, consolePath string
        go io.Copy(console, os.Stdin)
        go io.Copy(os.Stdout, console)

-       state, err := term.SetRawTerminal(os.Stdin.Fd())
+       //state, err := term.SetRawTerminal(os.Stdin.Fd())
+       state, err := term.SetRawTerminal(console.Fd())
        if err != nil {
                return nil, fmt.Errorf("failed to set the terminal from the stdin: %v", err)
        }

I'm not sure if we should set stdin as raw mode.
BTW this default +onlcr is really suspicious.

[hqhq]

$ stty -a
speed 38400 baud; rows 52; columns 211; line = 0;
intr = ^C; quit = ^\; erase = ^?; kill = ^U; eof = ^D; eol = <undef>; eol2 = <undef>; swtch = <undef>; start = ^Q; stop = ^S; susp = ^Z; rprnt = ^R; werase = ^W; lnext = ^V; flush = ^O; min = 1; time = 0;
-parenb -parodd cs8 -hupcl -cstopb cread -clocal -crtscts
-ignbrk -brkint -ignpar -parmrk -inpck -istrip -inlcr -igncr icrnl ixon -ixoff -iuclc -ixany -imaxbel -iutf8
opost -olcuc -ocrnl onlcr -onocr -onlret -ofill -ofdel nl0 cr0 tab0 bs0 vt0 ff0
isig icanon iexten echo echoe echok -echonl -noflsh -xcase -tostop -echoprt echoctl echoke

Looks like linux default tty setting enables onlcr? Do we still miss something?

[cyphar]

We shouldn't set stdin in raw mode. The default +ONCLR is a kernel default (you can take a look at drivers/pty/n_tty.c if you want to hate yourself). The reason you don't notice it normally is that a pty is only created by your terminal emulator for your shell. You never see the weird stuff because all subprocess output is done with non-magical pipes.

So this isn't weird, it's just that we need to make it appear as though the output came from a pipe. We can work on this later (adding more settings), but this is enough to fix the immediate issue.

[cyphar]

Do we still miss something?

I don't think so. And if we end up missing something we can always update SaneTerminal to make the change.

[crosbymichael]

Why did you put this function in utils and not with the other terminal handling code in console_linux.go?

[cyphar]

I'm actually not sure (I think I wanted to make it so other people could use it but I'm not sure why that's necessary). Fixed.

[mrunalp]

LGTM

[crosbymichael]

I'm guessing this fixes moby/moby#22450 right?

[mrunalp]

Looks like it should.

On Nov 3, 2016, at 10:46 AM, Michael Crosby [email protected] wrote:

I'm guessing this fixes moby/moby#22450 right?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

[crosbymichael]

LGTM