]> Cypherpunks repositories - gostls13.git/commitdiff
Revert "runtime: make netpollBreak entries identifiable on Windows"
authorQuim Muntal <quimmuntal@gmail.com>
Mon, 5 Feb 2024 14:15:26 +0000 (14:15 +0000)
committerQuim Muntal <quimmuntal@gmail.com>
Wed, 7 Feb 2024 20:08:01 +0000 (20:08 +0000)
This reverts commit 29746b4814bb76a3d5a36fc86967b358f11bbb93.

Reason for revert: Windows builders are flaky since this CL. Needs investigation. See https://build.golang.org/log/70d5d039b57b505870c9cc4e61de320df06a6f3a.

Change-Id: I8a5874bb057785497d03b9450819578de7faeb47
Reviewed-on: https://go-review.googlesource.com/c/go/+/561276
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

src/internal/poll/fd_windows_test.go
src/runtime/netpoll_windows.go

index 117da1aa1d38d3bd4c0cc0dea03566e0b8e3dfa3..1cee18dcba53fc8d0a4081f9e1071ac05226a24c 100644 (file)
@@ -133,14 +133,24 @@ func TestWSASocketConflict(t *testing.T) {
        var outbuf _TCP_INFO_v0
        cbbr := uint32(0)
 
-       var ov syscall.Overlapped
+       var ovs []syscall.Overlapped = make([]syscall.Overlapped, 2)
+       // Attempt to exercise behavior where a user-owned syscall.Overlapped
+       // induces an invalid pointer dereference in the Windows-specific version
+       // of runtime.netpoll.
+       ovs[1].Internal -= 1
+
        // Create an event so that we can efficiently wait for completion
        // of a requested overlapped I/O operation.
-       ov.HEvent, _ = windows.CreateEvent(nil, 0, 0, nil)
-       if ov.HEvent == 0 {
+       ovs[0].HEvent, _ = windows.CreateEvent(nil, 0, 0, nil)
+       if ovs[0].HEvent == 0 {
                t.Fatalf("could not create the event!")
        }
 
+       // Set the low bit of the Event Handle so that the completion
+       // of the overlapped I/O event will not trigger a completion event
+       // on any I/O completion port associated with the handle.
+       ovs[0].HEvent |= 0x1
+
        if err = fd.WSAIoctl(
                SIO_TCP_INFO,
                (*byte)(unsafe.Pointer(&inbuf)),
@@ -148,7 +158,7 @@ func TestWSASocketConflict(t *testing.T) {
                (*byte)(unsafe.Pointer(&outbuf)),
                uint32(unsafe.Sizeof(outbuf)),
                &cbbr,
-               &ov,
+               &ovs[0],
                0,
        ); err != nil && !errors.Is(err, syscall.ERROR_IO_PENDING) {
                t.Fatalf("could not perform the WSAIoctl: %v", err)
@@ -157,12 +167,12 @@ func TestWSASocketConflict(t *testing.T) {
        if err != nil && errors.Is(err, syscall.ERROR_IO_PENDING) {
                // It is possible that the overlapped I/O operation completed
                // immediately so there is no need to wait for it to complete.
-               if res, err := syscall.WaitForSingleObject(ov.HEvent, syscall.INFINITE); res != 0 {
+               if res, err := syscall.WaitForSingleObject(ovs[0].HEvent, syscall.INFINITE); res != 0 {
                        t.Fatalf("waiting for the completion of the overlapped IO failed: %v", err)
                }
        }
 
-       if err = syscall.CloseHandle(ov.HEvent); err != nil {
+       if err = syscall.CloseHandle(ovs[0].HEvent); err != nil {
                t.Fatalf("could not close the event handle: %v", err)
        }
 }
index 7f68077d099bf221f29867f6a8dfe935bc5ae056..59377bc5885d5678acc4f6e9eec4b8fb051e5529 100644 (file)
@@ -13,49 +13,18 @@ const _DWORD_MAX = 0xffffffff
 
 const _INVALID_HANDLE_VALUE = ^uintptr(0)
 
-// Sources are used to identify the event that created an overlapped entry.
-// The source values are arbitrary. There is no risk of collision with user
-// defined values because the only way to set the key of an overlapped entry
-// is using the iocphandle, which is not accessible to user code.
-const (
-       netpollSourceReady = iota + 1
-       netpollSourceBreak
-)
-
-const (
-       netPollKeySourceBits = 8
-       // Use 19 bits for the fdseq, which is enough to represent all possible
-       // values on 64-bit systems (fdseq is truncated to taggedPointerBits).
-       // On 32-bit systems, taggedPointerBits is set to 32 bits, so we are
-       // losing precision here, but still have enough entropy to avoid collisions
-       // (see netpollopen).
-       netPollKeyFDSeqBits = 19
-       netPollKeyFDSeqMask = 1<<netPollKeyFDSeqBits - 1
-)
-
-// packNetpollKey creates a key from a source and a tag.
-// Tag bits that don't fit in the result are discarded.
-func packNetpollKey(source uint8, tag uintptr) uintptr {
-       return uintptr(source) | tag<<netPollKeySourceBits
-}
-
-// unpackNetpollKey returns the source and the tag from a taggedPointer.
-func unpackNetpollKey(key uintptr) (source uint8, tag uintptr) {
-       return uint8(key), key >> netPollKeySourceBits
-}
-
 // net_op must be the same as beginning of internal/poll.operation.
 // Keep these in sync.
 type net_op struct {
        // used by windows
-       _ overlapped
+       o overlapped
        // used by netpoll
        pd   *pollDesc
        mode int32
 }
 
 type overlappedEntry struct {
-       key      uintptr
+       key      *pollDesc
        op       *net_op // In reality it's *overlapped, but we cast it to *net_op anyway.
        internal uintptr
        qty      uint32
@@ -80,19 +49,8 @@ func netpollIsPollDescriptor(fd uintptr) bool {
 }
 
 func netpollopen(fd uintptr, pd *pollDesc) int32 {
-       // The tag is used for two purposes:
-       // - identify stale pollDescs. See go.dev/issue/59545.
-       // - differentiate between entries from internal/poll and entries from
-       //   outside the Go runtime, which we want to skip. User code has access
-       //   to fd, therefore it can run async operations on it that will end up
-       //   adding overlapped entries to our iocp queue. See go.dev/issue/58870.
-       //   By setting the tag to the pollDesc's fdseq, the only chance of
-       //   collision is if a user creates an overlapped struct with a fdseq that
-       //   matches the fdseq of the pollDesc passed to netpollopen, which is quite
-       //   unlikely given that fdseq is not exposed to user code.
-       tag := pd.fdseq.Load() & netPollKeyFDSeqMask
-       key := packNetpollKey(netpollSourceReady, tag)
-       if stdcall4(_CreateIoCompletionPort, fd, iocphandle, key, 0) == 0 {
+       // TODO(iant): Consider using taggedPointer on 64-bit systems.
+       if stdcall4(_CreateIoCompletionPort, fd, iocphandle, uintptr(unsafe.Pointer(pd)), 0) == 0 {
                return int32(getlasterror())
        }
        return 0
@@ -113,8 +71,7 @@ func netpollBreak() {
                return
        }
 
-       key := packNetpollKey(netpollSourceBreak, 0)
-       if stdcall4(_PostQueuedCompletionStatus, iocphandle, 0, key, 0) == 0 {
+       if stdcall4(_PostQueuedCompletionStatus, iocphandle, 0, 0, 0) == 0 {
                println("runtime: netpoll: PostQueuedCompletionStatus failed (errno=", getlasterror(), ")")
                throw("runtime: netpoll: PostQueuedCompletionStatus failed")
        }
@@ -129,6 +86,7 @@ func netpoll(delay int64) (gList, int32) {
        var entries [64]overlappedEntry
        var wait, n, i uint32
        var errno int32
+       var op *net_op
        var toRun gList
 
        mp := getg().m
@@ -169,31 +127,21 @@ func netpoll(delay int64) (gList, int32) {
        mp.blocked = false
        delta := int32(0)
        for i = 0; i < n; i++ {
-               e := &entries[i]
-               key, tag := unpackNetpollKey(e.key)
-               switch {
-               case key == netpollSourceBreak:
-                       netpollWakeSig.Store(0)
-                       if delay == 0 {
-                               // Forward the notification to the blocked poller.
-                               netpollBreak()
-                       }
-               case key == netpollSourceReady:
-                       if e.op == nil || e.op.pd == nil || e.op.pd.fdseq.Load()&netPollKeyFDSeqMask != tag&netPollKeyFDSeqMask {
-                               // Stale entry or entry from outside the Go runtime and internal/poll, ignore.
-                               // See go.dev/issue/58870.
-                               continue
-                       }
-                       // Entry from internal/poll.
-                       mode := e.op.mode
+               op = entries[i].op
+               if op != nil && op.pd == entries[i].key {
+                       mode := op.mode
                        if mode != 'r' && mode != 'w' {
                                println("runtime: GetQueuedCompletionStatusEx returned net_op with invalid mode=", mode)
                                throw("runtime: netpoll failed")
                        }
-                       delta += netpollready(&toRun, e.op.pd, mode)
-               default:
-                       println("runtime: GetQueuedCompletionStatusEx returned net_op with invalid key=", e.key)
-                       throw("runtime: netpoll failed")
+                       delta += netpollready(&toRun, op.pd, mode)
+               } else {
+                       netpollWakeSig.Store(0)
+                       if delay == 0 {
+                               // Forward the notification to the
+                               // blocked poller.
+                               netpollBreak()
+                       }
                }
        }
        return toRun, delta