fix: prevent crashing when napi_register_module_v1 is called twice (#1554)

* fix: prevent crashing when napi_register_module_v1 is called twice

Currently napi-rs addons can lead to the Node.js process aborting with
the following error when initialising the addon on Windows:

```
c:\ws\src\cleanup_queue-inl.h:32: Assertion `(insertion_info.second)
== (true)' failed.
```

This happens because `napi_add_env_cleanup_hook` must not be called
with the same arguments multiple times unless the previously scheduled
cleanup hook with the same arguments was already executed. However,
the cleanup hook added by `napi_register_module_v1` in napi-rs on
Windows was always created with `ptr::null_mut()` as an argument.

One case where this causes a problem is when using the addon from
multiple contexts (e.g. Node.js worker threads) at the same
time. However, Node.js doesn't provide any guarantees that the N-API
addon initialisation code will run only once even per thread and
context. In fact, it's totally valid to run `process.dlopen()`
multiple times from JavaScript land in Node.js, and this will lead to
the initialisation code being run multiple times as different
`exports` objects may need to be populated. This may happen in
numerous cases, e.g.:

- When it's not possible or not desirable to use `require()` and users
  must resort to using `process.dlopen()` (one use case is passing
  non-default flags to `dlopen(3)`, another is ES modules). Caching
  the results of `process.dlopen()` to avoid running it more than once
  may not always be possible reliably in all cases (for example,
  because of Jest sandbox).

- When the `require` cache is cleared.

- On Windows: `require("./addon.node")` and then
  `require(path.toNamespacedPath("./addon.node"))`.

Another issue is fixed inside `napi::tokio_runtime::drop_runtime`:
there's no need to call `napi_remove_env_cleanup_hook` (it's only
useful to cancel the hooks that haven't been executed yet). Null
pointer retrieved from `arg` was being passed as the `env` argument of
that function, so it didn't do anything and just returned
`napi_invalid_arg`.

This patch makes `napi_register_module_v1` use a counter as the
cleanup hook argument, so that the value is always different. An
alternative might have been to use a higher-level abstraction around
`sys::napi_env_cleanup_hook` that would take ownership of a boxed
closure, if there is something like this in the API already. Another
alternative could have been to heap-allocate a value so that we would
have a unique valid memory address.

The patch also contains a minor code cleanup related to
`RT_REFERENCE_COUNT` along the way: the counter is encapsulated inside
its module and `ensure_runtime` takes care of incrementing it, and
less strict memory ordering is now used as there's no need for
`SeqCst` here. If desired, it can be further optimised to
`Ordering::Release` and a separate acquire fence inside the if
statement in `drop_runtime`, as `AcqRel` for every decrement is also a
bit stricter than necessary (although simpler). These changes are not
necessary to fix the issue and can be extracted to a separate patch.

At first it was tempting to use the loaded value of
`RT_REFERENCE_COUNT` as the argument for the cleanup hook but it would
have been wrong: a simple counterexample is the following sequence:

1. init in the first context (queue: 0)
2. init in the second context (queue: 0, 1)
3. destroy the first context (queue: 1)
4. init in the third context (queue: 1, 1)

* test(napi): unload test was excluded unexpected

---------

Co-authored-by: LongYinan <lynweklm@gmail.com>

crates/napi/src/bindgen_runtime/module_register.rs

@@ -5,8 +5,7 @@ use std::hash::Hash;
 #[cfg(all(feature = "napi4", not(target_arch = "wasm32")))]
 use std::ops::Deref;
 use std::ptr;
-use std::sync::atomic::AtomicUsize;
-use std::sync::atomic::{AtomicBool, AtomicPtr, Ordering};
+use std::sync::atomic::{AtomicBool, AtomicPtr, AtomicUsize, Ordering};
 use std::thread::ThreadId;
 
 use once_cell::sync::Lazy;
@@ -534,15 +533,19 @@ unsafe extern "C" fn napi_register_module_v1(
   });
 
   #[cfg(all(windows, feature = "napi4", feature = "tokio_rt"))]
+  #[cfg(all(feature = "napi4", feature = "tokio_rt"))]
   {
     crate::tokio_runtime::ensure_runtime();
 
-    crate::tokio_runtime::RT_REFERENCE_COUNT.fetch_add(1, Ordering::SeqCst);
+    static init_counter: AtomicUsize = AtomicUsize::new(0);
+    let cleanup_hook_payload =
+      init_counter.fetch_add(1, Ordering::Relaxed) as *mut std::ffi::c_void;
+
     unsafe {
       sys::napi_add_env_cleanup_hook(
         env,
         Some(crate::tokio_runtime::drop_runtime),
-        ptr::null_mut(),
+        cleanup_hook_payload,
       )
     };
   }

crates/napi/src/tokio_runtime.rs

@@ -24,8 +24,7 @@ fn create_runtime() -> Option<Runtime> {
 pub(crate) static RT: Lazy<RwLock<Option<Runtime>>> = Lazy::new(|| RwLock::new(create_runtime()));
 
 #[cfg(windows)]
-pub(crate) static RT_REFERENCE_COUNT: std::sync::atomic::AtomicUsize =
-  std::sync::atomic::AtomicUsize::new(0);
+static RT_REFERENCE_COUNT: std::sync::atomic::AtomicUsize = std::sync::atomic::AtomicUsize::new(0);
 
 /// Ensure that the Tokio runtime is initialized.
 /// In windows the Tokio runtime will be dropped when Node env exits.
@@ -33,24 +32,23 @@ pub(crate) static RT_REFERENCE_COUNT: std::sync::atomic::AtomicUsize =
 /// So we need to ensure that the Tokio runtime is initialized when the Node env is created.
 #[cfg(windows)]
 pub(crate) fn ensure_runtime() {
+  use std::sync::atomic::Ordering;
+
   let mut rt = RT.write().unwrap();
   if rt.is_none() {
     *rt = create_runtime();
   }
+
+  RT_REFERENCE_COUNT.fetch_add(1, Ordering::Relaxed);
 }
 
 #[cfg(windows)]
-pub(crate) unsafe extern "C" fn drop_runtime(arg: *mut std::ffi::c_void) {
+pub(crate) unsafe extern "C" fn drop_runtime(_arg: *mut std::ffi::c_void) {
   use std::sync::atomic::Ordering;
 
-  if RT_REFERENCE_COUNT.fetch_sub(1, Ordering::SeqCst) == 1 {
+  if RT_REFERENCE_COUNT.fetch_sub(1, Ordering::AcqRel) == 1 {
     RT.write().unwrap().take();
   }
-
-  unsafe {
-    let env: sys::napi_env = arg as *mut sys::napi_env__;
-    sys::napi_remove_env_cleanup_hook(env, Some(drop_runtime), arg);
-  }
 }
 
 /// Spawns a future onto the Tokio runtime.

examples/napi/__tests__/unload.spec.js

@@ -1,5 +1,7 @@
 // use the commonjs syntax to prevent compiler from transpiling the module syntax
 
+const path = require('path')
+
 const test = require('ava').default
 
 test('unload module', (t) => {
@@ -9,3 +11,12 @@ test('unload module', (t) => {
   const { add: add2 } = require('../index.node')
   t.is(add2(1, 2), 3)
 })
+
+test('load module multi times', (t) => {
+  const { add } = require('../index.node')
+  t.is(add(1, 2), 3)
+  const { add: add2 } = require(path.toNamespacedPath(
+    path.join(__dirname, '../index.node'),
+  ))
+  t.is(add2(1, 2), 3)
+})

examples/napi/__tests__/values.spec.ts

@@ -389,6 +389,7 @@ test('return Undefined', (t) => {
 test('pass symbol in', (t) => {
   const sym = Symbol('test')
   const obj = setSymbolInObj(sym)
+  // @ts-expect-error
   t.is(obj[sym], 'a symbol')
 })
 
@@ -415,6 +416,7 @@ test('custom status code in Error', (t) => {
 })
 
 test('function ts type override', (t) => {
+  // @ts-expect-error
   t.deepEqual(tsRename({ foo: 1, bar: 2, baz: 2 }), ['foo', 'bar', 'baz'])
 })
 

examples/napi/package.json

@@ -18,13 +18,15 @@
   "ava": {
     "extensions": [
       "ts",
-      "tsx"
+      "tsx",
+      "js"
     ],
     "require": [
       "ts-node/register/transpile-only"
     ],
     "files": [
-      "__tests__/**/*.spec.ts"
+      "__tests__/**/*.spec.ts",
+      "__tests__/**/*.spec.js"
     ],
     "environmentVariables": {
       "TS_NODE_PROJECT": "../tsconfig.json"

tsconfig.json

@@ -13,8 +13,6 @@
     "noUnusedParameters": true,
     "strict": true,
     "skipLibCheck": true,
-    "suppressImplicitAnyIndexErrors": true,
-    "suppressExcessPropertyErrors": true,
     "forceConsistentCasingInFileNames": true,
     "preserveSymlinks": true,
     "target": "ES2022",