os/exec: avoid NewFile on unknown FDs

exec_test.go's init function uses os.NewFile(fd) + f.Stat as a portable
mechanism to determine if an FD is in use.

Unfortunately, the current use is racy: if an unused FD becomes used
between NewFile and f.Close, then we will unintentionally close an FD we
do not use.

We cannot simply drop Close, as the finalizer will close the FD. We
could hold all of the os.Files in a global for the lifetime of the
process, but the need for such a hack is indicative of the larger
problem: we should not create an os.File for an FD that we do not own.

Instead, the new fdtest.Exists provides a helper that performs the
equivalent of fstat(2) on each OS to determine if the FD is valid,
without using os.File.

We also reuse this helper on a variety of other tests that look at open
FDs.

Fixes #49533

Change-Id: I36e2bdb15f271ab01e55c18db6564271995a15af
Reviewed-on: https://go-review.googlesource.com/c/go/+/364035
Trust: Michael Pratt <mpratt@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
Reviewed-by: Bryan C. Mills <bcmills@google.com>

1 files changed, 41 insertions, 51 deletions

diff --git a/src/os/exec/read3.go b/src/os/exec/read3.go
index 8aae5735c4..10cbfbd54a 100644
--- a/src/os/exec/read3.go
+++ b/src/os/exec/read3.go
@@ -18,12 +18,15 @@ import (
 	"io"
 	"os"
 	"os/exec"
+	"os/exec/internal/fdtest"
 	"runtime"
 	"strings"
 )
 
 func main() {
 	fd3 := os.NewFile(3, "fd3")
+	defer fd3.Close()
+
 	bs, err := io.ReadAll(fd3)
 	if err != nil {
 		fmt.Printf("ReadAll from fd 3: %v\n", err)
@@ -37,65 +40,52 @@ func main() {
 	// descriptor from parent == 3
 	// All descriptors 4 and up should be available,
 	// except for any used by the network poller.
-	var files []*os.File
-	for wantfd := uintptr(4); wantfd <= 100; wantfd++ {
-		if poll.IsPollDescriptor(wantfd) {
+	for fd := uintptr(4); fd <= 100; fd++ {
+		if poll.IsPollDescriptor(fd) {
 			continue
 		}
-		f, err := os.Open(os.Args[0])
-		if err != nil {
-			fmt.Printf("error opening file with expected fd %d: %v", wantfd, err)
-			os.Exit(1)
+
+		if !fdtest.Exists(fd) {
+			continue
 		}
-		if got := f.Fd(); got != wantfd {
-			fmt.Printf("leaked parent file. fd = %d; want %d\n", got, wantfd)
-			fdfile := fmt.Sprintf("/proc/self/fd/%d", wantfd)
-			link, err := os.Readlink(fdfile)
-			fmt.Printf("readlink(%q) = %q, %v\n", fdfile, link, err)
-			var args []string
-			switch runtime.GOOS {
-			case "plan9":
-				args = []string{fmt.Sprintf("/proc/%d/fd", os.Getpid())}
-			case "aix", "solaris", "illumos":
-				args = []string{fmt.Sprint(os.Getpid())}
-			default:
-				args = []string{"-p", fmt.Sprint(os.Getpid())}
-			}
 
-			// Determine which command to use to display open files.
-			ofcmd := "lsof"
-			switch runtime.GOOS {
-			case "dragonfly", "freebsd", "netbsd", "openbsd":
-				ofcmd = "fstat"
-			case "plan9":
-				ofcmd = "/bin/cat"
-			case "aix":
-				ofcmd = "procfiles"
-			case "solaris", "illumos":
-				ofcmd = "pfiles"
-			}
+		fmt.Printf("leaked parent file. fdtest.Exists(%d) got true want false\n", fd)
+
+		fdfile := fmt.Sprintf("/proc/self/fd/%d", fd)
+		link, err := os.Readlink(fdfile)
+		fmt.Printf("readlink(%q) = %q, %v\n", fdfile, link, err)
 
-			cmd := exec.Command(ofcmd, args...)
-			out, err := cmd.CombinedOutput()
-			if err != nil {
-				fmt.Fprintf(os.Stderr, "%s failed: %v\n", strings.Join(cmd.Args, " "), err)
-			}
-			fmt.Printf("%s", out)
-			os.Exit(1)
+		var args []string
+		switch runtime.GOOS {
+		case "plan9":
+			args = []string{fmt.Sprintf("/proc/%d/fd", os.Getpid())}
+		case "aix", "solaris", "illumos":
+			args = []string{fmt.Sprint(os.Getpid())}
+		default:
+			args = []string{"-p", fmt.Sprint(os.Getpid())}
 		}
-		files = append(files, f)
-	}
 
-	for _, f := range files {
-		f.Close()
-	}
+		// Determine which command to use to display open files.
+		ofcmd := "lsof"
+		switch runtime.GOOS {
+		case "dragonfly", "freebsd", "netbsd", "openbsd":
+			ofcmd = "fstat"
+		case "plan9":
+			ofcmd = "/bin/cat"
+		case "aix":
+			ofcmd = "procfiles"
+		case "solaris", "illumos":
+			ofcmd = "pfiles"
+		}
 
-	// Referring to fd3 here ensures that it is not
-	// garbage collected, and therefore closed, while
-	// executing the wantfd loop above. It doesn't matter
-	// what we do with fd3 as long as we refer to it;
-	// closing it is the easy choice.
-	fd3.Close()
+		cmd := exec.Command(ofcmd, args...)
+		out, err := cmd.CombinedOutput()
+		if err != nil {
+			fmt.Fprintf(os.Stderr, "%s failed: %v\n", strings.Join(cmd.Args, " "), err)
+		}
+		fmt.Printf("%s", out)
+		os.Exit(1)
+	}
 
 	os.Stdout.Write(bs)
 }