net: fix TCPListener file leak to child processes

Hold ForkLock during dup of fd + cloexec in the net pkg,
per the locking policy documented in syscall/exec_unix.go.

R=golang-dev, dsymonds, adg
CC=golang-dev
https://golang.org/cl/6457080

diff --git a/src/pkg/net/fd.go b/src/pkg/net/fd.go
index 76c953b9b4e006cfed589e178cb861ee29d73aaf..ff4f4f899e54129a008bfb4d8e6eb55b70ab74b3 100644 (file)
--- a/src/pkg/net/fd.go
+++ b/src/pkg/net/fd.go
@@ -645,10 +645,14 @@ func (fd *netFD) accept(toAddr func(syscall.Sockaddr) Addr) (netfd *netFD, err e
 }
 
 func (fd *netFD) dup() (f *os.File, err error) {
+       syscall.ForkLock.RLock()
        ns, err := syscall.Dup(fd.sysfd)
        if err != nil {
+               syscall.ForkLock.RUnlock()
                return nil, &OpError{"dup", fd.net, fd.laddr, err}
        }
+       syscall.CloseOnExec(ns)
+       syscall.ForkLock.RUnlock()
 
        // We want blocking mode for the new fd, hence the double negative.
        if err = syscall.SetNonblock(ns, false); err != nil {

diff --git a/src/pkg/os/exec/exec_test.go b/src/pkg/os/exec/exec_test.go
index 52f4bce3aea3f495e22d38ea997d983187489613..aead57d7993502f527bd93173d4554b2ba82b5b0 100644 (file)
--- a/src/pkg/os/exec/exec_test.go
+++ b/src/pkg/os/exec/exec_test.go
@@ -203,6 +203,56 @@ func TestExtraFiles(t *testing.T) {
        }
 }
 
+func TestExtraFilesRace(t *testing.T) {
+       if runtime.GOOS == "windows" {
+               t.Logf("no operating system support; skipping")
+               return
+       }
+       listen := func() net.Listener {
+               ln, err := net.Listen("tcp", "127.0.0.1:0")
+               if err != nil {
+                       t.Fatal(err)
+               }
+               return ln
+       }
+       listenerFile := func(ln net.Listener) *os.File {
+               f, err := ln.(*net.TCPListener).File()
+               if err != nil {
+                       t.Fatal(err)
+               }
+               return f
+       }
+       runCommand := func(c *Cmd, out chan<- string) {
+               bout, err := c.CombinedOutput()
+               if err != nil {
+                       out <- "ERROR:" + err.Error()
+               } else {
+                       out <- string(bout)
+               }
+       }
+
+       for i := 0; i < 10; i++ {
+               la := listen()
+               ca := helperCommand("describefiles")
+               ca.ExtraFiles = []*os.File{listenerFile(la)}
+               lb := listen()
+               cb := helperCommand("describefiles")
+               cb.ExtraFiles = []*os.File{listenerFile(lb)}
+               ares := make(chan string)
+               bres := make(chan string)
+               go runCommand(ca, ares)
+               go runCommand(cb, bres)
+               if got, want := <-ares, fmt.Sprintf("fd3: listener %s\n", la.Addr()); got != want {
+                       t.Errorf("iteration %d, process A got:\n%s\nwant:\n%s\n", i, got, want)
+               }
+               if got, want := <-bres, fmt.Sprintf("fd3: listener %s\n", lb.Addr()); got != want {
+                       t.Errorf("iteration %d, process B got:\n%s\nwant:\n%s\n", i, got, want)
+               }
+               la.Close()
+               lb.Close()
+       }
+}
+
 // TestHelperProcess isn't a real test. It's used as a helper process
 // for TestParameterRun.
 func TestHelperProcess(*testing.T) {
@@ -318,6 +368,16 @@ func TestHelperProcess(*testing.T) {
        case "exit":
                n, _ := strconv.Atoi(args[0])
                os.Exit(n)
+       case "describefiles":
+               for fd := uintptr(3); fd < 25; fd++ {
+                       f := os.NewFile(fd, fmt.Sprintf("fd-%d", fd))
+                       ln, err := net.FileListener(f)
+                       if err == nil {
+                               fmt.Printf("fd%d: listener %s\n", fd, ln.Addr())
+                               ln.Close()
+                       }
+               }
+               os.Exit(0)
        default:
                fmt.Fprintf(os.Stderr, "Unknown command %q\n", cmd)
                os.Exit(2)