Skip to content

Commit 11c7b44

Browse files
ianlancetaylorianlancetaylor
committed
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 golang#7970 Change-Id: I7a3daa1a6b07b7345bdce6f0cd7164bd4eaee952 Reviewed-on: https://go-review.googlesource.com/41674 Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
1 parent 9459c03 commit 11c7b44

File tree

9 files changed

+46
-17
lines changed

9 files changed

+46
-17
lines changed

‎src/cmd/dist/test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1096,9 +1096,9 @@ func (t *tester) runFlag(rx string) string {
10961096
}
10971097

10981098
func (t *tester) raceTest(dt *distTest) error {
1099-
t.addCmd(dt, "src", "go", "test", "-race", "-i", "runtime/race", "flag", "os/exec")
1099+
t.addCmd(dt, "src", "go", "test", "-race", "-i", "runtime/race", "flag", "os", "os/exec")
11001100
t.addCmd(dt, "src", "go", "test", "-race", t.runFlag("Output"), "runtime/race")
1101-
t.addCmd(dt, "src", "go", "test", "-race", "-short", t.runFlag("TestParse|TestEcho|TestStdinCloseRace"), "flag", "os/exec")
1101+
t.addCmd(dt, "src", "go", "test", "-race", "-short", t.runFlag("TestParse|TestEcho|TestStdinCloseRace|TestClosedPipeRace"), "flag", "os", "os/exec")
11021102
// We don't want the following line, because it
11031103
// slows down all.bash (by 10 seconds on my laptop).
11041104
// The race builder should catch any error here, but doesn't.

‎src/os/dir_windows.go‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,6 @@ func (file *File) readdir(n int) (fi []FileInfo, err error) {
1717
if !file.isdir() {
1818
return nil, &PathError{"Readdir", file.name, syscall.ENOTDIR}
1919
}
20-
if !file.dirinfo.isempty && file.pfd.Sysfd == syscall.InvalidHandle {
21-
return nil, syscall.EINVAL
22-
}
2320
wantAll := n <= 0
2421
size := n
2522
if wantAll {

‎src/os/file.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ package os
3838

3939
import (
4040
"errors"
41+
"internal/poll"
4142
"io"
4243
"syscall"
4344
)
@@ -101,6 +102,9 @@ func (f *File) Read(b []byte) (n int, err error) {
101102
}
102103
n, e := f.read(b)
103104
if e != nil {
105+
if e == poll.ErrClosing {
106+
e = ErrClosed
107+
}
104108
if e == io.EOF {
105109
err = e
106110
} else {

‎src/os/file_posix.go‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -165,8 +165,5 @@ func (f *File) checkValid(op string) error {
165165
if f == nil {
166166
return ErrInvalid
167167
}
168-
if f.pfd.Sysfd == badFd {
169-
return &PathError{op, f.name, ErrClosed}
170-
}
171168
return nil
172169
}

‎src/os/file_unix.go‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -183,14 +183,13 @@ func (f *File) Close() error {
183183
}
184184

185185
func (file *file) close() error {
186-
if file == nil || file.pfd.Sysfd == badFd {
186+
if file == nil {
187187
return syscall.EINVAL
188188
}
189189
var err error
190190
if e := file.pfd.Close(); e != nil {
191191
err = &PathError{"close", file.name, e}
192192
}
193-
file.pfd.Sysfd = badFd // so it can't be closed again
194193

195194
// no need for a finalizer anymore
196195
runtime.SetFinalizer(file, nil)

‎src/os/file_windows.go‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -179,7 +179,7 @@ func (file *File) Close() error {
179179
}
180180

181181
func (file *file) close() error {
182-
if file == nil || file.pfd.Sysfd == badFd {
182+
if file == nil {
183183
return syscall.EINVAL
184184
}
185185
if file.isdir() && file.dirinfo.isempty {
@@ -190,7 +190,6 @@ func (file *file) close() error {
190190
if e := file.pfd.Close(); e != nil {
191191
err = &PathError{"close", file.name, e}
192192
}
193-
file.pfd.Sysfd = badFd // so it can't be closed again
194193

195194
// no need for a finalizer anymore
196195
runtime.SetFinalizer(file, nil)
@@ -394,5 +393,3 @@ func Symlink(oldname, newname string) error {
394393
}
395394
return nil
396395
}
397-
398-
const badFd = syscall.InvalidHandle

‎src/os/pipe_test.go‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,10 @@ import (
1313
"os"
1414
osexec "os/exec"
1515
"os/signal"
16+
"runtime"
1617
"syscall"
1718
"testing"
19+
"time"
1820
)
1921

2022
func TestEPIPE(t *testing.T) {
@@ -111,3 +113,38 @@ func TestStdPipeHelper(t *testing.T) {
111113
// For descriptor 3, a normal exit is expected.
112114
os.Exit(0)
113115
}
116+
117+
func TestClosedPipeRace(t *testing.T) {
118+
switch runtime.GOOS {
119+
case "freebsd":
120+
t.Skip("FreeBSD does not use the poller; issue 19093")
121+
}
122+
123+
r, w, err := os.Pipe()
124+
if err != nil {
125+
t.Fatal(err)
126+
}
127+
defer r.Close()
128+
defer w.Close()
129+
130+
// Close the read end of the pipe in a goroutine while we are
131+
// writing to the write end.
132+
go func() {
133+
// Give the main goroutine a chance to enter the Read call.
134+
// This is sloppy but the test will pass even if we close
135+
// before the read.
136+
time.Sleep(20 * time.Millisecond)
137+
138+
if err := r.Close(); err != nil {
139+
t.Error(err)
140+
}
141+
}()
142+
143+
if _, err := r.Read(make([]byte, 1)); err == nil {
144+
t.Error("Read of closed pipe unexpectedly succeeded")
145+
} else if pe, ok := err.(*os.PathError); !ok {
146+
t.Errorf("Read of closed pipe returned unexpected error type %T; expected os.PathError", pe)
147+
} else {
148+
t.Logf("Read returned expected error %q", err)
149+
}
150+
}

‎src/os/stat_windows.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ func (file *File) Stat() (FileInfo, error) {
1616
if file == nil {
1717
return nil, ErrInvalid
1818
}
19-
if file == nil || file.pfd.Sysfd < 0 {
19+
if file == nil {
2020
return nil, syscall.EINVAL
2121
}
2222
if file.isdir() {

‎src/os/types_unix.go‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,5 +29,3 @@ func (fs *fileStat) Sys() interface{} { return &fs.sys }
2929
func sameFile(fs1, fs2 *fileStat) bool {
3030
return fs1.sys.Dev == fs2.sys.Dev && fs1.sys.Ino == fs2.sys.Ino
3131
}
32-
33-
const badFd = -1

0 commit comments

Comments
 (0)