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

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

Closed
KalleOlaviNiemitalo opened this issue Aug 6, 2020 · 13 comments · Fixed by #7243
Labels
Area-TerminalConnection Issues pertaining to the terminal<->backend connection interface Culprit-Centennial Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@KalleOlaviNiemitalo
Copy link

KalleOlaviNiemitalo commented Aug 6, 2020

Environment

Windows build number: Microsoft Windows [Version 10.0.19041.388]
Windows Terminal version (if applicable): Windows Terminal Preview 1.2.2022.0

Steps to reproduce

  1. Enable the wt.exe app execution alias of Windows Terminal Preview.
  2. Press Windows+R to open the Run dialog box.
  3. Type wt.exe and press Enter to start Windows Terminal Preview.
  4. In Windows Terminal Preview, start the Command Prompt profile.
  5. In the Command Prompt session, type PATH and press Enter.

Expected behavior

The displayed value of PATH does not include the directory where Windows Terminal Preview was installed.

Actual behavior

C:\Program Files\WindowsApps\Microsoft.WindowsTerminalPreview_1.2.2022.0_x64__8wekyb3d8bbwe is the first directory in PATH.

Notes

This is presumably caused by the Registry values that were created by AppXSvc when Windows Terminal Preview was installed:

Windows Registry Editor Version 5.00

[HKEY_CURRENT_USER\SOFTWARE\Microsoft\Windows\CurrentVersion\App Paths\wt.exe]
@="C:\\Program Files\\WindowsApps\\Microsoft.WindowsTerminalPreview_1.2.2022.0_x64__8wekyb3d8bbwe\\wt.exe"
"Path"="C:\\Program Files\\WindowsApps\\Microsoft.WindowsTerminalPreview_1.2.2022.0_x64__8wekyb3d8bbwe"

[HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows\CurrentVersion\App Paths\wt.exe]
@="C:\\Program Files\\WindowsApps\\Microsoft.WindowsTerminalPreview_1.2.2022.0_x64__8wekyb3d8bbwe\\wt.exe"
"Path"="C:\\Program Files\\WindowsApps\\Microsoft.WindowsTerminalPreview_1.2.2022.0_x64__8wekyb3d8bbwe"

According to Application Registration, ShellExecuteEx prepends the data of the "Path" Registry value to the PATH environment variable of the new process. That seems unnecessary here, as selecting Windows Terminal Preview from the Start menu does not add to PATH.

Because non-packaged processes of users have no access to the "C:\Program Files\WindowsApps" directory, this addition to PATH perhaps does not cause problems. However, if wt.exe is run as administrator, then the PATH addition could cause a process in the terminal session to load DLLs from the installation directory of Windows Terminal Preview. (If a packaged application is run within the terminal session, then that too would have access to the directory, but Search Order for Windows Store apps looks like the PATH environment variable will not affect DLL loading in that case.)

Related to #6860, #6748, #7188. Found while searching for possible causes of #7195.

@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 Aug 6, 2020
@DHowett
Copy link
Member

DHowett commented Aug 6, 2020

So, a few notes.

  • I think this is harmless. There's no executable in our package that the user shouldn't be able to launch, and nothing that will incorrectly eclipse things that are on their PATHs.
  • We don't have any control over this.
  • I can't find this entry in PATH when I elevate wt.exe
  • Packaged applications don't follow the traditional DLL search order, so I don't think it likely that they'll inherit WT's libraries

@DHowett
Copy link
Member

DHowett commented Aug 6, 2020

Incidentally, this and the last seven issues are the first (and second through seventh) times I've ever heard of App Paths, and I didn't know the system was maintaining it on our behalf.

@KalleOlaviNiemitalo
Copy link
Author

KalleOlaviNiemitalo commented Aug 6, 2020

  • I can't find this entry in PATH when I elevate wt.exe

How do you elevate it? If I type wt.exe in the Run dialog box and press Ctrl+Shift+Enter, then the WT installation directory does appear in PATH in the Command Prompt session.

@DHowett
Copy link
Member

DHowett commented Aug 6, 2020

Yep, using Ctrl+Shift+Enter from the Run dialog.

@KalleOlaviNiemitalo
Copy link
Author

We don't have any control over this.

I imagine Windows Terminal could explicitly delete its installation directory from PATH in the environment block that it passes to CreateProcess.

Or use CreateEnvironmentBlock as suggested in #1125, instead of copying PATH and other variables from the environment of the WindowsTerminal.exe process.

Yep, using Ctrl+Shift+Enter from the Run dialog.

That's strange… Are you using the same version of Windows 10? Does the PATH change even get to the WindowsTerminal.exe process? Process Monitor filtering with Operation = Process Start is handy for checking this.

DHowett added a commit that referenced this issue Aug 11, 2020
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)
@ghost ghost added the In-PR This issue has a related PR label Aug 11, 2020
@zadjii-msft zadjii-msft added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-TerminalConnection Issues pertaining to the terminal<->backend connection interface Product-Terminal The new Windows Terminal. Priority-2 A description (P2) and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Aug 11, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Aug 11, 2020
@ghost ghost closed this as completed in #7243 Aug 11, 2020
ghost pushed a commit that referenced this issue Aug 11, 2020
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)
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Aug 11, 2020
@ghost
Copy link

ghost commented Aug 26, 2020

🎉This issue was addressed in #7243, which has now been successfully released as Windows Terminal Preview v1.3.2382.0.:tada:

Handy links:

@KalleOlaviNiemitalo
Copy link
Author

#7243 did not fix this for the following child processes:

  • the editor that Windows Terminal starts for editing settings.json (and presumably defaults.json)
  • C:\WINDOWS\system32\wsl.exe --list
  • Web browser processes started by the Feedback menu item or by hyperlinks on the About screen

According to #7460, the extra entry in Path interferes with the loading of vcruntime140.dll from other directories in Path, if C:\Windows\System32\VCRUNTIME140.dll does not exist.

Please reopen.

@DHowett DHowett reopened this Sep 4, 2020
@DHowett
Copy link
Member

DHowett commented Sep 4, 2020

Huh.

@DHowett DHowett removed the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Sep 4, 2020
@DHowett DHowett added this to the Terminal v1.4 milestone Sep 4, 2020
@JustinGrote
Copy link

JustinGrote commented Sep 24, 2020

@DHowett not sure if this is related but when I do Win+R then wt.exe, currently getting this:
image

EDIT: It was this problem. I deleted the above registry keys and restarted, put the wt preview in my path, and everything works fine.

@KalleOlaviNiemitalo
Copy link
Author

Retested with Windows Terminal Preview 1.12.3472.0 on Windows 10.0.19044.1415:

  • 🆗 The Command Prompt session in the tab does not inherit the extra PATH.
  • 🆗 No wsl.exe --list process is started.
  • 🆗 The wsl.exe -d Debian process does not inherit the extra PATH.
  • ⚠️ The Web browser process started by the Documentation link in the About dialog box inherits the extra path.
  • ⚠️ The JSON file editor inherits the extra path.

@zadjii-msft
Copy link
Member

zadjii-msft commented Jul 28, 2022

Merging notes:

I can reproduce this, but it is not specific to WSL.

  1. Pin Windows Terminal Preview 1.15.2002.0 to the task bar on Windows 10.0.19044.1826. (maintainer note: also repros on Win11)
  2. Set Command Prompt as the default profile.
  3. Close Windows Terminal Preview.
  4. Start Windows Terminal Preview by left-clicking the task-bar button.
  5. Execute PATH in the Command Prompt session. The output does not include the Windows Terminal Preview installation directory. ✅
  6. Open another tab by right-clicking the task-bar button and left-clicking the "Command Prompt" task.
  7. Execute PATH in the Command Prompt session in the second tab. The output does not include the Windows Terminal Preview installation directory. ✅
  8. Close Windows Terminal Preview.
  9. Start Windows Terminal Preview by right-clicking the task-bar button and left-clicking the "Command Prompt" task.
  10. Execute PATH in the Command Prompt session. The output starts with Windows Terminal Preview installation directory. ❌
  11. Open another tab by right-clicking the task-bar button and left-clicking the "Command Prompt" task.
  12. Execute PATH in the Command Prompt session in the second tab. The output starts with Windows Terminal Preview installation directory. ❌

Probably also related:
#7188 (comment)

I can reproduce with Windows Terminal 1.1.2021.0 and Windows Terminal Preview 1.2.2022.0 on Windows 10 version 2004 (OS Build 19041.388):

  • If I enable the wt.exe app execution alias of Windows Terminal Preview, then wt in File Explorer starts Windows Terminal Preview. ✔
  • If I enable the wt.exe app execution alias of Windows Terminal, then wt in File Explorer shows the access-denied dialog box. ❌
  • If I disable both wt.exe app execution aliases, then wt in File Explorer shows the access-denied dialog box. 🙄 In this case though, I shouldn't expect wt to work anyway.

When I install Windows Terminal Preview, AppXSvc creates the following Registry keys:

  • HKEY_CURRENT_USER\SOFTWARE\Microsoft\Windows\CurrentVersion\App Paths\wt.exe
  • HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows\CurrentVersion\App Paths\wt.exe

Those Registry keys stay there when I enable or disable the wt.exe app execution aliases. They seem related to #6748 and #6860. If I rename the Registry keys, then the results change:

  • If I enable the wt.exe app execution alias of Windows Terminal Preview, then wt in File Explorer runs Windows Terminal Preview. ✔
  • If I enable the wt.exe app execution alias of Windows Terminal, then wt in File Explorer runs Windows Terminal. ✔
  • If I disable both wt.exe app execution aliases, then wt in File Explorer opens http://wt/ in my Web browser. 🙄

If I disable the skype.exe app execution alias and type skype.exe in the Windows+R run box or in the File Explorer address bar, then I get a similar access-denied dialog box, so this is not specific to Windows Terminal.

If Windows needs a Registry key in App Paths, then perhaps it should keep the per-user key always synchronized with the app execution aliases, and not create the per-machine key.

So, we added wt.exe to the package[1], so that the alias could point at an exe with the same literal name. But that caused MORE issues. Sometimes, the package's install directory is prepended to the path. That causes the OS to load the wt from inside WindowsApps, instead of the actual alias.

dupes?

internal work

  • MSFT:28217948 was duped to MSFT:29353059 which was fixed in os.2020!5514481, which should have shipped in 21307, around the time of Windows 11.

here's my understanding:
image
I believe that bad ACLs on WindowsApps can cause the first PERMISSION_DENIED to fail (user was NOT denied access), and bad ACLs on the WindowsTerminal package can cause the "Try Again" to fail (Package-ID-Token was rejected?)

Conclusions:

  • "Win+R / Explorer.exe failed to start wt.exe": That should probably be fixed in Windows 11
  • **"wt is launched with C:\Program Files\WindowsApps\Microsoft.WindowsTerminal* prepended to the path: We should probably just cut that out of the PATH ourselves, if we see it. It'll do no good.

[1]: See #6860 which was supposed to fix #6625:

It turns out that when an application's alias name doesn't match its filename (as in our case, wt.exe is an alias for WindowsTerminal.exe ), it fails out during elevated launch.

@zadjii-msft
Copy link
Member

A thought: This might have been fixed by #14999.

@zadjii-msft zadjii-msft modified the milestones: 22H2, Terminal v1.18 Mar 21, 2023
@ianjoneill
Copy link
Contributor

A thought: This might have been fixed by #14999.

Yep - looks like it:

image

And if I set compatibility.reloadEnvironmentVariables to false:

image

@zadjii-msft zadjii-msft added the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Mar 22, 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 Culprit-Centennial Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants