Skip to content

Commit b0c030c

Browse files
everett1992aduh95
authored andcommitted
child_process: inherit a reassigned process.env
Since v26.11.0, spawning without options.env builds the child's environment from the OS environment block. Code that replaces process.env, e.g. process.env = { ...process.env, FOO: 'x' }, had its changes silently dropped. The child_process docs give process.env as the default for options.env, and { env: process.env } still saw the replacement object, so the two disagreed. Make getEnvPairs() take the object to read and return undefined unless it is still the native env proxy. When it isn't, copy process.env as before. A plain object is copied without going through the env interceptors, so that path stays cheap. Assisted-by: a closed-source coding agent Signed-off-by: Caleb ツ Everett <[email protected]> PR-URL: #66883 Fixes: #66882 Refs: #65325 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
1 parent d59e71d commit b0c030c

3 files changed

Lines changed: 117 additions & 3 deletions

File tree

‎lib/child_process.js‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -713,8 +713,11 @@ function normalizeSpawnArguments(file, args, options) {
713713
// instead of copying process.env (one interceptor round trip and one
714714
// getenv() scan per variable). Everything copyProcessEnvToEnv() would
715715
// propagate below is part of it by definition, and entries of the real
716-
// environment block cannot contain null bytes.
717-
envPairs = getEnvPairs();
716+
// environment block cannot contain null bytes. Undefined if process.env
717+
// was reassigned.
718+
envPairs = getEnvPairs(process.env);
719+
}
720+
if (envPairs !== undefined) {
718721
if (process.platform === 'win32') {
719722
envPairs = dedupeWindowsEnvPairs(envPairs);
720723
}

‎src/process_wrap.cc‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -346,9 +346,17 @@ class ProcessWrap : public HandleWrap {
346346

347347
// The current environment as ["KEY=value", ...], i.e. what a spawned
348348
// child inherits by default, produced in one pass over the environment
349-
// block instead of one interceptor round trip per variable.
349+
// block instead of one interceptor round trip per variable. Returns
350+
// undefined if args[0] is not the env proxy, e.g. because process.env was
351+
// reassigned, in which case the caller has to copy that object instead.
350352
static void GetEnvPairs(const FunctionCallbackInfo<Value>& args) {
351353
Environment* env = Environment::GetCurrent(args);
354+
Local<FunctionTemplate> env_proxy_ctor_template =
355+
env->env_proxy_ctor_template();
356+
if (env_proxy_ctor_template.IsEmpty() ||
357+
!env_proxy_ctor_template->HasInstance(args[0])) {
358+
return;
359+
}
352360
Local<Array> pairs;
353361
if (env->env_vars()->Pairs(env->isolate()).ToLocal(&pairs)) {
354362
args.GetReturnValue().Set(pairs);
Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
'use strict';
2+
// When no `env` option is given, a child process inherits process.env, even
3+
// if process.env has been replaced with another object.
4+
// Refs: https://github.com/nodejs/node/issues/66882
5+
const common = require('../common');
6+
const assert = require('assert');
7+
const {
8+
exec,
9+
execFile,
10+
execFileSync,
11+
execSync,
12+
fork,
13+
spawn,
14+
spawnSync,
15+
} = require('child_process');
16+
17+
if (process.argv[2] === 'child') {
18+
process.send(process.env.TEST_REASSIGNED);
19+
return;
20+
}
21+
22+
const printEnv = ['-p', 'process.env.TEST_REASSIGNED'];
23+
const shellEcho = common.isWindows ?
24+
'echo %TEST_REASSIGNED%' :
25+
'echo "$TEST_REASSIGNED"';
26+
27+
function runSync(options) {
28+
const child = spawnSync(process.execPath, printEnv,
29+
{ encoding: 'utf8', ...options });
30+
assert.strictEqual(child.status, 0, child.stderr || String(child.error));
31+
return child.stdout.trim();
32+
}
33+
34+
const originalEnv = process.env;
35+
// Lives in the real environment, but not in the replacement object.
36+
process.env.TEST_ONLY_IN_ORIGINAL = 'original';
37+
process.env = { ...process.env, TEST_REASSIGNED: 'reassigned' };
38+
delete process.env.TEST_ONLY_IN_ORIGINAL;
39+
40+
assert.strictEqual(runSync(), 'reassigned');
41+
assert.strictEqual(
42+
execFileSync(process.execPath, printEnv, { encoding: 'utf8' }).trim(),
43+
'reassigned');
44+
assert.strictEqual(execSync(shellEcho, { encoding: 'utf8' }).trim(),
45+
'reassigned');
46+
47+
{
48+
const child = spawnSync(
49+
process.execPath,
50+
['-p', 'process.env.TEST_ONLY_IN_ORIGINAL'],
51+
{ encoding: 'utf8' });
52+
assert.strictEqual(child.stdout.trim(), 'undefined');
53+
}
54+
55+
// The default env matches an explicit { env: process.env }.
56+
{
57+
const printAll = ['-e', 'process.stdout.write(JSON.stringify(process.env))'];
58+
const run = (options) => JSON.parse(
59+
spawnSync(process.execPath, printAll, { encoding: 'utf8', ...options })
60+
.stdout);
61+
assert.deepStrictEqual(run(), run({ env: process.env }));
62+
}
63+
64+
// The replacement object is used for command lookup.
65+
if (!common.isWindows) {
66+
const saved = process.env;
67+
process.env = { ...saved, PATH: '/nonexistent' };
68+
const child = spawnSync('ls');
69+
process.env = saved;
70+
assert.strictEqual(child.error?.code, 'ENOENT');
71+
}
72+
73+
{
74+
const child = spawn(process.execPath, printEnv);
75+
let out = '';
76+
child.stdout.setEncoding('utf8').on('data', (d) => { out += d; });
77+
let err = '';
78+
child.stderr.setEncoding('utf8').on('data', (d) => { err += d; });
79+
child.on('close', common.mustCall((code) => {
80+
assert.strictEqual(code, 0, err);
81+
assert.strictEqual(out.trim(), 'reassigned');
82+
}));
83+
}
84+
85+
exec(shellEcho, common.mustSucceed((stdout) => {
86+
assert.strictEqual(stdout.trim(), 'reassigned');
87+
}));
88+
89+
execFile(process.execPath, printEnv, common.mustSucceed((stdout) => {
90+
assert.strictEqual(stdout.trim(), 'reassigned');
91+
}));
92+
93+
fork(__filename, ['child']).on('message', common.mustCall((value) => {
94+
assert.strictEqual(value, 'reassigned');
95+
}));
96+
97+
// Putting the original object back restores inheriting the real environment.
98+
process.env = originalEnv;
99+
assert.strictEqual(runSync(), 'undefined');
100+
assert.strictEqual(
101+
spawnSync(process.execPath, ['-p', 'process.env.TEST_ONLY_IN_ORIGINAL'],
102+
{ encoding: 'utf8' }).stdout.trim(),
103+
'original');

0 commit comments

Comments
 (0)