-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems a bit strange to take There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I concur - why isn't the parameter just a |
||
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) }; | ||
|
There was a problem hiding this comment.
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..If that doesn't restore
USERNAME
, can you try passingTOKEN_QUERY|TOKEN_DUPLICATE
as the first arg to open_current_acc?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops - @nathpete-msft
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That does it!
There was a problem hiding this comment.
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=SYSTEMGetCurrentProcessToken()
=> no USERNAMEGetCurrentThreadEffectiveToken()
=> no USERNAMEOpenProcessToken(GetCurrentProcess(), TOKEN_QUERY | TOKEN_QUERY_SOURCE, &hToken)
=> no USERNAMEOpenProcessToken(GetCurrentProcess(), TOKEN_QUERY | TOKEN_DUPLICATE, &hToken)
=> correct USERNAME(HANDLE)0x123456
=> error 6Perhaps it is because GetCurrentProcessToken() lacks TOKEN_DUPLICATE access.
There was a problem hiding this comment.
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 😄