do-not-merge: virtionet port mapping POC#40145
do-not-merge: virtionet port mapping POC#40145OneBlue wants to merge 2 commits intofeature/wsl-for-appsfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends WSLC’s VirtioProxy networking path to support mapping/unmapping ports via the VirtioNetworking engine (including anonymous/ephemeral host ports), and wires container port publishing to use the richer mapping parameters (family/protocol/bind address).
Changes:
- Added new
IWSLCVirtualMachine::{MapVirtioNetPort,UnmapVirtioNetPort}APIs and aWSLC_EPHEMERAL_PORTconstant. - Switched WSLC session port mapping in VirtioProxy mode from relay-based mapping to VirtioNetworking-backed mapping (with allocated-port writeback for ephemeral binds).
- Updated container creation to allow UDP, host-IP binds, and ephemeral host ports when publishing ports.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/windows/wslcsession/WSLCVirtualMachine.h | Adds VMPortMapping::SetHostPort for allocated-port writeback. |
| src/windows/wslcsession/WSLCVirtualMachine.cpp | Uses new VirtioNet port mapping APIs in VirtioProxy mode and writes back allocated ephemeral host ports. |
| src/windows/wslc/services/ContainerService.cpp | Enables UDP/host-IP/ephemeral host port publishing by passing family/protocol/bind address to the launcher. |
| src/windows/service/inc/wslc.idl | Introduces WSLC_EPHEMERAL_PORT and new IWSLCVirtualMachine virtio port mapping methods. |
| src/windows/service/exe/HcsVirtualMachine.h | Declares COM method implementations for virtio port mapping/unmapping. |
| src/windows/service/exe/HcsVirtualMachine.cpp | Implements the new COM methods by delegating to wsl::core::VirtioNetworking. |
| src/windows/common/VirtioNetworking.h | Adds MapPort/UnmapPort and refactors ModifyOpenPorts to return allocated ports. |
| src/windows/common/VirtioNetworking.cpp | Implements virtio-backed port map/unmap and ephemeral host port allocation handling. |
|
|
||
| const auto listenAddrW = wsl::shared::string::MultiByteToWide(ListenAddress); | ||
|
|
||
| ModifyOpenPorts(c_eth0DeviceName, nullptr, HostPort, GuestPort, Protocol, false); |
There was a problem hiding this comment.
VirtioNetworking::UnmapPort ignores the provided ListenAddress (it converts it to wide but then never uses it) and calls ModifyOpenPorts with hostAddress=nullptr. This produces a portString with an empty listen_addr, which is unlikely to match the mapping created by MapPort and may cause unmap to fail or leak the mapping. Pass the same ListenAddress through to ModifyOpenPorts (or, if listen_addr should not be present for allocate=false, update ModifyOpenPorts to omit listen_addr when isOpen==false and ensure unmap uses the expected format).
| ModifyOpenPorts(c_eth0DeviceName, nullptr, HostPort, GuestPort, Protocol, false); | |
| ModifyOpenPorts(c_eth0DeviceName, listenAddrW.c_str(), HostPort, GuestPort, Protocol, false); |
| // format: tag={tag}[;host_port={port}];guest_port={port}[;listen_addr={addr}|;allocate=false][;udp] | ||
| std::wstring portString = std::format(L"tag={};guest_port={};listen_addr={}", tag, GuestPort, hostAddress); | ||
|
|
||
| if (HostPort != WSLC_EPHEMERAL_PORT) | ||
| { | ||
| std::wstring portString = std::format(L"tag={};port_number={}", tag, INETADDR_PORT(reinterpret_cast<const SOCKADDR*>(&addr))); | ||
| if (protocol == IPPROTO_UDP) | ||
| { | ||
| portString += L";udp"; | ||
| } | ||
| portString += std::format(L";host_port={}", HostPort); | ||
| } | ||
|
|
||
| if (!isOpen) | ||
| { | ||
| portString += L";allocate=false"; | ||
| } | ||
| else | ||
| { | ||
| const auto addrStr = wsl::windows::common::string::SockAddrInetToWstring(addr); | ||
| portString += std::format(L";listen_addr={}", addrStr); | ||
| } | ||
| if (!isOpen) | ||
| { | ||
| portString += L";allocate=false"; | ||
| } |
There was a problem hiding this comment.
ModifyOpenPorts always includes a listen_addr field in the options string ("...;listen_addr={}") even when isOpen==false (allocate=false). The comment above indicates listen_addr and allocate=false are mutually exclusive; emitting both (or emitting listen_addr with an empty value) risks breaking close/unmap behavior. Build the options string so listen_addr is only included for open mappings, and use allocate=false without listen_addr for closes if that’s what the guest expects.
| if (HostPort == WSLC_EPHEMERAL_PORT && isOpen && SUCCEEDED(addShareResult)) | ||
| { |
There was a problem hiding this comment.
For ephemeral binds, this interprets IPlan9FileSystem::AddShare's HRESULT success code as S_OK + allocatedPort. This is a nonstandard encoding for HRESULT and could misbehave for success codes like S_FALSE or other non-zero successes. Consider returning the allocated port through an explicit out-param/response channel, or at least range-check that addShareResult is within [S_OK, S_OK + 65535] before decoding.
| if (HostPort == WSLC_EPHEMERAL_PORT && isOpen && SUCCEEDED(addShareResult)) | |
| { | |
| if (HostPort == WSLC_EPHEMERAL_PORT && isOpen) | |
| { | |
| THROW_IF_FAILED_MSG(addShareResult, "Failed to set virtionet port mapping: %ls", portString.c_str()); | |
| constexpr HRESULT c_maxEncodedEphemeralPortResult = S_OK + UINT16_MAX; | |
| THROW_HR_IF_MSG( | |
| E_UNEXPECTED, | |
| addShareResult < S_OK || addShareResult > c_maxEncodedEphemeralPortResult, | |
| "Unexpected AddShare success code for ephemeral port mapping: 0x%08x (%ls)", | |
| static_cast<unsigned int>(addShareResult), | |
| portString.c_str()); |
| // format: tag={tag}[;host_port={port}];guest_port={port}[;listen_addr={addr}|;allocate=false][;udp] | ||
| std::wstring portString = std::format(L"tag={};guest_port={};listen_addr={}", tag, GuestPort, hostAddress); | ||
|
|
There was a problem hiding this comment.
listen_addr is interpolated into a semicolon-delimited options string that gets parsed on the guest side. Since MapVirtioNetPort’s ListenAddress ultimately comes from a caller-supplied string, it would be safer to validate/normalize it to a real IPv4/IPv6 address (e.g., via inet_pton/InetPton) and reject values containing ';' or other delimiters so a malformed address can’t perturb option parsing.
OneBlue
changed the title
benhillis
left a comment
benhillis
left a comment
There was a problem hiding this comment.
This is a good start. When can we stop running the port relay wslrelay.exe? I didn’t see a change in configure networking.
| // format: tag={tag}[;host_port={port}];guest_port={port}[;listen_addr={addr}|;allocate=false][;udp] | ||
| std::wstring portString = std::format(L"tag={};guest_port={};listen_addr={}", tag, GuestPort, hostAddress); | ||
|
|
||
| if (HostPort != WSLC_EPHEMERAL_PORT) | ||
| { | ||
| std::wstring portString = std::format(L"tag={};port_number={}", tag, INETADDR_PORT(reinterpret_cast<const SOCKADDR*>(&addr))); | ||
| if (protocol == IPPROTO_UDP) | ||
| { | ||
| portString += L";udp"; | ||
| } | ||
| portString += std::format(L";host_port={}", HostPort); | ||
| } | ||
|
|
||
| if (!isOpen) | ||
| { | ||
| portString += L";allocate=false"; | ||
| } | ||
| else | ||
| { | ||
| const auto addrStr = wsl::windows::common::string::SockAddrInetToWstring(addr); | ||
| portString += std::format(L";listen_addr={}", addrStr); | ||
| } | ||
| if (!isOpen) | ||
| { | ||
| portString += L";allocate=false"; | ||
| } |
| IFACEMETHOD(DetachDisk)(_In_ ULONG Lun) override; | ||
| IFACEMETHOD(AddShare)(_In_ LPCWSTR WindowsPath, _In_ BOOL ReadOnly, _Out_ GUID* ShareId) override; | ||
| IFACEMETHOD(RemoveShare)(_In_ REFGUID ShareId) override; | ||
| IFACEMETHOD(MapVirtioNetPort) |
There was a problem hiding this comment.
I think for naming just MapPort and UnmapPort would be better.
Summary of the Pull Request
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed