]> Cypherpunks repositories - gostls13.git/commitdiff
internal/poll: avoid race between execIO and DisassociateIOCP
authorqmuntal <quimmuntal@gmail.com>
Mon, 2 Feb 2026 09:42:28 +0000 (10:42 +0100)
committerQuim Muntal <quimmuntal@gmail.com>
Thu, 12 Feb 2026 04:16:26 +0000 (20:16 -0800)
This is a step towards deferring adding the handle to IOCP until the
first IO operation.

There is a small race windows between execIO and DisassociateIOCP where
execIO checks if the fd is disassociated before passing the operation to
the OS. DisassociateIOCP can set the disassociated flag right after that
check but before Windows started processing the IO operation. Once
Windows takes over, the race doesn't matter anymore because Windows
doesn't allow disassociating a handle that has pending IO operations.
If that still hasn't happened, an overlapped IO operation will start
assuming the they can be waited using the Go runtime IOCP, which is
wrong due to the disassociation, leading to undefined behavior.

Fix that race by trying to take a write/read lock in DisassociateIOCP
before setting the disassociated flag, but failing if there is an
ongoing execIO operation so that DisassociateIOCP doesn't block
indefinitely waiting for execIO to finish.

For #76391

Cq-Include-Trybots: luci.golang.try:gotip-windows-amd64-longtest,gotip-windows-amd64-race
Change-Id: Iec265fa1900383aace50051d2be750bc76aa0944
Reviewed-on: https://go-review.googlesource.com/c/go/+/741020
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
src/internal/poll/export_test.go
src/internal/poll/fd_mutex.go
src/internal/poll/fd_windows.go
src/internal/poll/file_plan9.go
src/os/os_windows_test.go

index 66d7c3274bed638dbd53d670e6b602881d4f29e9..65549378e276f3407c8fea7a4f9f6cc414ecc3c1 100644 (file)
@@ -27,7 +27,7 @@ func (mu *XFDMutex) Decref() bool {
 }
 
 func (mu *XFDMutex) RWLock(read bool) bool {
-       return mu.rwlock(read)
+       return mu.rwlock(read, waitLock)
 }
 
 func (mu *XFDMutex) RWUnlock(read bool) bool {
index aa8f425235aae87b31ce69fde747d4e5b0966d26..aba22e986758a9195c04cafcd1eb2f3e25945c38 100644 (file)
@@ -34,11 +34,18 @@ const (
        mutexWMask   = (1<<20 - 1) << 43
 )
 
+const (
+       readlock  = true
+       writeLock = false
+       waitLock  = true
+       tryLock   = false
+)
+
 const overflowMsg = "too many concurrent operations on a single file or socket (max 1048575)"
 
-// Read operations must do rwlock(true)/rwunlock(true).
+// Read operations must do rwlock(readlock, waitLock)/rwunlock(readlock).
 //
-// Write operations must do rwlock(false)/rwunlock(false).
+// Write operations must do rwlock(writeLock, waitLock)/rwunlock(writeLock).
 //
 // Misc operations must do incref/decref.
 // Misc operations include functions like setsockopt and setDeadline.
@@ -114,7 +121,8 @@ func (mu *fdMutex) decref() bool {
 
 // lock adds a reference to mu and locks mu.
 // It reports whether mu is available for reading or writing.
-func (mu *fdMutex) rwlock(read bool) bool {
+// If wait is false, lock fails immediately if mu is not available.
+func (mu *fdMutex) rwlock(read bool, wait bool) bool {
        var mutexBit, mutexWait, mutexMask uint64
        var mutexSema *uint32
        if read {
@@ -142,6 +150,9 @@ func (mu *fdMutex) rwlock(read bool) bool {
                        }
                } else {
                        // Wait for lock.
+                       if !wait {
+                               return false
+                       }
                        new = old + mutexWait
                        if new&mutexMask == 0 {
                                panic(overflowMsg)
@@ -218,7 +229,7 @@ func (fd *FD) decref() error {
 // readLock adds a reference to fd and locks fd for reading.
 // It returns an error when fd cannot be used for reading.
 func (fd *FD) readLock() error {
-       if !fd.fdmu.rwlock(true) {
+       if !fd.fdmu.rwlock(readlock, waitLock) {
                return errClosing(fd.isFile)
        }
        return nil
@@ -228,7 +239,7 @@ func (fd *FD) readLock() error {
 // It also closes fd when the state of fd is set to closed and there
 // is no remaining reference.
 func (fd *FD) readUnlock() {
-       if fd.fdmu.rwunlock(true) {
+       if fd.fdmu.rwunlock(readlock) {
                fd.destroy()
        }
 }
@@ -236,7 +247,7 @@ func (fd *FD) readUnlock() {
 // writeLock adds a reference to fd and locks fd for writing.
 // It returns an error when fd cannot be used for writing.
 func (fd *FD) writeLock() error {
-       if !fd.fdmu.rwlock(false) {
+       if !fd.fdmu.rwlock(writeLock, waitLock) {
                return errClosing(fd.isFile)
        }
        return nil
@@ -246,7 +257,7 @@ func (fd *FD) writeLock() error {
 // It also closes fd when the state of fd is set to closed and there
 // is no remaining reference.
 func (fd *FD) writeUnlock() {
-       if fd.fdmu.rwunlock(false) {
+       if fd.fdmu.rwunlock(writeLock) {
                fd.destroy()
        }
 }
@@ -254,22 +265,42 @@ func (fd *FD) writeUnlock() {
 // readWriteLock adds a reference to fd and locks fd for reading and writing.
 // It returns an error when fd cannot be used for reading and writing.
 func (fd *FD) readWriteLock() error {
-       if !fd.fdmu.rwlock(true) {
+       if !fd.fdmu.rwlock(readlock, waitLock) {
                return errClosing(fd.isFile)
        }
-       if !fd.fdmu.rwlock(false) {
-               fd.fdmu.rwunlock(true) // unlock read lock acquired above
+       if !fd.fdmu.rwlock(writeLock, waitLock) {
+               fd.fdmu.rwunlock(readlock) // unlock read lock acquired above
                return errClosing(fd.isFile)
        }
        return nil
 }
 
+// tryReadWriteLock tries to add a reference to fd and lock fd for reading and writing.
+// It returns (false, nil) when fd is not available for reading and writing but is not closing.
+// It returns (false, errClosing) when fd is closing.
+func (fd *FD) tryReadWriteLock() (bool, error) {
+       if !fd.fdmu.rwlock(readlock, tryLock) {
+               if fd.closing() {
+                       return false, errClosing(fd.isFile)
+               }
+               return false, nil
+       }
+       if !fd.fdmu.rwlock(writeLock, tryLock) {
+               fd.fdmu.rwunlock(readlock) // unlock read lock acquired above
+               if fd.closing() {
+                       return false, errClosing(fd.isFile)
+               }
+               return false, nil
+       }
+       return true, nil
+}
+
 // readWriteUnlock removes a reference from fd and unlocks fd for reading and writing.
 // It also closes fd when the state of fd is set to closed and there
 // is no remaining reference.
 func (fd *FD) readWriteUnlock() {
-       fd.fdmu.rwunlock(true)
-       if fd.fdmu.rwunlock(false) {
+       fd.fdmu.rwunlock(readlock)
+       if fd.fdmu.rwunlock(writeLock) {
                fd.destroy()
        }
 }
index 949c5ea9385db7893417ca5e2399412d85979ee0..c9e74d0699dde15ce6153d4c51eea56374a76a3f 100644 (file)
@@ -11,7 +11,6 @@ import (
        "io"
        "runtime"
        "sync"
-       "sync/atomic"
        "syscall"
        "unicode/utf16"
        "unicode/utf8"
@@ -371,7 +370,7 @@ type FD struct {
        // Whether FILE_FLAG_OVERLAPPED was not set when opening the file.
        isBlocking bool
 
-       disassociated atomic.Bool
+       disassociated bool
 
        // readPinner and writePinner are automatically unpinned
        // before execIO returns.
@@ -404,7 +403,7 @@ func (fd *FD) addOffset(off int) {
 // pollable should be used instead of fd.pd.pollable(),
 // as it is aware of the disassociated state.
 func (fd *FD) pollable() bool {
-       return fd.pd.pollable() && !fd.disassociated.Load()
+       return fd.pd.pollable() && !fd.disassociated
 }
 
 // fileKind describes the kind of file.
@@ -477,12 +476,19 @@ func (fd *FD) Init(net string, pollable bool) error {
 
 // DisassociateIOCP disassociates the file handle from the IOCP.
 // The disassociate operation will not succeed if there is any
-// in-progress IO operation on the file handle.
+// in-progress I/O operation on the file handle.
 func (fd *FD) DisassociateIOCP() error {
-       if err := fd.incref(); err != nil {
+       // There is a small race window between execIO checking fd.disassociated and
+       // DisassociateIOCP setting it. NtSetInformationFile will fail anyway if
+       // there is any in-progress I/O operation, so just take a read-write lock
+       // to ensure there is no in-progress I/O and fail early if we can't get the lock.
+       if ok, err := fd.tryReadWriteLock(); err != nil || !ok {
+               if err == nil {
+                       err = errors.New("can't disassociate the handle while there is in-progress I/O")
+               }
                return err
        }
-       defer fd.decref()
+       defer fd.readWriteUnlock()
 
        if fd.isBlocking || !fd.pollable() {
                // Nothing to disassociate.
@@ -493,7 +499,8 @@ func (fd *FD) DisassociateIOCP() error {
        if err := windows.NtSetInformationFile(fd.Sysfd, &windows.IO_STATUS_BLOCK{}, unsafe.Pointer(&info), uint32(unsafe.Sizeof(info)), windows.FileReplaceCompletionInformation); err != nil {
                return err
        }
-       fd.disassociated.Store(true)
+       // tryReadWriteLock means we have exclusive access to fd.
+       fd.disassociated = true
        // Don't call fd.pd.close(), it would be too racy.
        // There is no harm on leaving fd.pd open until Close is called.
        return nil
index 57dc0c668f1d8479a78a98ce17e6ed9b7187c834..8634736387553d920db395bfa17d8e3c0bbc1303 100644 (file)
@@ -26,17 +26,17 @@ func (fdmu *FDMutex) IncrefAndClose() bool {
 }
 
 func (fdmu *FDMutex) ReadLock() bool {
-       return fdmu.fdmu.rwlock(true)
+       return fdmu.fdmu.rwlock(readlock, waitLock)
 }
 
 func (fdmu *FDMutex) ReadUnlock() bool {
-       return fdmu.fdmu.rwunlock(true)
+       return fdmu.fdmu.rwunlock(readlock)
 }
 
 func (fdmu *FDMutex) WriteLock() bool {
-       return fdmu.fdmu.rwlock(false)
+       return fdmu.fdmu.rwlock(writeLock, waitLock)
 }
 
 func (fdmu *FDMutex) WriteUnlock() bool {
-       return fdmu.fdmu.rwunlock(false)
+       return fdmu.fdmu.rwunlock(writeLock)
 }
index d549608b482d63288814761d52965110cf357139..540364cf420cdb78e616b972e9bfe68d115363e4 100644 (file)
@@ -1617,7 +1617,7 @@ func TestStdinOverlappedPipe(t *testing.T) {
        name := pipeName()
 
        // Create the read handle inherited by the child process.
-       r := newPipe(t, name, false, true)
+       r := newPipe(t, name, 4096, false, true)
        defer r.Close()
 
        // Create a write handle.
@@ -1674,18 +1674,18 @@ var currentProcess = sync.OnceValue(func() string {
 var pipeCounter atomic.Uint64
 
 func newBytePipe(t testing.TB, name string, overlapped bool) *os.File {
-       return newPipe(t, name, false, overlapped)
+       return newPipe(t, name, 4096, false, overlapped)
 }
 
 func newMessagePipe(t testing.TB, name string, overlapped bool) *os.File {
-       return newPipe(t, name, true, overlapped)
+       return newPipe(t, name, 4096, true, overlapped)
 }
 
 func pipeName() string {
        return `\\.\pipe\go-os-test-` + currentProcess() + `-` + strconv.FormatUint(pipeCounter.Add(1), 10)
 }
 
-func newPipe(t testing.TB, name string, message, overlapped bool) *os.File {
+func newPipe(t testing.TB, name string, bufSize uint32, message, overlapped bool) *os.File {
        wname, err := syscall.UTF16PtrFromString(name)
        if err != nil {
                t.Fatal(err)
@@ -1699,7 +1699,7 @@ func newPipe(t testing.TB, name string, message, overlapped bool) *os.File {
        if message {
                typ = windows.PIPE_TYPE_MESSAGE | windows.PIPE_READMODE_MESSAGE
        }
-       h, err := windows.CreateNamedPipe(wname, uint32(flags), uint32(typ), 1, 4096, 4096, 0, nil)
+       h, err := windows.CreateNamedPipe(wname, uint32(flags), uint32(typ), 1, bufSize, bufSize, 0, nil)
        if err != nil {
                t.Fatal(err)
        }
@@ -2172,6 +2172,58 @@ func TestFileWriteFdRace(t *testing.T) {
        }
 }
 
+func TestFileFdWithConcurrentIO(t *testing.T) {
+       t.Parallel()
+       name := pipeName()
+       pipe := newPipe(t, name, 0, true, true) // unbuffered pipe so Write blocks
+       file := newFileOverlapped(t, name, true)
+       const writeSize = 2
+       var wg sync.WaitGroup
+       wg.Go(func() {
+               // Ensure the Write is pending.
+               var tmp [writeSize / 2]byte
+               if _, err := file.Read(tmp[:]); err != nil {
+                       t.Error(err)
+               }
+               // Try to dissaciate the file from any IOCP.
+               pipe.Fd()
+               // Complete the Write.
+               if _, err := file.Read(tmp[:]); err != nil {
+                       t.Error(err)
+               }
+       })
+       // Write will block until the goroutine reads all 2 bytes.
+       var tmp [writeSize]byte
+       n, err := pipe.Write(tmp[:])
+       if err != nil {
+               t.Fatal(err)
+       }
+       if n != writeSize {
+               t.Fatalf("expected to write %d bytes, got %d", writeSize, n)
+       }
+       wg.Wait()
+
+       // Verify that the pipe is still associated with the Go runtime IOCP
+       // by trying to associate it with a new IOCP, which should fail.
+       iocp, err := windows.CreateIoCompletionPort(syscall.InvalidHandle, 0, 0, 0)
+       if err != nil {
+               t.Fatal(err)
+       }
+       defer syscall.CloseHandle(iocp)
+       sc, err := pipe.SyscallConn()
+       if err != nil {
+               t.Fatal(err)
+       }
+       if err := sc.Control(func(fd uintptr) {
+               _, err = windows.CreateIoCompletionPort(syscall.Handle(fd), iocp, 0, 0)
+               if err == nil {
+                       t.Fatal("pipe should still be associated with the Go runtime IOCP")
+               }
+       }); err != nil {
+               t.Fatal(err)
+       }
+}
+
 func TestSplitPath(t *testing.T) {
        t.Parallel()
        for _, tt := range []struct{ path, wantDir, wantBase string }{