From 103b5b66921b351f8db4fc6e83bf147b1a0d7580 Mon Sep 17 00:00:00 2001 From: Greg Thelen Date: Sat, 25 May 2019 11:44:44 -0700 Subject: [PATCH] syscall: use Ctty before fd shuffle On unix if exec.Command() is given both ExtraFiles and Ctty, and the Ctty file descriptor overlaps the range of FDs intended for the child, then cmd.Start() the ioctl(fd,TIOCSCTTY) call fails with an "inappropriate ioctl for device" error. When child file descriptors overlap the new child's ctty the ctty will be closed in the fd shuffle before the TIOCSCTTY. Thus TIOCSCTTY is used on one of the ExtraFiles rather than the intended Ctty file. Thus the error. exec.Command() callers can workaround this by ensuring the Ctty fd is larger than any ExtraFiles destined for the child. Fix this by doing the ctty ioctl before the fd shuffle. Test for this issue by modifying TestTerminalSignal to use more ExtraFiles. The test fails on linux and freebsd without this change's syscall/*.go changes. Other platforms (e.g. darwin, aix, solaris) have the same fd shuffle logic, so the same fix is applied to them. However, I was only able to test on linux (32 and 64 bit) and freebsd (64 bit). Manual runs of the test in https://golang.org/issue/29458 start passing with this patch: Before: % /tmp/src/go/bin/go run t successfully ran child process with ParentExtraFileFdNum=5, ChildExtraFileFd=6, ParentPtyFd=7 panic: failed to run child process with ParentExtraFileFdNum=10, ChildExtraFileFd=11, ParentPtyFd=11: fork/exec /bin/true: inappropriate ioctl for device After: % /tmp/src/go/bin/go run t successfully ran child process with ParentExtraFileFdNum=5, ChildExtraFileFd=6, ParentPtyFd=7 successfully ran child process with ParentExtraFileFdNum=10, ChildExtraFileFd=11, ParentPtyFd=11 Fixes #29458 Change-Id: I99513de7b6073c7eb855f1eeb4d1f9dc0454ef8b Reviewed-on: https://go-review.googlesource.com/c/go/+/178919 Run-TryBot: Brad Fitzpatrick TryBot-Result: Gobot Gobot Reviewed-by: Ian Lance Taylor --- src/os/signal/signal_cgo_test.go | 11 +++++++++ src/syscall/exec_bsd.go | 32 ++++++++++++------------ src/syscall/exec_darwin.go | 32 ++++++++++++------------ src/syscall/exec_libc.go | 42 ++++++++++++++++---------------- src/syscall/exec_linux.go | 32 ++++++++++++------------ 5 files changed, 80 insertions(+), 69 deletions(-) diff --git a/src/os/signal/signal_cgo_test.go b/src/os/signal/signal_cgo_test.go index 3c23090489..075e8c11cb 100644 --- a/src/os/signal/signal_cgo_test.go +++ b/src/os/signal/signal_cgo_test.go @@ -101,6 +101,17 @@ func TestTerminalSignal(t *testing.T) { Ctty: int(slave.Fd()), } + // Test ctty management by sending enough child fd to overlap the + // parent's fd intended for child's ctty. + for 2+len(cmd.ExtraFiles) < cmd.SysProcAttr.Ctty { + dummy, err := os.Open(os.DevNull) + if err != nil { + t.Fatal(err) + } + defer dummy.Close() + cmd.ExtraFiles = append(cmd.ExtraFiles, dummy) + } + if err := cmd.Start(); err != nil { t.Fatal(err) } diff --git a/src/syscall/exec_bsd.go b/src/syscall/exec_bsd.go index 30b88eba7a..632b711ce8 100644 --- a/src/syscall/exec_bsd.go +++ b/src/syscall/exec_bsd.go @@ -162,6 +162,22 @@ func forkAndExecInChild(argv0 *byte, argv, envv []*byte, chroot, dir *byte, attr } } + // Detach fd 0 from tty + if sys.Noctty { + _, _, err1 = RawSyscall(SYS_IOCTL, 0, uintptr(TIOCNOTTY), 0) + if err1 != 0 { + goto childerror + } + } + + // Set the controlling TTY to Ctty + if sys.Setctty { + _, _, err1 = RawSyscall(SYS_IOCTL, uintptr(sys.Ctty), uintptr(TIOCSCTTY), 0) + if err1 != 0 { + goto childerror + } + } + // Pass 1: look for fd[i] < i and move those up above len(fd) // so that pass 2 won't stomp on an fd it needs later. if pipe < nextfd { @@ -219,22 +235,6 @@ func forkAndExecInChild(argv0 *byte, argv, envv []*byte, chroot, dir *byte, attr RawSyscall(SYS_CLOSE, uintptr(i), 0, 0) } - // Detach fd 0 from tty - if sys.Noctty { - _, _, err1 = RawSyscall(SYS_IOCTL, 0, uintptr(TIOCNOTTY), 0) - if err1 != 0 { - goto childerror - } - } - - // Set the controlling TTY to Ctty - if sys.Setctty { - _, _, err1 = RawSyscall(SYS_IOCTL, uintptr(sys.Ctty), uintptr(TIOCSCTTY), 0) - if err1 != 0 { - goto childerror - } - } - // Time to exec. _, _, err1 = RawSyscall(SYS_EXECVE, uintptr(unsafe.Pointer(argv0)), diff --git a/src/syscall/exec_darwin.go b/src/syscall/exec_darwin.go index f860f4628e..a7af3afe94 100644 --- a/src/syscall/exec_darwin.go +++ b/src/syscall/exec_darwin.go @@ -160,6 +160,22 @@ func forkAndExecInChild(argv0 *byte, argv, envv []*byte, chroot, dir *byte, attr } } + // Detach fd 0 from tty + if sys.Noctty { + _, _, err1 = rawSyscall(funcPC(libc_ioctl_trampoline), 0, uintptr(TIOCNOTTY), 0) + if err1 != 0 { + goto childerror + } + } + + // Set the controlling TTY to Ctty + if sys.Setctty { + _, _, err1 = rawSyscall(funcPC(libc_ioctl_trampoline), uintptr(sys.Ctty), uintptr(TIOCSCTTY), 0) + if err1 != 0 { + goto childerror + } + } + // Pass 1: look for fd[i] < i and move those up above len(fd) // so that pass 2 won't stomp on an fd it needs later. if pipe < nextfd { @@ -217,22 +233,6 @@ func forkAndExecInChild(argv0 *byte, argv, envv []*byte, chroot, dir *byte, attr rawSyscall(funcPC(libc_close_trampoline), uintptr(i), 0, 0) } - // Detach fd 0 from tty - if sys.Noctty { - _, _, err1 = rawSyscall(funcPC(libc_ioctl_trampoline), 0, uintptr(TIOCNOTTY), 0) - if err1 != 0 { - goto childerror - } - } - - // Set the controlling TTY to Ctty - if sys.Setctty { - _, _, err1 = rawSyscall(funcPC(libc_ioctl_trampoline), uintptr(sys.Ctty), uintptr(TIOCSCTTY), 0) - if err1 != 0 { - goto childerror - } - } - // Time to exec. _, _, err1 = rawSyscall(funcPC(libc_execve_trampoline), uintptr(unsafe.Pointer(argv0)), diff --git a/src/syscall/exec_libc.go b/src/syscall/exec_libc.go index 0133139000..11cd2bb9f3 100644 --- a/src/syscall/exec_libc.go +++ b/src/syscall/exec_libc.go @@ -180,6 +180,27 @@ func forkAndExecInChild(argv0 *byte, argv, envv []*byte, chroot, dir *byte, attr } } + // Detach fd 0 from tty + if sys.Noctty { + err1 = ioctl(0, uintptr(TIOCNOTTY), 0) + if err1 != 0 { + goto childerror + } + } + + // Set the controlling TTY to Ctty + if sys.Setctty { + // On AIX, TIOCSCTTY is undefined + if TIOCSCTTY == 0 { + err1 = ENOSYS + goto childerror + } + err1 = ioctl(uintptr(sys.Ctty), uintptr(TIOCSCTTY), 0) + if err1 != 0 { + goto childerror + } + } + // Pass 1: look for fd[i] < i and move those up above len(fd) // so that pass 2 won't stomp on an fd it needs later. if pipe < nextfd { @@ -240,27 +261,6 @@ func forkAndExecInChild(argv0 *byte, argv, envv []*byte, chroot, dir *byte, attr close(uintptr(i)) } - // Detach fd 0 from tty - if sys.Noctty { - err1 = ioctl(0, uintptr(TIOCNOTTY), 0) - if err1 != 0 { - goto childerror - } - } - - // Set the controlling TTY to Ctty - if sys.Setctty { - // On AIX, TIOCSCTTY is undefined - if TIOCSCTTY == 0 { - err1 = ENOSYS - goto childerror - } - err1 = ioctl(uintptr(sys.Ctty), uintptr(TIOCSCTTY), 0) - if err1 != 0 { - goto childerror - } - } - // Time to exec. err1 = execve( uintptr(unsafe.Pointer(argv0)), diff --git a/src/syscall/exec_linux.go b/src/syscall/exec_linux.go index a2242b2057..f62f2c633e 100644 --- a/src/syscall/exec_linux.go +++ b/src/syscall/exec_linux.go @@ -431,6 +431,22 @@ func forkAndExecInChild1(argv0 *byte, argv, envv []*byte, chroot, dir *byte, att } } + // Detach fd 0 from tty + if sys.Noctty { + _, _, err1 = RawSyscall(SYS_IOCTL, 0, uintptr(TIOCNOTTY), 0) + if err1 != 0 { + goto childerror + } + } + + // Set the controlling TTY to Ctty + if sys.Setctty { + _, _, err1 = RawSyscall(SYS_IOCTL, uintptr(sys.Ctty), uintptr(TIOCSCTTY), 1) + if err1 != 0 { + goto childerror + } + } + // Pass 1: look for fd[i] < i and move those up above len(fd) // so that pass 2 won't stomp on an fd it needs later. if pipe < nextfd { @@ -488,22 +504,6 @@ func forkAndExecInChild1(argv0 *byte, argv, envv []*byte, chroot, dir *byte, att RawSyscall(SYS_CLOSE, uintptr(i), 0, 0) } - // Detach fd 0 from tty - if sys.Noctty { - _, _, err1 = RawSyscall(SYS_IOCTL, 0, uintptr(TIOCNOTTY), 0) - if err1 != 0 { - goto childerror - } - } - - // Set the controlling TTY to Ctty - if sys.Setctty { - _, _, err1 = RawSyscall(SYS_IOCTL, uintptr(sys.Ctty), uintptr(TIOCSCTTY), 1) - if err1 != 0 { - goto childerror - } - } - // Enable tracing if requested. // Do this right before exec so that we don't unnecessarily trace the runtime // setting up after the fork. See issue #21428. -- 2.50.0