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

Install-ChocolateyPath Expands Variables in PATH, Overwriting Preexisting Variables #303

Closed
mnadel opened this issue Jun 2, 2015 · 15 comments

Comments

@mnadel
Copy link
Contributor

mnadel commented Jun 2, 2015

Install-ChocolateyPath will expand all variables in your PATH, and then write back the expanded values, clobbering any variables that existed in PATH prior to Install-ChocolateyPath being called.

e.g. %PYTHON_HOME%;C:\Utils -> C:\Python27;C:\Utils

@ferventcoder
Copy link
Member

Clobbering sounds strong - it's really just accidentally expanding the values, right?

@ferventcoder
Copy link
Member

Okay - I get it about clobbering variables in entries - might need a better way of stating this - I took it wrong on first read.

@mnadel mnadel changed the title Install-ChocolateyPath Clobbers Variables in PATH Install-ChocolateyPath Expands Variables in PATH, Overwriting Preexisting Variables Jun 3, 2015
@mnadel
Copy link
Contributor Author

mnadel commented Jun 3, 2015

Sorry about that -- updated the title and added an example.

@ferventcoder
Copy link
Member

👍

@mnadel
Copy link
Contributor Author

mnadel commented Jun 3, 2015

This seem like a reasonable approach? Would be happy to open a PR.

http://stackoverflow.com/questions/4598532/powershell-get-tmp-environment-variable-raw-value

@ferventcoder
Copy link
Member

Yep, seems reasonable - I did similar in Puppet's windows installer and planned to reimplement here at some point. Target stable branch.

@ferventcoder
Copy link
Member

Also make sure you read and agree to CONTRIBUTING.

mnadel added a commit to mnadel/choco that referenced this issue Jun 4, 2015
…erwriting

Preexisting Variables

When adding a new element to PATH, Install-ChocolateyPath reads PATH
(wh ich contains expanded variables), appends the new element, and then
writes the value back to PATH. This results in PATH having its
variables overwritten with its values. Instead of reading $env:PATH to
find the current path, read it from the registry, which will retain the
PATH's original variables.
@ferventcoder ferventcoder modified the milestones: 0.9.9.7, 0.9.9.8 Jun 12, 2015
@ferventcoder ferventcoder modified the milestones: 0.9.9.8, 0.9.9.9 Jun 26, 2015
mnadel added a commit to mnadel/choco that referenced this issue Jul 30, 2015
…erwriting

Preexisting Variables

When adding a new element to PATH, Install-ChocolateyPath reads PATH
(wh ich contains expanded variables), appends the new element, and then
writes the value back to PATH. This results in PATH having its
variables overwritten with its values. Instead of reading $env:PATH to
find the current path, read it from the registry, which will retain the
PATH's original variables.
mnadel added a commit to mnadel/choco that referenced this issue Jul 30, 2015
…erwriting

Preexisting Variables

When adding a new element to PATH, Install-ChocolateyPath reads PATH
(wh ich contains expanded variables), appends the new element, and then
writes the value back to PATH. This results in PATH having its
variables overwritten with its values. Instead of reading $env:PATH to
find the current path, read it from the registry, which will retain the
PATH's original variables.
mnadel added a commit to mnadel/choco that referenced this issue Jul 31, 2015
…erwriting

Preexisting Variables

When adding a new element to PATH, Install-ChocolateyPath reads PATH
(wh ich contains expanded variables), appends the new element, and then
writes the value back to PATH. This results in PATH having its
variables overwritten with its values. Instead of reading $env:PATH to
find the current path, read it from the registry, which will retain the
PATH's original variables.
@ferventcoder ferventcoder modified the milestones: 0.9.10, 0.9.9.9 Oct 2, 2015
@ferventcoder
Copy link
Member

Bumping this to 0.9.10 while waiting for updates from @mnadel

@mnadel
Copy link
Contributor Author

mnadel commented Oct 3, 2015

In reviewing this, I believe the latest commit on 30 Jul - bafe091 contains all changes per our latest conversation. Let me know if you've got any other feedback, otherwise I believe it's ready for merging.

@ferventcoder
Copy link
Member

@mnadel If you can review the PR, it doesn't appear to be updated yet. Example #373 (comment)

@DarwinJS
Copy link
Contributor

DarwinJS commented Jan 8, 2016

deleted

@ferventcoder ferventcoder modified the milestones: 0.9.10, 0.9.10.1 Jan 29, 2016
@ferventcoder ferventcoder self-assigned this Jan 29, 2016
ferventcoder pushed a commit to ferventcoder/choco that referenced this issue Apr 9, 2016
…n PATH

When adding a new element to PATH, Install-ChocolateyPath reads PATH
(wh ich contains expanded variables), appends the new element, and then
writes the value back to PATH. This results in PATH having its
variables overwritten with its values. Instead of reading $env:PATH to
find the current path, read it from the registry, which will retain the
PATH's original variables.
ferventcoder pushed a commit to ferventcoder/choco that referenced this issue Apr 9, 2016
- Declare argument as `switch` instead of `bool`.
- Renamed some variables and added comments.
- Updated references to Get-EnvironmentVariable.
- Cleaned up formatting.
- Allowed getting process scoped environment variables again.
ferventcoder added a commit to ferventcoder/choco that referenced this issue Apr 9, 2016
* pr600-path-cont:
  (chocolateyGH-303) Get-EnvironmentVariable Changes
  (chocolateyGH-303) Install-ChocolateyPath - Do Not Expand Variables in PATH
ferventcoder added a commit to ferventcoder/choco that referenced this issue Apr 9, 2016
* stable:
  (chocolateyGH-303) Get-EnvironmentVariable Changes
  (chocolateyGH-303) Install-ChocolateyPath - Do Not Expand Variables in PATH
ferventcoder added a commit that referenced this issue Apr 11, 2016
Process environment variables should be returned immediately
ferventcoder added a commit that referenced this issue Apr 11, 2016
* stable:
  (maint) installer messages / checks
  (GH-303) Additional fixes for process env vars
  (doc) update changelog
@ferventcoder
Copy link
Member

This change had a very bad side effect.

If you call Environment.SetEnvironmentVariable("NewExpandedPATH", "%WinDir%, EnvironmentVariableTarget.Machine) and then call Environment.GetEnvironmentVariable("NewExpandedPATH"); the value returned is null.

The reason is that SetEnvironmentVariable creates the NewExpandedPath value in the registry as a REG_SZ data type instead of REG_EXPAND_SZ. For some reason the BCL team decided to leave this feature out.

When you don't expand the value and then you update using [Environment]::SetEnvironmentVariable($Name, $Value, $Scope), it has the side effect of overwriting the original registry value type and saves the values as REG_SZ instead of keeping it as REG_EXPAND_SZ. Then all of those tools in WinDir no longer work all of a sudden and it's somewhat perplexing what happened, causing #699.

@DarwinJS
Copy link
Contributor

DarwinJS commented May 5, 2016

Unbelievable!
I guess writing the registry key directly would be next best (though a little more work).

Although the information you found has very profound effects in the case of the system path - does it seem like the unintended expansion of the environment variable references themselves in the current chocolatey implementation still needs addressing?

The original issue as well as the inept .NET approach both have bad implications for Chocolatey packages breaking other things on the machine if they update the path.

@ferventcoder
Copy link
Member

@DarwinJS I have a fix I'm testing now. This only affects the releases listed at #699 (comment)

@ferventcoder
Copy link
Member

@DarwinJS this actually only affects a limited number of systems. You would have had to install those beta versions directly for it to occur - here's why: #699 (comment)

ferventcoder added a commit that referenced this issue May 5, 2016
The fixes for GH-303 (e050c22) had an interesting and terrible side
effect. When you don't expand the value and then you update using
`[Environment]::SetEnvironmentVariable($Name, $Value, $Scope)`, it has
the side effect of overwriting the original registry value type and
saves the values as `REG_SZ` instead of keeping it as `REG_EXPAND_SZ`.
Then all of the tools in WinDir no longer work all of a sudden and
it's somewhat perplexing what happened as everything looks appropriate
when you inspect the values.

You can verify if you are affected by opening a new command line and
typing `where.exe choco` and the system cannot find where.exe, which is
at `C:\Windows\System32\where.exe`.

You are only affected if you installed (explicitly installed, not upgraded)
one of the following beta releases directly:

 * https://chocolatey.org/packages/chocolatey/0.9.10-beta-20160411
 * https://chocolatey.org/packages/chocolatey/0.9.10-beta-20160422
 * https://chocolatey.org/packages/chocolatey/0.9.10-beta-20160503

This is fixed by attempting to ask the registry what the value type is
for the particular key and falling back to `REG_SZ` when it is not
found. Environment variables are always either String or ExpandString. As
a further step, if it is the `PATH` variable, it will always be saved as
ExpandString (`REG_EXPAND_SZ`).

Then set the value using `[Microsoft.Win32.Registry]::SetValue` instead
because it allows setting the value type.
ferventcoder added a commit that referenced this issue May 6, 2016
Removing environment variables sets empty environment variables. This
is related to the changes for GH-303.

So determine if the value is null or empty, and perform the older
behavior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants