From 043ae03b867309668d80ee9ed8964a5da6c75ec2 Mon Sep 17 00:00:00 2001 From: Dustin Howett Date: Mon, 10 Aug 2020 18:46:15 -0700 Subject: [PATCH 1/3] Always create a new environment block before we spawn a process This commit ensures that we always furnish a new process with the cleanest, most up-to-date environment variables we can. There is a minor cost here in that WT will no longer pass environment variables that it itself inherited to its child processes. This could be considered a reasonable sacrifice. It will also remove somebody else's TERM, TERM_PROGRAM and TERM_PROGRAM_VERSION from the environment, which could be considered a win. Related to #1125 (but it does not close it or resolve any of the other issues it calls out.) Fixes #7239 Fixes #7204 ("App Paths" value creeping into wt's environment) --- .../TerminalConnection/ConptyConnection.cpp | 8 +++-- src/types/Environment.cpp | 36 ++++++++++++------- src/types/inc/Environment.hpp | 5 ++- src/types/precomp.h | 1 + 4 files changed, 35 insertions(+), 15 deletions(-) diff --git a/src/cascadia/TerminalConnection/ConptyConnection.cpp b/src/cascadia/TerminalConnection/ConptyConnection.cpp index 1fe8399b4ed..ab1c9ab5c55 100644 --- a/src/cascadia/TerminalConnection/ConptyConnection.cpp +++ b/src/cascadia/TerminalConnection/ConptyConnection.cpp @@ -10,6 +10,7 @@ #include "ConptyConnection.h" #include +#include #include "ConptyConnection.g.cpp" @@ -97,8 +98,11 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation environment.clear(); }); - // Populate the environment map with the current environment. - RETURN_IF_FAILED(Utils::UpdateEnvironmentMapW(environment)); + { + const auto newEnvironmentBlock{ Utils::CreateEnvironmentBlock() }; + // Populate the environment map with the current environment. + RETURN_IF_FAILED(Utils::UpdateEnvironmentMapW(environment, newEnvironmentBlock.get())); + } { // Convert connection Guid to string and ignore the enclosing '{}'. diff --git a/src/types/Environment.cpp b/src/types/Environment.cpp index 106503d8404..a770608e23d 100644 --- a/src/types/Environment.cpp +++ b/src/types/Environment.cpp @@ -9,30 +9,42 @@ using namespace ::Microsoft::Console::Utils; // We cannot use spand or not_null because we're dealing with \0\0-terminated buffers of unknown length #pragma warning(disable : 26481 26429) +// Function Description: +// - Wraps win32's CreateEnvironmentBlock to return a smart pointer. +EnvironmentBlockPtr Microsoft::Console::Utils::CreateEnvironmentBlock() +{ + void* newEnvironmentBlock{ nullptr }; + if (!::CreateEnvironmentBlock(&newEnvironmentBlock, GetCurrentProcessToken(), FALSE)) + { + return nullptr; + } + return EnvironmentBlockPtr{ newEnvironmentBlock }; +} + // Function Description: // - Updates an EnvironmentVariableMapW with the current process's unicode // environment variables ignoring ones already set in the provided map. // Arguments: // - map: The map to populate with the current processes's environment variables. +// - environmentBlock: Optional environment block to use when filling map. If omitted, +// defaults to the current environment. // Return Value: // - S_OK if we succeeded, or an appropriate HRESULT for failing -HRESULT Microsoft::Console::Utils::UpdateEnvironmentMapW(EnvironmentVariableMapW& map) noexcept +HRESULT Microsoft::Console::Utils::UpdateEnvironmentMapW(EnvironmentVariableMapW& map, std::optional environmentBlock) noexcept try { - LPWCH currentEnvVars{}; - auto freeCurrentEnv = wil::scope_exit([&] { - if (currentEnvVars) - { - FreeEnvironmentStringsW(currentEnvVars); - currentEnvVars = nullptr; - } - }); + wchar_t const* activeEnvironmentBlock{ reinterpret_cast(environmentBlock.value_or(nullptr)) }; - currentEnvVars = ::GetEnvironmentStringsW(); - RETURN_HR_IF_NULL(E_OUTOFMEMORY, currentEnvVars); + wil::unique_environstrings_ptr currentEnvVars; + if (!activeEnvironmentBlock) + { + currentEnvVars.reset(::GetEnvironmentStringsW()); + RETURN_HR_IF_NULL(E_OUTOFMEMORY, currentEnvVars); + activeEnvironmentBlock = currentEnvVars.get(); + } // Each entry is NULL-terminated; block is guaranteed to be double-NULL terminated at a minimum. - for (wchar_t const* lastCh{ currentEnvVars }; *lastCh != '\0'; ++lastCh) + for (wchar_t const* lastCh{ activeEnvironmentBlock }; *lastCh != '\0'; ++lastCh) { // Copy current entry into temporary map. const size_t cchEntry{ ::wcslen(lastCh) }; diff --git a/src/types/inc/Environment.hpp b/src/types/inc/Environment.hpp index 0994a177762..bf830e834e7 100644 --- a/src/types/inc/Environment.hpp +++ b/src/types/inc/Environment.hpp @@ -21,9 +21,12 @@ namespace Microsoft::Console::Utils } }; + using EnvironmentBlockPtr = wil::unique_any; + [[nodiscard]] EnvironmentBlockPtr CreateEnvironmentBlock(); + using EnvironmentVariableMapW = std::map; - [[nodiscard]] HRESULT UpdateEnvironmentMapW(EnvironmentVariableMapW& map) noexcept; + [[nodiscard]] HRESULT UpdateEnvironmentMapW(EnvironmentVariableMapW& map, std::optional environmentBlock = std::nullopt) noexcept; [[nodiscard]] HRESULT EnvironmentMapToEnvironmentStringsW(EnvironmentVariableMapW& map, std::vector& newEnvVars) noexcept; diff --git a/src/types/precomp.h b/src/types/precomp.h index b8b77ae4d6c..193b79decf3 100644 --- a/src/types/precomp.h +++ b/src/types/precomp.h @@ -29,6 +29,7 @@ Module Name: // Windows Header Files: #include +#include #include #include #include From 58fd4cdcbb3fc19c68626ca1aa08ebfb2e7aabc0 Mon Sep 17 00:00:00 2001 From: Dustin Howett Date: Tue, 11 Aug 2020 11:40:42 -0700 Subject: [PATCH 2/3] use std::optional> instead --- src/types/Environment.cpp | 4 ++-- src/types/inc/Environment.hpp | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/types/Environment.cpp b/src/types/Environment.cpp index a770608e23d..010f6c8f8f8 100644 --- a/src/types/Environment.cpp +++ b/src/types/Environment.cpp @@ -30,10 +30,10 @@ EnvironmentBlockPtr Microsoft::Console::Utils::CreateEnvironmentBlock() // defaults to the current environment. // Return Value: // - S_OK if we succeeded, or an appropriate HRESULT for failing -HRESULT Microsoft::Console::Utils::UpdateEnvironmentMapW(EnvironmentVariableMapW& map, std::optional environmentBlock) noexcept +HRESULT Microsoft::Console::Utils::UpdateEnvironmentMapW(EnvironmentVariableMapW& map, void* environmentBlock) noexcept try { - wchar_t const* activeEnvironmentBlock{ reinterpret_cast(environmentBlock.value_or(nullptr)) }; + wchar_t const* activeEnvironmentBlock{ static_cast(environmentBlock) }; wil::unique_environstrings_ptr currentEnvVars; if (!activeEnvironmentBlock) diff --git a/src/types/inc/Environment.hpp b/src/types/inc/Environment.hpp index bf830e834e7..a982db60432 100644 --- a/src/types/inc/Environment.hpp +++ b/src/types/inc/Environment.hpp @@ -26,7 +26,7 @@ namespace Microsoft::Console::Utils using EnvironmentVariableMapW = std::map; - [[nodiscard]] HRESULT UpdateEnvironmentMapW(EnvironmentVariableMapW& map, std::optional environmentBlock = std::nullopt) noexcept; + [[nodiscard]] HRESULT UpdateEnvironmentMapW(EnvironmentVariableMapW& map, void* environmentBlock = nullptr) noexcept; [[nodiscard]] HRESULT EnvironmentMapToEnvironmentStringsW(EnvironmentVariableMapW& map, std::vector& newEnvVars) noexcept; From 8b50d743b22beaae2d562bd484b27f63c11dcd68 Mon Sep 17 00:00:00 2001 From: Dustin Howett Date: Tue, 11 Aug 2020 11:45:12 -0700 Subject: [PATCH 3/3] and spell it --- .github/actions/spell-check/dictionary/apis.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/actions/spell-check/dictionary/apis.txt b/.github/actions/spell-check/dictionary/apis.txt index 1e33da10d7e..9dc4909592f 100644 --- a/.github/actions/spell-check/dictionary/apis.txt +++ b/.github/actions/spell-check/dictionary/apis.txt @@ -4,6 +4,7 @@ alignof bitfield bitfields CLASSNOTAVAILABLE +environstrings EXPCMDFLAGS EXPCMDSTATE fullkbd @@ -46,3 +47,4 @@ STDCPP syscall tmp tx +userenv