Wait for a cancelled HID read to actually end - #56
Merged
Conversation
CancelIo only requests cancellation and returns at once, so the driver could still complete the read afterwards — writing into buf and into overlapped after both had been handed back. overlapped is a local that dies with the return, and the caller reuses buf immediately, so the kernel wrote into memory the runtime had given to somebody else. That is heap corruption, and it surfaced far from here: seventeen crash reports since 1.9.4, most of them "fatal error: selectgo: bad wakeup" pointing at a select in the cooling poll loop that has nothing to do with any of it. CancelIoEx, because CancelIo only cancels what the calling thread started and a goroutine does not stay on one thread — it could cancel nothing and still report success. Then GetOverlappedResult with wait=true, which is what makes the operation over before the buffers go back. Only reachable with the special-hardware option on, which is off by default. It went from rare to constant in 1.9.4, when the cooling source began polling once a second instead of reading on the measurement tick. Fixes #55
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #55.
What was wrong
CancelIoonly requests cancellation and returns immediately. The driver could still complete the read afterwards, writing intobufand intooverlapped— butoverlappedis a local that dies with the return, and the caller reusesbuffor the next read. The kernel therefore wrote into memory the Go runtime had already handed to something else.That is heap corruption. It surfaces nowhere near the cause: seventeen crash reports since 1.9.4, most of them
fatal error: selectgo: bad wakeupwith a stack pointing at aselectin the cooling poll loop that has nothing to do with HID at all.A second defect in the same two lines:
CancelIocancels only what the calling thread issued, and a goroutine does not stay on one thread. It could cancel nothing and still report success.The fix
CancelIoEx, thenGetOverlappedResultwithwait=true. The waiting is the point — it is what makes the operation over before the buffers go back.ERROR_OPERATION_ABORTEDis the expected outcome of a cancelled read and is not reported.Scope
Only reachable with the special-hardware option on, which is off by default. It went from rare to constant in 1.9.4, when the cooling source started polling once a second instead of reading on the measurement tick — the timeout path went from occasional to continuous.
On testing
There is no unit test. The failure needs a real HID device, a read that times out, and the allocator to reuse the buffer before the driver writes — a race that cannot be staged in a test without faking the whole overlapped I/O layer, at which point the test would only assert the shape of the code it was written from.
build.ps1 -Checkpasses; the real verification is a machine with a Kraken running for a day without a crash report.