os, net: avoid races between dup, set-blocking-mode, and closing

Fixes #24481
Fixes #24483

Change-Id: Id7da498425a440c91582aa5480c253ae7a9c932c
Reviewed-on: https://go-review.googlesource.com/119955
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

src/cmd/dist/test.go

@@ -1346,7 +1346,7 @@ func (t *tester) runFlag(rx string) string {
 func (t *tester) raceTest(dt *distTest) error {
 	t.addCmd(dt, "src", t.goTest(), "-race", "-i", "runtime/race", "flag", "os", "os/exec")
 	t.addCmd(dt, "src", t.goTest(), "-race", t.runFlag("Output"), "runtime/race")
-	t.addCmd(dt, "src", t.goTest(), "-race", t.runFlag("TestParse|TestEcho|TestStdinCloseRace|TestClosedPipeRace|TestTypeRace"), "flag", "os", "os/exec", "encoding/gob")
+	t.addCmd(dt, "src", t.goTest(), "-race", t.runFlag("TestParse|TestEcho|TestStdinCloseRace|TestClosedPipeRace|TestTypeRace|TestFdRace|TestFileCloseRace"), "flag", "net", "os", "os/exec", "encoding/gob")
 	// We don't want the following line, because it
 	// slows down all.bash (by 10 seconds on my laptop).
 	// The race builder should catch any error here, but doesn't.

src/internal/poll/fd_unix.go

@@ -9,6 +9,7 @@ package poll
 import (
 	"io"
 	"runtime"
+	"sync/atomic"
 	"syscall"
 )
 
@@ -102,6 +103,8 @@ func (fd *FD) Close() error {
 	// reference, it is already closed. Only wait if the file has
 	// not been set to blocking mode, as otherwise any current I/O
 	// may be blocking, and that would block the Close.
+	// No need for a lock to read isBlocking, increfAndClose means
+	// we have exclusive access to fd.
 	if !fd.isBlocking {
 		runtime_Semacquire(&fd.csema)
 	}
@@ -120,10 +123,12 @@ func (fd *FD) Shutdown(how int) error {
 
 // SetBlocking puts the file into blocking mode.
 func (fd *FD) SetBlocking() error {
-	if err := fd.incref(); err != nil {
+	// Take an exclusive lock, rather than calling incref, so that
+	// we can safely modify isBlocking.
+	if err := fd.readLock(); err != nil {
 		return err
 	}
-	defer fd.decref()
+	defer fd.readUnlock()
 	fd.isBlocking = true
 	return syscall.SetNonblock(fd.Sysfd, false)
 }
@@ -439,6 +444,50 @@ func (fd *FD) Fstat(s *syscall.Stat_t) error {
 	return syscall.Fstat(fd.Sysfd, s)
 }
 
+// tryDupCloexec indicates whether F_DUPFD_CLOEXEC should be used.
+// If the kernel doesn't support it, this is set to 0.
+var tryDupCloexec = int32(1)
+
+// DupCloseOnExec dups fd and marks it close-on-exec.
+func DupCloseOnExec(fd int) (int, string, error) {
+	if atomic.LoadInt32(&tryDupCloexec) == 1 {
+		r0, _, e1 := syscall.Syscall(syscall.SYS_FCNTL, uintptr(fd), syscall.F_DUPFD_CLOEXEC, 0)
+		switch e1 {
+		case 0:
+			return int(r0), "", nil
+		case syscall.EINVAL:
+			// Old kernel. Fall back to the portable way
+			// from now on.
+			atomic.StoreInt32(&tryDupCloexec, 0)
+		default:
+			return -1, "fcntl", e1
+		}
+	}
+	return dupCloseOnExecOld(fd)
+}
+
+// dupCloseOnExecUnixOld is the traditional way to dup an fd and
+// set its O_CLOEXEC bit, using two system calls.
+func dupCloseOnExecOld(fd int) (int, string, error) {
+	syscall.ForkLock.RLock()
+	defer syscall.ForkLock.RUnlock()
+	newfd, err := syscall.Dup(fd)
+	if err != nil {
+		return -1, "dup", err
+	}
+	syscall.CloseOnExec(newfd)
+	return newfd, "", nil
+}
+
+// Dup duplicates the file descriptor.
+func (fd *FD) Dup() (int, string, error) {
+	if err := fd.incref(); err != nil {
+		return -1, "", err
+	}
+	defer fd.decref()
+	return DupCloseOnExec(fd.Sysfd)
+}
+
 // On Unix variants only, expose the IO event for the net code.
 
 // WaitWrite waits until data can be read from fd.

src/net/fd_unix.go

@@ -11,7 +11,6 @@ import (
 	"internal/poll"
 	"os"
 	"runtime"
-	"sync/atomic"
 	"syscall"
 	"time"
 )
@@ -257,43 +256,12 @@ func (fd *netFD) accept() (netfd *netFD, err error) {
 	return netfd, nil
 }
 
-// tryDupCloexec indicates whether F_DUPFD_CLOEXEC should be used.
-// If the kernel doesn't support it, this is set to 0.
-var tryDupCloexec = int32(1)
-
-func dupCloseOnExec(fd int) (newfd int, err error) {
-	if atomic.LoadInt32(&tryDupCloexec) == 1 {
-		r0, _, e1 := syscall.Syscall(syscall.SYS_FCNTL, uintptr(fd), syscall.F_DUPFD_CLOEXEC, 0)
-		switch e1 {
-		case 0:
-			return int(r0), nil
-		case syscall.EINVAL:
-			// Old kernel. Fall back to the portable way
-			// from now on.
-			atomic.StoreInt32(&tryDupCloexec, 0)
-		default:
-			return -1, os.NewSyscallError("fcntl", e1)
-		}
-	}
-	return dupCloseOnExecOld(fd)
-}
-
-// dupCloseOnExecUnixOld is the traditional way to dup an fd and
-// set its O_CLOEXEC bit, using two system calls.
-func dupCloseOnExecOld(fd int) (newfd int, err error) {
-	syscall.ForkLock.RLock()
-	defer syscall.ForkLock.RUnlock()
-	newfd, err = syscall.Dup(fd)
-	if err != nil {
-		return -1, os.NewSyscallError("dup", err)
-	}
-	syscall.CloseOnExec(newfd)
-	return
-}
-
 func (fd *netFD) dup() (f *os.File, err error) {
-	ns, err := dupCloseOnExec(fd.pfd.Sysfd)
+	ns, call, err := fd.pfd.Dup()
 	if err != nil {
+		if call != "" {
+			err = os.NewSyscallError(call, err)
+		}
 		return nil, err
 	}
 

src/net/file_test.go

@@ -293,3 +293,57 @@ func TestFilePacketConn(t *testing.T) {
 		}
 	}
 }
+
+// Issue 24483.
+func TestFileCloseRace(t *testing.T) {
+	switch runtime.GOOS {
+	case "nacl", "plan9", "windows":
+		t.Skipf("not supported on %s", runtime.GOOS)
+	}
+	if !testableNetwork("tcp") {
+		t.Skip("tcp not supported")
+	}
+
+	handler := func(ls *localServer, ln Listener) {
+		c, err := ln.Accept()
+		if err != nil {
+			return
+		}
+		defer c.Close()
+		var b [1]byte
+		c.Read(b[:])
+	}
+
+	ls, err := newLocalServer("tcp")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer ls.teardown()
+	if err := ls.buildup(handler); err != nil {
+		t.Fatal(err)
+	}
+
+	const tries = 100
+	for i := 0; i < tries; i++ {
+		c1, err := Dial(ls.Listener.Addr().Network(), ls.Listener.Addr().String())
+		if err != nil {
+			t.Fatal(err)
+		}
+		tc := c1.(*TCPConn)
+
+		var wg sync.WaitGroup
+		wg.Add(2)
+		go func() {
+			defer wg.Done()
+			f, err := tc.File()
+			if err == nil {
+				f.Close()
+			}
+		}()
+		go func() {
+			defer wg.Done()
+			c1.Close()
+		}()
+		wg.Wait()
+	}
+}

src/net/file_unix.go

@@ -13,8 +13,11 @@ import (
 )
 
 func dupSocket(f *os.File) (int, error) {
-	s, err := dupCloseOnExec(int(f.Fd()))
+	s, call, err := poll.DupCloseOnExec(int(f.Fd()))
 	if err != nil {
+		if call != "" {
+			err = os.NewSyscallError(call, err)
+		}
 		return -1, err
 	}
 	if err := syscall.SetNonblock(s, true); err != nil {

src/os/pipe_test.go

@@ -372,3 +372,26 @@ func TestPipeEOF(t *testing.T) {
 		r.Close()
 	}
 }
+
+// Issue 24481.
+func TestFdRace(t *testing.T) {
+	r, w, err := os.Pipe()
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer r.Close()
+	defer w.Close()
+
+	var wg sync.WaitGroup
+	call := func() {
+		defer wg.Done()
+		w.Fd()
+	}
+
+	const tries = 100
+	for i := 0; i < tries; i++ {
+		wg.Add(1)
+		go call()
+	}
+	wg.Wait()
+}