Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always create a new environment block before we spawn a process #7243

Merged
3 commits merged into from
Aug 11, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/actions/spell-check/dictionary/apis.txt
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ alignof
bitfield
bitfields
CLASSNOTAVAILABLE
environstrings
EXPCMDFLAGS
EXPCMDSTATE
fullkbd
Expand Down Expand Up @@ -46,3 +47,4 @@ STDCPP
syscall
tmp
tx
userenv
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))
Copy link
Member Author

Choose a reason for hiding this comment

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

@Nathpete can you test what happens if you replace GetCurrentProcessToken with..

auto processToken{ wil::open_current_access_token() };
CreateEnv(...processToken.get()...)

If that doesn't restore USERNAME, can you try passing TOKEN_QUERY|TOKEN_DUPLICATE as the first arg to open_current_acc?

Copy link
Member Author

Choose a reason for hiding this comment

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

whoops - @nathpete-msft

Copy link
Member

Choose a reason for hiding this comment

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

auto processToken{ wil::open_current_access_token(TOKEN_QUERY | TOKEN_DUPLICATE) };

That does it!

Copy link

@KalleOlaviNiemitalo KalleOlaviNiemitalo Aug 25, 2020

Choose a reason for hiding this comment

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

Ugh, I am surprised CreateEnvironmentBlock behaves that way. Tested:

  • NULL => USERNAME=SYSTEM
  • GetCurrentProcessToken() => no USERNAME
  • GetCurrentThreadEffectiveToken() => no USERNAME
  • OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY | TOKEN_QUERY_SOURCE, &hToken) => no USERNAME
  • OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY | TOKEN_DUPLICATE, &hToken) => correct USERNAME
  • (HANDLE)0x123456 => error 6

Perhaps it is because GetCurrentProcessToken() lacks TOKEN_DUPLICATE access.

Copy link
Member Author

Choose a reason for hiding this comment

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

You know, you're probably right about that last bit. That makes sense.

At least there's a nice WIL helper for us to get our current effective access token 😄

{
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

Choose a reason for hiding this comment

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

It seems a bit strange to take std::optional<void*> and then treat nullptr as equivalent to std::nullopt. I don't see std::optional<T*> anywhere else in the source tree. But perhaps it makes sense if you consider nullptr an invalid argument here.

Copy link
Member

Choose a reason for hiding this comment

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

I concur - why isn't the parameter just a EnvironmentBlockPtr that we compare to nullptr to check if it was provided?

HRESULT Microsoft::Console::Utils::UpdateEnvironmentMapW(EnvironmentVariableMapW& map, void* environmentBlock) noexcept
try
{
LPWCH currentEnvVars{};
auto freeCurrentEnv = wil::scope_exit([&] {
if (currentEnvVars)
{
FreeEnvironmentStringsW(currentEnvVars);
currentEnvVars = nullptr;
}
});
wchar_t const* activeEnvironmentBlock{ static_cast<wchar_t const*>(environmentBlock) };

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, void* environmentBlock = nullptr) 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