do-not-merge: virtionet port mapping POC#40145
do-not-merge: virtionet port mapping POC#40145OneBlue wants to merge 4 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.
| else | ||
| { | ||
| THROW_HR_IF(WSLC_E_INVALID_SESSION_NAME, Settings->DisplayName == nullptr || wcslen(Settings->DisplayName) == 0); | ||
| THROW_HR_IF(E_INVALIDARG, Settings->StoragePath != nullptr && wcslen(Settings->StoragePath) == 0); |
There was a problem hiding this comment.
CreateSession’s validation now calls wcslen(Settings->DisplayName) without first validating that Settings->DisplayName is non-null and non-empty. This can AV if a caller passes a null DisplayName, and it also changes behavior by allowing empty names to reach the reserved-name check. Re-introduce a null/empty check (or guard the wcslen calls) before using DisplayName.
| THROW_HR_IF(E_INVALIDARG, Settings->StoragePath != nullptr && wcslen(Settings->StoragePath) == 0); | |
| THROW_HR_IF(E_INVALIDARG, Settings->StoragePath != nullptr && wcslen(Settings->StoragePath) == 0); | |
| THROW_HR_IF(E_INVALIDARG, Settings->DisplayName == nullptr || Settings->DisplayName[0] == L'\0'); |
| int SessionService::Attach(const std::wstring& sessionName, bool raw) | ||
| { | ||
| wil::com_ptr<IWSLCSessionManager> manager; | ||
| THROW_IF_FAILED(CoCreateInstance(__uuidof(WSLCSessionManager), nullptr, CLSCTX_LOCAL_SERVER, IID_PPV_ARGS(&manager))); | ||
| wsl::windows::common::security::ConfigureForCOMImpersonation(manager.get()); | ||
|
|
||
| wil::com_ptr<IWSLCSession> session; | ||
| HRESULT hr = manager->OpenSessionByName(sessionName.empty() ? nullptr : sessionName.c_str(), &session); | ||
|
|
There was a problem hiding this comment.
The new raw parameter to SessionService::Attach is never used inside the function, so the CLI --raw flag currently has no effect (and may trigger unused-parameter warnings depending on build settings). Either plumb this through to session creation/initialization (e.g., set a feature flag passed to the service) or remove the parameter/flag until it is supported.
| _(Publish, "publish", L"p", Kind::Value, Localization::WSLCCLI_PublishArgDescription()) \ | ||
| /*_(Pull, "pull", NO_ALIAS, Kind::Value, Localization::WSLCCLI_PullArgDescription())*/ \ | ||
| _(Quiet, "quiet", L"q", Kind::Flag, Localization::WSLCCLI_QuietArgDescription()) \ | ||
| _(Raw, "raw", NO_ALIAS, Kind::Flag, L"Start the session without dockerd or container services") \ |
There was a problem hiding this comment.
The new Raw argument description is a hard-coded English string, while the surrounding argument descriptions are localized via Localization::WSLCCLI_*. Since this string is user-facing help text, it should follow the repo’s localization pattern (add a localized resource entry and reference it here) to keep CLI help localizable.
| _(Raw, "raw", NO_ALIAS, Kind::Flag, L"Start the session without dockerd or container services") \ | |
| _(Raw, "raw", NO_ALIAS, Kind::Flag, Localization::WSLCCLI_RawArgDescription()) \ |
|
|
||
| *AllocatedHostPort = 0; | ||
|
|
||
| return HandlePortNotification(ListenAddress, Protocol, GuestPort, true); |
There was a problem hiding this comment.
MapPort never populates *AllocatedHostPort for anonymous binds. ModifyOpenPorts can compute an allocated host port (return value), but MapPort currently just calls HandlePortNotification and leaves *AllocatedHostPort as 0, so callers (e.g. WSLCVirtualMachine::MapPort) can’t discover the chosen host port. Plumb the allocated host port back to AllocatedHostPort (and ensure the COM method returns it).
| return HandlePortNotification(ListenAddress, Protocol, GuestPort, true); | |
| const auto hostPort = INETADDR_PORT(reinterpret_cast<const SOCKADDR*>(&ListenAddress)); | |
| *AllocatedHostPort = ModifyOpenPorts(c_defaultDeviceTag, ListenAddress, hostPort, GuestPort, Protocol, true); | |
| return S_OK; |
| auto listenAddr = wsl::windows::common::string::StringToSockAddrInet( | ||
| wsl::shared::string::MultiByteToWide(ListenAddress)); | ||
|
|
||
| listenAddr.Ipv4.sin_port = HostPort; | ||
|
|
There was a problem hiding this comment.
MapVirtioNetPort sets listenAddr.Ipv4.sin_port = HostPort without converting to network byte order. Callers (e.g. VMPortMapping::HostPort/SetHostPort) treat ports as host-order, and INETADDR_PORT expects the stored port to be in network order, so this will produce incorrect ports for non-zero values. Use htons(HostPort) or INETADDR_SET_PORT (and apply the same fix in UnmapVirtioNetPort).
| } | ||
|
|
||
| THROW_LAST_ERROR_IF(MountInit(std::format("{}/wsl-init", target).c_str()) < 0); // Required to call /gns later | ||
| THROW_LAST_ERROR_IF(MountInit(std::format("{}/init", target).c_str()) < 0); // Required to call /gns later | ||
|
|
There was a problem hiding this comment.
MountInit is now bind-mounting WSL init into the chroot at {target}/init. However, WSLCEnableCrashDumpCollection/CreateCaptureCrashSymlink in this file still points the crash-capture symlink at /wsl-init (not /init), while the main init path used elsewhere in the repo is /init (LX_INIT_PATH). To avoid breaking crash dump collection in the chroot, align the symlink target with the mounted init path (or ensure /wsl-init is present inside the chroot).
| // 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, hostAddressStr.c_str()); | ||
|
|
There was a problem hiding this comment.
ModifyOpenPorts always includes listen_addr=... in the options string even when isOpen is false and allocate=false is appended. The comment immediately above indicates listen_addr and allocate=false are mutually exclusive; emitting both risks breaking close/unmap behavior. Build the string so listen_addr is only included for open mappings, and for closes emit allocate=false without listen_addr (matching the expected guest contract).
| try | ||
| { | ||
| hostPort = ModifyOpenPorts(c_loopbackDeviceName, localAddr, hostPort, guestPort, protocol, allocate); | ||
| } | ||
| catch (...) | ||
| { | ||
| LOG_CAUGHT_EXCEPTION_MSG("Failure adding localhost relay port %d", guestPort); | ||
| } | ||
| } | ||
|
|
||
| if (!loopback) | ||
| { | ||
| const int localResult = ModifyOpenPorts(c_eth0DeviceName, addr, protocol, allocate); | ||
| LOG_HR_IF_MSG(E_FAIL, localResult != S_OK, "Failure adding relay port %d", INETADDR_PORT(reinterpret_cast<const SOCKADDR*>(&addr))); | ||
| if (result == 0) | ||
| try | ||
| { | ||
| hostPort = ModifyOpenPorts(c_eth0DeviceName, addr, hostPort, guestPort, protocol, allocate); | ||
| } | ||
| catch (...) | ||
| { | ||
| result = localResult; | ||
| LOG_CAUGHT_EXCEPTION_MSG("Failure adding relay port %d", guestPort); | ||
| } | ||
| } | ||
|
|
||
| return result; | ||
| return S_OK; | ||
| } |
There was a problem hiding this comment.
HandlePortNotification swallows all failures (catch-all around ModifyOpenPorts calls) and unconditionally returns S_OK. Since MapPort/UnmapPort call into this path, explicit port map/unmap requests can report success even when the guest update failed, leaving the system in an inconsistent state. Consider propagating HRESULTs for explicit operations (or splitting tracked notifications vs explicit map/unmap so explicit calls surface errors).
| auto listenAddr = wsl::windows::common::string::StringToSockAddrInet( | ||
| wsl::shared::string::MultiByteToWide(ListenAddress)); | ||
|
|
||
| listenAddr.Ipv4.sin_port = HostPort; | ||
|
|
There was a problem hiding this comment.
UnmapVirtioNetPort also assigns listenAddr.Ipv4.sin_port = HostPort without converting to network byte order, which can cause unmap requests to target the wrong port and leak mappings. Set the port using htons(HostPort) / INETADDR_SET_PORT (consistent with how ports are stored in SOCKADDR_*).
Summary of the Pull Request
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed