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

[tarekgh]

Note, this issue is originally reported by @levicki https://github.com/Microsoft/dotnet-framework-early-access/issues/58 and we copying the issue here to address it in .Net Core.

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

[bartonjs]

I definitely don't like the notion of sniffing the value to determine it's hoping for REG_EXPAND_SZ; just because there's a % doesn't mean it's looking for environment expansion (and just because ExpandEnvironmentVariables on it produces a different answer doesn't mean it wanted the variable expanded; maybe it's a coincidence, maybe it's there as part of some sort of later composition).

One reasonable metric would be that if it's updating a value that's already REG_EXPAND_SZ that it keeps it REG_EXPAND_SZ; but I don't understand how you were getting the unexpanded form of path to append to... GetEnvironmentVariable will have already expanded it, so appending to the path has appended to the post-expansion value, meaning it doesn't matter if it says REG_SZ or REG_EXPAND_SZ (unless one environment variable expanded into another in the process; in which case REG_EXPAND_SZ is arguably wrong).

So this sort of feels like it would require something like GetUnexpandedVariable and SetExpandableVariable; but those effectively already require enough specialized knowledge that it seems like writing to Registry directly is just easier (and makes it clear that this is a Windows-only behavior)

I'm sympathetic to the problem, I agree it's weird that SetEnvironmentVariable(GetEnvironmentVariable()) changes meaning (particularly across process bitness); but without changing Get to not expand (which seems pretty breaking as an upgrade behavior) I don't see how anything sensible can be done to Set.

[levicki]

@bartonjs Hello.

In my opinion, those functions should mimic the behavior which the user is getting through the GUI.

For example, if you create a new environment variable which has % in the value field:

You get a key for that variable which is REG_EXPAND_SZ:

If API is not doing the same thing as the GUI, then from the user's perspective the program using said API is broken. In other words, you have to sniff the value because environment variables allow nesting of other environment variables.

Furthermore, if GUI is allowing or not allowing certain characters or lengths to be set, neither should API so some validation of the value provided should certainly happen.

I think that it is not important to validate whether the value provided can actually be expanded correctly because that involves WOW64 issues I mentioned, not to mention it is user's fault if they provide wrong input -- you just need to set the proper registry key type when you see a % in the value to allow for expansion to happen if proper expandable value is provided.

In any case, I hope you will at least agree that API and GUI should yield consistent results.

Finally, I am appaled that this won't be fixed in .NET Framework as well.

[bartonjs]

@levicki If it was a brand new method, I'd probably agree. But Environment.GetEnvironmentVariable has been doing expansion of a REG_EXPAND_SZ for a long time, changing it to not do so would be pretty disruptive.

[levicki]

@bartonjs I am talking about fixing SetEnvironmentVariable() overload which modifies system and user variables in registry. I never mentioned changing GetEnvironmentVariable() to not expand variables.

Please see my clarification here where I initially reported the issue. Also, please see my rationale as for why this should be fixed.

As for Environment.GetEnvironmentVariable() (which relies on InternalGetValue()), you could easily add a non-default GetEnvironmentVariable() overload which will not expand strings and which can be used in combination with Environment.ExpandEnvironmentVariables() method.

[bartonjs]

I never mentioned changing GetEnvironmentVariable() to not expand variables.

Right, so the scenario of adding to the path doesn't work, then; because this code is pre-expanding:

```C#
Environment.SetEnvironmentVariable(
"PATH",
Environment.GetEnvironmentVariable("PATH") + Path.PathSeparator + extraDir,
EnvironmentVariableTarget.User);

Unless I'm missing a scenario where you got the unexpanded version of the PATH variable..

> I am talking about fixing SetEnvironmentVariable() overload which modifies system and user variables in registry.

I think it'd be pretty weird that if I did

```C#
Environment.SetEnvironmentVariable("test", "%hello%");
string s = Environment.GetEnvironmentVariable("test");

that s is %hello%, but

C# Environment.SetEnvironmentVariable("test", "%hello%", EnvironmentVariableTarget.User); string s = Environment.GetEnvironmentVariable("test", EnvironmentVariableTarget.User);

results in s being the empty string.

[levicki]

So, just because another API is also broken you cannot fix this one either?

Or you could, you know, do something like this:

Environment.cs:653 -public static string GetEnvironmentVariable( string variable, EnvironmentVariableTarget target)
Environment.cs:653 +public static String GetEnvironmentVariable( string variable, EnvironmentVariableTarget target, bool doNotExpand = false)
...
Environment.cs:678 -string value = environmentKey.GetValue(variable) as string;
Environment.cs:678 +string value = environmentKey.GetValue(variable, doNotExpand) as string;
...
Environment.cs:691 -string value = environmentKey.GetValue(variable) as string;
Environment.cs:691 +string value = environmentKey.GetValue(variable, doNotExpand) as string;
...
Environment.cs:1152 -public Object GetValue(String name) {
Environment.cs:1152 +public Object GetValue(String name, bool doNotExpand) {
...
Environment.cs:1154 -return InternalGetValue(name, defaultValue, false, true);
Environment.cs:1154 +return InternalGetValue(name, defaultValue, doNotExpand, true);

And add a compiler deprecation warning when compiling code that calls GetEnvironmentVariable( string variable, EnvironmentVariableTarget target) without 3rd parameter explicitly set.

As for your example with process and user variable behavior, it just demonstrates that even GetEnvironmentVariable(string variable, EnvironmentVariableTarget target) method doesn't honor it's own contract. You just claimed that it always expands variables so ask yourself why it didn't expand %hello% in your example then, but is instead returning verbatim string?

Assuming that GetEnvironmentVariable() should always expand variables then yes, both GetEnvironmentVariable("test") and GetEnvironmentVariable("test", EnvironmentVariableTarget.User) should return string.Empty if environment variable %hello% is not defined.

On the other hand, if you want orthogonality with underlying WinAPI (and I am of the opinion that this should be the preferred solution and that providing expansion in the same method was terribly wrong), then you should always return non-expanded variables and require users to explicitly call ExpandEnvironmentVariables() method on returned value if they want them expanded.

My preferred solution:

1. SetEnvironmentVariable() shall set registry key type to REG_EXPAND_SZ when it detects % char anywhere in variable value just like GUI environment editor and SETX shell command do.
2. GetEnvironmentVariable("test") shall return %value% unexpanded.
3. GetEnvironmentVariable("test", EnvironmentVariableTarget.User or EnvironmentVariableTarget.Machine) shall return %value% unexpanded.
4. [Optional] ExpandEnvironmentVariables() should take into account WOW64 when doing expansion in 32-bit process on a 64-bit machine to avoid unintended consequences of incorrect expansion.

In any case, even if you don't agree with my proposal I hope that you guys will at least discuss this internally and try to find a better solution and not just brush it off as unimportant because as it is, those Environment APIs even when you disregard all the bugs are teaching new developers bad coding practices.

[bartonjs]

You just claimed that it always expands variables so ask yourself why it didn't expand %hello% in your example then, but is instead returning verbatim string?

There were two cases. The first (no target) set the environment variable in the local process. Local process variables are not expanded since they're not REG_EXPAND_SZ (since they're not in the registry at all); so with or without the proposed change it returns "%hello%", as it's just a string.

The second described what things would behave as if your change were implemented on making the value be REG_EXPAND_SZ when the value contained a %. The registry would save the value as "%hello%" / REG_EXPAND_SZ, then GetEnvironmentVariable would expand it to the empty string (since there's no variable by that name). The _current_ behavior for this example is that "%hello%" would come back, which matches the behavior for local-process-set.

---

Your proposal involves subtle breaking changes (in somewhat common cases, for GetEnvironmentVariable).

The suggestion of a new overload of GetEnvironmentVariable which suppresses expansion is reasonable, though I don't know that I agree with a compile warning for using the existing version. I'd put the proposed value semantics change to SetEnvironmentVariable in another overload as well... but I'm not sure what the overload/alternate would look like, since "expandable environment variables" only makes sense on Windows. (Though, user-and-machine persisted set also only makes sense on Windows, so that's possibly a moot point.)

[levicki]

The second described what things would behave as if your change were implemented on making the value be REG_EXPAND_SZ when the value contained a %. The registry would save the value as "%hello%" / REG_EXPAND_SZ, then GetEnvironmentVariable would expand it to the empty string (since there's no variable by that name).

Sorry, but that is just plain incorrect and you can see it for yourself if you do the following:

1. Execute setx test %hello% from command line.
2. Check in registry HKCU\Environment that key test has been created with REG_EXPAND_SZ.
3. Run the following code in debugger and step through the Environment.cs code:

using System;

namespace ConsoleApp1
{
    class Program
    {
        static void Main(string[] args)
        {
            string s = Environment.GetEnvironmentVariable("test", EnvironmentVariableTarget.User);
        }
    }
}

You will see that current GetEnvironmentVariable() implementation will attempt to expand test since that is what it always does because doNotExpand parameter for InternalGetValue() is hard-coded and always false.

If %hello% name does not exist (and it does not unless you defined hello variable as well), native ExpandEnvironmentStrings() API will return the value unexpanded.

However, if you define hello by executing say setx hello goodbye, .NET GetEnvironmentVariable("test") will return goodbye, and WinAPI GetEnvironmentVariable("test") will still return %hello%. That's hardly orthogonal API design there.

You must not focus only on what happens when you use Set/Get... methods from .NET -- data can come from the user (GUI, setx, even regedit) and all cases should work correctly and, in my opinion, give the same results as native API.

[vexx32]

Can we get some progress on this?

It's perfectly possible to completely brick your PATH values without any indication that anything is going wrong until something fails to find an executable or library it needs.

If you're adding / removing entries to the PATH, the only way that currently works is to use the Microsoft.Win32.Registry APIs. This API remaining in production is a serious hazard. If you try to pull the unexpanded PATH values via the registry APIs so you can do proper modification without ruining the original values, you cannot set values to the PATH variables with this API without breaking a _significant_ portion of the OS.

I would suggest a threefold solution:

1. Add an overload to GetEnvironmentVariable() that allows the caller to pass a flag to indicate whether they _want_ variables to be expanded or to be left unaltered.
2. Add an overload to SetEnvironmentVariable() that allows the caller to pass a flag indicating if the value contains variables and thus should be set as ExpandString in the Registry APIs instead of just String.
3. When calling SetEnvironmentVariable() without the above explicit flag, the API should first query the currently-set type of the value in the registry, and _never change the stored type_ of the value. ExpandString values should remain ExpandString, and String values should remain String. If someone wants to explicitly change that, they can use the new overload from point 2.

[levicki]

Because of this API which seems to be used by NVIDIA, both NVIDIA driver installer and CUDA installer (which share a common setup framework) are bricking the PATH variable by expanding everything in there.

What is worse, they do so from 32-bit process on a 64-bit machine so any instances of say %ProgramFiles% get expanded to Program Files (x86) thus completely breaking PATH for 64-bit programs.

I have filed a bug report with NVIDIA in April 2019 and it is still not fixed. I am mentioning it here because it demonstrates first hand how one badly designed API can have wide reaching impact even in big developer houses where you would think people would know better.

The fact that you are not willing to say "Mea culpa" and fix the source of the issue in the whole .Net Framework not just in the .Net Core once and for all even if it means breaking stuff (which was broken to begin with) says a lot about your entrenched corporate "compatibility at all costs" mindset, even if its your users who end up paying those costs.

TL;DR -- current implementation of SetEnvironmentVariable() in .Net Framework is irreparably broken. It should be converted to NOP so that badly written programs using this broken API cannot brick the users' PATH anymore and that developers using it have to write proper code.

[vexx32]

Can we please get this triaged and a fix sorted out?

It's kind of impossible to appropriately use this api with any kind of safety at the minute.

[joperezr]

This seems like something we should probably fix for 5.0 but @tarekgh please feel free to move to Future if you think that is the case.

[levicki]

I am the person who initially reported this almost two years ago. Please fix it for 5.0 if possible, it is deeply disturbing to see this kind of issue being unsolved and postponed for this long.

It may seem innocuous, but it can actually have adverse effects on system environment variables.

[joperezr]

We have opted to change the milestone to future for now (which doesn't mean it can't be done in 5.0) because we need to further discuss whether or not we should take in this breaking change and how to deal with it so that it is not as impactful, that means we might opt to either introduce a new API for this or a configuration switch for back-compat.

[levicki]

I would appreciate if new default behavior would be correct, and if you opted for a configuration switch for back-compat. That way developers would at least be aware that they have something to fix if they want to keep using the same API.

I would say that introducing new API (in some future .Net Framework release, let's call it N) makes sense only if it will be orthogonal with existing Win32 API (possibly improving on it in regards to Wow64 handling). Also, it would make sense to deprecate the old API in said release N, and convert it to no-operation in release N+1.

Either way, I hope that carefully re-reading my bug report as well as all subsequent clarifications and rationalizations can help cut down on the need for prolonged discussion and thus eliminate further delays in fixing this.

If there is anything more I can do to help please don't hesitate to let me know.

[vexx32]

@joperezr I suggested above (https://github.com/dotnet/runtime/issues/1442#issuecomment-567582433) an additive way to implement a thorough resolution for this without breaking any existing code.

Can we get _that_ done for 5.0 so it's at least _possible_ to utilize these APIs in a safe way if we want to?

[joperezr]

As I pointed above setting the milestone to Future doesn't mean we can't get it for 5.0, we will still take PR's for issues that are marked as future, milestone is just so that we can do planning management.