go: runtime,x/sys/windows: panic accessing (possibly) nil field of net_op
First of all, thank you all for everything that you do!
What version of Go are you using (go version)?
$ go version go version go1.20.1 windows/amd64
Does this issue reproduce with the latest release?
No, but reproducing the issue is timing dependent so my inability to reproduce seems to be the result of a different order of scheduling timeouts, etc.
What operating system and processor architecture are you using (go env)?
On Windows on amd64 hardware.
What did you do?
Using windows.WSAIoctl on an overlapped socket with a “zero” values in the windows.Overlapped struct will cause a panic in netpoll_windows.go at line 134 (as of the version of the code tagged at version 1.20.1) where the runtime attempts to dereference op.pd without checking whether it is nil.
The problem is that the runtime does not expect that something other than itself (i.e., the runtime) will add items to the list of operations that could be returned by a call to GetQueuedCompletionStatusEx. The comment
op *net_op // In reality it's *overlapped, but we cast it to *net_op anyway.
is telling.
Unfortunately, we cannot guarantee that is the case.
If an application added something to that list (e.g., through a use of windows.WSAIoctl) that does not complete before the scheduler does another “round” of netpoll, it is possible that op is not actually a net_op. If the application user does “the right thing” by passing a pointer to a “zerod” windows.Overlapped structure, the runtime will break.
There was a related issue from several years earlier that @bradfitz found and fixed: https://github.com/golang/go/issues/21172
This issue seems to be related.
I have two one-line patches to the sys and runtime packages that (I believe) provide a simple fix for the issue. I wanted to write this issue before posting them to gerrit so that there is something to reference. I will update this report with those patchsets.
See
sys/windows: Pad windows.Overlapped to protect the runtime | https://go-review.googlesource.com/c/sys/+/473163
and
473163: sys/windows: Pad windows.Overlapped to protect the runtime | https://go-review.googlesource.com/c/sys/+/473163
About this issue
- Original URL
- State: closed
- Created a year ago
- Comments: 42 (41 by maintainers)
Commits related to this issue
- Add a proof of concept for https://github.com/golang/go/issues/58870 — committed to hawkinsw/windowpane by hawkinsw a year ago
@qmuntal I believe that I know fully understand the problem and have a workable solution. I am going to spend time today writing a complete description of the problem and the proposed solution so that we have it for posterity and to use as a way to discuss whether to go forward with that solution. Let me know if that sounds okay!
@qmuntal – I have applied the patch and am running my test! Everything looks great … until it doesn’t!
I see an issue that might be specific to the that would not be visible to users. It would appear that under certain circumstances the
decrefmethod of theFDtype (I think that I got the go-specific terminology correct!) is called with (what appears to be) anilreceiver. It is on the tip of my brain why that happens but I working on writing out the specifics for why it would occur. I will keep you posted!Thanks (again!) for letting me work with you! Will
@qmuntal I am relieved – I was definitely starting to question my sanity! I really have enjoyed our co-investigation! Thank you, again, for letting me participate.
As I said, I had a thought about how to be proactive rather than reactive managing what completion entries (
overlappedEntrys) are retrieved fromGetQueuedCompletionStatusEx. I will let you know as soon as I have had a chance to do that. I apologize for any delay – my teaching load is a little high this semester!@qmuntal FYI: I can reproduce the error locally!
Please let me know if/how I can help! The repository that I posted has “reproducing” code written directly using the win32 APIs. It at least demonstrates that the behavior is real. I hope that the patch is helpful, too. It seems to do everything that needs to be done.