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>

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.

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 {

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 {

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
 }

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)

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

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)
+	}
+}

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() {

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