HvsockConn shutdown bugfix, added .IsClosed() functions#231
Merged
helsaawy merged 1 commit intomicrosoft:masterfrom Feb 17, 2022
Merged
HvsockConn shutdown bugfix, added .IsClosed() functions#231helsaawy merged 1 commit intomicrosoft:masterfrom
helsaawy merged 1 commit intomicrosoft:masterfrom
Conversation
Contributor
|
Is this change fixing some bug? Can you also add some more context on that please? |
Contributor
Author
|
@ambarve I added more context, ptal |
dcantah
approved these changes
Feb 15, 2022
ambarve
reviewed
Feb 17, 2022
Corrected `HvsockConn` `CloseRead`/`Write` to check if the socket has been closed before attempting to shutdown the socket for reading or writing. Additionally, `shutdown` was not respecting whether to shutdown reading or writing, and only shutting down the socket for reading. Added `IsClosed` function to `win32File` (and therefore `win32Pipe` and `win32MessageBytePipe`) and `HvsockConn` to check if the socket/pipe has been closed already. Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>
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
3 participants
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.
The PR fixes bugs with
HvsockConnCloseRead/Writewhere shutdown is attempted on closed sockets, which returns a cryptic error:close hvsock [...]: shutdown: An operation was attempted on something that is not a socket, and where the(*HvsockConn).shutdownfunction was not respecting whether to close a socket for reading or writing, and only shutting down the pipe for reading.Currently,
(*Process).CloseStdinruns into two issues:syscall.Shutdownon a null socket handle. This PR preemptively returnsErrFileClosedto prevent that, and allow upstream callers to check for the situation where the socket is already closed.(*HvsockConn).CloseWritedoes not actually close the connection for writing, since(*HvsockConn).shutdownignores its parameter and closes the socket only for reading. This prevents hcsshim from usingCloseWrite(from within(*Process).CloseStdin) to stop the copy operation from upstream pipes into the processesstd in.This PR fixes those issues.
Additionally, two functions have been added:
(*HvsockConn).CloseReadWriteto expose shutting down both ends of the socket.(*win32File).IsClosed(and thereforewin32Pipeandwin32MessageBytePipe) and(*HvsockConn).IsClosedand to check if the socket/pipe has already been closed, since the structs already track internally if they have been closed or not.Signed-off-by: Hamza El-Saawy hamzaelsaawy@microsoft.com