Environment.SetEnvironmentVariable() messing up system variables and registry keys

[levicki]

Issue Title

Environment.SetEnvironmentVariable() messing up system variables and registry keys

Version info

.NET Framework Early Access build 3745 (it applies to all .Net Framework versions until now)
Windows Version 10.0.17763.379

General

If you use Environment.SetEnvironmentVariable() to change PATH environment variable it will lead to the expansion of its content (any %var% contained in PATH variable will get replaced with its content), and the registry key HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Control\Session Manager\Environment\Path type will be changed from REG_EXPAND_SZ to REG_SZ.

What is worse, if you are running a 32-bit application on a 64-bit OS, expansions for system variables such as %CommonProgramFiles%, %COMSPEC%, %PROCESSOR_ARCHITECTURE%, or %ProgramFiles%, will be incorrect due to WOW64 redirection so if you had for example %ProgramFiles%\7-Zip in your PATH that will be expanded to C:\Program Files (x86)\7-Zip instead of C:\Program Files\7-Zip.

Finally, given that ComSpec, PSModulePath, and windir registry keys are also by default REG_EXPAND_SZ, if this function is used to change any of them it will mess them up as well.

Actual behavior:

1. Registry key type is changed
2. Variable contents are expanded

Expected behavior

1. Registry key type is preserved
2. Variable contents are not expanded

Early Access ID

N/A

[tarekgh]

Thanks for reporting the issue. The .NET framework is just using the underlying Windows Win32 API SetEnvironmentVariable which has this behavior and that is the behavior since this API is introduced. If you want to change the variable contents without changing the registry key type is to directly edit the registry contents.

[levicki]

@tarekgh As far as I know (and I am using Win32 API since at least 1997), native SetEnvironmentVariable() function cannot change system or user environment variables in the registry, only the ones in the process (and its child processes) environment block.

Environment.SetEnvironmentVariable() method on the other hand has an overload which by means of specifying EnvironmentVariableTarget parameter, can also change system and user environment variables directly in the registry and it is doing it incorrectly hence the bug report.

If you could take a moment to check the Environment.SetEnvironmentVariable() implementation, you will hopefully notice that SetValue() wrapper method is passing RegistryValueKind.Unknown as a default parameter for every kind of value passed in to the underlying SetValue() method which does the real work of changing the registry keys for System and User targets,, and that the CalculateValueKind() method is responsible for choosing the correct registry key type.

The bug I reported is caused by CalculateValueKind() treating all strings (except arrays) as being the same, which is incorrect behavior.

A potential fix would be CalculateValueKind() method checking for presence of environment variables (i.e. %var%) when it detects a string value passed in, and returning the RegistryValueKind.ExpandString so that the variables do not get expanded, and that the registry key type gets set properly.

In my opinion it is already bad that Environment.SetEnvironmentVariable() method is not completely orthogonal with native Win32 API. The only thing worse than having different behavior (i.e. .Net API capable of setting system/user variables in the registry and Win32 API not capable of it) is to have a poor implementation of this added functionality so I hope you will reconsider fixing it.

[tarekgh]

@levicki thanks for clarification. You are right regarding the overload one is not using the Win32 APIs. Unfortunately, this is something we cannot change in the current time for the full framework because there will be some app comparability risk with such change. We can consider addressing this in .NET Core. You may move this issue to corefx repo or I can do that if you agree.

[levicki]

@tarekgh Sorry if my report wasn't clear. Could you clarify what kind of compatibility risk you expect here? Do you think that some applications may be relying on broken behavior (i.e. expansion of variables)? How would not expanding them break anything if the registry key is properly marked as REG_EXPAND_SZ? Do you have any telemetry to back that up?

Sure it's your code and you can decide not to fix it, or fix only .NET Core, but please be aware that people might already be using this method in their setup utilities and if those are 32-bit and run on 64-bit OS they will expand variables incorrectly due to WOW64 redirection thus breaking user systems' ability to run some applications which depend on valid PATH.

Finally, I cannot say for sure whether NVIDIA driver and their CUDA toolkit setup are using .NET API or native Win32 API, but they already exhibit such behavior of incorrect PATH expansion. As a matter of fact I reported the bug to NVIDIA first (their internal bug #2532268), but then I realized that this .NET method is most likely what is enabling it. In my opinion, it should be fixed everywhere, but feel free to move it to .NET Core if you are still unconvinced.

[tarekgh]

Could you clarify what kind of compatibility risk you expect here?

Someone can be dependent on the current be behavior. expecting the data will be expanded and possibly expecting REG_SZ registry type.

Do you think that some applications may be relying on broken behavior (i.e. expansion of variables)?

It is possible. We have seen some weird dependencies before on wrong behaviors.

How would not expanding them break anything if the registry key is properly marked as REG_EXPAND_SZ?

Someone would just read the registry after setting the environment variable and expect it is expanded. This is possible.

Do you have any telemetry to back that up?

This is cannot be used as argument for such case. Even there is no telemetry data not supporting that doesn't mean the case cannot happen.

I am trying to say this is the .NET behavior for many years and changing that now can be risky especially there is a work around to just access the registry directly.

I'll move the issue to .NET Core and thanks for your report.

[levicki]

Someone can be dependent on the current be behavior. expecting the data will be expanded and possibly expecting REG_SZ registry type.

I don't mean to come off as argumentative, but how can that argument work when the OS default is REG_EXPAND_SZ for comspec, Path, PSModulePath and windir environment variable registry keys? It means their program will already fail to work on fresh OS install if it relies on expanded values or the registry key type, not to mention that setting any environment variable through system GUI will reset the corresponding registry key to REG_EXPAND_SZ.

Someone would just read the registry after setting the environment variable and expect it is expanded.

While it is possible it is a strawman. Why would they use SetEnvironmentVariable() in the first place and then bother to read the registry manually instead of using GetEnvironmentVariable() followed by ExpandEnvironmentVariables()?

This is cannot be used as argument for such case. Even there is no telemetry data not supporting that doesn't mean the case cannot happen.

It is irrelevant whether it will happen or not. If developer wants expanded variables they should use ExpandEnvironmentVariables(). It is in WinAPI documentation and it is what has been used for decades now. If anything you are breaking a good, documented WinAPI usage pattern beause of a few potential bad usages (which you cannot prove they even exist) of a buggy .NET API.

I am trying to say this is the .NET behavior for many years and changing that now can be risky especially there is a work around to just access the registry directly.

And what I hear you saying is "well yes, SetEnvironmentVariable() is broken, but you can write your own implementation".

I wonder what is the point of having a framework in the first place if everyone who wants to use it has to possess higher level of system and WinAPI knowledge than Microsoft's own software engineers who wrote and/or are maintaining the framework, and to spend time and resources to implement correct behavior for all the broken functions they find in it?

[tarekgh]

I don't mean to come off as argumentative, but how can that argument work when the OS default is REG_EXPAND_SZ for comspec, Path, PSModulePath and windir environment variable registry keys?

I meant the sequence of the operation and not depending on the default system. Someone after calling set environment expect it is expanded.
As I mentioned we have seen pattern like that before and that is why we should be careful assuming the app is always doing the right things. changing the behavior for such apps can have a serious consequences.

If developer wants expanded variables they should use ExpandEnvironmentVariables()

Again, you are assuming the apps always doing the right things which not always the case.

And what I hear you saying is "well yes, SetEnvironmentVariable() is broken, but you can write your own implementation".

No, what I am saying, if the current behavior of the API is not working for you, then there is a work around.

I wonder what is the point of having a framework in the first place if everyone who wants to use it has to possess higher level of system and WinAPI knowledge than Microsoft's own software engineers who wrote and/or are maintaining the framework, and to spend time and resources to implement correct behavior for all the broken functions they find in it?

This API exist for almost 20 years now and I didn't hear this complain before. Which means the majority of apps really didn't run into any problem or even cared about this issue. Also, I have opened .NET Core issue to track fixing this for the future releases.