worker: allow copied NODE_OPTIONS in the env setting

joyeecheung · marco-ippolito · commit d6d04279ca74 · 2024-08-19T10:32:31.000+02:00
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>
diff --git a/src/node_worker.cc b/src/node_worker.cc
@@ -549,12 +549,40 @@ void Worker::New(const FunctionCallbackInfo<Value>& args) {
       // [0] is expected to be the program name, add dummy string.
       env_argv.insert(env_argv.begin(), "");
       std::vector<std::string> 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<PerProcessOptions> per_process_opts =
+            std::make_unique<PerProcessOptions>();
+        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
@@ -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
@@ -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
@@ -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/
+  }
+);
