syscall: ExtraFiles FD used to set controlling TTY instead of SysProcAttr.Ctty field #29458
Description
What did you do?
In some corner cases, cmd/exec will start a child process with a controlling tty FD from the ExtraFiles field of Cmd instead of the FD specified in SysProcAttr.Ctty.
This occurs when the FD number of SysProcAttr.Ctty in the parent is 0, 1, 2 or 3+i, where i is an index that has been populated in the ExtraFiles field of the Cmd struct.
This happens because the dup2 loop of forkAndExecInChild1 runs before the ioctl calls to set the Ctty. When making the ioctl calls, it's still is using the Ctty FD value passed in from the parent, so if the child process happens to dup2 over that FD value, the Ctty passed from the parent is closed and the child's ExtraFile FD is used instead of the Ctty FD configured by the parent. This usually results in a ENOTTY error (unless the ExtraFile happens to also be a TTY).
The following reproducing code (compiled with CGo enabled, on Linux w/ glibc) first creates a child process where the bug doesn't occur and then one where the bug does occur. When the bug occurs, forkAndExecInChild1 will incorrectly use the parent's FD 10 (/dev/null) when making the ioctl to setup the ctty even though SysProcAttr configured it to use the parent's FD 11.
package main
import (
/*
#include <pty.h>
#cgo LDFLAGS: -lutil
*/
"C"
"fmt"
"os"
"os/exec"
"syscall"
)
func main() {
// Case with expected behavior.
// The parent process opens files with descriptors set to:
// * 5 -> /dev/null
// * 7 -> PTY (from under /dev/pts/)
// The child process is configured with the following valid mappings from the parent
// * 6 in the child -> 5 in the parent (/dev/null)
// * Ctty in the child -> 7 in the parent (PTY)
runChildWithCtty(5, 6, 7)
// Case where the bug occurs (child process fails to start with ENOTTY).
// The parent process opens files with descriptors set to:
// * 10 -> /dev/null
// * 11 -> PTY (from under /dev/pts/)
// The child process is configured with the following valid mappings from the parent
// * 11 in the child -> 10 in the parent (/dev/null)
// * Ctty in the child -> 11 in the parent (PTY)
runChildWithCtty(10, 11, 11)
}
// Run a child process (arbitrarily /bin/true) with
// * An ExtraFile where the FD is parentExtraFileFdNum in the parent and will be set to childExtraFileFdNum in the child
// * A Ctty where the PTY has FD num set to parentPtyFdNum
func runChildWithCtty(parentExtraFileFdNum int, childExtraFileFdNum int, parentPtyFdNum int) {
childCmd := exec.Command("/bin/true")
childCmd.ExtraFiles = make([]*os.File, childExtraFileFdNum-2)
childCmd.ExtraFiles[childExtraFileFdNum-3] = openNormalFileAtFd(parentExtraFileFdNum)
childCmd.SysProcAttr = &syscall.SysProcAttr{
Setsid: true,
Setctty: true,
Ctty: openPtyAtFd(parentPtyFdNum),
}
err := childCmd.Run()
if err != nil {
panic(fmt.Sprintf("failed to run child process with ParentExtraFileFdNum=%d, ChildExtraFileFd=%d, ParentPtyFd=%d: %v", parentExtraFileFdNum, childExtraFileFdNum, parentPtyFdNum, err))
} else {
fmt.Printf("successfully ran child process with ParentExtraFileFdNum=%d, ChildExtraFileFd=%d, ParentPtyFd=%d\n\n", parentExtraFileFdNum, childExtraFileFdNum, parentPtyFdNum)
}
}
// open a pty up and dup2 it to the requested FD num
func openPtyAtFd(wantedFd int) int {
m := C.int(0)
s := C.int(0)
_, err := C.openpty(&m, &s, nil, nil, nil)
if err != nil {
panic(fmt.Sprintf("failed to open pty: %v", err))
}
goS := int(s)
if goS != wantedFd {
err = syscall.Dup2(goS, wantedFd)
if err != nil {
panic(fmt.Sprintf("failed to dup2: %v", err))
}
syscall.Close(goS)
}
return wantedFd
}
// open /dev/null and dup2 it to the requested FD num
func openNormalFileAtFd(wantedFd int) *os.File {
f, err := os.Open(os.DevNull)
if err != nil {
panic(fmt.Sprintf("failed to open devnull: %v", err))
}
actualFd := int(f.Fd())
if actualFd != wantedFd {
err = syscall.Dup2(actualFd, wantedFd)
if err != nil {
panic(fmt.Sprintf("failed to dup2: %v", err))
}
f.Close()
return os.NewFile(uintptr(wantedFd), f.Name())
} else {
return f
}
}
This might be in something of a grey area as to whether it's a bug or expected behavior, but I'd consider this a bug because:
- When I configure the
SysProcAttr.Cttyfield, I am providing an FD of the parent, so it's very surprising that in some corner cases it ends up instead being a reference to an FD in the child process - To workaround this behavior, our codebase in which this was originally encountered now has to jump through a bunch of hoops to check that an FD number of the parent (the pty) never accidentally has the same value of completely unrelated FDs (set in
ExtraFiles) that will be set in the child. It seems like the whole point of the interface given bycmd/execis to make the FD numbers of the parent and child independent of one another and only related by a mapping, saving users from having to think about corner cases of FD inheritance.- We don't have direct control over the initial FD values returns by various syscalls, so we end up having to either
dupFDs in a loop until they no longer conflict between the Ctty field and ExtraFiles or make the FD numbers of our child process ExtraFiles dynamically configurable. Neither is ideal.
- We don't have direct control over the initial FD values returns by various syscalls, so we end up having to either
What did you expect to see?
I expected both cases in the reproducing code to work, making the ioctl call to set the ctty using the SysProcAttr field as configured in the parent process.
Running strace -f -b execve -e 'trace=desc' ./main, this is the (filtered) output in the working case:
[pid 15680] openat(AT_FDCWD, "/dev/null", O_RDONLY|O_CLOEXEC) = 3
[pid 15680] dup2(3, 5) = 5
[pid 15680] close(3) = 0
[pid 15680] open("/dev/ptmx", O_RDWR) = 3
[pid 15680] close(7) = 0
[pid 15680] close(6) = 0
[pid 15680] close(6) = 0
[pid 15680] open("/dev/pts/13", O_RDWR|O_NOCTTY) = 6
[pid 15680] dup2(6, 7) = 7
[pid 15680] close(6) = 0
[pid 15680] openat(AT_FDCWD, "/dev/null", O_RDONLY|O_CLOEXEC) = 6
[pid 15680] openat(AT_FDCWD, "/dev/null", O_WRONLY|O_CLOEXEC) = 8
[pid 15680] openat(AT_FDCWD, "/dev/null", O_WRONLY|O_CLOEXEC) = 9
[pid 15680] pipe2([10, 11], O_CLOEXEC) = 0
Process 15687 attached
[pid 15687] dup2(5, 10) = 10
[pid 15687] dup2(6, 0) = 0
[pid 15687] dup2(8, 1) = 1
[pid 15687] dup2(9, 2) = 2
[pid 15687] close(3) = 0
[pid 15687] close(4) = 0
[pid 15687] close(5) = 0
[pid 15687] dup2(10, 6) = 6
[pid 15687] ioctl(7, TIOCSCTTY, 1) = 0
What did you see instead?
In the case where the bug occurs, this is the filtered strace output:
[pid 15680] openat(AT_FDCWD, "/dev/null", O_RDONLY|O_CLOEXEC) = 6
[pid 15680] dup2(6, 10) = 10
[pid 15680] close(6) = 0
[pid 15680] open("/dev/ptmx", O_RDWR) = 6
[pid 15680] open("/dev/pts/14", O_RDWR|O_NOCTTY) = 8
[pid 15680] dup2(8, 11) = 11
[pid 15680] close(8) = 0
[pid 15680] openat(AT_FDCWD, "/dev/null", O_RDONLY|O_CLOEXEC <unfinished ...>
[pid 15680] <... openat resumed> ) = 8
[pid 15680] openat(AT_FDCWD, "/dev/null", O_WRONLY|O_CLOEXEC) = 9
[pid 15680] openat(AT_FDCWD, "/dev/null", O_WRONLY|O_CLOEXEC) = 12
[pid 15680] pipe2([13, 14], O_CLOEXEC) = 0
Process 15688 attached
[pid 15688] dup2(10, 13) = 13
[pid 15688] dup2(8, 0) = 0
[pid 15688] dup2(9, 1) = 1
[pid 15688] dup2(12, 2) = 2
[pid 15688] close(3) = 0
[pid 15688] close(4) = 0
[pid 15688] close(5) = 0
[pid 15688] close(6) = 0
[pid 15688] close(7) = 0
[pid 15688] close(8) = 0
[pid 15688] close(9) = 0
[pid 15688] close(10) = 0
[pid 15688] dup2(13, 11) = 11
[pid 15688] ioctl(11, TIOCSCTTY, 1) = -1 ENOTTY (Inappropriate ioctl for device)
Even though the child process does call the ioctl on FD 11 at the end, 11 was previously overwritten by a dup2 call (from 13, which itself was dup2'd from 10, which is a FD configured in ExtraFiles, not Ctty).
So the end effect is that the parent's FD 10 from ExtraFiles was used as the Ctty instead of the parent's FD 11.
Does this issue reproduce with the latest release (go1.11.4)?
Yes
System details
go version go1.11.4 linux/amd64
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/sipsma/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/sipsma/go"
GOPROXY=""
GORACE=""
GOROOT="/local/home/sipsma/go"
GOTMPDIR=""
GOTOOLDIR="/local/home/sipsma/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
GOROOT/bin/go version: go version go1.11.4 linux/amd64
GOROOT/bin/go tool compile -V: compile version go1.11.4
uname -sr: Linux 4.9.124-0.1.ac.198.73.329.metal1.x86_64
/lib64/libc.so.6: GNU C Library stable release version 2.12, by Roland McGrath et al.
gdb --version: GNU gdb (GDB) Amazon Linux (7.2-50.11.amzn1)