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 <[email protected]>
Reviewed-by: Brad Fitzpatrick <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>

Author: ianlancetaylor

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.

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

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