diff options
author | Ian Lance Taylor <iant@golang.org> | 2018-06-19 19:50:03 -0700 |
---|---|---|
committer | Ian Lance Taylor <iant@golang.org> | 2018-06-22 14:27:22 +0000 |
commit | 00eac8921e04c07df2d25df33b9401226f5d0326 (patch) | |
tree | 2aba45dee8777a8bc822ac27ee0e868dc0c6734e | |
parent | 75fdeaa801bc64b8b720433a868e1be197d03eaf (diff) | |
download | go-00eac8921e04c07df2d25df33b9401226f5d0326.tar.gz go-00eac8921e04c07df2d25df33b9401226f5d0326.zip |
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>
-rw-r--r-- | src/cmd/dist/test.go | 2 | ||||
-rw-r--r-- | src/internal/poll/fd_unix.go | 53 | ||||
-rw-r--r-- | src/net/fd_unix.go | 40 | ||||
-rw-r--r-- | src/net/file_test.go | 54 | ||||
-rw-r--r-- | src/net/file_unix.go | 5 | ||||
-rw-r--r-- | src/os/pipe_test.go | 23 |
6 files changed, 137 insertions, 40 deletions
diff --git a/src/cmd/dist/test.go b/src/cmd/dist/test.go index e146c2a3b8..49e5699120 100644 --- a/src/cmd/dist/test.go +++ b/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. diff --git a/src/internal/poll/fd_unix.go b/src/internal/poll/fd_unix.go index 5a196e7efe..5639a9dab6 100644 --- a/src/internal/poll/fd_unix.go +++ b/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. diff --git a/src/net/fd_unix.go b/src/net/fd_unix.go index 84613c778c..06439ee200 100644 --- a/src/net/fd_unix.go +++ b/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 } diff --git a/src/net/file_test.go b/src/net/file_test.go index 9fb5f2fd26..cd717747af 100644 --- a/src/net/file_test.go +++ b/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() + } +} diff --git a/src/net/file_unix.go b/src/net/file_unix.go index d67dff8e05..676798d693 100644 --- a/src/net/file_unix.go +++ b/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 { diff --git a/src/os/pipe_test.go b/src/os/pipe_test.go index 1d81f57eab..a6d955a8e4 100644 --- a/src/os/pipe_test.go +++ b/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() +} |