diff --git a/src/syscall/exec_linux.go b/src/syscall/exec_linux.go index dfbb38ac1678c7..42c81aca73d3c9 100644 --- a/src/syscall/exec_linux.go +++ b/src/syscall/exec_linux.go @@ -227,7 +227,6 @@ func forkAndExecInChild1(argv0 *byte, argv, envv []*byte, chroot, dir *byte, att // by an otherwise-correct change in the compiler. var ( err2 Errno - nextfd int i int caps caps fd1, flags uintptr @@ -263,18 +262,11 @@ func forkAndExecInChild1(argv0 *byte, argv, envv []*byte, chroot, dir *byte, att // Record parent PID so child can test if it has died. ppid, _ := rawSyscallNoError(SYS_GETPID, 0, 0, 0) - // Guard against side effects of shuffling fds below. - // Make sure that nextfd is beyond any currently open files so - // that we can't run the risk of overwriting any of them. + // Used to guard against side effects of shuffling fds below. fd := make([]int, len(attr.Files)) - nextfd = len(attr.Files) for i, ufd := range attr.Files { - if nextfd < int(ufd) { - nextfd = int(ufd) - } fd[i] = int(ufd) } - nextfd++ // Allocate another pipe for parent to child communication for // synchronizing writing of User ID/Group ID mappings. @@ -541,28 +533,26 @@ func forkAndExecInChild1(argv0 *byte, argv, envv []*byte, chroot, dir *byte, att } } - // 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 { - _, _, err1 = RawSyscall(SYS_DUP3, uintptr(pipe), uintptr(nextfd), O_CLOEXEC) - if err1 != 0 { - goto childerror - } - pipe = nextfd - nextfd++ - } + // Pass 1: duplicate any fd[i] < i as O_CLOEXEC to make sure we don't + // clobber them when we shuffle the fds to match the attr.Files layout. for i = 0; i < len(fd); i++ { if fd[i] >= 0 && fd[i] < i { - if nextfd == pipe { // don't stomp on pipe - nextfd++ - } - _, _, err1 = RawSyscall(SYS_DUP3, uintptr(fd[i]), uintptr(nextfd), O_CLOEXEC) + fd1, _, err1 = RawSyscall(fcntl64Syscall, uintptr(fd[i]), F_DUPFD_CLOEXEC, 0) if err1 != 0 { goto childerror } - fd[i] = nextfd - nextfd++ + fd[i] = int(fd1) + } + } + + // Guard pipe against being clobbered in the second pass by making sure + // it's greater than any fd we are about to dup3() on top of. + if pipe < len(fd) { + fd1, _, err1 = RawSyscall(fcntl64Syscall, uintptr(pipe), F_DUPFD_CLOEXEC, 0) + if err1 != 0 { + goto childerror } + pipe = int(fd1) } // Pass 2: dup fd[i] down onto i. diff --git a/src/syscall/exec_linux_test.go b/src/syscall/exec_linux_test.go index f894bbaae92399..9c270d447f6fe3 100644 --- a/src/syscall/exec_linux_test.go +++ b/src/syscall/exec_linux_test.go @@ -8,6 +8,7 @@ package syscall_test import ( "bytes" + "errors" "flag" "fmt" "internal/testenv" @@ -673,3 +674,78 @@ func testAmbientCaps(t *testing.T, userns bool) { t.Fatal(err.Error()) } } + +// Test to ensure that ForkExec doesn't clobber file descriptors not included +// in cmd.ExtraFiles. See https://golang.org/issue/61751. +func TestExtraFilesNoClobber(t *testing.T) { + testenv.MustHaveExec(t) + + if os.Getenv("GO_WANT_HELPER_PROCESS") == "1" { + // Successfully managed to self-exec! + os.Exit(0) + } + + exe, err := os.Executable() + if err != nil { + t.Fatal(err) + } + + // Grab a handle to our /proc/self/exe. + exeFileOriginal, err := os.Open(exe) + if err != nil { + t.Fatal(err) + } + defer exeFileOriginal.Close() + + // A file we cannot execute. + devNullOriginal, err := os.Open("/dev/null") + if err != nil { + t.Fatal(err) + } + defer devNullOriginal.Close() + if _, err := exec.LookPath(devNullOriginal.Name()); err == nil { + t.Skip("skipping test -- /dev/null is executable") + } + + // Change the file descriptors such that devNull is a large descriptor and + // exeFile is one higher. Before https://golang.org/cl/515799, this would + // cause ForkExec to clobber the descriptor. + // + // Unfortunately we can't really have a generic test for clobbering because + // we can only detect the clobbering of a single file descriptor using this + // method. It might be possible use {Ptrace: true} to detect clobbering but + // the behaviour of F_DUPFD_CLOEXEC is not guaranteed (and you never know + // if some other test has opened a file, throwing off the fd calculations). + devNullFd := 9000 + exeFileFd := devNullFd + 1 + + if err := syscall.Dup3(int(devNullOriginal.Fd()), devNullFd, syscall.O_CLOEXEC); err != nil { + t.Fatalf("dup %s to %d failed: %v", devNullOriginal.Name(), devNullFd, err) + } + devNull := os.NewFile(uintptr(devNullFd), "/dev/null (dup'd)") + defer devNull.Close() + + if err := syscall.Dup3(int(exeFileOriginal.Fd()), exeFileFd, syscall.O_CLOEXEC); err != nil { + t.Fatalf("dup %s to %d failed: %v", exeFileOriginal.Name(), exeFileFd, err) + } + exeFile := os.NewFile(uintptr(exeFileFd), exeFileOriginal.Name()+" (dup'd)") + defer exeFile.Close() + + // Try to run exeFile through /proc/self/fd/$n. + exePath := fmt.Sprintf("/proc/self/fd/%d", exeFile.Fd()) + if _, err := os.Stat(exePath); err != nil { + t.Skipf("skipping test -- cannot resolve %s", exePath) + } + cmd := testenv.Command(t, exePath, "-test.run="+t.Name()) + cmd.Env = append(cmd.Environ(), "GO_WANT_HELPER_PROCESS=1") + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + cmd.ExtraFiles = []*os.File{devNull} + if err := cmd.Run(); err != nil { + if errors.Is(err, os.ErrPermission) { + t.Fatalf("fd %d was clobbered during exec: execve %s: %v", exeFileFd, exePath, err) + } + t.Fatal(err) + } + runtime.KeepAlive(exeFile) +}