Fix use-after-free when Windows monitor fails with pending I/O#340
Fix use-after-free when Windows monitor fails with pending I/O#340broken-circle wants to merge 2 commits into
Conversation
The `reconcile*` methods coded the same general sequence. Replace both with `settlePendingOverlapped()`. The read `nil`-branch surfaces a nonzero return as `.read` and otherwise falls through to the drain, matching every arm of the former read reconcile; the write `nil`-branch discards the count, matching the former write reconcile. No behavior change.
The read and write cancellation paths reconciled a pending ReadFile/WriteFile before its buffer was freed, but only on the signal stream's finished (`nil`) branch. When the stream instead threw an error, the catch branch rethrew it with a ReadFile/WriteFile still pending against the caller's buffer, releasing it while the kernel could still read or write it. Route both catch branches through `settlePendingOverlapped()` before rethrowing, so the operation is cancelled and waited on before the buffer goes out of scope.
|
Great to see! Are there any additional tests you can think of adding which would have exposed some of these last few Windows issues you fixed? I understand it might be difficult due to the non-deterministic nature but thought I'd ask anyways. |
|
Thanks! I'll do some digging and follow up likely sometime tomorrow. The read (#325) and write (#336) bugs are races between an operation completing and a child terminating. I want to check whether that interleaving can be forced by controlling the child's lifecycle and the pipe's state without instrumenting the monitor. The catch-path bug in this PR triggers on an internal completion-port failure that nothing in the public API could induce. I think that one can't be structurally forced without a fault-injection hook in the monitor's loop, which we probably wouldn't want to ship. |
|
I thought about this and ran some tests, and reached the same conclusion as before: there are no meaningful regression tests we can add for these three fixes, on reachability and observability grounds. Overall, all three are the same shape: a branch that should call The read branch (#325) is reachable but not observable. The failure is a completed read dropped before its yielded count is consumed, and observing that requires catching the narrow window between the kernel completing the read and the monitor yielding the count. The competing event is: the subprocess exits and propagates through The write branch (#336) is also reachable but not observable. The failure is a subprocess terminating mid-write; the bytes were destined for a pipe whose reader is already gone, and the returned count is identical with or without the fix, so there's nothing to assert. The only trace is the lifetime violation, and no sanitizer in this CI sees it: ASan/TSan instrument userland accesses, whereas this is the kernel completing the write into freed storage (the borrowed The |
Follow-up to #325 and #336, which fixed the same class of lifetime bug on the Windows read and write cancellation paths. This PR fixes the same bug on the branch where the stream throws an error.
On Windows,
issueAndAwaitRead()andwrite(_:to:for:)await an overlappedReadFile()/WriteFile()completion through the signal stream. When the stream finishes without delivering that completion, both reconcile the pending operation before its buffer is freed. When the stream throws an error, thecatchbranch rethrows with the operation still pending against the caller's buffer: a pendingReadFile()is the kernel writing intoresultBuffer, and a pendingWriteFile()is the kernel reading from the caller's borrowedspanand poised to write into the stackoverlapped. Both are released as the frame unwinds, so the kernel can touch freed storage.This PR routes both
catchbranches throughsettlePendingOverlapped()before rethrowing, so a pending operation is cancelled withCancelIoEx()and waited on withGetOverlappedResult()before its buffer goes out of scope. On the thrown-error path the reconciliation provides only lifetime safety; the returned bytes are discarded. The previousreconcile*methods are folded into the new method.The stream throws only when the IOCP monitor finishes every continuation with an error, which it does on a fatal
GetQueuedCompletionStatus()failure that's neitherERROR_BROKEN_PIPEnorERROR_OPERATION_ABORTED. That same failure kills the monitor, so it's already gone by the time the reconciliation runs. The kernel still sets the file handle's signal on completion independently of any IOCP dequeue, so the wait resolves without it; the completion port stays open (onlyshutdown()closes it), so when the operation is still pending the packet the abort enqueues is never dequeued, producing a one-packet leak on an already-failed subsystem.On non-Windows platforms, the read and write paths issue a synchronous
read()/write()and retain no reference to the buffer across theawait. The stream can throw, but no operation is pending against a buffer when it does, so there's nothing to reconcile.Testing and documentation
As with #336, this bug produces no observable behavioral difference to assert against: the returned count and captured output are identical with or without the fix, and the defect would surface only as memory corruption under a checking allocator whose heap reuse lands in the freed window. Forcing the interleaving requires injecting a
GetQueuedCompletionStatus()failure while an overlapped operation is parked, which has no entry point without a hook in the monitor thread (which isn't suitable to ship, as discussed in #325).Related documentation:
CancelIoEx()completion semantics:GetOverlappedResult()NULLhEventfallback to file handle: