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

REG_EXPAND_SZ environment variables not properly expanded for shells #9741

Closed
chrisdjali-wrld3d opened this issue Apr 7, 2021 · 13 comments
Closed
Labels
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. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal.

Comments

@chrisdjali-wrld3d
Copy link

Windows Terminal version (or Windows build number)

1.7.572.0

Other Software

No response

Steps to reproduce

  1. Set a system environment variable including %USERPROFILE%, e.g. TestVariable set to %USERPROFILE%\bin.
  2. In Regedit, confirm that the type is REG_EXPAND_SZ by finding it under HKLM\SYSTEM\CurrentControlSet\Control\Session Manager\Environment.
  3. Start PowerShell or CMD normally, e.g. from the WinX menu, or Shift+RMB in Windows Explorer.
  4. Print the variable's value, e.g. echo $env:TestVariable.
  5. Observe that %USERPROFILE% has been expanded, e.g. C:\Users\Chris\bin is printed.
  6. Open Windows Terminal and create a new PowerShell or CMD tab.
  7. Print the variable's value, e.g. echo $env:TestVariable.
  8. Observe that %USERPROFILE% has not been expanded, e.g. %USERPROFILE%\bin is printed.

Expected Behavior

Like with standalone PowerShell or CMD via CONHOST, the environment variable is properly expanded.

Actual Behavior

The unexpanded value is used.

This isn't happening with every environment variable - others get expanded correctly, like windir being %SystemRoot% becoming C:\WINDOWS. My suspicion is that it's just variables not defined in the system scope not being expanded if they're used in the system scope.

I'm pretty sure this is Windows Terminal doing something weird rather than Windows itself as when viewing the WindowsTerminal.exe process' environment in Process Explorer, the variables are properly expanded, but when doing the same for the actual shell process, they aren't. If I had to guess, I'd say maybe Windows Terminal is attempting to read the up-to-date environment from the registry when starting new shells instead of letting them inherit its potentially outdated environment, but is doing so inconsistently with how Windows does it. If that's actually what's happening, it may be sensible to use Chocolatey's RefreshEnv batch script or PowerShell module as a reference implementation as that gets the same result as Windows.

This is happening in MSYS-esque Bash shells, too, but the Windows-to-Unix path conversion seems to be doing another round of expansion that masks the symptoms.

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Apr 7, 2021
@DHowett
Copy link
Member

DHowett commented Apr 7, 2021

So, this isn’t the first issue with our environment block. Thanks for reporting it!

We’re using the Win32 API that generates a new environment block and not doing anything special on top of that. It is rather alarming, but somewhat par for the course, that it doesn’t work quite right.

We probably need to do more things manually instead of fewer things, because the API fixtures available to us do those things wrong.

@eryksun
Copy link

eryksun commented Apr 8, 2021

A system variable can't reliably refer to a user variable. This is going to be unreliable in many cases. It works with the shell APIs private shell32!RegenerateUserEnvironment() function because it's explicitly for the current user. But other cases -- in particular CreateEnvironmentBlock() without inheritance -- load the environment in a systematic way that does not allow a system variable to refer to a user variable.

First load system variables:

  • inherit SystemRoot and SystemDrive from the current process
  • load ProgramData, ALLUSERSPROFILE, and PUBLIC from the registry
  • load REG_SZ values from "HKLM\SYSTEM\CurrentControlSet\Control\Session Manager\Environment"
  • load REG_EXPAND_SZ values from the above key
  • set COMPUTERNAME from GetComputerNameExW()
  • load ProgramFiles and CommonProgramFiles (and x86 and W6432 variants) from the registry

Next load user variables:

  • load USERPROFILE, APPDATA, and LOCALAPPDATA from the registry based on the user SID
  • load REG_SZ values from "HKU\<SID string>\Environment"
  • load REG_EXPAND_SZ values from "HKU\<SID string>\Environment"
  • append user Path, LibPath, and Os2LibPath variables to corresponding system value, if any
  • replace TEMP and TMP with short paths via GetShortPathNameW()
  • load volatile values for the logon session from "HKU\<SID string>\Volatile Environment": LOGONSERVER, USERNAME, USERDOMAIN, HOMEDRIVE, HOMEPATH, USERPROFILE, APPDATA, LOCALAPPDATA
  • load volatile values for the Windows session from "HKU\<SID string>\Volatile Environment\<session ID>": SESSION_NAME, CLIENT_NAME
  • set USERNAME from GetUserNameExW() (e.g. SYSTEM logons use "%COMPUTERNAME%$", not "SYSTEM")

At each step it's only reliable to refer to variables that were loaded in a previous step. For example user REG_EXPAND_SZ variables can reliably refer to system variables, user REG_SZ variables, USERPROFILE, APPDATA, and LOCALAPPDATA. They cannot reliably refer to other user REG_EXPAND_SZ variables (registry key enumeration order is arbitrary -- I think in creation order) or volatile variables such as USERNAME and HOMEDRIVE.

These limitations have to be abided if you want the user's environment to be correct in contexts such as a process spawned with "runas.exe" or Task Scheduler, and various other cases that use CreateProcessWithLogonW(), CreateProcessAsUserW(), and so on, that need to create a new environment block.

I'd prefer a more reasonable and useful load order. It's always annoyed me that I can't reliably use ProgramFiles in system environment variables or USERNAME in user environment variables. For system variables, I'd load COMPUTERNAME, ProgramFiles, CommonProgramFiles (and variants) before loading the system REG_SZ and REG_EXPAND_SZ values from the system "Environment" key. For user variables, I'd load the user "Volatile Environment" before the persistent user "Environment". That said, I would never expect to be able to reliably use USERPROFILE in a system variable.

@chrisdjali-wrld3d
Copy link
Author

When I originally set this up, the goal was to have a user-specific bin directory prepended onto the path, which can only be done via the system path as the user path gets appended. I don't think I necessarily expected it to work, but nothing that came up in a Google search implied it wouldn't (although it was the kind of search where it's a specific query and all the results are generic guides to the basics), and it did when I tried. It wasn't inconceivable that everything would be loaded from the registry before any substitution was done, and therefore this was expected to work.

As this is a single-user machine, I can easily work around the problem by hardcoding the path, but I imagine there are other cases where people have done something similar without knowing it only worked because they'd only tried it in the large subset of cases where it didn't matter. I guess whether Windows Terminal 'fixes' this should depend on whether the goal is to make everything that used to work in CONHOST work or whether anyone doing anything sketchy should be forced to start doing it properly.

@eryksun
Copy link

eryksun commented Apr 9, 2021

I guess whether Windows Terminal 'fixes' this should depend on whether the goal is to make everything that used to work in CONHOST work or whether anyone doing anything sketchy should be forced to start doing it properly.

I don't know the reason why they need a new environment block as opposed to creating a copy of the current environment that's returned by GetEnvironmentStrings.

It wasn't inconceivable that everything would be loaded from the registry before any substitution was done, and therefore this was expected to work.

Unfortunately, the order in which environment variables are loaded and expanded is not documented and not consistent in the system.

Check that the value is set as expected with a fresh boot, and that it's not nonsense in system processes that inherit and extend the environment as the system boots (smss.exe, csrss.exe, wininit.exe, services.exe, winlogon.exe). Also, make sure that the desktop shell reloads the right value after setting a new value -- i.e. that it's compatible with RegenerateUserEnvironment in shell32.dll. And make sure that it works with CreateEnvironmentBlock, the documented way to create a new environment block. The latter can be checked with runas.exe /user:%username% "cmd.exe /k set".

When I originally set this up, the goal was to have a user-specific bin directory prepended onto the path, which can only be done via the system path as the user path gets appended.

Or rather as close as one can get to 'prepended', reliably in practice. The default search paths for CreateProcessW() and SearchPathW() always check the application directory and system directories before PATH.

@chrisdjali-wrld3d
Copy link
Author

I guess whether Windows Terminal 'fixes' this should depend on whether the goal is to make everything that used to work in CONHOST work or whether anyone doing anything sketchy should be forced to start doing it properly.

I don't know the reason why they need a new environment block as opposed to creating a copy of the current environment that's returned by GetEnvironmentStrings.

There's one Windows Terminal process that's the parent of all the tabs, so GetEnvironmentStrings would make all the shells have whichever environment variables were set when the parent terminal was launched, not what was set when each shell was launched. Presumably someone decided this was worse than having it refresh them before launching each shell.

Check that the value is set as expected with a fresh boot, and that it's not nonsense in system processes that inherit and extend the environment as the system boots (smss.exe, csrss.exe, wininit.exe, services.exe, winlogon.exe).

It's survived at least a year of reboots without me noticing problems outside Windows Terminal, so it's at least consistent nonsense.

Also, make sure that the desktop shell reloads the right value after setting a new value -- i.e. that it's compatible with RegenerateUserEnvironment in shell32.dll.

Yep, if I edit the value in Advanced System Settings and launch a fresh PowerShell, the variable gets expanded 'properly'.

And make sure that it works with CreateEnvironmentBlock, the documented way to create a new environment block. The latter can be checked with runas.exe /user:%username% "cmd.exe /k set".

I see the same behaviour as Windows Terminal, i.e. %USERPROFILE% is not expanded. I also notice that the %PROGRAMFILES% in PSModulePath isn't expanded, either, but PowerShell seems to do that itself when it's launched, either directly via runas instead of CMD or from within the CMD shell.

When I originally set this up, the goal was to have a user-specific bin directory prepended onto the path, which can only be done via the system path as the user path gets appended.

Or rather as close as one can get to 'prepended', reliably in practice. The default search paths for CreateProcessW() and SearchPathW() always check the application directory and system directories before PATH.

Being Windows, it's more complicated than even that - things like the registry's list of known DLLs get involved before searching is even attempted. For what I needed, what I did was enough, though.

Anyhow, while looking into this, I noticed that I've also got %USERPROFILE%\.dnx\bin in my system path, and looking at its position, it looks like it was put there by the Visual Studio 2015 installer. I can be pretty confident of that as this machine was mostly set up by a script. It's never mattered as it either didn't install DNVM or installed it elsewhere, but it's evidence that I'm not the only person who's tried doing this.

@DHowett
Copy link
Member

DHowett commented Apr 9, 2021

I don't know the reason why they need a new environment block as opposed to creating a copy of the current environment that's returned by GetEnvironmentStrings.

Users continually complain that shells spawned by Terminal do not reflect the most up-to-date system environment. We don't usually do work without reason 😉

@eryksun
Copy link

eryksun commented Apr 9, 2021

GetEnvironmentStrings would make all the shells have whichever environment variables were set when the parent terminal was launched, not what was set when each shell was launched.

I should have seen that this was the reason.

If calling the undocumented function RegenerateUserEnvironment is off limits, then I don't see an easy way to make it agree with Explorer.

CreateEnvironmentBlock, the documented way to create a new environment block. The latter can be checked with runas.exe /user:%username% "cmd.exe /k set".

I see the same behaviour as Windows Terminal, i.e. %USERPROFILE% is not expanded.

Yes, both use CreateEnvironmentBlock. I meant this as a general test to quickly see if a variable will be okay in such cases.

I'd prefer a new CreateEnvironmentBlockEx function that's at least more consistent with RegenerateUserEnvironment. A creation flag could be added to get a new environment block with CreateProcessW, CreateProcessAsUserW, and CreateProcessWithLogonW when lpEnvironment is NULL.

%PROGRAMFILES% in PSModulePath isn't expanded, either, but PowerShell seems to do that itself

Since PowerShell modifes the value of PSModulePath, it doesn't surprise me that it expands it as well. However, it doesn't expand every variable in the entire environment.

Being Windows, it's more complicated than even that - things like the registry's list of known DLLs get involved before searching is even attempted. For what I needed, what I did was enough, though.

I didn't think about DLLs. It has been a while since I used PATH for that.

@DHowett
Copy link
Member

DHowett commented Apr 13, 2021

The current behavior is definitely by design for when we regenerate a new environment block, but if other applications seem to handle this then I think we should take a crack at it.

For reference, linking this up to #9233, #7418, #7204 for environment shenanigans. I suspect that we'll have to do something about them sooner rather than later.

@DHowett DHowett 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. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Apr 13, 2021
@DHowett DHowett added this to the Terminal Backlog milestone Apr 13, 2021
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Apr 13, 2021
@KalleOlaviNiemitalo
Copy link

In #8933 (comment) and nearby comments, HOME=%HOMEDRIVE%%HOMEPATH% gets expanded (by Explorer?) when Windows Terminal is started from the Start menu, but is not expanded when Windows Terminal is started via its shell extension.

@glenn-slayden
Copy link

glenn-slayden commented Sep 27, 2021

@eryksun wrote:
At each step it's only reliable to refer to variables that were loaded in a previous step. For example user REG_EXPAND_SZ variables can reliably refer to system variables, user REG_SZ variables, USERPROFILE, APPDATA, and LOCALAPPDATA. They cannot reliably refer to other user REG_EXPAND_SZ variables...

Does this explain why Path, in HKLM\SYSTEM\CurrentControlSet\Control\Session Manager\Environment, is not itself a REG_EXPAND_SZ?

@chrisdjali-wrld3d
Copy link
Author

It's a REG_EXPAND_SZ on my machine, so I think that might be something weird on your end. By default, it contains things like %SystemRoot%, so would have to expand. Possibly you've run a script which modified your path without being expansion-aware, so it's read the expanded value, changed it, and written it back pre-expanded.

@eryksun
Copy link

eryksun commented Oct 20, 2021

Does this explain why Path, in HKLM\SYSTEM\CurrentControlSet\Control\Session Manager\Environment, is not itself a REG_EXPAND_SZ?

The initial system "Path" value is a REG_EXPAND_SZ value, but it depends on just SystemRoot, which is reliable. It doesn't depend on ProgramFiles, which is unreliable in system REG_EXPAND_SZ values.

For a bad example, as mentioned above, the default value of PSModulePath has been mistakenly defined to include "%ProgramFiles%\WindowsPowerShell\Modules". PowerShell happens to manually expand this value, but anything else that directly uses a new environment block sees the value from the registry without expansion. One could argue that it's the fault of CreateEnvironmentBlock(). It should create all of the basic system configuration variables such as COMPUTERNAME and ProgramFiles before it expands dependent REG_EXPAND_SZ definitions.

That said, administrators and developers should still be made aware that dependent system environment variables cannot reliably depend on each other since they're expanded in arbitrary order. Similarly, dependent user environment variables cannot reliably depend on each other. This is a basic limitation of how the registry "Environment" keys are implemented.

@zadjii-msft
Copy link
Member

Pretty sure this got fixed by #14839 & #14999.

image

@zadjii-msft zadjii-msft removed this from the Backlog milestone Mar 21, 2023
@zadjii-msft zadjii-msft added this to the Terminal v1.18 milestone Mar 21, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal.
Projects
None yet
Development

No branches or pull requests

7 participants
@DHowett @eryksun @glenn-slayden @zadjii-msft @KalleOlaviNiemitalo @chrisdjali-wrld3d and others