Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Commit d6d0427

Browse filesBrowse files
joyeecheungmarco-ippolito
authored andcommitted
worker: allow copied NODE_OPTIONS in the env setting
When the worker spawning code copies NODE_OPTIONS from process.env, previously we do a check again on the copied NODE_OPTIONS and throw if it contains per-process settings. This can be problematic if the end user starts the process with a NODE_OPTIONS and the worker is spawned by a third-party that tries to extend process.env with some overrides before passing them into the worker. This patch adds another exception that allows the per-process options in the NODE_OPTIONS passed to a worker if the NODE_OPTIONS is character-by-character equal to the parent NODE_OPTIONS. While some more intelligent filter can be useful too, this works good enough for the inheritance case, when the worker spawning code does not intend to modify NODE_OPTIONS. PR-URL: #53596 Refs: #41103 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Xuguang Mei <meixuguang@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
1 parent 60ee41d commit d6d0427
Copy full SHA for d6d0427

4 files changed

+106-6Lines changed: 106 additions & 6 deletions

File tree

Expand file treeCollapse file tree
Open diff view settings
Filter options
Expand file treeCollapse file tree
Open diff view settings
Collapse file

‎src/node_worker.cc‎

Copy file name to clipboardExpand all lines: src/node_worker.cc
+34-6Lines changed: 34 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -549,12 +549,40 @@ void Worker::New(const FunctionCallbackInfo<Value>& args) {
549549
// [0] is expected to be the program name, add dummy string.
550550
env_argv.insert(env_argv.begin(), "");
551551
std::vector<std::string> invalid_args{};
552-
options_parser::Parse(&env_argv,
553-
nullptr,
554-
&invalid_args,
555-
per_isolate_opts.get(),
556-
kAllowedInEnvvar,
557-
&errors);
552+
553+
std::string parent_node_options;
554+
USE(env->env_vars()->Get("NODE_OPTIONS").To(&parent_node_options));
555+
556+
// If the worker code passes { env: { ...process.env, ... } } or
557+
// the NODE_OPTIONS is otherwise character-for-character equal to the
558+
// original NODE_OPTIONS, allow per-process options inherited into
559+
// the worker since worker spawning code is not usually in charge of
560+
// how the NODE_OPTIONS is configured for the parent.
561+
// TODO(joyeecheung): a more intelligent filter may be more desirable.
562+
// but a string comparison is good enough(TM) for the case where the
563+
// worker spawning code just wants to pass the parent configuration down
564+
// and does not intend to modify NODE_OPTIONS.
565+
if (parent_node_options == node_options) {
566+
// Creates a wrapper per-process option over the per_isolate_opts
567+
// to allow per-process options copied from the parent.
568+
std::unique_ptr<PerProcessOptions> per_process_opts =
569+
std::make_unique<PerProcessOptions>();
570+
per_process_opts->per_isolate = per_isolate_opts;
571+
options_parser::Parse(&env_argv,
572+
nullptr,
573+
&invalid_args,
574+
per_process_opts.get(),
575+
kAllowedInEnvvar,
576+
&errors);
577+
} else {
578+
options_parser::Parse(&env_argv,
579+
nullptr,
580+
&invalid_args,
581+
per_isolate_opts.get(),
582+
kAllowedInEnvvar,
583+
&errors);
584+
}
585+
558586
if (!errors.empty() && args[1]->IsObject()) {
559587
// Only fail for explicitly provided env, this protects from failures
560588
// when NODE_OPTIONS from parent's env is used (which is the default).
Collapse file
+17Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
'use strict';
2+
3+
// This test is meant to be spawned with NODE_OPTIONS=--title=foo
4+
const assert = require('assert');
5+
if (process.platform !== 'sunos') { // --title is unsupported on SmartOS.
6+
assert.strictEqual(process.title, 'foo');
7+
}
8+
9+
// Spawns a worker that may copy NODE_OPTIONS if it's set by the parent.
10+
const { Worker } = require('worker_threads');
11+
new Worker(`require('assert').strictEqual(process.env.TEST_VAR, 'bar')`, {
12+
env: {
13+
...process.env,
14+
TEST_VAR: 'bar',
15+
},
16+
eval: true,
17+
});
Collapse file
+20Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
'use strict';
2+
3+
const { Worker, isMainThread } = require('worker_threads')
4+
5+
// Tests that valid per-isolate/env NODE_OPTIONS are allowed and
6+
// work in child workers.
7+
if (isMainThread) {
8+
new Worker(__filename, {
9+
env: {
10+
...process.env,
11+
NODE_OPTIONS: '--trace-exit'
12+
}
13+
})
14+
} else {
15+
setImmediate(() => {
16+
process.nextTick(() => {
17+
process.exit(0);
18+
});
19+
});
20+
}
Collapse file
+35Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
'use strict';
2+
3+
require('../common');
4+
const {
5+
spawnSyncAndExitWithoutError,
6+
spawnSyncAndAssert,
7+
} = require('../common/child_process');
8+
const fixtures = require('../common/fixtures');
9+
spawnSyncAndExitWithoutError(
10+
process.execPath,
11+
[
12+
fixtures.path('spawn-worker-with-copied-env'),
13+
],
14+
{
15+
env: {
16+
...process.env,
17+
NODE_OPTIONS: '--title=foo'
18+
}
19+
}
20+
);
21+
22+
spawnSyncAndAssert(
23+
process.execPath,
24+
[
25+
fixtures.path('spawn-worker-with-trace-exit'),
26+
],
27+
{
28+
env: {
29+
...process.env,
30+
}
31+
},
32+
{
33+
stderr: /spawn-worker-with-trace-exit\.js:17/
34+
}
35+
);

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.