From 54c46328ccb9d559fa21c09fd8e2dff22a99c72c Mon Sep 17 00:00:00 2001 From: qmuntal Date: Mon, 2 Feb 2026 10:42:28 +0100 Subject: [PATCH] internal/poll: avoid race between execIO and DisassociateIOCP 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 Reviewed-by: Damien Neil Reviewed-by: Michael Pratt Reviewed-by: Alex Brainman --- src/internal/poll/export_test.go | 2 +- src/internal/poll/fd_mutex.go | 55 +++++++++++++++++++++------- src/internal/poll/fd_windows.go | 21 +++++++---- src/internal/poll/file_plan9.go | 8 ++--- src/os/os_windows_test.go | 62 +++++++++++++++++++++++++++++--- 5 files changed, 119 insertions(+), 29 deletions(-) diff --git a/src/internal/poll/export_test.go b/src/internal/poll/export_test.go index 66d7c3274b..65549378e2 100644 --- a/src/internal/poll/export_test.go +++ b/src/internal/poll/export_test.go @@ -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 { diff --git a/src/internal/poll/fd_mutex.go b/src/internal/poll/fd_mutex.go index aa8f425235..aba22e9867 100644 --- a/src/internal/poll/fd_mutex.go +++ b/src/internal/poll/fd_mutex.go @@ -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() } } diff --git a/src/internal/poll/fd_windows.go b/src/internal/poll/fd_windows.go index 949c5ea938..c9e74d0699 100644 --- a/src/internal/poll/fd_windows.go +++ b/src/internal/poll/fd_windows.go @@ -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 diff --git a/src/internal/poll/file_plan9.go b/src/internal/poll/file_plan9.go index 57dc0c668f..8634736387 100644 --- a/src/internal/poll/file_plan9.go +++ b/src/internal/poll/file_plan9.go @@ -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) } diff --git a/src/os/os_windows_test.go b/src/os/os_windows_test.go index d549608b48..540364cf42 100644 --- a/src/os/os_windows_test.go +++ b/src/os/os_windows_test.go @@ -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 }{ -- 2.52.0