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 <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

diff --git a/src/os/signal/signal_cgo_test.go b/src/os/signal/signal_cgo_test.go
index 3c23090489f953dd89260208eccf12a864a1e9cc..075e8c11cb3f664f8e2e4f16512c0cedea26a71d 100644 (file)
--- 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 30b88eba7a5b301d839cf74841f944758ef85a1e..632b711ce8f852fb8944af27cd9e0c6866ecea26 100644 (file)
--- 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 f860f4628ebe2ce700b31927471970e308ddf44c..a7af3afe94397d79b23a8d2070f86cc81b6c8903 100644 (file)
--- 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 0133139000b69b1cbdf382f366625809ad267e79..11cd2bb9f379b0107a3431488f61b4d42c09f159 100644 (file)
--- 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 a2242b2057cc3cc9d21865fe4282f957bb089ca5..f62f2c633edb8d2cc3a7f44ff513d520e9c781f6 100644 (file)
--- 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.