Skip to content

Commit

Permalink
Always create a new environment block before we spawn a process
Browse files Browse the repository at this point in the history
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)
  • Loading branch information
DHowett committed Aug 11, 2020
1 parent c03677b commit 043ae03
Show file tree
Hide file tree
Showing 4 changed files with 35 additions and 15 deletions.
8 changes: 6 additions & 2 deletions src/cascadia/TerminalConnection/ConptyConnection.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "ConptyConnection.h"

#include <windows.h>
#include <userenv.h>

#include "ConptyConnection.g.cpp"

Expand Down Expand Up @@ -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 '{}'.
Expand Down
36 changes: 24 additions & 12 deletions src/types/Environment.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<void*> environmentBlock) noexcept
try
{
LPWCH currentEnvVars{};
auto freeCurrentEnv = wil::scope_exit([&] {
if (currentEnvVars)
{
FreeEnvironmentStringsW(currentEnvVars);
currentEnvVars = nullptr;
}
});
wchar_t const* activeEnvironmentBlock{ reinterpret_cast<wchar_t const*>(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) };
Expand Down
5 changes: 4 additions & 1 deletion src/types/inc/Environment.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,12 @@ namespace Microsoft::Console::Utils
}
};

using EnvironmentBlockPtr = wil::unique_any<void*, decltype(::DestroyEnvironmentBlock), ::DestroyEnvironmentBlock>;
[[nodiscard]] EnvironmentBlockPtr CreateEnvironmentBlock();

using EnvironmentVariableMapW = std::map<std::wstring, std::wstring, WStringCaseInsensitiveCompare>;

[[nodiscard]] HRESULT UpdateEnvironmentMapW(EnvironmentVariableMapW& map) noexcept;
[[nodiscard]] HRESULT UpdateEnvironmentMapW(EnvironmentVariableMapW& map, std::optional<void*> environmentBlock = std::nullopt) noexcept;

[[nodiscard]] HRESULT EnvironmentMapToEnvironmentStringsW(EnvironmentVariableMapW& map,
std::vector<wchar_t>& newEnvVars) noexcept;
Expand Down
1 change: 1 addition & 0 deletions src/types/precomp.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ Module Name:

// Windows Header Files:
#include <windows.h>
#include <userenv.h>
#include <combaseapi.h>
#include <UIAutomation.h>
#include <objbase.h>
Expand Down

1 comment on commit 043ae03

@github-actions
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New misspellings found, please review:

  • environstrings
  • userenv
To accept these changes, run the following commands
perl -e '
my @expect_files=qw('".github/actions/spell-check/expect/af1ff90dc512c83c902762b02f284c1c61603b4a.txt
.github/actions/spell-check/expect/alphabet.txt
.github/actions/spell-check/expect/expect.txt
.github/actions/spell-check/expect/web.txt"');
@ARGV=@expect_files;
my @stale=qw('"notypeopt "');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
  if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
  next if /^($re)(?:$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spell-check/expect/043ae03b867309668d80ee9ed8964a5da6c75ec2.txt";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"environstrings userenv "');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a) cmp lc($b)} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;'
git add .github/actions/spell-check/expect || echo '... you want to ensure .github/actions/spell-check/expect/043ae03b867309668d80ee9ed8964a5da6c75ec2.txt is added to your repository...'
✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. You can copy the contents of each perl command excluding the outer ' marks and dropping any '"/"' quotation mark pairs into a file and then run perl file.pl from the root of the repository to run the code. Alternatively, you can manually insert the items...

If the listed items are:

  • ... misspelled, then please correct them instead of using the command.
  • ... names, please add them to .github/actions/spell-check/dictionary/names.txt.
  • ... APIs, you can add them to a file in .github/actions/spell-check/dictionary/.
  • ... just things you're using, please add them to an appropriate file in .github/actions/spell-check/expect/.
  • ... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spell-check/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The :check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

⚠️ Reviewers

At present, the action that triggered this message will not show its ❌ in this PR unless the branch is within this repository.
Thus, you should make sure that this comment has been addressed before encouraging the merge bot to merge this PR.

Please sign in to comment.