From 5c3f42aa4e086fbdb5976ece09adc0a1e9e6ba79 Mon Sep 17 00:00:00 2001 From: Vitaly Potyarkin Date: Thu, 27 Apr 2023 14:00:42 +0000 Subject: [PATCH 1/6] Add tests to detect forever hanging Read() --- io_test.go | 102 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 102 insertions(+) create mode 100644 io_test.go diff --git a/io_test.go b/io_test.go new file mode 100644 index 0000000..f68956c --- /dev/null +++ b/io_test.go @@ -0,0 +1,102 @@ +package pty + +import ( + "testing" + + "context" + "os" + "sync" + "time" +) + +const ( + errMarker byte = 0xEE + timeout = time.Second +) + +var mu sync.Mutex + +// Check that SetDeadline() works for ptmx. +// Outstanding Read() calls must be interrupted by deadline. +// +// https://github.com/creack/pty/issues/162 +func TestReadDeadline(t *testing.T) { + ptmx, success := prepare(t) + + err := ptmx.SetDeadline(time.Now().Add(timeout / 10)) + if err != nil { + t.Fatalf("error: set deadline: %v\n", err) + } + + var buf = make([]byte, 1) + n, err := ptmx.Read(buf) + success() + + if n != 0 && buf[0] != errMarker { + t.Errorf("received unexpected data from pmtx (%d bytes): 0x%X; err=%v", n, buf, err) + } +} + +// Check that ptmx.Close() interrupts outstanding ptmx.Read() calls +// +// https://github.com/creack/pty/issues/114 +// https://github.com/creack/pty/issues/88 +func TestReadClose(t *testing.T) { + ptmx, success := prepare(t) + + go func() { + time.Sleep(timeout / 10) + err := ptmx.Close() + if err != nil { + t.Errorf("failed to close ptmx: %v", err) + } + }() + + var buf = make([]byte, 1) + n, err := ptmx.Read(buf) + success() + + if n != 0 && buf[0] != errMarker { + t.Errorf("received unexpected data from pmtx (%d bytes): 0x%X; err=%v", n, buf, err) + } +} + +// Open pty and setup watchdogs for graceful and not so graceful failure modes +func prepare(t *testing.T) (ptmx *os.File, done func()) { + + // Due to data race potential in (*os.File).Fd() + // we should never run these two tests in parallel + mu.Lock() + t.Cleanup(mu.Unlock) + + ptmx, pts, err := Open() + if err != nil { + t.Fatalf("error: open: %v\n", err) + } + t.Cleanup(func() { _ = ptmx.Close() }) + t.Cleanup(func() { _ = pts.Close() }) + + ctx, done := context.WithCancel(context.Background()) + go func() { + select { + case <-ctx.Done(): + // ptmx.Read() did not block forever, yay! + case <-time.After(timeout): + _, err := pts.Write([]byte{errMarker}) // unblock ptmx.Read() + if err != nil { + t.Errorf("failed to write to pts: %v", err) + } + t.Error("ptmx.Read() was not unblocked") + done() // cancel panic() + } + }() + go func() { + select { + case <-ctx.Done(): + // Test has either failed or succeeded; it definitely did not hang + case <-time.After(timeout * 10 / 9): // timeout +11% + panic("ptmx.Read() was not unblocked; avoid hanging forever") // just in case + } + }() + return ptmx, done +} From 291695431fdefc7e3d73745714cf3f03c3a0da8d Mon Sep 17 00:00:00 2001 From: Vitaly Potyarkin Date: Thu, 27 Apr 2023 15:04:19 +0000 Subject: [PATCH 2/6] Do not set file descriptor into blocking mode (*os.File).Fd() implicitly sets file descriptor into blocking mode [1], [2] We avoid that by calling (*os.File).SyscallConn instead. This method was added in go1.12 (released on 2019-02-25) [1]: https://pkg.go.dev/os#File.Fd [2]: https://cs.opensource.google/go/go/+/refs/tags/go1.20.3:src/os/file_unix.go;l=80-87 --- ioctl.go => ioctl_inner.go | 2 +- ioctl_outer.go | 24 ++++++++++++++++++++++ ioctl_solaris.go | 2 +- ioctl_unsupported.go | 2 +- pty_darwin.go | 6 +++--- pty_dragonfly.go | 10 +++++----- pty_freebsd.go | 8 ++++---- pty_linux.go | 4 ++-- pty_netbsd.go | 4 ++-- pty_openbsd.go | 2 +- pty_solaris.go | 41 ++++++++++++++++++++++++++++---------- winsize_unix.go | 4 ++-- 12 files changed, 76 insertions(+), 33 deletions(-) rename ioctl.go => ioctl_inner.go (85%) create mode 100644 ioctl_outer.go diff --git a/ioctl.go b/ioctl_inner.go similarity index 85% rename from ioctl.go rename to ioctl_inner.go index 3cabedd..fd5dbef 100644 --- a/ioctl.go +++ b/ioctl_inner.go @@ -10,7 +10,7 @@ const ( TIOCSWINSZ = syscall.TIOCSWINSZ ) -func ioctl(fd, cmd, ptr uintptr) error { +func ioctl_inner(fd, cmd, ptr uintptr) error { _, _, e := syscall.Syscall(syscall.SYS_IOCTL, fd, cmd, ptr) if e != 0 { return e diff --git a/ioctl_outer.go b/ioctl_outer.go new file mode 100644 index 0000000..4e26755 --- /dev/null +++ b/ioctl_outer.go @@ -0,0 +1,24 @@ +//go:build !windows +// +build !windows + +// +package pty + +import "os" + +func ioctl(f *os.File, cmd, ptr uintptr) error { + sc, e := f.SyscallConn() + if e != nil { + return ioctl_inner(f.Fd(), cmd, ptr) // fall back to blocking io (old behavior) + } + + ch := make(chan error, 1) + defer close(ch) + + e = sc.Control(func(fd uintptr) { ch <- ioctl_inner(fd, cmd, ptr) }) + if e != nil { + return e + } + e = <-ch + return e +} diff --git a/ioctl_solaris.go b/ioctl_solaris.go index bff22da..c0231df 100644 --- a/ioctl_solaris.go +++ b/ioctl_solaris.go @@ -40,7 +40,7 @@ type strioctl struct { // Defined in asm_solaris_amd64.s. func sysvicall6(trap, nargs, a1, a2, a3, a4, a5, a6 uintptr) (r1, r2 uintptr, err syscall.Errno) -func ioctl(fd, cmd, ptr uintptr) error { +func ioctl_inner(fd, cmd, ptr uintptr) error { if _, _, errno := sysvicall6(uintptr(unsafe.Pointer(&procioctl)), 3, fd, cmd, ptr, 0, 0, 0); errno != 0 { return errno } diff --git a/ioctl_unsupported.go b/ioctl_unsupported.go index 2449a27..79ad4c6 100644 --- a/ioctl_unsupported.go +++ b/ioctl_unsupported.go @@ -8,6 +8,6 @@ const ( TIOCSWINSZ = 0 ) -func ioctl(fd, cmd, ptr uintptr) error { +func ioctl_inner(fd, cmd, ptr uintptr) error { return ErrUnsupported } diff --git a/pty_darwin.go b/pty_darwin.go index 9bdd71d..eadf6ab 100644 --- a/pty_darwin.go +++ b/pty_darwin.go @@ -46,7 +46,7 @@ func open() (pty, tty *os.File, err error) { func ptsname(f *os.File) (string, error) { n := make([]byte, _IOC_PARM_LEN(syscall.TIOCPTYGNAME)) - err := ioctl(f.Fd(), syscall.TIOCPTYGNAME, uintptr(unsafe.Pointer(&n[0]))) + err := ioctl(f, syscall.TIOCPTYGNAME, uintptr(unsafe.Pointer(&n[0]))) if err != nil { return "", err } @@ -60,9 +60,9 @@ func ptsname(f *os.File) (string, error) { } func grantpt(f *os.File) error { - return ioctl(f.Fd(), syscall.TIOCPTYGRANT, 0) + return ioctl(f, syscall.TIOCPTYGRANT, 0) } func unlockpt(f *os.File) error { - return ioctl(f.Fd(), syscall.TIOCPTYUNLK, 0) + return ioctl(f, syscall.TIOCPTYUNLK, 0) } diff --git a/pty_dragonfly.go b/pty_dragonfly.go index aa916aa..12803de 100644 --- a/pty_dragonfly.go +++ b/pty_dragonfly.go @@ -45,17 +45,17 @@ func open() (pty, tty *os.File, err error) { } func grantpt(f *os.File) error { - _, err := isptmaster(f.Fd()) + _, err := isptmaster(f) return err } func unlockpt(f *os.File) error { - _, err := isptmaster(f.Fd()) + _, err := isptmaster(f) return err } -func isptmaster(fd uintptr) (bool, error) { - err := ioctl(fd, syscall.TIOCISPTMASTER, 0) +func isptmaster(f *os.File) (bool, error) { + err := ioctl(f, syscall.TIOCISPTMASTER, 0) return err == nil, err } @@ -68,7 +68,7 @@ func ptsname(f *os.File) (string, error) { name := make([]byte, _C_SPECNAMELEN) fa := fiodgnameArg{Name: (*byte)(unsafe.Pointer(&name[0])), Len: _C_SPECNAMELEN, Pad_cgo_0: [4]byte{0, 0, 0, 0}} - err := ioctl(f.Fd(), ioctl_FIODNAME, uintptr(unsafe.Pointer(&fa))) + err := ioctl(f, ioctl_FIODNAME, uintptr(unsafe.Pointer(&fa))) if err != nil { return "", err } diff --git a/pty_freebsd.go b/pty_freebsd.go index bcd3b6f..47afcfe 100644 --- a/pty_freebsd.go +++ b/pty_freebsd.go @@ -44,8 +44,8 @@ func open() (pty, tty *os.File, err error) { return p, t, nil } -func isptmaster(fd uintptr) (bool, error) { - err := ioctl(fd, syscall.TIOCPTMASTER, 0) +func isptmaster(f *os.File) (bool, error) { + err := ioctl(f, syscall.TIOCPTMASTER, 0) return err == nil, err } @@ -55,7 +55,7 @@ var ( ) func ptsname(f *os.File) (string, error) { - master, err := isptmaster(f.Fd()) + master, err := isptmaster(f) if err != nil { return "", err } @@ -68,7 +68,7 @@ func ptsname(f *os.File) (string, error) { buf = make([]byte, n) arg = fiodgnameArg{Len: n, Buf: (*byte)(unsafe.Pointer(&buf[0]))} ) - if err := ioctl(f.Fd(), ioctlFIODGNAME, uintptr(unsafe.Pointer(&arg))); err != nil { + if err := ioctl(f, ioctlFIODGNAME, uintptr(unsafe.Pointer(&arg))); err != nil { return "", err } diff --git a/pty_linux.go b/pty_linux.go index a3b368f..e12e2c8 100644 --- a/pty_linux.go +++ b/pty_linux.go @@ -40,7 +40,7 @@ func open() (pty, tty *os.File, err error) { func ptsname(f *os.File) (string, error) { var n _C_uint - err := ioctl(f.Fd(), syscall.TIOCGPTN, uintptr(unsafe.Pointer(&n))) //nolint:gosec // Expected unsafe pointer for Syscall call. + err := ioctl(f, syscall.TIOCGPTN, uintptr(unsafe.Pointer(&n))) //nolint:gosec // Expected unsafe pointer for Syscall call. if err != nil { return "", err } @@ -50,5 +50,5 @@ func ptsname(f *os.File) (string, error) { func unlockpt(f *os.File) error { var u _C_int // use TIOCSPTLCK with a pointer to zero to clear the lock - return ioctl(f.Fd(), syscall.TIOCSPTLCK, uintptr(unsafe.Pointer(&u))) //nolint:gosec // Expected unsafe pointer for Syscall call. + return ioctl(f, syscall.TIOCSPTLCK, uintptr(unsafe.Pointer(&u))) //nolint:gosec // Expected unsafe pointer for Syscall call. } diff --git a/pty_netbsd.go b/pty_netbsd.go index 2b20d94..dd5611d 100644 --- a/pty_netbsd.go +++ b/pty_netbsd.go @@ -47,7 +47,7 @@ func ptsname(f *os.File) (string, error) { * ioctl(fd, TIOCPTSNAME, &pm) == -1 ? NULL : pm.sn; */ var ptm ptmget - if err := ioctl(f.Fd(), uintptr(ioctl_TIOCPTSNAME), uintptr(unsafe.Pointer(&ptm))); err != nil { + if err := ioctl(f, uintptr(ioctl_TIOCPTSNAME), uintptr(unsafe.Pointer(&ptm))); err != nil { return "", err } name := make([]byte, len(ptm.Sn)) @@ -65,5 +65,5 @@ func grantpt(f *os.File) error { * from grantpt(3): Calling grantpt() is equivalent to: * ioctl(fd, TIOCGRANTPT, 0); */ - return ioctl(f.Fd(), uintptr(ioctl_TIOCGRANTPT), 0) + return ioctl(f, uintptr(ioctl_TIOCGRANTPT), 0) } diff --git a/pty_openbsd.go b/pty_openbsd.go index aada5e3..337c39f 100644 --- a/pty_openbsd.go +++ b/pty_openbsd.go @@ -36,7 +36,7 @@ func open() (pty, tty *os.File, err error) { defer p.Close() var ptm ptmget - if err := ioctl(p.Fd(), uintptr(ioctl_PTMGET), uintptr(unsafe.Pointer(&ptm))); err != nil { + if err := ioctl(p, uintptr(ioctl_PTMGET), uintptr(unsafe.Pointer(&ptm))); err != nil { return nil, nil, err } diff --git a/pty_solaris.go b/pty_solaris.go index 37f933e..4e22416 100644 --- a/pty_solaris.go +++ b/pty_solaris.go @@ -65,7 +65,7 @@ func open() (pty, tty *os.File, err error) { } func ptsname(f *os.File) (string, error) { - dev, err := ptsdev(f.Fd()) + dev, err := ptsdev(f) if err != nil { return "", err } @@ -84,12 +84,12 @@ func unlockpt(f *os.File) error { icLen: 0, icDP: nil, } - return ioctl(f.Fd(), I_STR, uintptr(unsafe.Pointer(&istr))) + return ioctl(f, I_STR, uintptr(unsafe.Pointer(&istr))) } func minor(x uint64) uint64 { return x & 0377 } -func ptsdev(fd uintptr) (uint64, error) { +func ptsdev(f *os.File) (uint64, error) { istr := strioctl{ icCmd: ISPTM, icTimeout: 0, @@ -97,14 +97,33 @@ func ptsdev(fd uintptr) (uint64, error) { icDP: nil, } - if err := ioctl(fd, I_STR, uintptr(unsafe.Pointer(&istr))); err != nil { + if err := ioctl(f, I_STR, uintptr(unsafe.Pointer(&istr))); err != nil { return 0, err } - var status syscall.Stat_t - if err := syscall.Fstat(int(fd), &status); err != nil { + var errors = make(chan error, 1) + var results = make(chan uint64, 1) + defer close(errors) + defer close(results) + + var err error + var sc syscall.RawConn + sc, err = f.SyscallConn() + if err != nil { + return 0, err + } + err = sc.Control(func(fd uintptr) { + var status syscall.Stat_t + if err := syscall.Fstat(int(fd), &status); err != nil { + results <- 0 + errors <- err + } + results <- uint64(minor(status.Rdev)) + errors <- nil + }) + if err != nil { return 0, err } - return uint64(minor(status.Rdev)), nil + return <-results, <-errors } type ptOwn struct { @@ -113,7 +132,7 @@ type ptOwn struct { } func grantpt(f *os.File) error { - if _, err := ptsdev(f.Fd()); err != nil { + if _, err := ptsdev(f); err != nil { return err } pto := ptOwn{ @@ -127,7 +146,7 @@ func grantpt(f *os.File) error { icLen: int32(unsafe.Sizeof(strioctl{})), icDP: unsafe.Pointer(&pto), } - if err := ioctl(f.Fd(), I_STR, uintptr(unsafe.Pointer(&istr))); err != nil { + if err := ioctl(f, I_STR, uintptr(unsafe.Pointer(&istr))); err != nil { return errors.New("access denied") } return nil @@ -145,8 +164,8 @@ func streamsPush(f *os.File, mod string) error { // but since we are not using libc or XPG4.2, we should not be // double-pushing modules - if err := ioctl(f.Fd(), I_FIND, uintptr(unsafe.Pointer(&buf[0]))); err != nil { + if err := ioctl(f, I_FIND, uintptr(unsafe.Pointer(&buf[0]))); err != nil { return nil } - return ioctl(f.Fd(), I_PUSH, uintptr(unsafe.Pointer(&buf[0]))) + return ioctl(f, I_PUSH, uintptr(unsafe.Pointer(&buf[0]))) } diff --git a/winsize_unix.go b/winsize_unix.go index 5d99c3d..ad98c8a 100644 --- a/winsize_unix.go +++ b/winsize_unix.go @@ -20,7 +20,7 @@ type Winsize struct { // Setsize resizes t to s. func Setsize(t *os.File, ws *Winsize) error { //nolint:gosec // Expected unsafe pointer for Syscall call. - return ioctl(t.Fd(), syscall.TIOCSWINSZ, uintptr(unsafe.Pointer(ws))) + return ioctl(t, syscall.TIOCSWINSZ, uintptr(unsafe.Pointer(ws))) } // GetsizeFull returns the full terminal size description. @@ -28,7 +28,7 @@ func GetsizeFull(t *os.File) (size *Winsize, err error) { var ws Winsize //nolint:gosec // Expected unsafe pointer for Syscall call. - if err := ioctl(t.Fd(), syscall.TIOCGWINSZ, uintptr(unsafe.Pointer(&ws))); err != nil { + if err := ioctl(t, syscall.TIOCGWINSZ, uintptr(unsafe.Pointer(&ws))); err != nil { return nil, err } return &ws, nil From 87d82d3af069ec9e7804be496a0bedfc3711594c Mon Sep 17 00:00:00 2001 From: Vitaly Potyarkin Date: Fri, 28 Apr 2023 09:26:11 +0000 Subject: [PATCH 3/6] Rename ioctl.go back This filename trick forces git to render previous commit diff in a more comprehensible form --- ioctl_outer.go => ioctl.go | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename ioctl_outer.go => ioctl.go (100%) diff --git a/ioctl_outer.go b/ioctl.go similarity index 100% rename from ioctl_outer.go rename to ioctl.go From 418593def3022a9a89bfbc503bbbe877b3757910 Mon Sep 17 00:00:00 2001 From: Vitaly Potyarkin Date: Fri, 28 Apr 2023 10:09:56 +0000 Subject: [PATCH 4/6] Restore support for older Go versions When compiled with go older than 1.12 creack/pty will not include a fix for blocking Read() and will be prone to data races - but at least it will work For more information see issues: #88 #114 #156 #162 --- io_test.go | 3 +++ ioctl.go | 5 ++--- ioctl_legacy.go | 10 ++++++++++ 3 files changed, 15 insertions(+), 3 deletions(-) create mode 100644 ioctl_legacy.go diff --git a/io_test.go b/io_test.go index f68956c..57fbd3d 100644 --- a/io_test.go +++ b/io_test.go @@ -1,3 +1,6 @@ +//go:build go1.12 +// +build go1.12 + package pty import ( diff --git a/ioctl.go b/ioctl.go index 4e26755..60ac9b8 100644 --- a/ioctl.go +++ b/ioctl.go @@ -1,7 +1,6 @@ -//go:build !windows -// +build !windows +//go:build !windows && go1.12 +// +build !windows,go1.12 -// package pty import "os" diff --git a/ioctl_legacy.go b/ioctl_legacy.go new file mode 100644 index 0000000..f00b1a1 --- /dev/null +++ b/ioctl_legacy.go @@ -0,0 +1,10 @@ +//go:build !windows && !go1.12 +// +build !windows,!go1.12 + +package pty + +import "os" + +func ioctl(f *os.File, cmd, ptr uintptr) error { + return ioctl_inner(f.Fd(), cmd, ptr) // fall back to blocking io (old behavior) +} From 75e52f11032651aed206bc5116bf6f2914c46a2a Mon Sep 17 00:00:00 2001 From: Vitaly Potyarkin Date: Fri, 28 Apr 2023 10:23:52 +0000 Subject: [PATCH 5/6] Do not fail if SetDeadline is not supported by OS --- io_test.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/io_test.go b/io_test.go index 57fbd3d..5884665 100644 --- a/io_test.go +++ b/io_test.go @@ -7,7 +7,9 @@ import ( "testing" "context" + "errors" "os" + "runtime" "sync" "time" ) @@ -28,7 +30,11 @@ func TestReadDeadline(t *testing.T) { err := ptmx.SetDeadline(time.Now().Add(timeout / 10)) if err != nil { - t.Fatalf("error: set deadline: %v\n", err) + if errors.Is(err, os.ErrNoDeadline) { + t.Skipf("deadline is not supported on %s/%s", runtime.GOOS, runtime.GOARCH) + } else { + t.Fatalf("error: set deadline: %v\n", err) + } } var buf = make([]byte, 1) @@ -80,6 +86,7 @@ func prepare(t *testing.T) (ptmx *os.File, done func()) { t.Cleanup(func() { _ = pts.Close() }) ctx, done := context.WithCancel(context.Background()) + t.Cleanup(done) go func() { select { case <-ctx.Done(): From 3abf458f02ea9f96d9c45f7e735aea501ae65ef8 Mon Sep 17 00:00:00 2001 From: Vitaly Potyarkin Date: Fri, 28 Apr 2023 11:26:34 +0000 Subject: [PATCH 6/6] Notify about intentionally using blocking io on Darwin --- io_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/io_test.go b/io_test.go index 5884665..0837d9e 100644 --- a/io_test.go +++ b/io_test.go @@ -72,6 +72,13 @@ func TestReadClose(t *testing.T) { // Open pty and setup watchdogs for graceful and not so graceful failure modes func prepare(t *testing.T) (ptmx *os.File, done func()) { + if runtime.GOOS == "darwin" { + t.Log("creack/pty uses blocking i/o on darwin intentionally:") + t.Log("> https://github.com/creack/pty/issues/52") + t.Log("> https://github.com/creack/pty/pull/53") + t.Log("> https://github.com/golang/go/issues/22099") + t.SkipNow() + } // Due to data race potential in (*os.File).Fd() // we should never run these two tests in parallel