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

syscall: CL 178919 breaks use of github.com/kr/pty #32527

Closed
myitcv opened this issue Jun 10, 2019 · 9 comments
Closed

syscall: CL 178919 breaks use of github.com/kr/pty #32527

myitcv opened this issue Jun 10, 2019 · 9 comments

Comments

@myitcv
Copy link
Member

@myitcv myitcv commented Jun 10, 2019

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

$ go version
go version devel +103b5b6692 Thu May 30 22:02:03 2019 +0000 linux/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN="/home/myitcv/gostuff/src/github.com/myitcv/govim/cmd/govim/.bin"
GOCACHE="/home/myitcv/.cache/go-build"
GOENV="/home/myitcv/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/myitcv/gostuff"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/myitcv/gos"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/myitcv/gos/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/myitcv/gostuff/src/github.com/myitcv/govim/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build470307944=/tmp/go-build -gno-record-gcc-switches"

What did you do?

CL 178919 has broken my test setup with govim.

I'm ever so slightly out of my depth when it comes to understanding what's going on, so I'll simply describe the setup and offer to provide as many further details as necessary (if someone can give me pointers as to how) 😄!

  • I use github.com/kr/pty to start an instance of Vim
  • The call to pty.Start fails with:
fork/exec /home/myitcv/usr/vim/bin/vim: inappropriate ioctl for device

I haven't yet managed to create a smaller reproduction outside of govim, rather I wanted to report the issue first given how close we are to beta1.

cc @ianlancetaylor

@myitcv
Copy link
Member Author

@myitcv myitcv commented Jun 10, 2019

Forgot to cc @kr

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 10, 2019

When you call pty.Start, are you setting the Stdin, Stdout, and Stderr fields?

I don't quite see how github.com/kr/pty works today. It uses os.OpenFile to open the tty, but that will mark the file descriptor as close-on-exec. Maybe it worked before because before CL 178919 we dup'ed the ctty to stdin/stdout/stderr before calling TIOCSCTTY, and so the ctty designation carried over. After the CL we call TIOCSCTTY first, and maybe that doesn't survive the later dup'ing. Or something.

It might help to compare an strace -f of your program before and after CL 178919, to see what changed.

It might fix the problem if github.com/kr/pty were changed to use syscall.Open rather than os.OpenFile for the pty, so that it is not marked close-on-exec.

Edit: this analysis was flawed because I was not looking at the same version of the pty package that @myitcv was using.

@ianlancetaylor ianlancetaylor added this to the Go1.13 milestone Jun 10, 2019
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 10, 2019

@kr
Copy link
Contributor

@kr kr commented Jun 11, 2019

I'm happy to help with changes to github.com/kr/pty if necessary, especially if there are steps we can follow to reproduce this.

Also cc @creack.

@creack
Copy link
Contributor

@creack creack commented Jun 11, 2019

Happy to help, however, I cloned and installed 103b5b6 on linux/amd64 (ubuntu 16.04) and have not been able to reproduce.

I tried vim, bash, emacs, tmux using the sample code form kr/pty readme without a problem.
I tried tried govim, but I think I'm doing something wrong as it didn't start anything in a pty..

Could you provide a more detailed way to reproduce?

Using syscall.Open instead of os.OpenFile could be done as well.

For reference, here is what I tested with, from https://github.com/kr/pty/blob/master/README.md#shell

package main

import (
        "io"
        "log"
        "os"
        "os/exec"
        "os/signal"
        "syscall"

        "github.com/kr/pty"
        "golang.org/x/crypto/ssh/terminal"
)

func test() error {
        // Create arbitrary command.
        c := exec.Command("vim")
        // Tried this and variations.
        c.SysProcAttr = &syscall.SysProcAttr{
                Setsid:     true,
                Setctty:    true,
        }

        // Start the command with a pty.
        ptmx, err := pty.Start(c)
        if err != nil {
                return err
        }
        // Make sure to close the pty at the end.
        defer func() { _ = ptmx.Close() }() // Best effort.

        // Handle pty size.
        ch := make(chan os.Signal, 1)
        signal.Notify(ch, syscall.SIGWINCH)
        go func() {
                for range ch {
                        if err := pty.InheritSize(os.Stdin, ptmx); err != nil {
                                log.Printf("error resizing pty: %s", err)
                        }
                }
        }()
        ch <- syscall.SIGWINCH // Initial resize.

        // Set stdin in raw mode.
        oldState, err := terminal.MakeRaw(int(os.Stdin.Fd()))
        if err != nil {
                panic(err)
        }
        defer func() { _ = terminal.Restore(int(os.Stdin.Fd()), oldState) }() // Best effort.

        // Copy stdin to the pty and the pty to stdout.
        go func() { _, _ = io.Copy(ptmx, os.Stdin) }()
        _, _ = io.Copy(os.Stdout, ptmx)

        return nil
}

func main() {
        if err := test(); err != nil {
                log.Fatal(err)
        }
}
@gthelen
Copy link
Contributor

@gthelen gthelen commented Jun 11, 2019

I'm also happy to help, but have no problems running creack's vim workload (#32527 (comment)) with golang 103b5b6 on linux/amd64.

@myitcv
Copy link
Member Author

@myitcv myitcv commented Jun 11, 2019

Thanks very much for all the offers of help debugging this.

I've not actually managed to create a repro that shows exactly the same symptoms, but I do have a small example that fails pre/post that CL in a slightly different but perhaps related way. That follows at the end of this comment.

In what follows I will use:

  • "pre" to refer to a53b465, the commit immediately prior to CL 178919
  • "post" to refer to 103b5b6, the commit that resulted from the merge of CL 178919

I've also tried to keep this issue a bit shorter by putting details in separate repo.

Vim version details for all my tests, attempted repros etc: 00 - vim version

Original issue

Per @ianlancetaylor I've used strace -f on the original problem. This was triggered as part of a test, so I've compiled the test binary and run strace -f on that; the issue remains (but I mention that for full disclosure on the setup).

$ go test -c -o test.prog
$ strace -f ./test.prog -test.run TestScripts/scripts/autocmd

In answer to your specific question, @ianlancetaylor :

When you call pty.Start, are you setting the Stdin, Stdout, and Stderr fields?

No; simply setting a working directory and environment.

Please excuse the code in testdriver; it's horrendous and needs a rewrite.

Small example

The simple example in https://github.com/myitcvscratch/ptyrepro also breaks as a result of CL 178919, but not in the same way.

Pre:

$ go run main.go

No output; i.e. no panic.

Post:

panic: fork/exec /home/myitcv/usr/vim/bin/vim: operation not permitted

goroutine 1 [running]:
main.main()
        /home/myitcv/dev/ptyrepro/main.go:15 +0xdb
exit status 2

Here is the strace -f output for both:

Again, happy to help provide any further details that help to track this down.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 11, 2019

Thanks for the strace listings and the small repro.

When I go run your repro, it downloads pty 1.1.4. That is missing change kr/pty@b6e1bdd which sets the Ctty field in SysProcAttr. The effect is that it always sets the controlling terminal to file descriptor 0. Before CL 178919, that worked for your test case because setting the ctty ran after dup'ing the pty to descriptor 0. After CL 178919, it fails, because before the dup'ing descriptor 0 is not a pty.

If I change the go.mod file in your small repro to

require github.com/kr/pty b6e1bdd4a4f88614e0c6e5e8089c7abed98aae17

then your program succeeds.

@myitcv
Copy link
Member Author

@myitcv myitcv commented Jun 11, 2019

@ianlancetaylor - thank you for taking the time to look into this, very much appreciated.

I can confirm that with kr/pty@b6e1bdd both my repro and the original issue are resolved.

I will close this issue and open an issue in https://github.com/kr/pty about cutting a new release.

@myitcv myitcv closed this Jun 11, 2019
myitcv added a commit to govim/govim that referenced this issue Jun 11, 2019
See golang/go#32527 for details
myitcv added a commit to govim/govim that referenced this issue Jun 11, 2019
See golang/go#32527 for details
myitcv added a commit to govim/govim that referenced this issue Jun 11, 2019
See golang/go#32527 for details
@golang golang locked and limited conversation to collaborators Jun 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.