MSBuild should sanitize environment block on startup

[asklar]

Issue Description

We have a command-line set of tools written in JavaScript (nodeJS) which launch msbuild. Our command line tool is launched via yarn.

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

See how the same environment variable is set twice, with different casing.

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 gets launched from within node.exe, which later launches CL.exe and other build tools.

The Azure DevOps CI pipeline sets NPM_CONFIG_CACHE (all caps), while yarn will add the lowercase npm_config_cache to the environment block when a process is launched via child_process.exec(). See https://github.com/actions/virtual-environments/blob/b47ba413c9bd9411407b8a5cf18e2c14ea8bda78/images/win/scripts/Installers/Install-NodeLts.ps1.

As a result of this, we are hitting an error in our CI because MultiTaskTool is probably putting variables in a case-insensitive dictionary and doesn't expect to find the same variable twice:
C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\MSBuild\Microsoft\VC\v160\Microsoft.CppCommon.targets(375,5): error MSB6001: Invalid command line switch for "CL.exe". System.ArgumentException: Item has already been added. Key in dictionary: 'NPM_CONFIG_CACHE' Key being added: 'npm_config_cache' [D:\a\1\s\vnext\Common\Common.vcxproj]

Steps to Reproduce

nodeJS file: test.js

console.log(process.env);
child_process.execSync("path_to_msbuild/msbuild.exe myproj.vcxproj");

myproj.vcxproj, just a regular C++ project

in packages.json add an entry e.g.

"run": "test.js"

in cmd, set NPM_CONFIG_CACHE

then run yarn run. See that the console.log in test.js shows the variable duplicated, and see that the task to launch CL breaks with an error similar to the above.

Expected Behavior

Build doesn't break

Actual Behavior

Build breaks :)

Analysis

Arguably System.Diagnostics.Process should sanitize its environment variables, but I'm not sure if that is fixable without introducing breaking changes so that's why I'd like msbuild to guard against this.

Versions & Configurations

all versions

Attach a binlog

msbuild_2720.zip

[rainersigwald]

Looks like the one layer you didn't file a bug in is MultiToolTask--can you file a Visual Studio Feedback issue for that and I can expedite routing? That feels like the easiest immediate solution to the problem you're seeing.

[asklar]

@rainersigwald Thanks!
DevCom: https://developercommunity.visualstudio.com/content/problem/1179475/multitooltask-should-sanitize-environment-variable.html

[rainersigwald]

Failing stack (from an email):

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() 

So the problem is not actually fixable in MultiToolTask, sorry. I think you're right that MSBuild would have to defensively reset our own environment at startup to work around the .NET Framework bug. That has concerning implications, and I'm actually not sure how to do it--does calling the win32 SetEnvironmentVariable actually clean up the environment block? Or does it just replace the first match occurrence and go on its merry way?

[rainersigwald]

Is it possible to add a layer of indirection, like invoking a batch file that does

set NPM_CONFIG_CACHE=%NPM_CONFIG_CACHE%
msbuild...

?

Or even maybe "clear it out harder" like so:

set ORIGINAL_NPM_CONFIG_CACHE=%NPM_CONFIG_CACHE%
set NPM_CONFIG_CACHE=
set npm_config_cache=

set NPM_CONFIG_CACHE=%ORIGINAL_NPM_CONFIG_CACHE%

msbuild...

[asklar]

@rainersigwald I checked the implementation of SetEnvironmentVariable - we first search for the variable name (case insensitive) and then just replace the value (without resetting the name).
So resetting the value like in your 2nd example would work, in fact that's what @jtpetty did for a test as I found out when searching for other usages of this: https://github.com/microsoft/azure-pipelines-tasks/blob/e2b6d30d68bd4cc2d465560f3be4293c6dd2956c/Tests/lib/Start-TestRunner.ps1#L65-L71

The problem is that we won't know ahead of time all of the other possible variables that npm decides to throw at msbuild, so I feel MSBuild needs to guard against this (if it can't be done in MTT)

[benvillalobos]

Team Triage: We are hesitant to make a change like this due to it being fairly invasive, expensive and able to be worked around at other levels.

[TomzBench]

Can please make clear the work around? I get the same error except the duplicated key is "Path" and "PATH".

[wheezil]

This issue is worse than reported. As i note in my maven-exec-plugin pull request, even if the plugin doesn't send duplicate env vars, this is an issue even if the env vars are converted to UPPERCASE, because MSBuild seems to reach into the registry and try to re-add the env vars it finds, but it fails because it doesn't check for case-insensitive dupes until it is too late