os: fix race between file I/O and Close

Now that the os package uses internal/poll on Unix and Windows systems,
it can rely on internal/poll reference counting to ensure that the
file descriptor is not closed until all I/O is complete.

That was already working. This CL completes the job by not trying to
modify the Sysfd field when it might still be used by the I/O routines.

Fixes #7970

Change-Id: I7a3daa1a6b07b7345bdce6f0cd7164bd4eaee952
Reviewed-on: https://go-review.googlesource.com/41674
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

diff --git a/src/cmd/dist/test.go b/src/cmd/dist/test.go
index 9aa966d14c46a2aea5685119a18f904a372f7f0d..917aae19f69961a30053c3267a83eba1968ab82c 100644 (file)
--- a/src/cmd/dist/test.go
+++ b/src/cmd/dist/test.go
@@ -1096,9 +1096,9 @@ func (t *tester) runFlag(rx string) string {
 }
 
 func (t *tester) raceTest(dt *distTest) error {
-       t.addCmd(dt, "src", "go", "test", "-race", "-i", "runtime/race", "flag", "os/exec")
+       t.addCmd(dt, "src", "go", "test", "-race", "-i", "runtime/race", "flag", "os", "os/exec")
        t.addCmd(dt, "src", "go", "test", "-race", t.runFlag("Output"), "runtime/race")
-       t.addCmd(dt, "src", "go", "test", "-race", "-short", t.runFlag("TestParse|TestEcho|TestStdinCloseRace"), "flag", "os/exec")
+       t.addCmd(dt, "src", "go", "test", "-race", "-short", t.runFlag("TestParse|TestEcho|TestStdinCloseRace|TestClosedPipeRace"), "flag", "os", "os/exec")
        // 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/os/dir_windows.go b/src/os/dir_windows.go
index 2a012a8a1236d8f5101e77d5efa6f530e08edfbd..2e3046d73672c7dac3e0548f1e1cf2e7aade05bb 100644 (file)
--- a/src/os/dir_windows.go
+++ b/src/os/dir_windows.go
@@ -17,9 +17,6 @@ func (file *File) readdir(n int) (fi []FileInfo, err error) {
        if !file.isdir() {
                return nil, &PathError{"Readdir", file.name, syscall.ENOTDIR}
        }
-       if !file.dirinfo.isempty && file.pfd.Sysfd == syscall.InvalidHandle {
-               return nil, syscall.EINVAL
-       }
        wantAll := n <= 0
        size := n
        if wantAll {

diff --git a/src/os/file.go b/src/os/file.go
index d61124b338e9a26909cba5bbd066087c41a33854..e5a3efa8843b368d8836d477f78ff2460e287d83 100644 (file)
--- a/src/os/file.go
+++ b/src/os/file.go
@@ -38,6 +38,7 @@ package os
 
 import (
        "errors"
+       "internal/poll"
        "io"
        "syscall"
 )
@@ -101,6 +102,9 @@ func (f *File) Read(b []byte) (n int, err error) {
        }
        n, e := f.read(b)
        if e != nil {
+               if e == poll.ErrClosing {
+                       e = ErrClosed
+               }
                if e == io.EOF {
                        err = e
                } else {

diff --git a/src/os/file_posix.go b/src/os/file_posix.go
index e38668684c80d4df12ba06129aecd18a6b745043..98c87ee4cdbd2794f9115a4c915ea704184223c8 100644 (file)
--- a/src/os/file_posix.go
+++ b/src/os/file_posix.go
@@ -165,8 +165,5 @@ func (f *File) checkValid(op string) error {
        if f == nil {
                return ErrInvalid
        }
-       if f.pfd.Sysfd == badFd {
-               return &PathError{op, f.name, ErrClosed}
-       }
        return nil
 }

diff --git a/src/os/file_unix.go b/src/os/file_unix.go
index 6e00f483930922b1e6c69986f39936f2e35104e8..6850ff7a56623e623fd0bf669eda3ab7a119273c 100644 (file)
--- a/src/os/file_unix.go
+++ b/src/os/file_unix.go
@@ -183,14 +183,13 @@ func (f *File) Close() error {
 }
 
 func (file *file) close() error {
-       if file == nil || file.pfd.Sysfd == badFd {
+       if file == nil {
                return syscall.EINVAL
        }
        var err error
        if e := file.pfd.Close(); e != nil {
                err = &PathError{"close", file.name, e}
        }
-       file.pfd.Sysfd = badFd // so it can't be closed again
 
        // no need for a finalizer anymore
        runtime.SetFinalizer(file, nil)

diff --git a/src/os/file_windows.go b/src/os/file_windows.go
index b7d4275d17e259f9b90d1e3a4997f9e8f3c17669..a6cdb3ff478d31efe3be919f354e849af4507ddb 100644 (file)
--- a/src/os/file_windows.go
+++ b/src/os/file_windows.go
@@ -179,7 +179,7 @@ func (file *File) Close() error {
 }
 
 func (file *file) close() error {
-       if file == nil || file.pfd.Sysfd == badFd {
+       if file == nil {
                return syscall.EINVAL
        }
        if file.isdir() && file.dirinfo.isempty {
@@ -190,7 +190,6 @@ func (file *file) close() error {
        if e := file.pfd.Close(); e != nil {
                err = &PathError{"close", file.name, e}
        }
-       file.pfd.Sysfd = badFd // so it can't be closed again
 
        // no need for a finalizer anymore
        runtime.SetFinalizer(file, nil)
@@ -394,5 +393,3 @@ func Symlink(oldname, newname string) error {
        }
        return nil
 }
-
-const badFd = syscall.InvalidHandle

diff --git a/src/os/pipe_test.go b/src/os/pipe_test.go
index 74cce80ee4b027f4d36884d1349fdc0fcb997518..032173b7592f8519e29d3cf31c7faa871070a37d 100644 (file)
--- a/src/os/pipe_test.go
+++ b/src/os/pipe_test.go
@@ -13,8 +13,10 @@ import (
        "os"
        osexec "os/exec"
        "os/signal"
+       "runtime"
        "syscall"
        "testing"
+       "time"
 )
 
 func TestEPIPE(t *testing.T) {
@@ -111,3 +113,38 @@ func TestStdPipeHelper(t *testing.T) {
        // For descriptor 3, a normal exit is expected.
        os.Exit(0)
 }
+
+func TestClosedPipeRace(t *testing.T) {
+       switch runtime.GOOS {
+       case "freebsd":
+               t.Skip("FreeBSD does not use the poller; issue 19093")
+       }
+
+       r, w, err := os.Pipe()
+       if err != nil {
+               t.Fatal(err)
+       }
+       defer r.Close()
+       defer w.Close()
+
+       // Close the read end of the pipe in a goroutine while we are
+       // writing to the write end.
+       go func() {
+               // Give the main goroutine a chance to enter the Read call.
+               // This is sloppy but the test will pass even if we close
+               // before the read.
+               time.Sleep(20 * time.Millisecond)
+
+               if err := r.Close(); err != nil {
+                       t.Error(err)
+               }
+       }()
+
+       if _, err := r.Read(make([]byte, 1)); err == nil {
+               t.Error("Read of closed pipe unexpectedly succeeded")
+       } else if pe, ok := err.(*os.PathError); !ok {
+               t.Errorf("Read of closed pipe returned unexpected error type %T; expected os.PathError", pe)
+       } else {
+               t.Logf("Read returned expected error %q", err)
+       }
+}

diff --git a/src/os/stat_windows.go b/src/os/stat_windows.go
index 4e586ab78ff14a349e52579798e1ddbb624aef5b..9b10f8b5cb4e3e74ee129554cfd8b1bad64584ac 100644 (file)
--- a/src/os/stat_windows.go
+++ b/src/os/stat_windows.go
@@ -16,7 +16,7 @@ func (file *File) Stat() (FileInfo, error) {
        if file == nil {
                return nil, ErrInvalid
        }
-       if file == nil || file.pfd.Sysfd < 0 {
+       if file == nil {
                return nil, syscall.EINVAL
        }
        if file.isdir() {

diff --git a/src/os/types_unix.go b/src/os/types_unix.go
index 1f614812fdd9cab99baee6d6972cee63cded2e08..c0259ae0e8411f791fadfd3a57ed3f6d9c44620b 100644 (file)
--- a/src/os/types_unix.go
+++ b/src/os/types_unix.go
@@ -29,5 +29,3 @@ func (fs *fileStat) Sys() interface{}   { return &fs.sys }
 func sameFile(fs1, fs2 *fileStat) bool {
        return fs1.sys.Dev == fs2.sys.Dev && fs1.sys.Ino == fs2.sys.Ino
 }
-
-const badFd = -1