Various improvements to interactions with user-provided handles#14486
Various improvements to interactions with user-provided handles#14486OneBlue wants to merge 2 commits intofeature/wsl-for-appsfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves WSLA service handling of user-provided handles by (1) attempting to unblock session termination when operations are using user handles, and (2) reducing access rights when duplicating handles between the service and client processes.
Changes:
- Add
UserHandleRAII + session tracking of in-flight user handles, and invoke cancellation duringWSLASession::Terminate(). - Reduce access rights on duplicated handles (both from caller → service and service → caller) to least-needed flags.
- Add a WSLA test intended to validate cancellation/unblocking behavior for synchronous I/O scenarios.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/windows/WSLATests.cpp | Adds a new test covering session termination while a user-handle I/O operation is blocked. |
| src/windows/wslasession/WSLASession.h | Introduces UserHandle and per-session tracking of user handles undergoing I/O. |
| src/windows/wslasession/WSLASession.cpp | Implements UserHandle, OpenUserHandle/ReleaseUserHandle, and termination-time cancellation logic; updates call sites to use reduced-access user handle duplication. |
| src/windows/wslasession/WSLAContainer.cpp | Reduces access when duplicating stdin/stdout/stderr/tty handles back to the client; uses OpenUserHandle for export output. |
| src/windows/common/wslutil.h | Extends DuplicateHandleFromCallingProcess to accept an optional desired access mask. |
| src/windows/common/wslutil.cpp | Implements the desired-access behavior for DuplicateHandleFromCallingProcess. |
| UserHandle& UserHandle::operator=(UserHandle&& Other) | ||
| { | ||
| if (this != &Other) | ||
| { | ||
| m_session = Other.m_session; | ||
| m_handle = std::move(Other.m_handle); | ||
|
|
||
| Other.m_session = nullptr; | ||
| } |
There was a problem hiding this comment.
UserHandle move-assignment overwrites an existing m_handle without first removing the old handle from the session’s tracking list. If a UserHandle is ever reassigned while already holding a handle (e.g., via std::optional reassignment), the old HANDLE will be closed by wil::unique_handle but will remain in m_userHandles, leading to stale entries and potentially cancelling/acting on an unrelated handle value later. Update operator= to release/unregister any currently-held handle before taking ownership, or delete move-assignment if it’s not intended to be used.
| void WSLASession::CancelUserHandleIO() | ||
| { | ||
| std::lock_guard lock(m_userHandlesLock); | ||
| for (auto handle : m_userHandles) | ||
| { | ||
| // Cancell all IO on the handle. | ||
| // N.B. This only cancels IO happening in this process. | ||
| if (!CancelIoEx(handle, nullptr)) | ||
| { | ||
| LOG_LAST_ERROR_IF(GetLastError() != ERROR_NOT_FOUND); | ||
| } | ||
| } |
There was a problem hiding this comment.
CancelUserHandleIO calls CancelIoEx(handle, nullptr), which only cancels I/O issued by the calling thread. In the scenario this PR targets (a worker thread blocked in synchronous ReadFile/WriteFile on a non-overlapped handle), calling CancelIoEx from the termination thread will not unblock that stuck thread. Consider tracking the issuing thread(s) and using CancelSynchronousIo on those thread handles, or restructuring the relay to avoid synchronous blocking I/O on non-overlapped user handles.
| std::lock_guard lock(m_userHandlesLock); | ||
| for (auto handle : m_userHandles) | ||
| { | ||
| // Cancell all IO on the handle. |
There was a problem hiding this comment.
Typo in comment: "Cancell" → "Cancel".
| // Cancell all IO on the handle. | |
| // Cancel all IO on the handle. |
| // Create a blocked operation that will cause the service to get stuck on WriteFile() call. | ||
| // Because the pipe handle that we're passing in doesn't support overlapped IO, the service will get stuck in a | ||
| // synchronous WriteFile() call. Validate that terminating the session correctly cancels the IO. |
There was a problem hiding this comment.
The comment describes the service getting stuck in a synchronous WriteFile(), but the rest of the test (and the note below) discusses being stuck in ReadFile() on the pipe. This is misleading when debugging failures—please update the comment to reflect the actual blocking call being exercised.
| // Create a blocked operation that will cause the service to get stuck on WriteFile() call. | |
| // Because the pipe handle that we're passing in doesn't support overlapped IO, the service will get stuck in a | |
| // synchronous WriteFile() call. Validate that terminating the session correctly cancels the IO. | |
| // Create a blocked operation that will cause the service to get stuck on a ReadFile() call on the pipe. | |
| // Because the pipe handle that we're passing in doesn't support overlapped IO, the service will get stuck in a | |
| // synchronous ReadFile() call. Validate that terminating the session correctly cancels the IO. |
Summary of the Pull Request
This change brings improvements to the way the service deals with user handles.
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed