Use COM marshalling to exchange handles #40056
Use COM marshalling to exchange handles #40056OneBlue merged 15 commits intofeature/wsl-for-appsfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to switch image load/import APIs from passing raw handle values (ULONG) to using COM/RPC handle marshalling, by introducing a WSLCHandle discriminated union and updating call sites to pass marshalled handles.
Changes:
- Introduces
WSLCHandle/WSLCHandleTypein the IDL and updatesIWSLCSession::LoadImage/ImportImageto accept it. - Updates session-side implementation and several test/CLI call sites to pass handles via the new helper(s).
- Adds
ToCOMInputHandle/FromCOMInputHandlehelpers intended to translate betweenHANDLEandWSLCHandle.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| test/windows/WSLCTests.cpp | Updates tests to call LoadImage/ImportImage with ToCOMInputHandle and adds a broad using namespace ...wslutil. |
| src/windows/wslcsession/WSLCSession.h | Updates WSLCSession method signatures to match the new handle type. |
| src/windows/wslcsession/WSLCSession.cpp | Updates image load/import implementation to use FromCOMInputHandle and pass the handle to relay IO. |
| src/windows/WslcSDK/wslcsdk.cpp | Currently stubs out SDK image load/import wrappers (returns S_OK without performing work). |
| src/windows/wslc/services/ImageService.cpp | Updates CLI/service-side image load to build a WSLCHandle and call LoadImage. |
| src/windows/service/inc/wslc.idl | Adds WSLCHandle types and changes IWSLCSession method signatures to accept them. |
| src/windows/common/wslutil.h / .cpp | Adds ToCOMInputHandle and FromCOMInputHandle helper APIs for the new handle path. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/windows/service/inc/wslc.idl:504
- Changing WSLCBuildImageOptions.DockerfileHandle from ULONG to WSLCHandle changes the struct layout on the wire/ABI. Existing callers passing the old struct will break at runtime even if they don’t call the updated methods. Consider versioning this options struct (e.g., WSLCBuildImageOptions2) and adding a new BuildImage entry point/interface that consumes it, while keeping the original struct/method for compatibility.
typedef struct _WSLCBuildImageOptions
{
LPCWSTR ContextPath;
WSLCHandle DockerfileHandle;
WSLCStringArray Tags;
WSLCStringArray BuildArgs; // KEY=VALUE pairs passed as --build-arg to docker.
BOOL Verbose; // Show all build progress including internal steps and all statuses.
} WSLCBuildImageOptions;
benhillis
left a comment
There was a problem hiding this comment.
Couple questions. Can we also add a test that launches a session as non-admin, then tries to attach to reuse the session as admin? I think we already have one that verifies the other way does not work (and should not).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
src/windows/service/inc/wslc.idl:504
- Changing
WSLCBuildImageOptions.DockerfileHandlefromULONGtoWSLCHandlechanges the struct layout/wire format used by the existingIWSLCSession::BuildImagemethod, which is also an ABI-breaking change for existing clients. Prefer versioning the options struct (e.g.,WSLCBuildImageOptions2) and/or adding a newBuildImage2method on a versioned session interface, keeping the original struct unchanged.
typedef struct _WSLCBuildImageOptions
{
LPCWSTR ContextPath;
WSLCHandle DockerfileHandle;
WSLCStringArray Tags;
WSLCStringArray BuildArgs; // KEY=VALUE pairs passed as --build-arg to docker.
BOOL Verbose; // Show all build progress including internal steps and all statuses.
} WSLCBuildImageOptions;
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/windows/service/inc/wslc.idl:504
- WSLCBuildImageOptions.DockerfileHandle changed from ULONG to WSLCHandle. If this struct is part of the COM/SDK contract, changing the field type is also an ABI break for already-built clients. Consider introducing a new options struct (e.g., WSLCBuildImageOptions2) and a new BuildImage2 method on a versioned interface, rather than changing the existing struct layout in-place.
typedef struct _WSLCBuildImageOptions
{
LPCWSTR ContextPath;
WSLCHandle DockerfileHandle;
WSLCStringArray Tags;
WSLCStringArray BuildArgs; // KEY=VALUE pairs passed as --build-arg to docker.
BOOL Verbose; // Show all build progress including internal steps and all statuses.
} WSLCBuildImageOptions;
| [ | ||
| uuid(EF0661E4-6364-40EA-B433-E2FDF11F3519), | ||
| pointer_default(unique), | ||
| object | ||
| ] | ||
| interface IWSLCSession : IUnknown | ||
| { | ||
| HRESULT GetId([out] ULONG* Id); | ||
| HRESULT GetState([out] WSLCSessionState* State); | ||
|
|
||
| // Image management. | ||
| HRESULT PullImage([in] LPCSTR Image, [in, unique] const WslcRegistryAuthInformation* RegistryAuthenticationInformation, [in, unique] IProgressCallback* ProgressCallback); | ||
| HRESULT BuildImage([in] const WSLCBuildImageOptions* Options, [in, unique] IProgressCallback* ProgressCallback, [in, unique, system_handle(sh_event)] HANDLE CancelEvent); | ||
| HRESULT LoadImage([in] ULONG ImageHandle, [in, unique] IProgressCallback* ProgressCallback, [in] ULONGLONG ContentLength); | ||
| HRESULT ImportImage([in] ULONG ImageHandle, [in] LPCSTR ImageName, [in, unique] IProgressCallback* ProgressCallback, [in] ULONGLONG ContentLength); | ||
| HRESULT SaveImage([in] ULONG OutputHandle, [in] LPCSTR ImageNameOrID, [in, unique] IProgressCallback * ProgressCallback, [in, unique, system_handle(sh_event)] HANDLE CancelEvent); | ||
| HRESULT LoadImage([in] WSLCHandle ImageHandle, [in, unique] IProgressCallback* ProgressCallback, [in] ULONGLONG ContentLength); | ||
| HRESULT ImportImage([in] WSLCHandle ImageHandle, [in] LPCSTR ImageName, [in, unique] IProgressCallback* ProgressCallback, [in] ULONGLONG ContentLength); | ||
| HRESULT SaveImage([in] WSLCHandle OutputHandle, [in] LPCSTR ImageNameOrID, [in, unique] IProgressCallback * ProgressCallback, [in, unique, system_handle(sh_event)] HANDLE CancelEvent); | ||
| HRESULT ListImages([in, unique] const WSLCListImageOptions* Options, [out, size_is(, *Count)] WSLCImageInformation** Images, [out] ULONG* Count); |
There was a problem hiding this comment.
IWSLCSession's LoadImage/ImportImage/SaveImage parameter types changed from ULONG to WSLCHandle without versioning the interface UUID. This is a binary breaking change for any client compiled against the previous IDL. Please introduce a versioned interface (e.g., IWSLCSession2) for the WSLCHandle-based methods and keep the existing IWSLCSession contract stable.
…oneblue/handle-fix-2
Summary of the Pull Request
This change switches our HANDLE exchange logic to use COM marshalling instead of duplicating handles from / to the calling process.
This allows us to support exchanging handles with calling processes that have a higher privilege than the WSLASession.exe process, which allows for instance an elevated instance of wslc.exe to interact with a non-elevated session
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed