internal/poll: don't take read lock in SetBlocking

Taking a read lock in SetBlocking could cause SetBlocking to block
waiting for a Read in another goroutine to complete. Since SetBlocking
is called by os.(*File).Fd, that could lead to deadlock if the
goroutine calling Fd is going to use it to unblock the Read.
Use an atomic store instead.

Updates #24481

Change-Id: I79413328e06ddf28b6d5b8af7a0e29d5b4e1e6ff
Reviewed-on: https://go-review.googlesource.com/123176
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

diff --git a/src/cmd/dist/test.go b/src/cmd/dist/test.go
index a6c0f387ff1ff524bdd168d5d30b5075f24b5ca6..c8c918d36ba2f032649caca3df1d90d81cdbd9e4 100644 (file)
--- a/src/cmd/dist/test.go
+++ b/src/cmd/dist/test.go
@@ -1354,7 +1354,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|TestFdRace|TestFileCloseRace"), "flag", "net", "os", "os/exec", "encoding/gob")
+       t.addCmd(dt, "src", t.goTest(), "-race", t.runFlag("TestParse|TestEcho|TestStdinCloseRace|TestClosedPipeRace|TestTypeRace|TestFdRace|TestFdReadRace|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 c10ac8949681e7f90876757e97e858711f71c9e8..b311049ad701cc298112549f69c546f4edb04851 100644 (file)
--- a/src/internal/poll/fd_unix.go
+++ b/src/internal/poll/fd_unix.go
@@ -31,6 +31,9 @@ type FD struct {
        // Semaphore signaled when file is closed.
        csema uint32
 
+       // Non-zero if this file has been set to blocking mode.
+       isBlocking uint32
+
        // Whether this is a streaming descriptor, as opposed to a
        // packet-based descriptor like a UDP socket. Immutable.
        IsStream bool
@@ -41,9 +44,6 @@ type FD struct {
 
        // Whether this is a file rather than a network socket.
        isFile bool
-
-       // Whether this file has been set to blocking mode.
-       isBlocking bool
 }
 
 // Init initializes the FD. The Sysfd field should already be set.
@@ -57,14 +57,14 @@ func (fd *FD) Init(net string, pollable bool) error {
                fd.isFile = true
        }
        if !pollable {
-               fd.isBlocking = true
+               fd.isBlocking = 1
                return nil
        }
        err := fd.pd.init(fd)
        if err != nil {
                // If we could not initialize the runtime poller,
                // assume we are using blocking mode.
-               fd.isBlocking = true
+               fd.isBlocking = 1
        }
        return err
 }
@@ -103,9 +103,9 @@ 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
+       // No need for an atomic read of isBlocking, increfAndClose means
        // we have exclusive access to fd.
-       if !fd.isBlocking {
+       if fd.isBlocking == 0 {
                runtime_Semacquire(&fd.csema)
        }
 
@@ -123,13 +123,14 @@ func (fd *FD) Shutdown(how int) error {
 
 // SetBlocking puts the file into blocking mode.
 func (fd *FD) SetBlocking() error {
-       // Take an exclusive lock, rather than calling incref, so that
-       // we can safely modify isBlocking.
-       if err := fd.readLock(); err != nil {
+       if err := fd.incref(); err != nil {
                return err
        }
-       defer fd.readUnlock()
-       fd.isBlocking = true
+       defer fd.decref()
+       // Atomic store so that concurrent calls to SetBlocking
+       // do not cause a race condition. isBlocking only ever goes
+       // from 0 to 1 so there is no real race here.
+       atomic.StoreUint32(&fd.isBlocking, 1)
        return syscall.SetNonblock(fd.Sysfd, false)
 }
 

diff --git a/src/os/pipe_test.go b/src/os/pipe_test.go
index a6d955a8e4097b797a86ca58d738e6f0034f9ed5..59d31e5837c9d107566c085893db2442513f6694 100644 (file)
--- a/src/os/pipe_test.go
+++ b/src/os/pipe_test.go
@@ -395,3 +395,45 @@ func TestFdRace(t *testing.T) {
        }
        wg.Wait()
 }
+
+func TestFdReadRace(t *testing.T) {
+       t.Parallel()
+
+       r, w, err := os.Pipe()
+       if err != nil {
+               t.Fatal(err)
+       }
+       defer r.Close()
+       defer w.Close()
+
+       c := make(chan bool)
+       var wg sync.WaitGroup
+       wg.Add(1)
+       go func() {
+               defer wg.Done()
+               var buf [10]byte
+               r.SetReadDeadline(time.Now().Add(time.Second))
+               c <- true
+               if _, err := r.Read(buf[:]); os.IsTimeout(err) {
+                       t.Error("read timed out")
+               }
+       }()
+
+       wg.Add(1)
+       go func() {
+               defer wg.Done()
+               <-c
+               // Give the other goroutine a chance to enter the Read.
+               // It doesn't matter if this occasionally fails, the test
+               // will still pass, it just won't test anything.
+               time.Sleep(10 * time.Millisecond)
+               r.Fd()
+
+               // The bug was that Fd would hang until Read timed out.
+               // If the bug is fixed, then closing r here will cause
+               // the Read to exit before the timeout expires.
+               r.Close()
+       }()
+
+       wg.Wait()
+}