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

env: always expand PATH vars #15444

Merged
merged 4 commits into from
May 26, 2023
Merged

env: always expand PATH vars #15444

merged 4 commits into from
May 26, 2023

Conversation

sotteson1
Copy link
Contributor

@sotteson1 sotteson1 commented May 25, 2023

Make sure we always expand path env vars, even if they're REG_SZ in the registry.

Detailed Description of the Pull Request / Additional comments

On some systems path vars are REG_SZ instead of REG_EXPAND_SZ. We need to make sure we always expand them. We looked at the system code, and it also makes to sure to always expand them.

Validation Steps Performed

Built locally and made sure the problem went away. Also stepped through in the debugger to make sure things were working correctly.

Closes #15442

@microsoft-github-policy-service microsoft-github-policy-service bot added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-TerminalConnection Issues pertaining to the terminal<->backend connection interface Priority-1 A description (P1) Product-Terminal The new Windows Terminal. labels May 25, 2023
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

This seems sensible to me

src/inc/til/env.h Outdated Show resolved Hide resolved
Co-authored-by: Mike Griese <migrie@microsoft.com>
@DHowett DHowett changed the title Always expand path vars env: always expand PATH vars May 26, 2023
@DHowett DHowett enabled auto-merge (squash) May 26, 2023 18:06
@DHowett
Copy link
Member

DHowett commented May 26, 2023

Thanks @sotteson1 for fixing this! Great find!

@DHowett DHowett merged commit 709189d into microsoft:main May 26, 2023
DHowett pushed a commit that referenced this pull request May 26, 2023
Make sure we always expand path env vars, even if they're REG_SZ in the
registry.

## Detailed Description of the Pull Request / Additional comments
On some systems path vars are REG_SZ instead of REG_EXPAND_SZ. We need
to make sure we always expand them. We looked at the system code, and it
also makes to sure to always expand them.

## Validation Steps Performed
Built locally and made sure the problem went away. Also stepped through
in the debugger to make sure things were working correctly.

Closes #15442

(cherry picked from commit 709189d)
Service-Card-Id: 89337408
Service-Version: 1.18
@sotteson1 sotteson1 deleted the env-bug branch May 26, 2023 21:09
@sba923
Copy link

sba923 commented May 29, 2023

One might wonder why on some systems path vars are REG_SZ (containing candidates for expansion!) instead of REG_EXPAND_SZ in the first place, and why the Windows code does perform expansion on said REG_SZs...

@DHowett
Copy link
Member

DHowett commented May 29, 2023

PATH is probably the variable changed by the most installers, and so it gets exposed to all sorts of bugs. If I were to guess, expanding it is a defense mechanism against poorly-written software.

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-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal.
Projects
Development

Successfully merging this pull request may close these issues.

PATH not getting constructed correctly
5 participants