Permalink
Browse files

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>
  • Loading branch information...
ianlancetaylor committed Apr 25, 2017
1 parent 9459c03 commit 11c7b4491bd2cd1deb7b50433f431be9ced330db
View
@@ -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.
View
@@ -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 {
View
@@ -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 {
View
@@ -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
}
View
@@ -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)
View
@@ -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
View
@@ -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)
+ }
+}
View
@@ -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() {
View
@@ -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

0 comments on commit 11c7b44

Please sign in to comment.