diff --git a/src/windows/common/relay.cpp b/src/windows/common/relay.cpp index 9b65283db..d85236f35 100644 --- a/src/windows/common/relay.cpp +++ b/src/windows/common/relay.cpp @@ -445,6 +445,26 @@ void ScopedRelay::Sync() } } +void ScopedRelay::Sync(std::chrono::milliseconds Timeout) +{ + // Drain to natural EOF within the timeout; otherwise cancel before joining. + if (!m_thread.joinable()) + { + return; + } + + // Keep the wait bounded and below INFINITE. + const DWORD timeoutMs = + Timeout.count() <= 0 ? 0 : static_cast(std::min(Timeout.count(), static_cast(INFINITE) - 1)); + + if (!m_completed.wait(timeoutMs)) + { + m_exitEvent.SetEvent(); + } + + m_thread.join(); +} + ScopedRelay::~ScopedRelay() { try diff --git a/src/windows/common/relay.hpp b/src/windows/common/relay.hpp index 1cbeb01a6..66cb6638a 100644 --- a/src/windows/common/relay.hpp +++ b/src/windows/common/relay.hpp @@ -15,6 +15,7 @@ Module Name: #pragma once #include +#include #include "ConsoleState.h" #include "HandleIO.h" @@ -22,6 +23,9 @@ namespace wsl::windows::common::relay { using namespace wsl::windows::common::io; +// Default cap for bounded relay drains. +constexpr auto c_relayDrainTimeout = std::chrono::seconds{60}; + std::thread CreateThread(_In_ HANDLE InputHandle, _In_ HANDLE OutputHandle, _In_opt_ HANDLE ExitHandle = nullptr, _In_ size_t BufferSize = LX_RELAY_BUFFER_SIZE); std::thread CreateThread(_In_ wil::unique_handle&& InputHandle, _In_ HANDLE OutputHandle, _In_opt_ HANDLE ExitHandle = nullptr, _In_ size_t BufferSize = LX_RELAY_BUFFER_SIZE); @@ -98,6 +102,8 @@ class ScopedRelay m_onDestroy(std::move(OnDestroy)) { m_thread = std::thread{[this, Input = std::move(Input), Output = std::move(Output), BufferSize = BufferSize]() { + // Signal completion for bounded Sync(). + auto signalCompleted = wil::scope_exit([this]() { m_completed.SetEvent(); }); try { Run(GetUnderlyingHandle(Input), GetUnderlyingHandle(Output), BufferSize); @@ -108,7 +114,7 @@ class ScopedRelay ~ScopedRelay(); - ScopedRelay(ScopedRelay&& other) = default; + ScopedRelay(ScopedRelay&&) = delete; ScopedRelay(const ScopedRelay&) = delete; ScopedRelay& operator=(const ScopedRelay&) = delete; @@ -119,6 +125,9 @@ class ScopedRelay // the content has been flushed before exiting. void Sync(); + // Blocks until EOF, or cancels after Timeout. + void Sync(std::chrono::milliseconds Timeout); + private: template static HANDLE GetUnderlyingHandle(THandle& handle) @@ -150,6 +159,7 @@ class ScopedRelay std::thread m_thread; wil::unique_event m_exitEvent{wil::EventOptions::ManualReset}; + wil::unique_event m_completed{wil::EventOptions::ManualReset}; std::function m_onDestroy; }; diff --git a/src/windows/common/svccomm.cpp b/src/windows/common/svccomm.cpp index 1e75011d6..5b173646f 100644 --- a/src/windows/common/svccomm.cpp +++ b/src/windows/common/svccomm.cpp @@ -249,7 +249,8 @@ wsl::windows::common::SvcComm::ExportDistribution(_In_opt_ LPCGUID DistroGuid, _ } stdErrWrite.reset(); - stdErrRelay.Sync(); + // Client relay is EOF-bounded; timeout is a defensive cap. + stdErrRelay.Sync(relay::c_relayDrainTimeout); RETURN_HR(result); } @@ -421,19 +422,23 @@ wsl::windows::common::SvcComm::LaunchProcess( // Create stdin, stdout and stderr worker threads. // - std::thread StdOutWorker; - std::thread StdErrWorker; + // StdOut/StdErr drain to EOF, bounded in case a guest socket wedges. + std::optional StdOutWorker; + std::optional StdErrWorker; auto ExitEvent = wil::unique_event(wil::EventOptions::ManualReset); auto outWorkerExit = wil::scope_exit_log(WI_DIAGNOSTICS_INFO, [&StdOutWorker, &StdErrWorker, &ExitEvent] { + // Signal the detached stdin relay (it uses ExitEvent as its ExitHandle). ExitEvent.SetEvent(); - if (StdOutWorker.joinable()) + + // Drain stdout/stderr, bounded in case a guest socket wedges. + if (StdOutWorker.has_value()) { - StdOutWorker.join(); + StdOutWorker->Sync(relay::c_relayDrainTimeout); } - if (StdErrWorker.joinable()) + if (StdErrWorker.has_value()) { - StdErrWorker.join(); + StdErrWorker->Sync(relay::c_relayDrainTimeout); } }); @@ -476,9 +481,10 @@ wsl::windows::common::SvcComm::LaunchProcess( } auto StdOut = GetStdHandle(STD_OUTPUT_HANDLE); - StdOutWorker = relay::CreateThread(std::move(StdOutSocket), IS_VALID_HANDLE(StdOut) ? StdOut : nullptr); + // Relay guest stdout with bounded teardown drain. + StdOutWorker.emplace(std::move(StdOutSocket), IS_VALID_HANDLE(StdOut) ? StdOut : nullptr); auto StdErr = GetStdHandle(STD_ERROR_HANDLE); - StdErrWorker = relay::CreateThread(std::move(StdErrSocket), IS_VALID_HANDLE(StdErr) ? StdErr : nullptr); + StdErrWorker.emplace(std::move(StdErrSocket), IS_VALID_HANDLE(StdErr) ? StdErr : nullptr); // // Spawn wslhost to handle interop requests from processes that have @@ -612,7 +618,8 @@ std::pair wsl::windows::common::SvcComm::Reg } stdErrWrite.reset(); - stdErrRelay.Sync(); + // Client relay is EOF-bounded; timeout is a defensive cap. + stdErrRelay.Sync(relay::c_relayDrainTimeout); THROW_IF_FAILED(Result); @@ -648,7 +655,8 @@ wsl::windows::common::SvcComm::ResizeDistribution(_In_ LPCGUID DistroGuid, _In_ const auto result = m_userSession->ResizeDistribution(DistroGuid, outputWrite.get(), NewSize, context.OutError()); outputWrite.reset(); - outputRelay.Sync(); + // Client relay is EOF-bounded; timeout is a defensive cap. + outputRelay.Sync(relay::c_relayDrainTimeout); RETURN_HR(result); } diff --git a/src/windows/service/exe/LxssUserSession.cpp b/src/windows/service/exe/LxssUserSession.cpp index a89aaafca..cc8cb308b 100644 --- a/src/windows/service/exe/LxssUserSession.cpp +++ b/src/windows/service/exe/LxssUserSession.cpp @@ -1156,8 +1156,8 @@ HRESULT LxssUserSessionImpl::ExportDistribution(_In_opt_ LPCGUID DistroGuid, _In ULONG exitCode = 1; vmContext.instance->GetInitPort()->Receive(&exitCode, sizeof(exitCode), clientProcess.get()); - // Flush any pending IO on the error relay before exiting. - stdErrRelay.Sync(); + // Drain error output, bounded in case the guest socket wedges. + stdErrRelay.Sync(wsl::windows::common::relay::c_relayDrainTimeout); THROW_HR_IF(WSL_E_EXPORT_FAILED, (exitCode != 0)); } @@ -1603,10 +1603,10 @@ HRESULT LxssUserSessionImpl::RegisterDistribution( gsl::span span; const auto& message = channel->GetChannel().ReceiveMessage(&span); - // Flush any pending IO on the error relay before exiting. + // Drain error output, bounded in case the guest socket wedges. if (errorRelay.has_value()) { - errorRelay->Sync(); + errorRelay->Sync(wsl::windows::common::relay::c_relayDrainTimeout); } // Process the import result message. diff --git a/test/windows/CMakeLists.txt b/test/windows/CMakeLists.txt index 54831cd62..b065bd562 100644 --- a/test/windows/CMakeLists.txt +++ b/test/windows/CMakeLists.txt @@ -12,7 +12,8 @@ set(SOURCES WSLCTests.cpp WslcSdkTests.cpp WslcSdkWinRtTests.cpp - WindowsUpdateTests.cpp) + WindowsUpdateTests.cpp + ScopedRelayUnitTests.cpp) set(HEADERS Common.h diff --git a/test/windows/ScopedRelayUnitTests.cpp b/test/windows/ScopedRelayUnitTests.cpp new file mode 100644 index 000000000..c555156ac --- /dev/null +++ b/test/windows/ScopedRelayUnitTests.cpp @@ -0,0 +1,215 @@ +/*++ + +Copyright (c) Microsoft. All rights reserved. + +Module Name: + + ScopedRelayUnitTests.cpp + +Abstract: + + Unit tests for bounded relay::ScopedRelay::Sync(). + +--*/ + +#include "precomp.h" +#include "Common.h" + +#include +#include +#include +#include "relay.hpp" + +namespace ScopedRelayUnitTests { + +using namespace std::chrono_literals; +using wsl::windows::common::relay::ScopedRelay; + +namespace { + + // Create an overlapped (async) named pipe pair. The returned 'ReadEnd' is the server end + // opened FILE_FLAG_OVERLAPPED so that a pending ReadFile returns ERROR_IO_PENDING and the + // relay's InterruptableWait can be cancelled via the exit event -- this mirrors a real + // guest-owned hvsocket. 'WriteEnd' is the client end used to feed data (and to control EOF + // by closing it, or to induce a hang by leaving it open). + struct OverlappedPipe + { + wil::unique_handle ReadEnd; + wil::unique_handle WriteEnd; + }; + + OverlappedPipe CreateOverlappedPipe() + { + GUID guid{}; + THROW_IF_FAILED(CoCreateGuid(&guid)); + + wchar_t name[64]; + swprintf_s(name, L"\\\\.\\pipe\\wsl-relay-test-%08x%04x%04x", guid.Data1, guid.Data2, guid.Data3); + + OverlappedPipe pipe; + pipe.ReadEnd.reset(CreateNamedPipeW( + name, + PIPE_ACCESS_INBOUND | FILE_FLAG_OVERLAPPED | FILE_FLAG_FIRST_PIPE_INSTANCE, + PIPE_TYPE_BYTE | PIPE_READMODE_BYTE | PIPE_WAIT, + 1, + 64 * 1024, + 64 * 1024, + 0, + nullptr)); + THROW_LAST_ERROR_IF(!pipe.ReadEnd); + + pipe.WriteEnd.reset(CreateFileW(name, GENERIC_WRITE, 0, nullptr, OPEN_EXISTING, 0, nullptr)); + THROW_LAST_ERROR_IF(!pipe.WriteEnd); + + // Complete the named-pipe handshake before starting overlapped reads. + wil::unique_event connectEvent{wil::EventOptions::ManualReset}; + OVERLAPPED connectOverlapped{}; + connectOverlapped.hEvent = connectEvent.get(); + if (!ConnectNamedPipe(pipe.ReadEnd.get(), &connectOverlapped)) + { + const auto error = GetLastError(); + if (error == ERROR_IO_PENDING) + { + DWORD transferred = 0; + THROW_IF_WIN32_BOOL_FALSE(GetOverlappedResult(pipe.ReadEnd.get(), &connectOverlapped, &transferred, TRUE)); + } + else + { + THROW_HR_IF(HRESULT_FROM_WIN32(error), error != ERROR_PIPE_CONNECTED); + } + } + + return pipe; + } + + // Create a temporary output file (overlapped, delete-on-close) that the relay writes into. + // The test keeps ownership so it can read the bytes back after Sync(). + wil::unique_handle CreateTempOutputFile() + { + wchar_t tempPath[MAX_PATH]; + THROW_LAST_ERROR_IF(GetTempPathW(ARRAYSIZE(tempPath), tempPath) == 0); + + wchar_t tempFile[MAX_PATH]; + THROW_LAST_ERROR_IF(GetTempFileNameW(tempPath, L"rly", 0, tempFile) == 0); + + wil::unique_handle file{CreateFileW( + tempFile, GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ, nullptr, CREATE_ALWAYS, FILE_FLAG_OVERLAPPED | FILE_FLAG_DELETE_ON_CLOSE, nullptr)}; + THROW_LAST_ERROR_IF(!file); + + return file; + } + + // Read the full contents of an (overlapped) file handle from offset 0 using a synchronous + // overlapped read, so the relayed payload can be verified deterministically. + std::vector ReadAllFromStart(HANDLE File, size_t Size) + { + std::vector data(Size); + if (Size == 0) + { + return data; + } + + wil::unique_event event{wil::EventOptions::ManualReset}; + OVERLAPPED overlapped{}; + overlapped.hEvent = event.get(); + + DWORD bytesRead = 0; + if (!ReadFile(File, data.data(), gsl::narrow_cast(Size), &bytesRead, &overlapped)) + { + THROW_LAST_ERROR_IF(GetLastError() != ERROR_IO_PENDING); + THROW_IF_WIN32_BOOL_FALSE(GetOverlappedResult(File, &overlapped, &bytesRead, TRUE)); + } + + data.resize(bytesRead); + return data; + } + +} // namespace + +class ScopedRelayUnitTests +{ + WSL_TEST_CLASS(ScopedRelayUnitTests) + + TEST_CLASS_SETUP(TestClassSetup) + { + return true; + } + + TEST_CLASS_CLEANUP(TestClassCleanup) + { + return true; + } + + // Open input without EOF: bounded Sync() must return on timeout. + TEST_METHOD(SyncTimeoutReturnsWhenInputNeverReachesEof) + { + auto pipe = CreateOverlappedPipe(); + auto output = CreateTempOutputFile(); + + // Keep the write end open so the relay read stays pending. + constexpr auto timeout = 500ms; + constexpr auto budget = 5000ms; + + ScopedRelay relay{pipe.ReadEnd.get(), output.get()}; + + const auto start = std::chrono::steady_clock::now(); + relay.Sync(timeout); + const auto elapsed = std::chrono::duration_cast(std::chrono::steady_clock::now() - start); + + LogInfo("Sync(%lldms) returned after %lldms", static_cast(timeout.count()), static_cast(elapsed.count())); + + VERIFY_IS_TRUE(elapsed < budget); + + // Verify Sync returned due to timeout, not natural EOF. + VERIFY_IS_TRUE(elapsed >= (timeout - 100ms)); + } + + // Natural EOF should drain the full payload. + TEST_METHOD(SyncDrainsFullPayloadWithoutTruncation) + { + auto pipe = CreateOverlappedPipe(); + auto output = CreateTempOutputFile(); + + // Deterministic, verifiable payload (100000 bytes with a non-trivial pattern). + constexpr size_t payloadSize = 100000; + std::vector payload(payloadSize); + for (size_t i = 0; i < payloadSize; ++i) + { + payload[i] = static_cast((i * 31 + 7) & 0xff); + } + + ScopedRelay relay{pipe.ReadEnd.get(), output.get()}; + + // Write asynchronously so the relay can drain as the pipe fills. + std::thread writer{[&]() { + size_t written = 0; + while (written < payloadSize) + { + DWORD chunk = 0; + if (!WriteFile(pipe.WriteEnd.get(), payload.data() + written, gsl::narrow_cast(payloadSize - written), &chunk, nullptr)) + { + break; + } + + written += chunk; + } + + // Natural EOF: closing the write end lets the relay read 0 bytes and complete. + pipe.WriteEnd.reset(); + }}; + + relay.Sync(wsl::windows::common::relay::c_relayDrainTimeout); + + if (writer.joinable()) + { + writer.join(); + } + + const auto relayed = ReadAllFromStart(output.get(), payloadSize); + + VERIFY_ARE_EQUAL(relayed.size(), payloadSize); + VERIFY_IS_TRUE(relayed == payload); + } +}; + +} // namespace ScopedRelayUnitTests