From 6d0f9a99ddc7ec150a762c3e178cfd017741a3ef Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 5 Jul 2024 20:40:03 +0200 Subject: [PATCH] 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: https://github.com/nodejs/node/pull/53596 Refs: https://github.com/nodejs/node/issues/41103 Reviewed-By: James M Snell Reviewed-By: Xuguang Mei Reviewed-By: Chengzhong Wu --- src/node_worker.cc | 40 ++++++++++++++++--- test/fixtures/spawn-worker-with-copied-env.js | 17 ++++++++ test/fixtures/spawn-worker-with-trace-exit.js | 20 ++++++++++ test/parallel/test-worker-node-options.js | 35 ++++++++++++++++ 4 files changed, 106 insertions(+), 6 deletions(-) create mode 100644 test/fixtures/spawn-worker-with-copied-env.js create mode 100644 test/fixtures/spawn-worker-with-trace-exit.js create mode 100644 test/parallel/test-worker-node-options.js diff --git a/src/node_worker.cc b/src/node_worker.cc index f744c6a1a97ff1..f12dde0ae50a6c 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -549,12 +549,40 @@ void Worker::New(const FunctionCallbackInfo& args) { // [0] is expected to be the program name, add dummy string. env_argv.insert(env_argv.begin(), ""); std::vector invalid_args{}; - options_parser::Parse(&env_argv, - nullptr, - &invalid_args, - per_isolate_opts.get(), - kAllowedInEnvvar, - &errors); + + std::string parent_node_options; + USE(env->env_vars()->Get("NODE_OPTIONS").To(&parent_node_options)); + + // If the worker code passes { env: { ...process.env, ... } } or + // the NODE_OPTIONS is otherwise character-for-character equal to the + // original NODE_OPTIONS, allow per-process options inherited into + // the worker since worker spawning code is not usually in charge of + // how the NODE_OPTIONS is configured for the parent. + // TODO(joyeecheung): a more intelligent filter may be more desirable. + // but a string comparison is good enough(TM) for the case where the + // worker spawning code just wants to pass the parent configuration down + // and does not intend to modify NODE_OPTIONS. + if (parent_node_options == node_options) { + // Creates a wrapper per-process option over the per_isolate_opts + // to allow per-process options copied from the parent. + std::unique_ptr per_process_opts = + std::make_unique(); + per_process_opts->per_isolate = per_isolate_opts; + options_parser::Parse(&env_argv, + nullptr, + &invalid_args, + per_process_opts.get(), + kAllowedInEnvvar, + &errors); + } else { + options_parser::Parse(&env_argv, + nullptr, + &invalid_args, + per_isolate_opts.get(), + kAllowedInEnvvar, + &errors); + } + if (!errors.empty() && args[1]->IsObject()) { // Only fail for explicitly provided env, this protects from failures // when NODE_OPTIONS from parent's env is used (which is the default). diff --git a/test/fixtures/spawn-worker-with-copied-env.js b/test/fixtures/spawn-worker-with-copied-env.js new file mode 100644 index 00000000000000..51e64bd8755b6d --- /dev/null +++ b/test/fixtures/spawn-worker-with-copied-env.js @@ -0,0 +1,17 @@ +'use strict'; + +// This test is meant to be spawned with NODE_OPTIONS=--title=foo +const assert = require('assert'); +if (process.platform !== 'sunos') { // --title is unsupported on SmartOS. + assert.strictEqual(process.title, 'foo'); +} + +// Spawns a worker that may copy NODE_OPTIONS if it's set by the parent. +const { Worker } = require('worker_threads'); +new Worker(`require('assert').strictEqual(process.env.TEST_VAR, 'bar')`, { + env: { + ...process.env, + TEST_VAR: 'bar', + }, + eval: true, +}); diff --git a/test/fixtures/spawn-worker-with-trace-exit.js b/test/fixtures/spawn-worker-with-trace-exit.js new file mode 100644 index 00000000000000..6cf091c727a74f --- /dev/null +++ b/test/fixtures/spawn-worker-with-trace-exit.js @@ -0,0 +1,20 @@ +'use strict'; + +const { Worker, isMainThread } = require('worker_threads') + +// Tests that valid per-isolate/env NODE_OPTIONS are allowed and +// work in child workers. +if (isMainThread) { + new Worker(__filename, { + env: { + ...process.env, + NODE_OPTIONS: '--trace-exit' + } + }) +} else { + setImmediate(() => { + process.nextTick(() => { + process.exit(0); + }); + }); +} diff --git a/test/parallel/test-worker-node-options.js b/test/parallel/test-worker-node-options.js new file mode 100644 index 00000000000000..7a26154e2f4800 --- /dev/null +++ b/test/parallel/test-worker-node-options.js @@ -0,0 +1,35 @@ +'use strict'; + +require('../common'); +const { + spawnSyncAndExitWithoutError, + spawnSyncAndAssert, +} = require('../common/child_process'); +const fixtures = require('../common/fixtures'); +spawnSyncAndExitWithoutError( + process.execPath, + [ + fixtures.path('spawn-worker-with-copied-env'), + ], + { + env: { + ...process.env, + NODE_OPTIONS: '--title=foo' + } + } +); + +spawnSyncAndAssert( + process.execPath, + [ + fixtures.path('spawn-worker-with-trace-exit'), + ], + { + env: { + ...process.env, + } + }, + { + stderr: /spawn-worker-with-trace-exit\.js:17/ + } +);