-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
Node must de-duplicate environment variables before calling CreateProcess (and upon env.extend()) #35129
Comments
This is basically a duplicate of #34667 When In this case, yarn needs to be fixed to correctly handle environment variables on Windows. |
Closing as a duplicate. |
@bnoordhuis @bzoz The documentation only mentions PATH as being the special case but in fact all environment variables are susceptible to this problem. Could you please update the documentation to reflect this? Moreover, the doc says it will sort the variables lexicographically and use the first occurrence of PATH. This is the same behavior that the OS GetEnvironmentVariable will do, so once you get into the duplicate variable case, it doesn't matter which one was added last - the only one that is reachable via GetEnvironmentVariable is going to be the first lexicographically, so spawn could sanitize the env block by doing exactly that, only keep the first instance of each variable |
Those points make sense, both the doc update and the sanitization. I think keeping only one entry for each env variable would be a semver-major though. |
@bzoz any clue when we might see this fixed? |
I've opened a PR with a fix: #35210. I guess this is a semver-major though, so I would expect this to be shipped with the next major release of Node. |
thanks a lot @bzoz. From the release doc it sounds like the next release is October so that would be ok assuming we can get this in soon 🤞 |
On Windows environment variables are case-insensitive. When spawning child process certain apps can get confused if some of the variables are duplicated. This adds a step on Windows to normalizeSpawnArguments that removes such duplicates, keeping only the first (in lexicographic order) entry in the env key of options. This is partly already done for the PATH entry. Fixes: nodejs#35129
On Windows environment variables are case-insensitive. When spawning child process certain apps can get confused if some of the variables are duplicated. This adds a step on Windows to normalizeSpawnArguments that removes such duplicates, keeping only the first (in lexicographic order) entry in the env key of options. This is partly already done for the PATH entry. Fixes: nodejs#35129 PR-URL: nodejs#35210 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Denys Otrishko <shishugi@gmail.com>
What steps will reproduce the bug?
yarn sets up some environment variables (like
npm_config_cache
) and calls execSync. This calls the Win32 APICreateProcess
and passes an environment block which is a merge of the existing environment block (i.e. from the calling process) with some additional stuff that includesnpm_config_cache
among others.Environment variables in Windows are case-insensitive, but CreateProcess does not seem to sanitize the environment block. This is a problem because if the calling process had set up
NPM_CONFIG_CACHE
(all caps), then yarn will just set the lowercase variant of the variable, and node will callCreateProcess
which will contain the variable twice). This breaks some tools that create dictionaries out of the environment block because they expect to run into each variable exactly once (regardless of casing).In my case we have a nodejs script that we invoke via yarn, and the nodejs script calls a Windows build utility (msbuild) which crashes when trying to set up options for the compiler when it finds this option twice. We run into this when building in our CI in Azure DevOps pipeline, which sets up NPM_CONFIG_CACHE in the environment.
Related:
How often does it reproduce? Is there a required condition?
100%
What is the expected behavior?
node should de-duplicate variables before passing them to CreateProcess
What do you see instead?
CreateProcess gets an environment block with both variables:
Additional information
The text was updated successfully, but these errors were encountered: