From 34cd29a0ce600e35f5b07853925622e7c3134acd Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Tue, 5 Aug 2025 00:42:26 +0200 Subject: [PATCH] Fix WSL PATH corruption & potential use-after-free (#19211) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #19152 ## Validation Steps Performed * Set `PATH` on a linux profile * `PATH` isn't messed up inside WSL ✅ (cherry picked from commit 0a6394270e2026bbd2bf5f937870eb9242f326ed) Service-Card-Id: PVTI_lADOAF3p4s4AxadtzgdR6bA Service-Version: 1.23 --- .../TerminalConnection/ConptyConnection.cpp | 45 +++++++++---------- .../TerminalConnection/ConptyConnection.h | 2 +- 2 files changed, 21 insertions(+), 26 deletions(-) diff --git a/src/cascadia/TerminalConnection/ConptyConnection.cpp b/src/cascadia/TerminalConnection/ConptyConnection.cpp index 2834eb179e..f511ff8afa 100644 --- a/src/cascadia/TerminalConnection/ConptyConnection.cpp +++ b/src/cascadia/TerminalConnection/ConptyConnection.cpp @@ -30,8 +30,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation { // Function Description: // - launches the client application attached to the new pseudoconsole - HRESULT ConptyConnection::_LaunchAttachedClient() noexcept - try + void ConptyConnection::_LaunchAttachedClient() { STARTUPINFOEX siEx{ 0 }; siEx.StartupInfo.cb = sizeof(STARTUPINFOEX); @@ -43,15 +42,16 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation auto attrList{ std::make_unique(size) }; #pragma warning(suppress : 26490) // We have to use reinterpret_cast because we allocated a byte array as a proxy for the adjustable size list. siEx.lpAttributeList = reinterpret_cast(attrList.get()); - RETURN_IF_WIN32_BOOL_FALSE(InitializeProcThreadAttributeList(siEx.lpAttributeList, 1, 0, &size)); + THROW_IF_WIN32_BOOL_FALSE(InitializeProcThreadAttributeList(siEx.lpAttributeList, 1, 0, &size)); - RETURN_IF_WIN32_BOOL_FALSE(UpdateProcThreadAttribute(siEx.lpAttributeList, - 0, - PROC_THREAD_ATTRIBUTE_PSEUDOCONSOLE, - _hPC.get(), - sizeof(HPCON), - nullptr, - nullptr)); + THROW_IF_WIN32_BOOL_FALSE(UpdateProcThreadAttribute( + siEx.lpAttributeList, + 0, + PROC_THREAD_ATTRIBUTE_PSEUDOCONSOLE, + _hPC.get(), + sizeof(HPCON), + nullptr, + nullptr)); auto cmdline{ wil::ExpandEnvironmentStringsW(_commandline.c_str()) }; // mutable copy -- required for CreateProcessW auto environment = _initialEnv; @@ -72,7 +72,11 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation std::wstring additionalWslEnv; // WSLENV.2: Figure out what variables are already in WSLENV. - std::unordered_set wslEnvVars; + std::unordered_set wslEnvVars{ + // We never want to put a custom Windows PATH variable into WSLENV, + // because that would override WSL's computation of the NIX PATH. + L"PATH", + }; for (const auto& part : til::split_iterator{ std::wstring_view{ wslEnv }, L':' }) { // Each part may contain a variable name and flags (e.g., /p, /l, etc.) @@ -97,25 +101,19 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation } } + // add additional env vars if (_environment) { - // Order the environment variable names so that resolution order is consistent - // NOTE(lhecker): I'm like 99% sure that this is unnecessary. - std::set keys{}; for (const auto item : _environment) - { - keys.insert(std::wstring{ item.Key() }); - } - // add additional env vars - for (const auto& key : keys) { try { + const auto key = item.Key(); // This will throw if the value isn't a string. If that // happens, then just skip this entry. const auto value = winrt::unbox_value(_environment.Lookup(key)); - environment.set_user_environment_var(key.c_str(), value.c_str()); + environment.set_user_environment_var(key, value); // WSLENV.4: Add custom user environment variables to WSLENV. if (wslEnvVars.emplace(key).second) @@ -166,7 +164,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation auto [newCommandLine, newStartingDirectory] = Utils::MangleStartingDirectoryForWSL(cmdline, _startingDirectory); const auto startingDirectory = newStartingDirectory.size() > 0 ? newStartingDirectory.c_str() : nullptr; - RETURN_IF_WIN32_BOOL_FALSE(CreateProcessW( + THROW_IF_WIN32_BOOL_FALSE(CreateProcessW( nullptr, newCommandLine.data(), nullptr, // lpProcessAttributes @@ -193,10 +191,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation TraceLoggingWideString(_clientName.c_str(), "Client", "The attached client process"), TraceLoggingKeyword(MICROSOFT_KEYWORD_MEASURES), TelemetryPrivacyDataTag(PDT_ProductAndServiceUsage)); - - return S_OK; } - CATCH_RETURN(); // Who decided that? #pragma warning(suppress : 26455) // Default constructor should not throw. Declare it 'noexcept' (f.6). @@ -418,7 +413,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation THROW_IF_FAILED(ConptyShowHidePseudoConsole(_hPC.get(), _initialVisibility)); } - THROW_IF_FAILED(_LaunchAttachedClient()); + _LaunchAttachedClient(); } // But if it was an inbound handoff... attempt to synchronize the size of it with what our connection // window is expecting it to be on the first layout. diff --git a/src/cascadia/TerminalConnection/ConptyConnection.h b/src/cascadia/TerminalConnection/ConptyConnection.h index 4d847fdcd1..cdcd99981f 100644 --- a/src/cascadia/TerminalConnection/ConptyConnection.h +++ b/src/cascadia/TerminalConnection/ConptyConnection.h @@ -58,7 +58,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation static HRESULT NewHandoff(HANDLE* in, HANDLE* out, HANDLE signal, HANDLE reference, HANDLE server, HANDLE client, const TERMINAL_STARTUP_INFO* startupInfo) noexcept; static winrt::hstring _commandlineFromProcess(HANDLE process); - HRESULT _LaunchAttachedClient() noexcept; + void _LaunchAttachedClient(); void _indicateExitWithStatus(unsigned int status) noexcept; static std::wstring _formatStatus(uint32_t status); void _LastConPtyClientDisconnected() noexcept;