os/exec: don't run TestExtraFiles if extra files were open for the test

Our attempts to close existing open files are flaky. They will fail if,
for example, file descriptor 3 is open when the test binary starts.
Instead, report any such cases, and skip TestExtraFiles.

Updates #35469

Change-Id: I7caec083f3f4a31579bf28fc9c82ae89b1bde49a
Reviewed-on: https://go-review.googlesource.com/c/go/+/206939
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>

diff --git a/src/os/exec/exec_test.go b/src/os/exec/exec_test.go
index 19d2111743e42c6db319d55a7b56cec8f124319c..0498c7d9150bb79b994f8cfafade75db43341a1c 100644 (file)
--- a/src/os/exec/exec_test.go
+++ b/src/os/exec/exec_test.go
@@ -30,6 +30,42 @@ import (
        "time"
 )
 
+// haveUnexpectedFDs is set at init time to report whether any
+// file descriptors were open at program start.
+var haveUnexpectedFDs bool
+
+// unfinalizedFiles holds files that should not be finalized,
+// because that would close the associated file descriptor,
+// which we don't want to do.
+var unfinalizedFiles []*os.File
+
+func init() {
+       if os.Getenv("GO_WANT_HELPER_PROCESS") == "1" {
+               return
+       }
+       if runtime.GOOS == "windows" {
+               return
+       }
+       for fd := uintptr(3); fd <= 100; fd++ {
+               // We have no good portable way to check whether an FD is open.
+               // We use NewFile to create a *os.File, which lets us
+               // know whether it is open, but then we have to cope with
+               // the finalizer on the *os.File.
+               f := os.NewFile(fd, "")
+               if _, err := f.Stat(); err != nil {
+                       // Close the file to clear the finalizer.
+                       // We expect the Close to fail.
+                       f.Close()
+               } else {
+                       fmt.Printf("fd %d open at test start\n", fd)
+                       haveUnexpectedFDs = true
+                       // Use a global variable to avoid running
+                       // the finalizer, which would close the FD.
+                       unfinalizedFiles = append(unfinalizedFiles, f)
+               }
+       }
+}
+
 func helperCommandContext(t *testing.T, ctx context.Context, s ...string) (cmd *exec.Cmd) {
        testenv.MustHaveExec(t)
 
@@ -449,8 +485,6 @@ func numOpenFDsAndroid(t *testing.T) (n int, lsof []byte) {
        return bytes.Count(lsof, []byte("\n")), lsof
 }
 
-var testedAlreadyLeaked = false
-
 // basefds returns the number of expected file descriptors
 // to be present in a process at start.
 // stdin, stdout, stderr, epoll/kqueue, epoll/kqueue pipe, maybe testlog
@@ -470,29 +504,9 @@ func basefds() uintptr {
        return n
 }
 
-func closeUnexpectedFds(t *testing.T, m string) {
-       for fd := basefds(); fd <= 101; fd++ {
-               if poll.IsPollDescriptor(fd) {
-                       continue
-               }
-               err := os.NewFile(fd, "").Close()
-               if err == nil {
-                       t.Logf("%s: Something already leaked - closed fd %d", m, fd)
-               }
-       }
-}
-
 func TestExtraFilesFDShuffle(t *testing.T) {
        t.Skip("flaky test; see https://golang.org/issue/5780")
        switch runtime.GOOS {
-       case "darwin":
-               // TODO(cnicolaou): https://golang.org/issue/2603
-               // leads to leaked file descriptors in this test when it's
-               // run from a builder.
-               closeUnexpectedFds(t, "TestExtraFilesFDShuffle")
-       case "netbsd":
-               // https://golang.org/issue/3955
-               closeUnexpectedFds(t, "TestExtraFilesFDShuffle")
        case "windows":
                t.Skip("no operating system support; skipping")
        }
@@ -587,19 +601,29 @@ func TestExtraFilesFDShuffle(t *testing.T) {
 }
 
 func TestExtraFiles(t *testing.T) {
+       if haveUnexpectedFDs {
+               // The point of this test is to make sure that any
+               // descriptors we open are marked close-on-exec.
+               // If haveUnexpectedFDs is true then there were other
+               // descriptors open when we started the test,
+               // so those descriptors are clearly not close-on-exec,
+               // and they will confuse the test. We could modify
+               // the test to expect those descriptors to remain open,
+               // but since we don't know where they came from or what
+               // they are doing, that seems fragile. For example,
+               // perhaps they are from the startup code on this
+               // system for some reason. Also, this test is not
+               // system-specific; as long as most systems do not skip
+               // the test, we will still be testing what we care about.
+               t.Skip("skipping test because test was run with FDs open")
+       }
+
        testenv.MustHaveExec(t)
 
        if runtime.GOOS == "windows" {
                t.Skipf("skipping test on %q", runtime.GOOS)
        }
 
-       // Ensure that file descriptors have not already been leaked into
-       // our environment.
-       if !testedAlreadyLeaked {
-               testedAlreadyLeaked = true
-               closeUnexpectedFds(t, "TestExtraFiles")
-       }
-
        // Force network usage, to verify the epoll (or whatever) fd
        // doesn't leak to the child,
        ln, err := net.Listen("tcp", "127.0.0.1:0")