Solve various issues found by verifier - #41445
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request addresses handle-lifetime and handle-type correctness issues surfaced by running the test suite under verifier, focusing on avoiding use-after-close and ensuring socket handles aren’t treated like generic Win32 handles.
Changes:
- Introduces/expands
HandleWrapperto support shared ownership (wil::shared_handle/wil::shared_socket) and uses it broadly for “unknown handle type” scenarios. - Updates WSLC session/container plumbing to retain socket lifetimes correctly during relays (notably Docker attach/exec paths) and to stop calling
CloseHandle()on sockets. - Updates tests and test utilities (
PartialHandleRead,WaitForOutput, etc.) to work with the new handle wrapper and avoid verifier issues.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/windows/WSLCTests.cpp | Updates tests to use HandleWrapper (Get()/Reset()) and stops readers before closing borrowed handles. |
| test/windows/UnitTests.cpp | Updates a comment (currently introduced a typo/garbling). |
| test/windows/Common.h | Adds PartialHandleRead::Stop() and updates WaitForOutput to accept HandleWrapper with an overload for wil::unique_handle. |
| test/windows/Common.cpp | Implements PartialHandleRead destructor/Stop() and updates WaitForOutput to move a HandleWrapper into the wait loop. |
| src/windows/wslcsession/WSLCVirtualMachine.cpp | Switches networking handle usage to HandleWrapper for correct lifetime/type handling. |
| src/windows/wslcsession/WSLCProcessIO.h | Changes TypedHandle to store HandleWrapper and updates accessors accordingly. |
| src/windows/wslcsession/WSLCProcess.h | Changes GetStdHandle return type to HandleWrapper. |
| src/windows/wslcsession/WSLCProcess.cpp | Updates GetStdHandle implementation signature/return type to HandleWrapper. |
| src/windows/wslcsession/WSLCContainer.h | Changes CreateRelayedProcessIO to take a wil::shared_socket for proper socket lifetime. |
| src/windows/wslcsession/WSLCContainer.cpp | Uses wil::shared_socket for Docker attach streams and updates relays to keep sockets alive across independently completing relays. |
| src/windows/wslcsession/ServiceProcessLauncher.h | Updates GetStdHandle override to return HandleWrapper. |
| src/windows/wslcsession/ServiceProcessLauncher.cpp | Updates GetStdHandle override implementation for HandleWrapper. |
| src/windows/wslcsession/main.cpp | Adds scope-exit cleanup to clear the WinRT factory cache. |
| src/windows/wslc/services/SessionService.cpp | Updates tty handle usage to use HandleWrapper::Get(). |
| src/windows/wslc/services/ContainerService.cpp | Updates stdin handle usage to use HandleWrapper::Get() after Release(). |
| src/windows/wslc/services/ConsoleService.h | Changes non-tty relay signature to accept HandleWrapper rvalues. |
| src/windows/wslc/services/ConsoleService.cpp | Updates relays to use HandleWrapper (IsValid()/Get()/Reset()) and adjusts tty handling to keep the wrapper alive. |
| src/windows/common/wslutil.h | Fixes COM output handle reset to close sockets via closesocket() and changes Release() to return a HandleWrapper. |
| src/windows/common/wslutil.cpp | Implements COMOutputHandle::Release() returning a HandleWrapper that preserves socket vs handle semantics. |
| src/windows/common/WSLCProcessLauncher.h | Updates process interface to return HandleWrapper from GetStdHandle. |
| src/windows/common/WSLCProcessLauncher.cpp | Updates client process GetStdHandle implementation to return HandleWrapper. |
| src/windows/common/HandleIO.h | Extends HandleWrapper to support shared handle/socket ownership and explicit move operations. |
| src/windows/common/HandleIO.cpp | Implements HandleWrapper move operations, shared-handle/socket constructors, and IsValid(). |
Suppressed comments (1)
src/windows/wslcsession/WSLCContainer.cpp:2774
- shutdown() is a WinSock API; on failure its error is retrieved with WSAGetLastError(), but LOG_LAST_ERROR_IF() logs GetLastError(). Switching to LOG_LAST_ERROR_IF_MSG(..., "WSAGetLastError %d", WSAGetLastError()) avoids incorrect error reporting.
auto closeStdin = [stream]() { LOG_LAST_ERROR_IF(shutdown(stream.get(), SD_SEND) == SOCKET_ERROR); };
if (WI_IsFlagSet(flags, WSLCProcessFlagsStdin))
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+103
to
+110
| if (Type == WSLCHandleTypeSocket) | ||
| { | ||
| LOG_LAST_ERROR_IF(closesocket(reinterpret_cast<SOCKET>(Handle.Socket)) == SOCKET_ERROR); | ||
| } | ||
| else | ||
| { | ||
| LOG_IF_WIN32_BOOL_FALSE(CloseHandle(Handle.File)); | ||
| } |
Comment on lines
1032
to
1035
| // Keep the socket alive until both relays are destroyed since they complete independently. | ||
| // This is required for docker to know when stdin is closed. | ||
| auto onInputComplete = [handle = ioHandle.get()]() { LOG_LAST_ERROR_IF(shutdown(handle, SD_SEND) == SOCKET_ERROR); }; | ||
| auto onInputComplete = [ioHandle]() { LOG_LAST_ERROR_IF(shutdown(ioHandle.get(), SD_SEND) == SOCKET_ERROR); }; | ||
|
|
Comment on lines
+40
to
+50
| const auto type = Type; | ||
| const auto handle = Handle.File; | ||
| Handle.File = nullptr; | ||
| Type = WSLCHandleTypeUnknown; | ||
|
|
||
| if (type == WSLCHandleTypeSocket) | ||
| { | ||
| return wsl::windows::common::io::HandleWrapper{wil::unique_socket{reinterpret_cast<SOCKET>(handle)}}; | ||
| } | ||
|
|
||
| return wsl::windows::common::io::HandleWrapper{wil::unique_handle{handle}}; |
Craig Loewen (craigloewen-msft)
approved these changes
Aug 26, 2026
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.
Summary of the Pull Request
This change solves various issues found by verifier:
See: #41440
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed