Runtime: System.Environment.GetEnvironmentVariables() case sensitive but ProcessStartInfo.Environment/EnvironmentVariables insensitive

[devlead]

So I'm one of the maintainers of the Cake build system, a couple of days ago we got reports on the gitters that we had an issue (cake-build/cake#1319) on Windows where cake wouldn't start on a specific machine. I tracked down to user getting duplicate "home" environment variables just the casing was different from System.Environment.GetEnvironmentVariables().

I just did quickfix to ignore if duplicates had same value and warn if value differed, now Cake started fine.
Today when the original issuer tested the fix Cake indeed started fine but no tools would start.
We set a couple of Cake specific environment variables and in that process we got this error:

Error: System.ArgumentException: Item has already been added. Key in dictionary: 'home' Key being added: 'HOME'
at System.Collections.Hashtable.Insert(Object key, Object nvalue, Boolean add)
at System.Diagnostics.ProcessStartInfo.get_EnvironmentVariables()
at Cake.Core.Polyfill.ProcessHelper.SetEnvironmentVariable(ProcessStartInfo info, String key, String value)
at Cake.Core.IO.ProcessRunner.Start(FilePath filePath, ProcessSettings settings)
at Cake.Core.Tooling.Tool1.RunProcess(TSettings settings, ProcessArgumentBuilder arguments, ProcessSettings processSettings) at Cake.Core.Tooling.Tool1.Run(TSettings settings, ProcessArgumentBuilder arguments, ProcessSettings processSettings, Action1 postAction) at Cake.Core.Tooling.Tool1.Run(TSettings settings, ProcessArgumentBuilder arguments)
at Cake.Common.Tools.NuGet.Restore.NuGetRestorer.Restore(FilePath targetFilePath, NuGetRestoreSettings settings)
at Cake.Common.Tools.NuGet.NuGetAliases.NuGetRestore(ICakeContext context, FilePath targetFilePath, NuGetRestoreSettings settings)
at Cake.Common.Tools.NuGet.NuGetAliases.NuGetRestore(ICakeContext context, FilePath targetFilePath)

With help of strack trace I tracked down the issue to
ProcessStartInfo.EnvironmentVariables and ProcessStartInfo.Environment

Our code to set the Cake specific variables basically looked like below

#if NETCORE
            info.Environment[key] = value;
#else
            info.EnvironmentVariables[key] = value;
#endif

So in a naive initial attempt to work around I tried to lookup key and set it with that key so it was the same casing, that worked fine on posix, but not on Windows.
Strangely ProcessStartInfo didn't blow up if I didn't set any envvars, so I started thru reference source and corefx and found similar code we had in Cake before I added value comparison and warnings.

Reference source looked like this:

#if PLATFORM_UNIX
environmentVariables = new CaseSensitiveStringDictionary();
#else
environmentVariables = new StringDictionaryWithComparer ();
#endif // PLATFORM_UNIX

foreach (DictionaryEntry entry in System.Environment.GetEnvironmentVariables())
    environmentVariables.Add((string)entry.Key, (string)entry.Value);

And corefx looked like this:

if (_environmentVariables == null)
{
    IDictionary envVars = System.Environment.GetEnvironmentVariables();
    _environmentVariables = new Dictionary<string, string>(envVars.Count,
                        CaseSensitiveEnvironmentVariables ? StringComparer.Ordinal : StringComparer.OrdinalIgnoreCase);


    IDictionaryEnumerator e = envVars.GetEnumerator();
    try
    {
        while (e.MoveNext())
        {
            DictionaryEntry entry = e.Entry;
            _environmentVariables.Add((string)entry.Key, (string)entry.Value);
        }
    }
    finally
    {
        (e as IDisposable)?.Dispose();
    }
}

That explained why I only encountered the error when using the property as it was lazy loaded.

The original issue it was a GIT commit hook the resulted in the "double" home/HOME variables, but when digging deeper I found some non .NET based build agents happily all possible variants of "home".
So I dug deeper to see if I could repro in .NET and initial answer was no (due too StringDictionary lower casing and OrdinalIgnoreCase comparer on the dictionary).

Thru reflection though I was able to initialize the ProcessStartInfo environmentVariables field before property was used with a case sensitive implementation similar to the posix one.

Then I could add variables like this

startInfo.EnvironmentVariables["a"] = "B";
startInfo.EnvironmentVariables["A"] = "b";

The child process could easily iterate environment variables like this

foreach (DictionaryEntry entry in Environment.GetEnvironmentVariables())
{
    Console.WriteLine("Key: {0}, Value: {1}", entry.Key, entry.Value);
}

and it outputted:

Key: A, Value: b
Key: a, Value: B

but if I tried to start a child process with specified custom envars like this

var startInfo = new ProcessStartInfo(assemblyLocation, "3") {UseShellExecute = false};
startInfo.EnvironmentVariables["C"] = "d";
new Process
{
    StartInfo = startInfo
}.Start();
}

it would blow up (obviously because duplicate key)

System.ArgumentException: Item has already been added. Key in dictionary: 'A'  Key being added: 'a'...

Not setting custom environment variable worked fine though:

new Process
{
    StartInfo = new ProcessStartInfo(assemblyLocation, "4") {UseShellExecute = false}
}.Start();

So what am I missing? Is there some reason behind this behavior? It feels a bit fragile that adding one envvar to parent process can mess up the runtime for all child processes, not good at all.

I've made avail the quick n dirty repro here:
https://github.com/devlead/EnvironmentProcessStartRepro/tree/master/EnvironmentProcessStartRepro

[AlexGhiondea]

/cc @safern who is working on this area -- does the change we talked about solve this issue?

[safern]

Yes it should solve the issue, I'm writing some tests for it now so that I make sure it does solve it. But my fix should do it

[AlexGhiondea]

Just to be clear - the fix will work for the re-introduced ProcessStartInfo.EnvironmentVariables property.

We are not going to change the implementation of ProcessStartInfo.Environment.

[safern]

Yes that is correct.

[devlead]

Is this fix targeted for both .NET core and regular .NET framework? Interestingly see that Mono had pulled in reference source so they likely have same issue on Windows.

[AlexGhiondea]

@devlead the fix is currently just for .NET Core. I suspect the Full .NET Framework does not have this issue.

[devlead]

@AlexGhiondea actually my repro was on full .net, so it's very much an issue on there too.

[safern]

I think it makes sense that in Windows the dictionary is not case sensitive, actually that is what I'm doing for ProcessStartInfo.EnviromnentVariables because the OS is not case sensitive, so home should be the same as HOME in Windows. So my fix is to bring this property back to .NET Core and the behavior will be the same, it will only be case sensitive for UNIX platforms.

I think to avoid this kind of things, wouldn't you have to use EnvironmentVariables.ContainsKey ?

[devlead]

Issue it's that process envars clearly are case sensitive so you can get duplicates from GetEnvironmentVariables() which ProcessStartInfo lazy initializes within property getter only if it's changed/used in any way.
So even a EnvironmentVariables.ContainsKey would blow up.

[AlexGhiondea]

While the process envvars are case sensitive, I don't think you can get (on Windows) entries from the GetEnvironmentVariable() call that only differ on case. That means you cannot get (from Windows) both PATH and path in the same call.

I agree with you assessment that we are taking whatever GetEnvironmentVariables() is returning and populating the ProcessStartInfo's member lazily -- we even call .ToLower() on the key name before putting it in the StringDictionary. We are doing this on Windows only. On Unix we are actually using a case sensitive approach.

Would it be possible in your code to normalize the environment variables before using them to start a process?

[devlead]

@AlexGhiondea well of course Cake can iterate over all environment variables and null out any duplicates, and that's actually what the user that reported the issue ended up doing after me suggesting so.

Personally though I still find this to be an issue as most users reading the documentation won't assume this can happen, and you normally via PowerShell/.NET/cmd.exe/Win32 it'll mostly preserve( or modify) case but do a lookup and use existing key if that exists. That said process environment variables are "just a string" that GetEnvironmentVariables() just unpacks without normalizing so an external process can create duplicates, GetEnvironmentVariables() will gladly parse them and just then looking at the ProcessStartInfo.EnviromnentVariables property will cause an duplicate key exception, because it trusts the source string to always be well formed.

In wild we've now seen build server, git commit hooks and certain versions of Python cause this (so mainly xplat tools) and I could in 5min create an repro in C#.

GetEnvironmentVariables() just uses an HashTable (with default comparer so case sensitive) internally for .NET parses each key value pair and just does table[key]=value;

At quick glance .NET Core GetEnvironmentVariables() seems to call GetEnvironmentVariablesCore() which uses a LowLevelDictionary<string, string> to store environment variables with DefaultComparer which is very similar to EqualityComparer<T>.Default So that's case sensitive too.

This is an edge case yes, but it can occur in any place in the .NET framework where someone populates dictionaries from GetEnvironmentVariables assuming no duplicates are guaranteed, as all current documentation states Windows environment variables are case insensitive, that's an valid assumption.

So potentially I could break any piece that uses ProcessStartInfo.EnviromnentVariables on full framework or StartInfo.Environment on .NET core.

And that it's lazy loaded and Process looks at the internal field environmentVariables on ProcessStartInfo, stars has to align for this to happen so it can be an really hard issue to track down once in production.

Many people base their code behavior on environment variables so issues like this could have real code control flow effects.

[AlexGhiondea]

@devlead do you know how you got duplicated environment variables that differ on case?
I don't expect the GetEnvironmentVariables call to return values that only differ on case.

If GetEnvironmentVariables on Windows starts returning keys that only differ from case, then I agree that we need to figure out a way to fix it.

And on Unix I belive this is already a non-issue, right? Since we would have to be case-sensitive throughout.

[devlead]

Yes on unix this is a not an issue.

The Win32 API for create process is something like this

BOOL WINAPI CreateProcess(
  _In_opt_    LPCTSTR               lpApplicationName,
  _Inout_opt_ LPTSTR                lpCommandLine,
  _In_opt_    LPSECURITY_ATTRIBUTES lpProcessAttributes,
  _In_opt_    LPSECURITY_ATTRIBUTES lpThreadAttributes,
  _In_        BOOL                  bInheritHandles,
  _In_        DWORD                 dwCreationFlags,
  _In_opt_    LPVOID                lpEnvironment,
  _In_opt_    LPCTSTR               lpCurrentDirectory,
  _In_        LPSTARTUPINFO         lpStartupInfo,
  _Out_       LPPROCESS_INFORMATION lpProcessInformation
);

lpEnvironment is just a pointer to block of null terminated strings key and value separated by equals sign.
Home=value\0HOME=value\0\0 is perfectly "valid".
So i.e. posix software just ported without taking this in consideration could potentially internally work in a case sensitive way, not on user or machine level but at a process level. I.e. Win32 Get/Set environment variables handles this for them, so they could i.e. get home value with one casing and if they then concat that case sensitive keys they could end up with duplicates.

So the 5 min repro I did just changed the internal reference if ProcessStartInfo.environmentVariables to point to a case sensitive collection thru reflection.

Then I can do things like this

                    startInfo.EnvironmentVariables["home"] = "::1";
                    startInfo.EnvironmentVariables["Home"] = "::1";
                    startInfo.EnvironmentVariables["hOme"] = "::1";
                    startInfo.EnvironmentVariables["hoMe"] = "::1";
                    startInfo.EnvironmentVariables["homE"] = "::1";
                    startInfo.EnvironmentVariables["hoME"] = "::1";
                    startInfo.EnvironmentVariables["hOME"] = "::1";
                    startInfo.EnvironmentVariables["HOME"] = "127.0.0.1";
                    startInfo.EnvironmentVariables["HomE"] = "127.0.0.1";
                    startInfo.EnvironmentVariables["HOmE"] = "127.0.0.1";
                    startInfo.EnvironmentVariables["HOMe"] = "127.0.0.1";

It'll start child process, child process can iterate using

 foreach (DictionaryEntry entry in Environment.GetEnvironmentVariables())
{
    Console.WriteLine("Key: {0}, Value: {1}", entry.Key, entry.Value);
}

It will output (sorted for legacy reasons)

Key: hOme, Value: ::1
Key: HomE, Value: 127.0.0.1
Key: homE, Value: ::1
Key: hoME, Value: ::1
Key: HOmE, Value: 127.0.0.1
Key: hoMe, Value: ::1
Key: Home, Value: ::1
Key: HOME, Value: 127.0.0.1
Key: home, Value: ::1
Key: hOME, Value: ::1
Key: HOMe, Value: 127.0.0.1

But if you try to start a child process just looking at ProcessStartInfo.StartInfo will cause this exception

System.ArgumentException: Item has already been added. Key in dictionary: 'hOme'  Key being added: 'HomE'

[AlexGhiondea]

I see.

You are saying that if you control creating the process you can pass in whatever casing for the variables you want -- and even create duplicates (different only by casing). In that case, yes -- you are going to get into this weird state where you will get an exception if you try to use the ProcessStartInfo property.

But since you control the way the process is started, you can even pass the same variable (with the same name and casing) twice.
What should we do in that case? Concatenate the values? Last one wins? First one wins? Throw an exception?

While not ideal, would it be possible to use Environment.GetEnvironmentVariables() in your code, instead of inspecting the ProcessStartInfo.EnvironmentVariables property? I suspect even that will not give you full control in the case where you have duplicated name/casing for a environment variable.

Changing the way the property works on Full .NET Framework to account for this is very risky IMO from a compat perspective.

[devlead]

Issue is for child processes, they don't have any control how their started. The reason we issue was raised with Cake was that a user used it in git repo hooks.

Duplicates keys with same casing would've been overwritten as GetEnvironmentVariables for each value does table[key]=value, the "easy fix" would've been to on Windows create it's HashTable / LowLevelDictionary<string, string> with an case insensitive comparer, then the behavior at least would be the the same on Windows and Unix where the last key wins.

[Priya91]

@devlead With PSI.EnvironmentVariables, duplicate keys cannot be added to the StringDictionary, whereas with PSI.Environment, duplicate keys can be added with last one wins rule. This is the behavior on Desktop and will be on .NET core when this PR merges to master dotnet/corefx#18467. If you prefer being able to add duplicate keys with last one wins rule, then using PSI.Environment will be the way to go. And as @AlexGhiondea recommends the behavior is to match desktop, and changing desktop behavior will be risky, breaking apps which depend on this behavior.