syscall: exec_linux: switch to F_DUPFD_CLOEXEC in clobber-prevention logic

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.

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