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 updates PATH to REG_SZ, which may break using Windows dir and system32 tools #699

Closed
ferventcoder opened this issue Apr 20, 2016 · 19 comments

Comments

@ferventcoder
Copy link
Member

ferventcoder commented Apr 20, 2016

The fixes for #303 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 those 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.

The offending commit is e050c22

You will know you are affected if you open a new command line and type where.exe choco and the system cannot find where.exe.

_UPDATE:_ This would ONLY affect you if you installed the beta versions directly without upgrading to them. Previous versions of Chocolatey would have already expanded the values in PATH and saved it as REG_SZ. The bug that #303 fixes is saving you from this issue.

When you install Chocolatey, it will set itself in the PATH using Install-ChocolateyPath. This is the only time you could be affected by this issue, and only if you installed (not upgraded) an affected version directly. Otherwise the variables would have been expanded and your PATH would continue to work. #303 introduced not expanding the PATH, which causes this issue because you get both not expanding those variables AND switching it to REG_SZ (which it was already doing).

Original

7b0de8f...9b6946a

I haven't found it yet, but it causes Vagrant 1.8.1 to get to the next shell runner and never move forward.

Once I have more details on why I will reach out to the Vagrant folks if necessary.

==> default:
==> default: DEBUG: Add-ChocolateyProfile
==> default: DEBUG: Creating 'C:\Users\Administrator\Documents\WindowsPowerShell'
==> default: DEBUG: Creating
==> default: 'C:\Users\Administrator\Documents\WindowsPowerShell\Microsoft.PowerShell_profil
==> default: e.ps1'
==> default: Adding Chocolatey to the profile. This will provide tab completion, refreshenv, etc.
==> default: WARNING: Chocolatey profile installed. Reload your profile - type . $profile
==> default: DEBUG: Install-DotNet4IfMissing called with $forceFxInstall=False
==> default: Chocolatey (choco.exe) is now ready.
==> default: You can call choco from anywhere, command line or powershell by typing choco.
==> default: Run choco /? for a list of functions.
==> default: You may need to shut down and restart powershell and/or consoles
==> default:  first prior to using choco.
==> default: DEBUG: Remove-OldChocolateyInstall
==> default: Ensuring chocolatey commands are on the path
==> default: Ensuring chocolatey.nupkg is in the lib folder
==> default: Chocolatey v0.9.10-beta1-281-gfd2eea1
==> default: autoUninstaller was enabled by default. Explicitly setting value.
==> default: Enabled autoUninstaller
==> default: Chocolatey v0.9.10-beta1-281-gfd2eea1
==> default: Enabled allowGlobalConfirmation
==> default: Chocolatey v0.9.10-beta1-281-gfd2eea1
==> default: Enabled logEnvironmentValues

==> default: Running provisioner: shell...

It finishes the install of Chocolatey, and then when it goes to run the next provisioner, it just sits there. I've confirmed it in several places.

@ferventcoder
Copy link
Member Author

ferventcoder commented Apr 20, 2016

If I were to guess, I would say it has something to do with the change over to client profile and the log4net logger in the console (which is now a client profile version).

f883846

@ferventcoder
Copy link
Member Author

ferventcoder commented Apr 20, 2016

It could also be the addition of a second file logging source. 7b0de8f...9b6946a#diff-f5411bbcc87de708e96eb743636e04deR94

@ferventcoder
Copy link
Member Author

My plan of attack is to start with the sha of the commit prior to the log4net/client profile change over. That is bfa6c32

Build from that and run the Chocolatey test environment and see if I can reproduce the issue.

@ferventcoder ferventcoder changed the title Latest beta stalls vagrant shell runner PATH is updated to REG_SZ, breaking using Windows dir and system32 tools. May 5, 2016
@jnm2
Copy link

jnm2 commented May 5, 2016

I have 0.9.10-beta-20160411 currently installed. What do I do to detect and reverse the damage? Nothing obvious has broken for me yet.

@ferventcoder
Copy link
Member Author

ferventcoder commented May 5, 2016

@jnm2
You will know you are affected if you open a command line and type where.exe choco and it errors.

@ferventcoder
Copy link
Member Author

Otherwise you are good to go

@ferventcoder
Copy link
Member Author

Just don't install anything else until you install the fix that I'm wrapping up now.

@ferventcoder
Copy link
Member Author

ferventcoder commented May 5, 2016

_UPDATE:_ This would ONLY affect you if you installed the beta versions directly without upgrading to them. Previous versions of Chocolatey would have already expanded the values in PATH and saved it as REG_SZ. The bug that #303 fixes is saving you from this issue.

@ferventcoder ferventcoder changed the title PATH is updated to REG_SZ, breaking using Windows dir and system32 tools. Install-ChocolateyPath updates PATH to REG_SZ, which may break using Windows dir and system32 tools May 5, 2016
@jnm2
Copy link

jnm2 commented May 5, 2016

How is it okay to expand the strings and change the type to REG_SZ at all? Doesn't that mean computers with previous versions of chocolatey are set up to fail if a variable path is appended?

@ferventcoder
Copy link
Member Author

@jnm2 I'm not saying it is okay, I'm just saying you would not be in a broken state currently.

@ferventcoder
Copy link
Member Author

To be clear - broken in this context means "WTF? S--- doesn't work, srsly WTF?!!!", versus, "you changed my PATH variable to REG_SZ, how dare you"

@ferventcoder
Copy link
Member Author

And I think you are going to like the fixes I put in, we are addressing ensuring PATH is expand string.

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
Copy link
Member Author

ferventcoder commented May 5, 2016

Doesn't that mean computers with previous versions of chocolatey are set up to fail if a variable path is appended?

Not with the changes I just implemented (provided you have the updated version installed).

c3d4ff3#diff-2769f13bca26557f72bdf713dd85d6e9R40

On updating PATH it will ensure it is expand string type.

@jnm2
Copy link

jnm2 commented May 5, 2016

Haha, thanks for clarifying. So by forcing chocolatey to the latest beta for the seven computers I have it on, all potential future problems are resolved?

@ferventcoder
Copy link
Member Author

@jnm2 that's the intention. Continually improve. Sometimes you break things along the way. I believe #722 will help reduce regressions.

ferventcoder added a commit that referenced this issue May 5, 2016
* stable:
  (doc) update CHANGELOG/nuspec
  (GH-699) Fix: PATH may break on Install-ChocolateyPath
@ferventcoder
Copy link
Member Author

ferventcoder commented May 5, 2016

For others, as a way of understanding this:

So basically Windows has %SystemRoot%;%SystemRoot%\system32 and a few others in the PATH environment variable. When older versions of Chocolatey install, they will expand those paths out and then save the registry value as REG_SZ.

It does this when you install Chocolatey (as Chocolatey needs to add the bin directory to the PATH).
The behavior of expanding the path and saving it is a bug. That is what #303 was meant to address.
What #303 missed was that the PATH (the registry value) was being saved back as REG_SZ instead of keeping REG_EXPAND_SZ (the original type). That is this issue.

Here's a matrix that may help understand it better.

SystemState PATH Example Registry Type Result
Before Choco %SystemRoot%;%SystemRoot%\system32 REG_EXPAND_SZ No issues!
Installing Older Choco Versions C:\Windows;C:\Windows\System32;choco; REG_SZ Small issue*
Upgrading to affected betas C:\Windows;C:\Windows\System32;choco; REG_SZ Small issue*
Install (first time ever installing Chocolatey) Affected Betas %SystemRoot%;%SystemRoot%\system32;choco; REG_SZ Big issues!*
Upgrading to 0.9.10+/betas C:\Windows;C:\Windows\System32;choco; REG_SZ Small issue*
Install-ChocolateyPath after upgrade to 0.9.10+/betas C:\Windows;C:\Windows\System32;choco; REG_EXPAND_SZ No issues!
Install (first time ever installing Chocolatey) 0.9.10+/betas %SystemRoot%;%SystemRoot%\system32;choco; REG_EXPAND_SZ No issues!
  • Small issue* - when you attempt to add a variable value %something% to the PATH after it has been affected, expansion may not work. This is super miniscule. In the 5 years that Chocolatey has been doing this incorrectly, this issue has never come up.
  • Big Issues!* - None of the variables will expand, thus making everything that is in System32 or Windows directories no longer work.

NOTE: Install for the first time ever means you've never had Chocolatey installed before.

NOTE: choco in the path is shortened due to space. It's actually something like C:\ProgramData\chocolatey\bin.

Additionally if a package install calls Install-ChocolateyPATH and you have a fixed version, it will fix the PATH back to REG_EXPAND_SZ.

@ferventcoder
Copy link
Member Author

Here's the fixed version - https://chocolatey.org/packages/chocolatey/0.9.10-beta-20160505

It should be available within an hour - otherwise if you want to install it directly:

choco upgrade chocolatey --pre --version 0.9.10-beta-20160505

@ferventcoder
Copy link
Member Author

ferventcoder commented May 5, 2016

If you want to preemptively fix this issue, please take a look at the following (I tested it and it works on my machines, but no warranties).

Fix_PATH_Expand.ps1:

$Name = 'PATH'

Write-Output "Fixing Machine PATH to REG_EXPAND_SZ"
[Microsoft.Win32.RegistryKey] $win32RegistryKey = [Microsoft.Win32.Registry]::LocalMachine.OpenSubKey("SYSTEM\CurrentControlSet\Control\Session Manager\Environment")
$machineValue =  $win32RegistryKey.GetValue($Name, [string]::Empty, [Microsoft.Win32.RegistryValueOptions]::DoNotExpandEnvironmentNames)
Write-Output "Saving Machine PATH to '.\MachinePath.txt' in case something goes wrong and you need to manually fix."
$machineValue | Out-File MachinePath.txt

Write-Output "Machine PATH is '$machineValue'"
$machineValue = [System.Text.RegularExpressions.Regex]::Replace($machineValue, 'C:\\Windows($|(?=[\\/;]))', '%SystemRoot%', [System.Text.RegularExpressions.RegexOptions]::IgnoreCase)
[Microsoft.Win32.Registry]::SetValue("HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Control\Session Manager\Environment", $Name, $machineValue, [Microsoft.Win32.RegistryValueKind]::ExpandString)

Write-Output "Fixing Current User PATH to REG_EXPAND_SZ"
$win32RegistryKey = [Microsoft.Win32.Registry]::CurrentUser.OpenSubKey("Environment")
$userValue = $win32RegistryKey.GetValue($Name, [string]::Empty, [Microsoft.Win32.RegistryValueOptions]::DoNotExpandEnvironmentNames)
Write-Output "Saving Current User PATH to '.\UserPath.txt' in case something goes wrong and you need to manually fix."
$userValue | Out-File UserPath.txt

Write-Output "Current User PATH is '$userValue'"
$userValue = [System.Text.RegularExpressions.Regex]::Replace($userValue, 'C:\\Windows($|(?=[\\/;]))', '%SystemRoot%', [System.Text.RegularExpressions.RegexOptions]::IgnoreCase)
[Microsoft.Win32.Registry]::SetValue("HKEY_CURRENT_USER\Environment", $Name, $userValue, [Microsoft.Win32.RegistryValueKind]::ExpandString)

Updated with case insensitive fixes from @jnm2 . Thanks!

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

3 participants