Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/crypto/ssh/terminal: terminal using syscall.TCGETS (wrong for ppc64le) #21293

Closed
clnperez opened this issue Aug 3, 2017 · 4 comments
Closed
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@clnperez
Copy link

clnperez commented Aug 3, 2017

Please answer these questions before submitting your issue. Thanks!

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

master

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

go env
GOARCH="ppc64le"
GOBIN=""
GOEXE=""
GOHOSTARCH="ppc64le"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/christy/work"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/linux_ppc64le"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build365776170=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"

What did you do?

This surfaced via logrus with sirupsen/logrus#604

I hit this when revendoring logrus into moby/moby. I don't have a reproducer but it's a known issue that syscall.TCGETS and syscall.TCSETS are incorrect for ppc64le. I've been slowly trying to move projects over to use x/sys/.

I have a hacky test fix here: moby/moby#34332. Specifically, the windows bits were tricky (for me) to test because I don't have easy access to Windows.

When I have more time I can upstream the go bits unless someone else can get to it first.

@gopherbot gopherbot added this to the Unreleased milestone Aug 3, 2017
@bradfitz
Copy link
Contributor

bradfitz commented Aug 3, 2017

At crypto's master I see:

$ git grep TCGETS
ssh/terminal/util_linux.go:const ioctlReadTermios = 0x5401  // syscall.TCGETS
ssh/terminal/util_solaris.go:   val, err := unix.IoctlGetTermios(fd, unix.TCGETS)
ssh/terminal/util_solaris.go:   oldTermiosPtr, err := unix.IoctlGetTermios(fd, unix.TCGETS)
ssh/terminal/util_solaris.go:   oldTermiosPtr, err := unix.IoctlGetTermios(fd, unix.TCGETS)

It looks like util_linux.go uses a hard-coded value. I guess that's wrong on ppc64le?

$ git grep -E '\bTCGETS\b'
zerrors_linux_386.go:   TCGETS                               = 0x5401
zerrors_linux_amd64.go: TCGETS                               = 0x5401
zerrors_linux_arm.go:   TCGETS                               = 0x5401
zerrors_linux_arm64.go: TCGETS                               = 0x5401
zerrors_linux_mips.go:  TCGETS                               = 0x540d
zerrors_linux_mips64.go:        TCGETS                               = 0x540d
zerrors_linux_mips64le.go:      TCGETS                               = 0x540d
zerrors_linux_mipsle.go:        TCGETS                               = 0x540d
zerrors_linux_ppc64.go: TCGETS                               = 0x402c7413
zerrors_linux_ppc64le.go:       TCGETS                               = 0x402c7413
zerrors_linux_s390x.go: TCGETS                               = 0x5401
zerrors_linux_sparc64.go:       TCGETS                           = 0x40245408
zerrors_solaris_amd64.go:       TCGETS                        = 0x540d

Oh, yup.

That's easy. CL on its way.

/cc @tklauser

@gopherbot
Copy link

Change https://golang.org/cl/53073 mentions this issue: ssh/terminal: use TCGETS and TCSETS for Linux from the x/sys/unix package

@bradfitz bradfitz changed the title x/crypto: terminal using syscall.TCGETS (wrong for ppc64le) x/crypto/ssh/terminal: terminal using syscall.TCGETS (wrong for ppc64le) Aug 3, 2017
@bradfitz bradfitz self-assigned this Aug 3, 2017
@bradfitz bradfitz added the NeedsFix The path to resolution is known, but the work has not been done. label Aug 3, 2017
@tklauser
Copy link
Member

tklauser commented Aug 3, 2017

Should be fixed by https://golang.org/cl/51690 (commit golang/crypto@558b687) already.

@bradfitz
Copy link
Contributor

bradfitz commented Aug 3, 2017

@tklauser, hah. Whoops.

And now I see Gerrit said for mine:

Marking change merged without cherry-picking to branch, as the resulting commit would be empty.

I guess I hadn't synced either when I sent my change. Whoops.

@bradfitz bradfitz closed this as completed Aug 3, 2017
@golang golang locked and limited conversation to collaborators Aug 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants