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

Environment.GetEnvironmentVariables() should tolerate duplicate case-insensitive variable names #42029

Closed
asklar opened this issue Sep 9, 2020 · 16 comments

Comments

@asklar
Copy link

asklar commented Sep 9, 2020

Description

Some tools like Yarn will spawn nodeJS via CreateProcess and pass an environment block that contains duplicated environment variables, e.g:
image

Windows doesn't do any checking of environment variables being duplicated in CreateProcess so the node.exe process gets created with the two names. Then in our case, MSBuild (a .net app) gets launched from within node.exe, which later launches CL.exe and other build tools, which break because they don't expect to get the same variable twice in the env block.

The environment block as exposed in Process should be cleaned up to remove duplicates (i.e. variables that appear more than once)

Configuration

Windows 10, all .net versions, all archs.

Regression?

Other information

Related bugs:
actions/runner-images#1566
nodejs/node#35129
dotnet/msbuild#5726

@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Sep 9, 2020
@AaronRobinsonMSFT
Copy link
Member

@asklar This doesn't seem like a problem the runtime can solve properly. Which version of the environment variable is correct? What part of the runtime would we expect to handle this reconciliation process? I am really unclear how we could ever fix this issue in a way that is correct according to all parties.

@AaronRobinsonMSFT AaronRobinsonMSFT added area-Meta question Answer questions and provide assistance, not an issue with source code or documentation. needs more info labels Sep 9, 2020
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the Future milestone Sep 9, 2020
@AaronRobinsonMSFT AaronRobinsonMSFT removed the untriaged New issue has not been triaged by the area owner label Sep 9, 2020
@rainersigwald
Copy link
Member

There's a horrible solution: call win32 GetEnvironmentVariable to get "the" value for the duplicated variable, and use that for assignment--and don't throw when encountering the second one in the GetEnvironmentStrings block. I'm not sure that's worth doing but it'd be consistent with the behavior you get if you never call the get-all-environment-variables codepath.

@asklar
Copy link
Author

asklar commented Sep 9, 2020

Hi @AaronRobinsonMSFT
@rainersigwald is correct. GetEnvironmentVariable will stop at the first opportunity that it finds the variable name match in the env block. The other definition of the variable is forever inaccessible anyway, so unless apps are parsing the environment block manually themselves, they will never see the second definition.

@AaronRobinsonMSFT
Copy link
Member

@asklar Okay so let's accept this possible workaround - which I am not convinced will work for all scenarios. Where would this occur? Is the desire for the runtime on start up to sanitize it's own environment variables?

@asklar
Copy link
Author

asklar commented Sep 9, 2020

yes, I think that would be reasonable, or if you want to be more pay-for-play, upon the first call to the Environment.GetEnvironmentVariable / GetEnvironmentVariables / ExpandEnvironmentVariables methods

@asklar
Copy link
Author

asklar commented Sep 15, 2020

@rainersigwald @AaronRobinsonMSFT could you guys please re-triage this? right now it's marked as needs more info and question which neither seems correct.

@AaronRobinsonMSFT AaronRobinsonMSFT added area-System.Diagnostics.Process and removed needs more info question Answer questions and provide assistance, not an issue with source code or documentation. area-Meta labels Sep 15, 2020
@ghost
Copy link

ghost commented Sep 15, 2020

Tagging subscribers to this area: @eiriktsarpalis, @jeffhandley
See info in area-owners.md if you want to be subscribed.

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Sep 16, 2020

Stacktrace as reported in dotnet/msbuild#5726 (comment)

System.ArgumentException:
Item has already been added. Key in dictionary: 'NPM_CONFIG_CACHE'  Key
being added: 'npm_config_cache' 
   at System.Collections.Hashtable.Insert(Object key, Object nvalue, Boolean add)
   at System.Diagnostics.ProcessStartInfo.get_EnvironmentVariables()
   at Microsoft.Build.Utilities.ToolTask.GetProcessStartInfo(String pathToTool,
String commandLineCommands, String responseFileSwitch)
   at Microsoft.Build.Utilities.ToolTask.ExecuteTool(String pathToTool, String
responseFileCommands, String commandLineCommands)
   at Microsoft.Build.CPPTasks.TrackedVCToolTask.TrackerExecuteTool(String
pathToTool, String responseFileCommands, String commandLineCommands)
   at Microsoft.Build.CPPTasks.TrackedVCToolTask.ExecuteTool(String pathToTool,
String responseFileCommands, String commandLineCommands)
   at Microsoft.Build.Utilities.ToolTask.Execute() 

If I get this correctly the issue stems from the System.Environment.GetEnvironmentVariables() implementation not tolerating duplicate case-insensitive variable names on windows.

@ghost
Copy link

ghost commented Sep 16, 2020

Tagging subscribers to this area: @eiriktsarpalis, @jeffhandley
See info in area-owners.md if you want to be subscribed.

@eiriktsarpalis eiriktsarpalis modified the milestones: Future, 6.0.0 Sep 16, 2020
@eiriktsarpalis eiriktsarpalis changed the title Process environment vars should be sanitized Environment.GetEnvironmentVariables() should tolerate duplicate case-insensitive variable names Sep 16, 2020
@adamsitnik adamsitnik self-assigned this Dec 14, 2020
adamsitnik added a commit to adamsitnik/runtime that referenced this issue Dec 14, 2020
…e same

Example:

"NPM_CONFIG_CACHE=^"
"npm_config_cache=^"

fixes dotnet#42029
@adamsitnik
Copy link
Member

I've written a repro app and was able to reproduce it only for Full Framework, not Core. Since it's not a security issue, I am just going to close it as won't fix.

using System;
using System.Collections;
using System.Collections.Generic;
using System.Diagnostics;
using System.Reflection;

namespace DuplicateEnvVars
{
    class Program
    {
        static int Main(string[] args) => args.Length == 0 ? StartNewProcess() : PrintEnvVars();

        private static int StartNewProcess()
        {
            var startInfo = new ProcessStartInfo()
            {
                FileName = Process.GetCurrentProcess().MainModule.FileName,
                Arguments = "--printEnvVars"
            };

#if NETFRAMEWORK
            var caseSensitiveEnvVars = new Hashtable()
            {
                { "NPM_CONFIG_CACHE", "^" },
                { "npm_config_cache", "^" },
            };
            startInfo.UseShellExecute = false;
            startInfo.EnvironmentVariables
                .GetType()
                .GetField("contents", BindingFlags.NonPublic | BindingFlags.Instance)
                .SetValue(startInfo.EnvironmentVariables, caseSensitiveEnvVars);
#else
            var caseSensitiveEnvVars = new Dictionary<string, string>(StringComparer.Ordinal)
            {
                { "NPM_CONFIG_CACHE", "^" },
                { "npm_config_cache", "^" },
            };

            startInfo.Environment
                .GetType()
                .GetField("_contents", BindingFlags.NonPublic | BindingFlags.Instance)
                .SetValue(startInfo.Environment, caseSensitiveEnvVars);
#endif

            using (var process = Process.Start(startInfo))
            {
                process.WaitForExit();
            }

            return 0;
        }

        private static int PrintEnvVars()
        {
            Console.WriteLine("Output of Environment.GetEnvironmentVariables():");
            foreach (DictionaryEntry item in Environment.GetEnvironmentVariables())
            {
                Console.WriteLine($"\t{item.Key}={item.Value}");
            }

            Console.WriteLine("Output of new ProcessStartInfo().EnvironmentVariables:");
            foreach (DictionaryEntry item in new ProcessStartInfo().EnvironmentVariables)
            {
                Console.WriteLine($"\t{item.Key}={item.Value}");
            }

            Console.WriteLine("Output ofnew ProcessStartInfo().Environment:");
            foreach (KeyValuePair<string, string> item in new ProcessStartInfo().Environment)
            {
                Console.WriteLine($"\t{item.Key}={item.Value}");
            }

            return 0;
        }
    }
}
PS C:\Users\adsitnik\source\repos\DuplicateEnvVars> dotnet run -c Release -f net48                                                                                          
Output of Environment.GetEnvironmentVariables():
        NPM_CONFIG_CACHE=^
        npm_config_cache=^
Output of new ProcessStartInfo().EnvironmentVariables:

Unhandled Exception: System.ArgumentException: Item has already been added. Key in dictionary: 'NPM_CONFIG_CACHE'  Key being added: 'npm_config_cache'
   at System.Collections.Hashtable.Insert(Object key, Object nvalue, Boolean add)
   at System.Diagnostics.ProcessStartInfo.get_EnvironmentVariables()
   at DuplicateEnvVars.Program.PrintEnvVars()
   at DuplicateEnvVars.Program.Main(String[] args)
PS C:\Users\adsitnik\source\repos\DuplicateEnvVars> dotnet publish -c Release --self-contained -r win-x64 -f netcoreapp2.1                                                  
Microsoft (R) Build Engine version 16.8.0+126527ff1 for .NET
Copyright (C) Microsoft Corporation. All rights reserved.

  Determining projects to restore...
  All projects are up-to-date for restore.
  DuplicateEnvVars -> C:\Users\adsitnik\source\repos\DuplicateEnvVars\bin\Release\netcoreapp2.1\win-x64\DuplicateEnvVars.dll
  DuplicateEnvVars -> C:\Users\adsitnik\source\repos\DuplicateEnvVars\bin\Release\netcoreapp2.1\win-x64\publish\
PS C:\Users\adsitnik\source\repos\DuplicateEnvVars> C:\Users\adsitnik\source\repos\DuplicateEnvVars\bin\Release\netcoreapp2.1\win-x64\publish\DuplicateEnvVars.exe          
Output of Environment.GetEnvironmentVariables():
        NPM_CONFIG_CACHE=^
        npm_config_cache=^
Output of new ProcessStartInfo().EnvironmentVariables:
        NPM_CONFIG_CACHE=^
Output ofnew ProcessStartInfo().Environment:
        NPM_CONFIG_CACHE=^
PS C:\Users\adsitnik\source\repos\DuplicateEnvVars> dotnet run -c Release -f netcoreapp3.1                                                                                  
Output of Environment.GetEnvironmentVariables():
        NPM_CONFIG_CACHE=^
        npm_config_cache=^
Output of new ProcessStartInfo().EnvironmentVariables:
        NPM_CONFIG_CACHE=^
Output ofnew ProcessStartInfo().Environment:
        NPM_CONFIG_CACHE=^
PS C:\Users\adsitnik\source\repos\DuplicateEnvVars> dotnet run -c Release -f net5.0                                                                                         
Output of Environment.GetEnvironmentVariables():
        npm_config_cache=^
        NPM_CONFIG_CACHE=^
Output of new ProcessStartInfo().EnvironmentVariables:
        npm_config_cache=^
Output ofnew ProcessStartInfo().Environment:
        npm_config_cache=^

@adamsitnik adamsitnik removed this from the 6.0.0 milestone Dec 15, 2020
@asklar
Copy link
Author

asklar commented Dec 15, 2020

@adamsitnik This is currently affecting MSBuild.exe - without a fix to .net framework (or getting msbuild to move to .net core which includes moving Task DLLs to .net core, which seems unlikely in the foreseeable future), this will continue to crash.

Could you please reconsider?

@danmoseley
Copy link
Member

@asklar the bar for taking changes to .NET Framework is very high -- it is also a lengthy and costly process. I do not think this would be successful. I recommend a fix to Yarn, or how it's configured, or a wrapper script that fixes the environment block, or perhaps continue the discussion with MSBuild (as it ships in the SDK which is easier to change - I am not offering an opinion on whether the change is right for MSBuild).

@danmoseley
Copy link
Member

which includes moving Task DLLs to .net core, which seems unlikely in the foreseeable future

It may not be a near term solution, but I am curious why it is unlikely. Do you own the code? Just pointing out that building against .NET Standard may simply "just work" and then you could use the same DLL's for both .NET Framework and .NET Core.

@asklar
Copy link
Author

asklar commented Dec 15, 2020

@danmosemsft I don't own msbuild nor any of the Task DLLs being used, I'm just trying to get my C++ app builds to not fail when invoked from nodejs/yarn.
@rainersigwald is there any interest in moving msbuild and all of the inbox task dlls to .net core?

@AaronRobinsonMSFT
Copy link
Member

I actually think this may be a question/request for the C++ team to use the .NET Core version of MSBuild during their build. I will let @rainersigwald confirm that.

@ghost ghost locked as resolved and limited conversation to collaborators Jan 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants