x/crypto/ssh/terminal: wrapping logic sends a NL but no CR, resulting in misformatted output

[ziz]

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go version go1.7.1 linux/amd64

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/ziz/src/go"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0"
CXX="g++"
CGO_ENABLED="1"

What did you do?

If possible, provide a recipe for reproducing the error.
A complete runnable program is good.
A link on play.golang.org is best.

package main

import (
    "fmt"
    "os"
    "syscall"

    "golang.org/x/crypto/ssh/terminal"
)

func main() {
    oldTermState, err := terminal.MakeRaw(syscall.Stdin)
    if err != nil {
        panic(err)
    }
    defer terminal.Restore(syscall.Stdin, oldTermState)

    term := terminal.NewTerminal(os.Stdin, "Hello World> ")
    err = term.SetSize(50, 24)
    if err != nil {
        panic(err)
    }
    cmdline, err := term.ReadLine()
    // This will also stairstep, since we're still in raw mode at this point.
    fmt.Println(cmdline)
}

Run this program in a window that is more than 50 characters wide. Type or paste in several lines of text; I used the standard lorem ipsum text. (This also causes an issue when the terminal emulator is the width that crypto/ssh/terminal thinks it is - visible as an extra blank line between lines - but the effect is starker in an example with a larger difference between window size and term.SetSize size.)

What did you expect to see?

Expected: Input lines wrap correctly at the 50-character mark.

What did you see instead?

Actual: Lines newline but do not carriage return at the 50-character mark, leading to the classic stairstepped output:

This appears to be a consequence of #15625, the solution to which made the MakeRaw function behave more accurately - but now in Terminal.advanceCursor the lone \n is insufficient to return the cursor to column 0.

It is likely that sending \r\n rather than \n alone is the correct solution in light of the new MakeRaw behavior.

[unkaktus]

I confirm this issue. 911fafb introduced code that unsets OPOST flag. This leads to this weird behavior. It would be nice to have an option to set flags from other packages (like setting OPOST back to raw term) but field termios in State is unexported.

[bradfitz]

I wasn't aware of this bug and lazily worked around it recently in golang/net@fca93ed

@agl, is this still your package?

[bradfitz]

(I don't know enough of this to even know if this is a bug, which is part of why I worked around it, assuming it was my bug.)

[unkaktus]

btw disabled OPOST breaks outputs of log, fmt and panic() in the same way.

[agl]

Matching the behaviour of C when making a terminal raw does seem to be worthwhile; just to be consistent. It does mean that the backtrace from panic is going to be a bit of a mess though.

But a fix to terminal is certainly needed. I'll upload one in a sec.

[gopherbot]

CL https://golang.org/cl/33902 mentions this issue.

[unkaktus]

https://golang.org/cl/33902

@agl, what will happen with apps who already worked around this issue? \r\n seems to become \r\r\n, doesn't it?

IMO adding a method to set OPOST back (and offload postprocessing to terminal itself) is the best option. Thus apps will decide whether to switch it or not.

[agl]

Code that already worked around this will need to revert that change I'm afraid, but it shouldn't have baked in too deep at this point.

I don't want to start adding options unless there's no other option. It's crappy that I didn't realise that OPOST would have this effect when reviewing the change, but the code shouldn't have a scar forever because of that.