fixup: address feedback

nodejs · d3lm · Sep 2, 2021 · Sep 2, 2021 · Sep 2, 2021 · Sep 3, 2021
commit 59eeea9affe56bad25e06b6d04359d723372a63c
diff --git a/doc/api/cli.md b/doc/api/cli.md
@@ -598,6 +598,15 @@ added: v7.10.0
 
 This option is a no-op. It is kept for compatibility.
 
+### `--no-addons`
+<!-- YAML
+added: REPLACEME
+-->
+
+Enable a `no-addons` resolution condition as well as disable loading native
+addons. When `--no-addons` is specified, calling `process.dlopen` or requiring
+a native C++ addon will fail and throw an exception.
+
 ### `--no-deprecation`
 <!-- YAML
 added: v0.8.0
@@ -613,15 +622,6 @@ added: v9.0.0
 Disables runtime checks for `async_hooks`. These will still be enabled
 dynamically when `async_hooks` is enabled.
 
-### `--no-addons`
-<!-- YAML
-added: REPLACEME
--->
-
-Enable a `no-addons` resolution condition as well as disable loading native
-addons. When `--no-addons` is specified, calling `process.dlopen` or requiring
-a native C++ addon will fail and throw an exception.
-
 ### `--no-warnings`
 <!-- YAML
 added: v6.0.0
@@ -1430,10 +1430,10 @@ Node.js options that are allowed are:
 * `--inspect`
 * `--max-http-header-size`
 * `--napi-modules`
+* `--no-addons`
 * `--no-deprecation`
 * `--no-experimental-repl-await`
 * `--no-force-async-hooks-checks`
-* `--no-addons`
 * `--no-warnings`
 * `--node-memory-debug`
 * `--openssl-config`

diff --git a/doc/api/errors.md b/doc/api/errors.md
@@ -1027,6 +1027,14 @@ added:
 
 The [debugger][] timed out waiting for the required host/port to be free.
 
+<a id="ERR_DLOPEN_DISABLED"></a>
+### `ERR_DLOPEN_DISABLED`
+<!-- YAML
+added: REPLACEME
+-->
+
+Loading native addons has been disabled using [`--no-addons`][].
+
 <a id="ERR_DLOPEN_FAILED"></a>
 ### `ERR_DLOPEN_FAILED`
 <!-- YAML
@@ -2879,6 +2887,7 @@ The native call from `process.cpuUsage` could not be processed.
 [`'uncaughtException'`]: process.md#event-uncaughtexception
 [`--disable-proto=throw`]: cli.md#--disable-protomode
 [`--force-fips`]: cli.md#--force-fips
+[`--no-addons`]: cli.md#--no-addons
 [`Class: assert.AssertionError`]: assert.md#class-assertassertionerror
 [`ERR_INVALID_ARG_TYPE`]: #err_invalid_arg_type
 [`ERR_MISSING_MESSAGE_PORT_IN_TRANSFER_LIST`]: #err_missing_message_port_in_transfer_list

diff --git a/src/env-inl.h b/src/env-inl.h
@@ -861,6 +861,11 @@ inline bool Environment::is_main_thread() const {
   return worker_context() == nullptr;
 }
 
+inline bool Environment::no_native_addons() const {
+  return (flags_ & EnvironmentFlags::kNoNativeAddons) ||
+          !options_->allow_native_addons;
+}
+
 inline bool Environment::should_not_register_esm_loader() const {
   return flags_ & EnvironmentFlags::kNoRegisterESMLoader;
 }

diff --git a/src/env.h b/src/env.h
@@ -1197,6 +1197,7 @@ class Environment : public MemoryRetainer {
   inline void set_has_serialized_options(bool has_serialized_options);
 
   inline bool is_main_thread() const;
+  inline bool no_native_addons() const;
   inline bool should_not_register_esm_loader() const;
   inline bool owns_process_state() const;
   inline bool owns_inspector() const;

diff --git a/src/node.h b/src/node.h
@@ -407,7 +407,13 @@ enum Flags : uint64_t {
   // Set this flag to force hiding console windows when spawning child
   // processes. This is usually used when embedding Node.js in GUI programs on
   // Windows.
-  kHideConsoleWindows = 1 << 5
+  kHideConsoleWindows = 1 << 5,
+  // Set this flag to disable loading native addons via `process.dlopen`.
+  // This environment flag is especially important for worker threads
+  // so that a worker thread can't load a native addon even if `execArgv`
+  // is overwritten and `--no-addons` is not specified but was specified
+  // for this Environment instance.
+  kNoNativeAddons = 1 << 6
 };
 }  // namespace EnvironmentFlags
 

diff --git a/src/node_binding.cc b/src/node_binding.cc
@@ -416,8 +416,8 @@ inline napi_addon_register_func GetNapiInitializerCallback(DLib* dlib) {
 void DLOpen(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
 
-  if (UNLIKELY(!env->options()->allow_native_addons)) {
-    return THROW_ERR_DLOPEN_FAILED(
+  if (env->no_native_addons()) {
+    return THROW_ERR_DLOPEN_DISABLED(
       env, "Cannot load native addon because loading addons is disabled.");
   }
 

diff --git a/src/node_errors.h b/src/node_errors.h
@@ -57,6 +57,7 @@ void OnFatalError(const char* location, const char* message);
   V(ERR_CRYPTO_UNKNOWN_DH_GROUP, Error)                                        \
   V(ERR_CRYPTO_UNSUPPORTED_OPERATION, Error)                                   \
   V(ERR_CRYPTO_JOB_INIT_FAILED, Error)                                         \
+  V(ERR_DLOPEN_DISABLED, Error)                                                \
   V(ERR_DLOPEN_FAILED, Error)                                                  \
   V(ERR_EXECUTION_ENVIRONMENT_NOT_AVAILABLE, Error)                            \
   V(ERR_INVALID_ADDRESS, Error)                                                \

diff --git a/src/node_worker.cc b/src/node_worker.cc
@@ -560,6 +560,8 @@ void Worker::New(const FunctionCallbackInfo<Value>& args) {
     worker->environment_flags_ |= EnvironmentFlags::kTrackUnmanagedFds;
   if (env->hide_console_windows())
     worker->environment_flags_ |= EnvironmentFlags::kHideConsoleWindows;
+  if (env->no_native_addons())
+    worker->environment_flags_ |= EnvironmentFlags::kNoNativeAddons;
 }
 
 void Worker::StartThread(const FunctionCallbackInfo<Value>& args) {

diff --git a/test/addons/no-addons/test-worker.js b/test/addons/no-addons/test-worker.js
@@ -1,3 +1,5 @@
+// Flags: --no-addons
+
 'use strict';
 
 const common = require('../../common');
@@ -7,17 +9,39 @@ const { Worker } = require('worker_threads');
 
 const binding = path.resolve(__dirname, `./build/${common.buildType}/binding`);
 
-const worker = new Worker(`require(${JSON.stringify(binding)})`, {
-  eval: true,
-});
-
-worker.on(
-  'error',
-  common.mustCall((error) => {
-    assert.strictEqual(error.code, 'ERR_DLOPEN_FAILED');
-    assert.strictEqual(
-      error.message,
-      'Cannot load native addon because loading addons is disabled.'
-    );
-  })
-);
+const expectError = (error) => {
+  assert.strictEqual(error.code, 'ERR_DLOPEN_DISABLED');
+  assert.strictEqual(
+    error.message,
+    'Cannot load native addon because loading addons is disabled.'
+  );
+};
+
+{
+  // flags should be inherited
+  const worker = new Worker(`require(${JSON.stringify(binding)})`, {
+    eval: true,
+  });
+
+  worker.on('error', common.mustCall(expectError));
+}
+
+{
+  // explicitly pass `--no-addons`
+  const worker = new Worker(`require(${JSON.stringify(binding)})`, {
+    eval: true,
+    execArgv: ['--no-addons'],
+  });
+
+  worker.on('error', common.mustCall(expectError));
+}
+
+{
+  // if `execArgv` is overwritten it should still fail to load addons
+  const worker = new Worker(`require(${JSON.stringify(binding)})`, {
+    eval: true,
+    execArgv: [],
+  });
+
+  worker.on('error', common.mustCall(expectError));
+}
diff --git a/test/addons/no-addons/test.js b/test/addons/no-addons/test.js
@@ -1,3 +1,5 @@
+// Flags: --no-addons
+
 'use strict';
 
 const common = require('../../common');
@@ -12,7 +14,7 @@ try {
 } catch (error) {
   threw = true;
   assert(error instanceof Error);
-  assert.strictEqual(error.code, 'ERR_DLOPEN_FAILED');
+  assert.strictEqual(error.code, 'ERR_DLOPEN_DISABLED');
   assert.strictEqual(
     error.message,
     'Cannot load native addon because loading addons is disabled.'

diff --git a/test/es-module/test-esm-no-addons.mjs b/test/es-module/test-esm-no-addons.mjs
@@ -0,0 +1,27 @@
+import { mustCall } from '../common/index.mjs';
+import { Worker, isMainThread } from 'worker_threads';
+import assert from 'assert';
+import { fileURLToPath } from 'url';
+import { requireFixture, importFixture } from '../fixtures/pkgexports.mjs';
+
+if (isMainThread) {
+  const tests = [[], ['--no-addons']];
+
+  for (const execArgv of tests) {
+    new Worker(fileURLToPath(import.meta.url), { execArgv });
+  }
+} else {
+  [requireFixture, importFixture].forEach((loadFixture) => {
+    loadFixture('pkgexports/no-addons').then(
+      mustCall((module) => {
+        const message = module.default;
+
+        if (process.execArgv.length === 0) {
+          assert.strictEqual(message, 'using native addons');
+        } else {
+          assert.strictEqual(message, 'not using native addons');
+        }
+      })
+    );
+  });
+}