-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
Passing empty environment variables to child processes convert to 'undefined' using ConEmu + Node #14593
Comments
Thanks for the bug report. Does it reproduce outside of npm? The behavior of process.env hasn't changed between v7.x and v8.x so I expect it's caused by something in npm. |
I've thought of that as well, but I'm using npm 5.3.0 in all cases so that doesn't seem to be the issue. Maybe something changed to |
Can you reproduce without npm being involved? If so, can you post that test case? (It's fine to do that inline here in a comment.) |
Hmm good point. Yes, I can. In one simple script using // start.js
const execSync = require('child_process').execSync;
process.env.foobar = '';
const stdout = execSync('node -e "const execSync = require(\'child_process\').execSync;' +
'execSync(\'node -e "console.log(process.env.foobar);"\', { stdio: [0, 1, 2]});"')
.toString().trim();
if (stdout === '') {
console.log('foobar is empty, stuff works.');
} else {
console.error(`foobar was "${stdout}", using ConEmu?`);
} If i run that script from a ConEmu tap using cmd shell it says:
From command prompt and cmd.exe:
|
Well, that does point to ConEmu. FWIW, it works in the expected way on Linux and OS X too. |
ConEmu maintainer Maximus5 says it does not change any environment variables. Also, it is translated to the actual string value If you change my script to print the entire environment like this: const execSync = require('child_process').execSync;
process.env.foobar = '';
const stdout = execSync('node -e "require(\'child_process\').execSync(\'node -e "console.log(process.env);"\', { stdio: [0, 1, 2]});"')
.toString().trim();
console.log(stdout); If i run that script from a ConEmu tap it says:
From cmd.exe:
|
Can't reproduce, I'm afraid; it prints cc @nodejs/platform-windows in case any of you can reproduce. |
Just double checking: you have Node version 8.0.0 or 8.2.1 and running from inside ConEmu with cmd shell? Did you let it print the entire |
Unable to reproduce with ConEmu 161206 (stable build) and node 8.2.1 on Windows 10. Both |
170402 (preview), but just now also installed 161206 and same issue. These are my ConEmu settings, but also tried on an other laptop with default settings: https://gist.github.com/nicojs/3abd0b4b2a996c1633bd97c9fd33ec27 See this screenshot for a side by side comparison of what i'm doing. On the left you can see the content of the file "start.js" (which is the first code snippet i shared) and executed from within ConEmu. On the right site you can see the same file executed from command prompt. Also on screen is the about window of ConEmu (with the version). EDIT: These are my shell settings for cmd (which are default). But i can reproduce with all shells. Tested with gitbash, powershell and cmd (64bits and 32bits) |
@nicojs, could you provide output of |
And |
I feel slightly naked for doing this. Everything for the cause. Please don't make fun of the size of my P...ATH https://gist.github.com/nicojs/4de5af23fa937deb8372ced562ef0878 I did the same thing on both console applications and output can be found in the gist:
|
I can repro: CmdInitRef.cmd@rem !!! Do not change this file in-place, change its copy instead !!!
@rem !!! Otherwise you will lose your settings after next update !!!
@echo off
rem Simple "ver" prints empty line before Windows version
rem Use this construction to print just a version info
cmd /d /c ver | "%windir%\system32\find.exe" "Windows"
rem Now we form the command prompt
rem This will start prompt with `User@PC `
rem set ConEmuPrompt0=$E[m$E[32m$E]9;8;"USERNAME"$E\@$E]9;8;"COMPUTERNAME"$E\$S
set ConEmuPrompt0=
rem Followed by colored `Path`
set ConEmuPrompt1=%ConEmuPrompt0%$P
if NOT "%PROCESSOR_ARCHITECTURE%" == "AMD64" (
if "%PROCESSOR_ARCHITEW6432%" == "AMD64" if "%PROCESSOR_ARCHITECTURE%" == "x86" (
rem Use another text color if cmd was run from SysWow64
set ConEmuPrompt1=%ConEmuPrompt0%$P
)
)
rem Carriage return and `$` or `>`
rem Spare `$E[90m` was specially added because of GitShowBranch.cmd
if "%ConEmuIsAdmin%" == "ADMIN" (
set ConEmuPrompt2=$$
) else (
set ConEmuPrompt2=$G
)
rem Finally reset color and add space
set ConEmuPrompt3=$S
if /I "%~1" == "/git" goto git
if /I "%~1" == "-git" goto git
goto no_git
:git
call "%~dp0GitShowBranch.cmd" /i
goto :EOF
:no_git
rem Set new prompt
PROMPT %ConEmuPrompt1%%ConEmuPrompt2%%ConEmuPrompt3% |
We're all friends here... child (node) 4640
grandchild (cmd) 12272
|
@refack if it's not too much to ask, can you also confirm that you cannot reproduce it in Node v7.9? Just to make sure that it is indeed related to node 8 and up. |
@nicojs was just about to... but bad news:
Seems like it's a ConEmu thing, but I'm dubugging |
Wow! Disabled it and stuff works again like a charm! Anyone running into this issue: setting can be found here: So i guess an issue at ConEmu side after all (or in the injected dll). Tricky stuff. Thanks to all for your time! It took me several days to understand what was going wrong, but solving the issue only took a couple of hours. 💐 |
I'm reopening since there is a node bug here: |
This is documented behavior though. |
So how do we proceed? Apparently, node converts null to undefined, which it should not do? And only when the ConEmuHK.dll is injected into child processes an empty value is transformed to |
Node treats an empty string as an empty string. If it doesn't, that's a bug but I doubt that's the case here, or we would have received a small avalanche of bug reports by now. I think a more likely explanation is that the parasitic code that ConEmu injects does something to GetEnvironmentVariableW() that breaks node. In fact, it seems like an inescapable conclusion because you mentioned that disabling injection of that DLL fixes the issue. |
Looks like it was changed in node 8.x because version 7.x doesn't suffer from the problem. |
I'll dig a bit. |
@refack Any news? |
First @Maximus5 patched ConEmuHK.dll so it does not leak or swallow WIN32 errors anymore - Maximus5/ConEmu@5324aa0 |
I am still affected by this bug in some CI setup without ConEmu. For the time being I downgraded nodejs to 6.11.1. Could we put the label confirmed-bug on this issue? |
@payload which version were you working with before? |
8.4.0 |
Experiencing this too, was driving me insane preventing me from building sqlite3 with node-gyp. Win 10 / ConEmu / Node 8.8.0 After reading this thread I just tried it in cmd.exe and it worked first time. |
@v9Chris glad this issue could help you. I know the feeling 🤘 |
That's why software updates are recommended. |
Any news whether node can be hardened against this without fixing the emulator? Sometimes these things are not under your control. |
Possibly related, without ConEmu: var exec = require('child_process').exec
process.env.test = ''
if (process.argv[2] === 'trigger') {
try { require('fs').readFileSync('foobar') } catch (e) {}
}
exec('node -p "process.env.test === \'undefined\' ? \'BUG\' : \'OK\'"', function (err, stdout) {
if (err) throw err
console.log(stdout)
}) Output:
But I can only trigger this on 4.8.7 x86 (also tried 9.2.0, 8.4.0, 8.0.0, 7.9.0, no problems there). |
Hello, Maybe I have found something very similar related to the access of an undefined process.env variable in Windows. The env variable that does not exist in this test is "DUMMY_VAR". process.env.applicationpath = "";
console.log("typeof process.env.applicationpath = " + typeof(process.env.applicationpath));
if (!process.env.DUMMY_VAR) {
// if (true) {
console.log(" typeof process.env.applicationpath = " + typeof(process.env.applicationpath));
}
console.log("typeof process.env.applicationpath = " + typeof(process.env.applicationpath)); Output:
Which I think is wrong.
I have tested the same code with node v5.5 and it gives the right expected result. In some way it seems that accessing the undefined environment variable is affecting the results in the other? |
I suspect this is also why BTW @payload I was unable to trigger it on v6. |
Yes, it’s exactly that bug. /cc @nodejs/lts @gibfahn Just so you are aware, this seems to be a bug that quite a few people are running into and a (pretty much zero-risk) fix has been landed on master two weeks ago. It might be good to have this in 8.10.0, even if it doesn’t technically fulfill the lived-in-Current-for-2-weeks rule. |
Sounds like a good candidate for expediting, will comment in #18463. |
Can this be closed now? |
@bzoz I think it might be easier to wait until this has been fixed in the v8.x branch, at least so we don’t forget about it |
I've landed the fix in v8.x-staging, so I'll close. |
Problem description
When executing a process which spawns a child process, which in turn creates another child process, the empty environment variables are passed through as 'undefined' (the string literal, NOT an absent value). If you than for example do an
npm install
command from that child process, bad things happen:To my knowledge: this only happens in ConEmu, a popular console emulator on Windows. When i downgrade to node 7.9, this problem does not occur.
Steps to reproduce
I created a small github repo to reproduce the problem.
git clone [email protected]:nicojs/reproduce-conemu-child-process-environment-bug.git
npm install
.npm test
This test spawns a child process, which in turn starts a new child process which logs the
process.env
to console. The tests verifies thatnpm_config_onload_script
(one of the env variables) is set to''
.Actual results
When ran from within ConEmu: the test failes
Error: "npm_config_onload_script": "undefined"!"
The test also prints the environment variables to screen. You can see a lot of "undefined" values (the string literal, not an absent value).
For example: "npm_config_onload_script": "undefined",
Expected results
If ran from cmd or git-bash using mintty directly (or on a POSIX environment): the test passes
√ should contain "npm_config_onload_script"
If you now look at the environment variables on screen, you don't see the "undefined" values. Just empty strings.
For example: "npm_config_onload_script": "",
I reported this issue to ConEmu first, but the developer pointed out to me that this seems to be regression in node. See the discussion with Maximus5 here: Maximus5/ConEmu#1209
Screenshots:
ConEmu with cmd.exe shell
Command prompt with cmd.exe shell
The text was updated successfully, but these errors were encountered: