Always create a new environment block before we spawn a process by DHowett · Pull Request #7243 · microsoft/terminal

DHowett · 2020-08-11T01:52:52Z

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.

I validated that GetCurrentProcessToken returns a token we're
technically able to use with this API; it is roughly equivalent to
OpenProcessToken(GetCurrentProcess) in that it returns the current
active access token (which is what CreateEnvironmentBlock wants.)

There's been discussion about doing a 3-way merge between WT's
environment and the new one. This will be complicated and I'd like to
scream test the 0-way merge first ;P

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)

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)

KalleOlaviNiemitalo · 2020-08-11T09:15:16Z

src/types/Environment.cpp

+//   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


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.

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

zadjii-msft · 2020-08-11T12:31:25Z

I'm cool with this as-is, but I think I want to hear the optional<void*> discussion first

DHowett · 2020-08-11T17:34:57Z

So, that's a fair question and one for which I don't really have a good answer. std::optional<void*> was driven by my innate aversion to a raw void*. I would have passed an EnvironmentBlockPtr as @zadjii-msft suggested, but since that's a typedef over wil::unique_any it enforces ownership semantics at the function signature. It's generally incorrect for a function to require particular ownership semantics of its parameters.

I'm fine just dropping it to void*, but I somewhat wish that we had a better type to express that it's an environment block. I'd much rather not write more boilerplate just to do so, so I'm not interested in introducing another manual reference counting struct type. 🤷

DHowett · 2020-08-11T18:46:04Z

There, I switched it to a std::optional<gsl::not_null<void*>>.

KalleOlaviNiemitalo · 2020-08-11T19:08:50Z

I don't see any gsl::not_null in the diff….

DHowett · 2020-08-11T19:09:37Z

Well, I decided that an optional-not-null-void* was the same as a void* so I cut out the middle party 😉

miniksa

While I think the 3-way merge is probably the ideal solution, I can see it being prohibitively expensive to figure out before we know that it's warranted.

Making the 0-way merge of a fresh block solves the immediate problem. And I think it would/will be minor to add a follow-on of "which vars would you like to inherit and stomp in the new block" if that becomes necessary (at a much lower cost than a full merge resolution).

So I'm OK with this.

ghost · 2020-08-11T23:58:42Z

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

DHowett · 2020-08-25T17:07:24Z

src/types/Environment.cpp

+EnvironmentBlockPtr Microsoft::Console::Utils::CreateEnvironmentBlock()
+{
+    void* newEnvironmentBlock{ nullptr };
+    if (!::CreateEnvironmentBlock(&newEnvironmentBlock, GetCurrentProcessToken(), FALSE))


@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?

whoops - @nathpete-msft

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

That does it!

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.

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 😄

This fixes a regression in environment variable loading introduced as part of the new environment block creation that prevents some system-defined, volatile environment variables from being defined. ## References #7243 (comment) ## Validation Steps Performed Manually verified locally. Closes #7399

This fixes a regression in environment variable loading introduced as part of the new environment block creation that prevents some system-defined, volatile environment variables from being defined. ## References #7243 (comment) ## Validation Steps Performed Manually verified locally. Closes #7399 (cherry picked from commit 64f10a0)

ghost · 2020-08-26T17:03:15Z

🎉Windows Terminal Preview v1.3.2382.0 has been released which incorporates this pull request.:tada:

Handy links:

…ss (#7243)" This reverts commit 849243a. References #7418

This fixes a regression in environment variable loading introduced as part of the new environment block creation that prevents some system-defined, volatile environment variables from being defined. ## References microsoft/terminal#7243 (comment) ## Validation Steps Performed Manually verified locally. Closes #7399

…ss (#7243)" This reverts commit 849243a. References #7418 (cherry picked from commit 4204d25)

…ss (#7243)" This reverts commit 849243a. References #7418 (cherry picked from commit 4204d25) (cherry picked from commit fce00ff)

Revert "Fix environment block creation (#7401)" This reverts commit 7886f16. (cherry picked from commit e46ba65) Revert "Always create a new environment block before we spawn a process (#7243)" This reverts commit 849243a. References #7418 (cherry picked from commit 4204d25)

Revert "Fix environment block creation (#7401)" This reverts commit 7886f16. (cherry picked from commit e46ba65) Revert "Always create a new environment block before we spawn a process (#7243)" This reverts commit 849243a. References #7418 (cherry picked from commit 4204d25) (cherry picked from commit f8e8572)

Revert "Fix environment block creation (#7401)" This reverts commit 7886f16. (cherry picked from commit e46ba65) Revert "Always create a new environment block before we spawn a process (#7243)" This reverts commit 849243a. References #7418 (cherry picked from commit 4204d25) (cherry picked from commit f8e8572) (cherry picked from commit cb4c4f7)

Revert "Fix environment block creation (#7401)" This reverts commit 7886f16. (cherry picked from commit e46ba65) Revert "Always create a new environment block before we spawn a process (#7243)" This reverts commit 849243a. References #7418 (cherry picked from commit 4204d25) (cherry picked from commit f8e8572) (cherry picked from commit cb4c4f7) (cherry picked from commit afb0cac)

Revert "Fix environment block creation (#7401)" This reverts commit 7886f16. (cherry picked from commit e46ba65) Revert "Always create a new environment block before we spawn a process (#7243)" This reverts commit 849243a. References #7418 (cherry picked from commit 4204d25) (cherry picked from commit f8e8572) (cherry picked from commit cb4c4f7) (cherry picked from commit afb0cac) (cherry picked from commit b25dc74)

kpentaris · 2021-11-22T12:35:18Z

@DHowett Hello. Has this issue regressed? I'm on WT version 1.11.2921.0 and I'm having the same problem albeit using the "setx /M" command to update the system wide env variable. Is this different?

KalleOlaviNiemitalo · 2021-11-22T12:47:19Z

@kpentaris, these changes have been repeatedly reverted from stable releases of Windows Terminal, e.g. in 616a71d for Windows Terminal 1.11.2921.0. If you want this feature, use Windows Terminal Preview for now. I guess it will continue this way until the Windows PowerShell (x86) incompatibility #7418 is fixed somehow.

zadjii-msft · 2021-11-22T12:47:31Z

@kpentaris I believe we never ended up shipping this to stable, and it's only in the Preview builds of the Terminal

kpentaris · 2021-11-22T12:58:49Z

D'oh! It's actually mentioned in the latest commit... Very well, hopefully at some point this is fixed.

Revert "Fix environment block creation (#7401)" This reverts commit 7886f16. (cherry picked from commit e46ba65) Revert "Always create a new environment block before we spawn a process (#7243)" This reverts commit 849243a. References #7418 (cherry picked from commit 4204d25) (cherry picked from commit f8e8572) (cherry picked from commit cb4c4f7) (cherry picked from commit afb0cac) (cherry picked from commit b25dc74)

Revert "Fix environment block creation (#7401)" This reverts commit 7886f16. (cherry picked from commit e46ba65) Revert "Always create a new environment block before we spawn a process (#7243)" This reverts commit 849243a. References #7418 (cherry picked from commit 4204d25) (cherry picked from commit f8e8572) (cherry picked from commit cb4c4f7) (cherry picked from commit afb0cac) (cherry picked from commit b25dc74) (cherry picked from commit 5f7c66b)

Revert "Fix environment block creation (#7401)" This reverts commit 7886f16. (cherry picked from commit e46ba65) Revert "Always create a new environment block before we spawn a process (#7243)" This reverts commit 849243a. (cherry picked from commit 4204d25) (cherry picked from commit f8e8572) (cherry picked from commit cb4c4f7) (cherry picked from commit afb0cac) (cherry picked from commit b25dc74) (cherry picked from commit 5f7c66b) Fixes #7418

DHowett mentioned this pull request Aug 11, 2020

Environment changes are only picked up upon full restart #7239

Closed

KalleOlaviNiemitalo reviewed Aug 11, 2020

View reviewed changes

zadjii-msft assigned DHowett Aug 11, 2020

zadjii-msft added Area-TerminalConnection Issues pertaining to the terminal<->backend connection interface Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Aug 11, 2020

DHowett added 2 commits August 11, 2020 11:40

use std::optional<gsl::not_null<void*>> instead

58fd4cd

and spell it

8b50d74

zadjii-msft approved these changes Aug 11, 2020

View reviewed changes

DHowett added the Needs-Second It's a PR that needs another sign-off label Aug 11, 2020

ghost requested review from PankajBhojwani, carlos-zamora, leonMSFT and miniksa August 11, 2020 19:25

carlos-zamora approved these changes Aug 11, 2020

View reviewed changes

miniksa approved these changes Aug 11, 2020

View reviewed changes

ghost added Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) labels Aug 11, 2020

DHowett added AutoMerge Marked for automatic merge by the bot when requirements are met and removed Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) labels Aug 11, 2020

ghost merged commit 849243a into master Aug 11, 2020

ghost deleted the dev/duhowett/what_if_we_just_ branch August 11, 2020 23:58

DHowett commented Aug 25, 2020

View reviewed changes

ghost removed the Needs-Second It's a PR that needs another sign-off label Aug 25, 2020

nathpete-msft mentioned this pull request Aug 25, 2020

Fix environment block creation #7401

Merged

ghost mentioned this pull request Aug 26, 2020

PATH in child process includes Windows Terminal Preview installation directory, if started via Win+R wt.exe #7204

Closed

DHowett mentioned this pull request Aug 30, 2020

Profiles do not inherit environment variables **injected into explorer.exe with CreateRemoteThread** #7465

Closed

KalleOlaviNiemitalo mentioned this pull request Sep 3, 2020

Win+R: wt launches a Windows Terminal that doesn't work for some apps. #7460

Closed

DHowett added a commit that referenced this pull request Sep 21, 2020

Revert "Always create a new environment block before we spawn a proce…

4204d25

…ss (#7243)" This reverts commit 849243a. References #7418

DHowett added a commit that referenced this pull request Nov 9, 2020

Revert "Always create a new environment block before we spawn a proce…

fce00ff

…ss (#7243)" This reverts commit 849243a. References #7418 (cherry picked from commit 4204d25)

DHowett mentioned this pull request Nov 10, 2020

New Tabs do not read Environmental variables a-new #8217

Closed

zadjii-msft mentioned this pull request Dec 11, 2020

open new wt.exe can't update user path #8542

Closed

DHowett added a commit that referenced this pull request Jan 25, 2021

Revert "Always create a new environment block before we spawn a proce…

109f9c4

…ss (#7243)" This reverts commit 849243a. References #7418 (cherry picked from commit 4204d25) (cherry picked from commit fce00ff)

zadjii-msft mentioned this pull request Dec 15, 2021

Environment variables not updated when creating a new tab. #11938

Closed

DHowett mentioned this pull request Jan 11, 2022

Combined revert of Environment Block Changes #12140

Merged

This pull request was closed.

Conversation

DHowett commented Aug 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KalleOlaviNiemitalo Aug 11, 2020

Choose a reason for hiding this comment

Uh oh!

zadjii-msft Aug 11, 2020

Choose a reason for hiding this comment

Uh oh!

zadjii-msft commented Aug 11, 2020

Uh oh!

DHowett commented Aug 11, 2020

Uh oh!

DHowett commented Aug 11, 2020

Uh oh!

KalleOlaviNiemitalo commented Aug 11, 2020

Uh oh!

DHowett commented Aug 11, 2020

Uh oh!

miniksa left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Aug 11, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

DHowett Aug 25, 2020

Choose a reason for hiding this comment

Uh oh!

DHowett Aug 25, 2020

Choose a reason for hiding this comment

Uh oh!

nathpete-msft Aug 25, 2020

Choose a reason for hiding this comment

Uh oh!

KalleOlaviNiemitalo Aug 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DHowett Aug 26, 2020

Choose a reason for hiding this comment

Uh oh!

ghost commented Aug 26, 2020

Uh oh!

kpentaris commented Nov 22, 2021

Uh oh!

KalleOlaviNiemitalo commented Nov 22, 2021

Uh oh!

zadjii-msft commented Nov 22, 2021

Uh oh!

kpentaris commented Nov 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

DHowett commented Aug 11, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

KalleOlaviNiemitalo Aug 25, 2020 •

edited

Loading