Merge pull request #167 from sio/ioctl

Avoid calls to (*os.File).Fd() and operations on raw file descriptor ints

io_test.go

@@ -0,0 +1,119 @@
+//go:build go1.12
+// +build go1.12
+
+package pty
+
+import (
+	"testing"
+
+	"context"
+	"errors"
+	"os"
+	"runtime"
+	"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 {
+		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)
+	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()) {
+	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
+	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())
+	t.Cleanup(done)
+	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
+}

ioctl.go

@@ -1,19 +1,23 @@
-//go:build !windows && !solaris && !aix
-// +build !windows,!solaris,!aix
+//go:build !windows && go1.12
+// +build !windows,go1.12
 
 package pty
 
-import "syscall"
+import "os"
 
-const (
-	TIOCGWINSZ = syscall.TIOCGWINSZ
-	TIOCSWINSZ = syscall.TIOCSWINSZ
-)
+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)
 
-func ioctl(fd, cmd, ptr uintptr) error {
-	_, _, e := syscall.Syscall(syscall.SYS_IOCTL, fd, cmd, ptr)
-	if e != 0 {
+	e = sc.Control(func(fd uintptr) { ch <- ioctl_inner(fd, cmd, ptr) })
+	if e != nil {
 		return e
 	}
-	return nil
+	e = <-ch
+	return e
 }

ioctl_inner.go

@@ -0,0 +1,19 @@
+//go:build !windows && !solaris && !aix
+// +build !windows,!solaris,!aix
+
+package pty
+
+import "syscall"
+
+const (
+	TIOCGWINSZ = syscall.TIOCGWINSZ
+	TIOCSWINSZ = syscall.TIOCSWINSZ
+)
+
+func ioctl_inner(fd, cmd, ptr uintptr) error {
+	_, _, e := syscall.Syscall(syscall.SYS_IOCTL, fd, cmd, ptr)
+	if e != 0 {
+		return e
+	}
+	return nil
+}

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)
+}

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
 	}

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
 }

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)
 }

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
 	}

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
 	}
 

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.
 }

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)
 }

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
 	}