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

HvsockConn shutdown bugfix, added .IsClosed() functions #231

merged 1 commit into from Feb 17, 2022


Copy link

@helsaawy helsaawy commented Feb 7, 2022

The PR fixes bugs with HvsockConn CloseRead/Write where 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).shutdown function was not respecting whether to close a socket for reading or writing, and only shutting down the pipe for reading.

Currently, (*Process).CloseStdin runs into two issues:

  1. It attempts to shutdown a closed socket, which then calls syscall.Shutdown on a null socket handle. This PR preemptively returns ErrFileClosed to prevent that, and allow upstream callers to check for the situation where the socket is already closed.
  2. (*HvsockConn).CloseWrite does not actually close the connection for writing, since (*HvsockConn).shutdown ignores its parameter and closes the socket only for reading. This prevents hcsshim from using CloseWrite (from within (*Process).CloseStdin) to stop the copy operation from upstream pipes into the processes std in.

This PR fixes those issues.

Additionally, two functions have been added:

  1. (*HvsockConn).CloseReadWrite to expose shutting down both ends of the socket.
  2. (*win32File).IsClosed (and therefore win32Pipe and win32MessageBytePipe) and (*HvsockConn).IsClosed and 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

@helsaawy helsaawy requested a review from a team as a code owner February 7, 2022 23:43
@dcantah dcantah self-assigned this Feb 8, 2022
Copy link

ambarve commented Feb 8, 2022

Is this change fixing some bug? Can you also add some more context on that please?

@ambarve ambarve self-assigned this Feb 8, 2022
Copy link
Contributor Author

@ambarve I added more context, ptal

hvsock.go Outdated Show resolved Hide resolved
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 <>
Copy link

@ambarve ambarve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


@helsaawy helsaawy merged commit dfd7da8 into microsoft:master Feb 17, 2022
4 checks passed
@helsaawy helsaawy deleted the he/close branch February 22, 2022 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
None yet

Successfully merging this pull request may close these issues.

None yet

3 participants