Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Win32 potential resource handling and timeout issue with async I/O #1574

Closed
dcoutts opened this issue Feb 4, 2020 · 0 comments · Fixed by #1627
Closed

Win32 potential resource handling and timeout issue with async I/O #1574

dcoutts opened this issue Feb 4, 2020 · 0 comments · Fixed by #1627
Assignees
Labels
audit R9B audit issues raised by R9B Windows

Comments

@dcoutts
Copy link
Contributor

dcoutts commented Feb 4, 2020

ouroboros-network/Win32-network/src/System/Win32/Async.hs implements readHandle by initiating an async read then calling waitForCompletion, which will, via an MVar, wait for
c_GetQueuedCompletionStatus to complete in another thread, which passes in maxBound for timeout, which will be passed to HsGetQueuedCompletionStatus and then GetQueuedCompletionStatus, which can result in an infinite wait. ouroboros-network/Win32-network/test/Test/Async.hs includes a test interrupting this operation by calling killThread, which in combination with the surrounding code will close all the pipe and port handles. Although closing all related handles will break the infinite wait, it is a blunt hammer. Instead, prefer CancelIoEx to cancel a request individually or use timed instead of infinite waits.

As reported by Root9B.

@coot coot added this to the S7 2020-02-27 milestone Feb 24, 2020
@vhulchenko-iohk vhulchenko-iohk added the R9B audit issues raised by R9B label Feb 24, 2020
@coot coot removed this from the S7 2020-02-27 milestone Feb 28, 2020
iohk-bors bot added a commit that referenced this issue Mar 24, 2020
1627: CancelIoEx and overal review of Win32-network r=coot a=coot

The primary goal of this PR was to add `CancelIoEx` to fix #1574.  In doing so I changed the interface and the end result is that:

* we don't use C type level wrappers for windows syscalls any more (apart from `GetQueuedCompletionStatus` where we need to use a windwo kernel C macro.
* using `GADTs` to ensure that we use the right combination of `OVERLAPPED` / `WSAOVERLAPPED`, `HANDLE` / `SOCKET` and `ErrCode` / `WsaErrCode`.
* there is a closed type family which ensures that some of the `castPtr` usage is actually safe, it is not exposed.  It could be removed, but then using `castPtr` would leak out to various modules - so I preferred the more elegant and local, though type havier approach.
* more tests, especially for error conditions in which we need to assure that we are not deallocating the stable pointer twice 💣 

This PR is done on top of `coot/windows-vectored-io` (and it merges into that branch) - this makes the scope of this PR adequate for the proposed changes.  For the moment the windows stuff is stacked on CI, but when devops will help with it I will be able to merge all the PRs in turn.

1840: Update dependency on cardano-ledger-specs r=mrBliss a=mrBliss



Co-authored-by: Marcin Szamotulski <[email protected]>
Co-authored-by: Thomas Winant <[email protected]>
@iohk-bors iohk-bors bot closed this as completed in 5c253f7 Mar 24, 2020
@vhulchenko-iohk vhulchenko-iohk added this to the S10 2020-04-09 milestone Apr 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audit R9B audit issues raised by R9B Windows
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants