From 2d1e4144b315894164c8316a5fcfa2bc887a19d0 Mon Sep 17 00:00:00 2001 From: Alexey Orlenko Date: Sat, 8 Apr 2023 17:08:48 +0200 Subject: [PATCH] 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 --- .../napi/src/bindgen_runtime/module_register.rs | 11 +++++++---- crates/napi/src/tokio_runtime.rs | 16 +++++++--------- examples/napi/__test__/typegen.spec.ts.snap | Bin 3945 -> 0 bytes examples/napi/__tests__/unload.spec.js | 11 +++++++++++ examples/napi/__tests__/values.spec.ts | 2 ++ examples/napi/package.json | 6 ++++-- tsconfig.json | 2 -- 7 files changed, 31 insertions(+), 17 deletions(-) delete mode 100644 examples/napi/__test__/typegen.spec.ts.snap diff --git a/crates/napi/src/bindgen_runtime/module_register.rs b/crates/napi/src/bindgen_runtime/module_register.rs index 4788611a..56f1fbcd 100644 --- a/crates/napi/src/bindgen_runtime/module_register.rs +++ b/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, ) }; } diff --git a/crates/napi/src/tokio_runtime.rs b/crates/napi/src/tokio_runtime.rs index 1b769b5d..5aaa97b7 100644 --- a/crates/napi/src/tokio_runtime.rs +++ b/crates/napi/src/tokio_runtime.rs @@ -24,8 +24,7 @@ fn create_runtime() -> Option { pub(crate) static RT: Lazy>> = 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. diff --git a/examples/napi/__test__/typegen.spec.ts.snap b/examples/napi/__test__/typegen.spec.ts.snap deleted file mode 100644 index abcd4363458879ff7b100a57a89ba234541083f0..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 3945 zcmV-v50>yjRzV;yq>kq@lbcFsxs0ejQj zAs6(x5_gxeR?>-5>^+}>-W&Aq{!`q&O5dPQ(nsjbP?V?-?yhs`ov|Kg9o{r8vmZnI2o)Jw+-*X^pV-zO%bGc=LMnx8c$0sO(x$O!COjWE_F6v$t^?D6iLN zgEZxfCn`&2G<`-aUWiOF70*S{U0F3hrpl(Eu zN$U>&0Oyq`@7{M(2?Z0}ro}%kdYr*MFt>qPM822sYz9+Cs)3%zX*`!oJlW1Cldh3+ z>~xTanH-(R9#5x+nbp^&rLmlX2@Hb3o6d?&8^2v2x0Dq=Yi+*FdTFLtj|!T_sjWU zPoJ@mu?zqJf3&*C3I5oPoCq5X-Zr0`SN0<;S#UQpg4OpU%v>0*$>TKRY3Ab60jn8b zb0EZI$!J#cp|`EiFx(^Xa7`($1&clgyt!mg*(Li3{spn0vaJm^0V~Z@q2TS^O(oJm z-1(s5=;nKN)#ZcCYHjOeH%;TzmgO@{b}B%&=%v~#8qjpTyLaybFgP|IE#AE+40vj< zze2zh!Z6NY^vp99`%#i-fJX=;5UHZsVAk+NnpUIJlMMO-p94FJJ%Tz!hdc=?nt=AB zVS(@Y<4q$2;sEFIR$ZqqEmixM-X-^>2!kPRpZl;Y@d!TPjy9HQ&}ZB5Z?*L`oRseW zR%SEmPC%$?7jDNq`9gl|#?hOJ8?09d*ThW1TpdNi%V78Iv$rzHX70@k!V@uy59y;P z^~CN#o4bcUOXDcsp?(K445hkmKiD}o4`h^QqR|t7v?mE$70rl3zfzDfCZgbZ@zS0j z0lE~a-V%<|`uNIaRql8Wk@YItB`DLm?9)JTNbtbH6duNL910$_*w@Tc$5w4_t`R?6 z$q$|YciEuHB+1lD&1vv{1@=;DON~>lo|Sk9dG@&>cO=F(BGSMmU=s(R2T_bq>*H7R zuCbew?-bfGEf_)^CB_qXNLYIKwiOJ>1}1UbZ%uwhw5?qhPnQu53!)#|L|yWd-zmpF z04cZO@6a7q>Au@wXGdwd^Az1H>+>8NQO~uRC3iguCW`;cewebam&ag{BKp1FU9QAIT$7Q5Yf0(8i)^D_inS+4!#aVixIMF7+- z^Lz{0y^eW+-T>N*M`}KSm}G&C^l+=l+P;Z%z{fOq44d2;1D%6XL;`aZfElLnlSSgZ z)Eh7-ZQkwH)-YFDjA_*_zM+^$Bq8Ymk`Ice@Nt$3Y=3|+dvTt^^Fw?-l^5{%2p@Ol zbcQ;7j4y|Jq+e2Jfl=A!{VszBwxf9e!IlTK!XWjar!~f=gCDR2nv12$?FJ(nO#iE(p_Ku15pb|8OBtQw zS<*S`suvftM-K;~7)z8*L5Uwo)p19n9*}9a_z9{ z@*BQ8ER!5kufL3>azd_+k?Z=c=12A0RbFQz$FPKbUwYAM;t;}5aP_amLSZGt%wemD z?<}k=ucyBV<1r^C;L%GwVN+`8B3-Qts?WDoYNcRdXdIZ{z9^2ZnKr!hsd&C5vnpc3 z78a@p`n7p<+J`*=E6N_c-XENjR0ETPY-2>+l?Kv^5drx9ych}C0RdokxMT#A=tPep zC?IePsG)5zQ9wCsZn)j}SwoGb7M*IdyLrlb>DLgKOfisEX>HRMD)~kV!?)p#N7LeH z3oWd0OQN*Ndb63i=jHb$B}4R@uu&u;%@!!fr8zvKm7!hO21<;7L9FT|h_z8QNii_5 zwH}+bPArHc@k^aCLY^0usWiSXt=`to_{#dRbIO_=wI8^wGhJ0Nt`V0NUD_?X-5MTj z9f!HvynlbaxTtng8Ce>`TsHy1U|rwE(!Q17Rz$~|;3a-5L0s`px7cN~!k zm7U@l$q7tn05pk*2jf%$>h(L1A<8b059!GG80<YDGfOGxRO8b8vZao(;y(Is`qZaTtPQbHpF3G8aT! z2Wo>UIi(r`QJ>Pw8u;juupGq$#V1w*S~)651=CftfsU`^X!w`5?#hXn3WL52ob8|eunRh zBSNF3wr#y97z3ChWj?WzU`G$E*W#JNN@&<~AU4U~JlJX# zGmEB`H>&EDW~(sTtKm4R!R7%bI}p==C^ZcdHE>B;ARpH1M9Sea-DFSeSI=N*6YL z2*W{vcxMw&7Ry%{+xI!-G(zcK*^FnbxOfZJC6v!$*_x}j;i9j}zJ2%p^6K8V{hK$- zA?OU?s5-{SJ0|h8;|p#6bOblFEWzBcVXivp0ZqIY#pxWKTa_}zWfc=!FU#ESs@J?x zq%9kdjytkP+bruw(ddtu*8?D{8e(#76%D8nQ#^gA)bc2%Tvzg?8_(MzKNDMHJZBMj zUaQ}?^*bfjr}Ov>3%{7nk^i86#a(7u%c+Zz*bwH{Pr0JR!aEb7(Sk9&_xcU4jO9r{ z=US9tmcEg;icawXrIH<9It4CLJcZ#OJqpwC-w%=t;+SOFGB8P2l)CgOqdMKk0 ze3YkID7u&wLzLw;x1))PLyIk{XX_VjY)CN%Ti~662+>(a@P7h__K#m`{wWgYCp?*< zPAoaJ-5+puHjG2P?n`-!1yO6$-UB^hamfz@W&o z?8(5&iVG-nG|OS{r&$)d= z*+48kCv0+Fj)}4w*v)I)!wKsiY z;nQdv+>;=x{6Vy=<{a;nR=L_t;FTQbVyD}u9lsw^$E>GEO^g>*;Xb8*^%Yr4`ny(5 zb0}tO-zx$&x4%J`+NGf;gj<*O#jgcuUG!Tia_3FO)!p{wn26r#O59zU;r<$@?{QJ@ z7cWoDr0M(8CP`Qgz4a@w3;pUm&{IO#E)B6e_&v}A&eK%XPA4MR*BmC)Al|6a_C8Vi z)@+`LByLScAX9s(r4UQ&Iy2Jaa~BS_m*&{DMUz(_o}QlMp?ED7diJ4G=B;7p)d#`^ zua55>zcQsge!@{#r`aMDr!yfa0t8l7LuZE4z46rgX(26~rkECKQ;qcyB1QQd(whw{ zHDj|C!XSyzOULFy8t7>iFRi|@Dq@!)Td9=dKn=DWA39XbF2 DP9d=( diff --git a/examples/napi/__tests__/unload.spec.js b/examples/napi/__tests__/unload.spec.js index 21a32ca2..2b9266e6 100644 --- a/examples/napi/__tests__/unload.spec.js +++ b/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) +}) diff --git a/examples/napi/__tests__/values.spec.ts b/examples/napi/__tests__/values.spec.ts index 2c373e53..b0dbf3ae 100644 --- a/examples/napi/__tests__/values.spec.ts +++ b/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']) }) diff --git a/examples/napi/package.json b/examples/napi/package.json index 7d15eddc..f13384c9 100644 --- a/examples/napi/package.json +++ b/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" diff --git a/tsconfig.json b/tsconfig.json index d02b0536..664e01ca 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -13,8 +13,6 @@ "noUnusedParameters": true, "strict": true, "skipLibCheck": true, - "suppressImplicitAnyIndexErrors": true, - "suppressExcessPropertyErrors": true, "forceConsistentCasingInFileNames": true, "preserveSymlinks": true, "target": "ES2022",