summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorNathan Whitaker <17734409+nathanwhit@users.noreply.github.com>2024-06-13 15:31:42 -0700
committerGitHub <noreply@github.com>2024-06-13 22:31:42 +0000
commit368eb9073bff776b8bb49480b98ca4628ebdc7cd (patch)
tree50e76947b934312d0ebf6a6e95d8e2a48cc10f3b
parent4ec9250c409fc0734e192d6571b0cad3cbc8a7ee (diff)
fix(napi): Read reference ownership before calling finalizer to avoid crash (#24203)
Fixes #23493. What was happening here was that napi-rs was freeing the napi reference ([here](https://github.com/napi-rs/napi-rs/blob/19e3488efcbc601afa1f11a979372eb6c5ea6130/crates/napi/src/bindgen_runtime/mod.rs#L62)) during its finalize callback (which we call [here](https://github.com/denoland/deno/blob/fb31eaa9ca59f6daaee0210d5cd206185c7041b9/cli/napi/js_native_api.rs#L132)). We then were [reading the `ownership` field](https://github.com/denoland/deno/blob/fb31eaa9ca59f6daaee0210d5cd206185c7041b9/cli/napi/js_native_api.rs#L136) of that freed reference. For some reason on arm macs the freed memory gets zeroed, so the value of `ownership` was `0` when we read it (i.e. it was `ReferenceOwnership::Runtime`). We then freed it again (since we thought we owned it), causing the segfault.
-rw-r--r--cli/napi/js_native_api.rs6
-rw-r--r--tests/napi/object_wrap_test.js8
-rw-r--r--tests/napi/src/object_wrap.rs79
-rw-r--r--tests/napi/tests/napi_tests.rs1
4 files changed, 87 insertions, 7 deletions
diff --git a/cli/napi/js_native_api.rs b/cli/napi/js_native_api.rs
index fad13ba62..fe6535446 100644
--- a/cli/napi/js_native_api.rs
+++ b/cli/napi/js_native_api.rs
@@ -127,13 +127,16 @@ impl Reference {
let finalize_hint = reference.finalize_hint;
reference.reset();
+ // copy this value before the finalize callback, since
+ // it might free the reference (which would be a UAF)
+ let ownership = reference.ownership;
if let Some(finalize_cb) = finalize_cb {
unsafe {
finalize_cb(reference.env as _, finalize_data, finalize_hint);
}
}
- if reference.ownership == ReferenceOwnership::Runtime {
+ if ownership == ReferenceOwnership::Runtime {
unsafe { drop(Reference::from_raw(reference)) }
}
}
@@ -3440,7 +3443,6 @@ fn napi_add_finalizer(
} else {
ReferenceOwnership::Userland
};
-
let reference = Reference::new(
env,
value.into(),
diff --git a/tests/napi/object_wrap_test.js b/tests/napi/object_wrap_test.js
index de6391fb1..ee6d4af86 100644
--- a/tests/napi/object_wrap_test.js
+++ b/tests/napi/object_wrap_test.js
@@ -40,3 +40,11 @@ Deno.test("napi external arraybuffer", function () {
assertEquals(new Uint8Array(buf), new Uint8Array([1, 2, 3]));
buf = null;
});
+
+Deno.test("napi object wrap userland owned", function () {
+ let obj = new objectWrap.NapiObjectOwned(1);
+ assertEquals(obj.get_value(), 1);
+ obj = null;
+ // force finalize callback to get called
+ globalThis.gc();
+});
diff --git a/tests/napi/src/object_wrap.rs b/tests/napi/src/object_wrap.rs
index 8c29caec5..63e9e2e23 100644
--- a/tests/napi/src/object_wrap.rs
+++ b/tests/napi/src/object_wrap.rs
@@ -5,6 +5,8 @@ use crate::napi_get_callback_info;
use crate::napi_new_property;
use napi_sys::ValueType::napi_number;
use napi_sys::*;
+use std::cell::RefCell;
+use std::collections::HashMap;
use std::os::raw::c_char;
use std::os::raw::c_void;
use std::ptr;
@@ -13,9 +15,36 @@ pub struct NapiObject {
counter: i32,
}
+thread_local! {
+ // map from native object ptr to napi reference (this is similar to what napi-rs does)
+ static REFS: RefCell<HashMap<*mut c_void, napi_ref>> = RefCell::new(HashMap::new());
+}
+
+pub extern "C" fn finalize_napi_object(
+ env: napi_env,
+ finalize_data: *mut c_void,
+ _finalize_hint: *mut c_void,
+) {
+ let obj = unsafe { Box::from_raw(finalize_data as *mut NapiObject) };
+ drop(obj);
+ if let Some(reference) =
+ REFS.with_borrow_mut(|map| map.remove(&finalize_data))
+ {
+ unsafe { napi_delete_reference(env, reference) };
+ }
+}
+
impl NapiObject {
- #[allow(clippy::new_ret_no_self)]
- pub extern "C" fn new(env: napi_env, info: napi_callback_info) -> napi_value {
+ fn new_inner(
+ env: napi_env,
+ info: napi_callback_info,
+ finalizer: napi_finalize,
+ out_ptr: Option<*mut napi_ref>,
+ ) -> napi_value {
+ assert!(matches!(
+ (finalizer, out_ptr),
+ (None, None) | (Some(_), Some(_))
+ ));
let mut new_target: napi_value = ptr::null_mut();
assert_napi_ok!(napi_get_new_target(env, info, &mut new_target));
let is_constructor = !new_target.is_null();
@@ -33,21 +62,42 @@ impl NapiObject {
assert_napi_ok!(napi_get_value_int32(env, args[0], &mut value));
let obj = Box::new(Self { counter: value });
+ let obj_raw = Box::into_raw(obj) as *mut c_void;
assert_napi_ok!(napi_wrap(
env,
this,
- Box::into_raw(obj) as *mut c_void,
- None,
- ptr::null_mut(),
+ obj_raw,
+ finalizer,
ptr::null_mut(),
+ out_ptr.unwrap_or(ptr::null_mut())
));
+ if let Some(p) = out_ptr {
+ if finalizer.is_some() {
+ REFS.with_borrow_mut(|map| map.insert(obj_raw, unsafe { p.read() }));
+ }
+ }
+
return this;
}
unreachable!();
}
+ #[allow(clippy::new_ret_no_self)]
+ pub extern "C" fn new(env: napi_env, info: napi_callback_info) -> napi_value {
+ Self::new_inner(env, info, None, None)
+ }
+
+ #[allow(clippy::new_ret_no_self)]
+ pub extern "C" fn new_with_finalizer(
+ env: napi_env,
+ info: napi_callback_info,
+ ) -> napi_value {
+ let mut out = ptr::null_mut();
+ Self::new_inner(env, info, Some(finalize_napi_object), Some(&mut out))
+ }
+
pub extern "C" fn set_value(
env: napi_env,
info: napi_callback_info,
@@ -148,4 +198,23 @@ pub fn init(env: napi_env, exports: napi_value) {
"NapiObject\0".as_ptr() as *const c_char,
cons,
));
+
+ let mut cons: napi_value = ptr::null_mut();
+ assert_napi_ok!(napi_define_class(
+ env,
+ c"NapiObjectOwned".as_ptr(),
+ usize::MAX,
+ Some(NapiObject::new_with_finalizer),
+ ptr::null_mut(),
+ properties.len(),
+ properties.as_ptr(),
+ &mut cons,
+ ));
+
+ assert_napi_ok!(napi_set_named_property(
+ env,
+ exports,
+ "NapiObjectOwned\0".as_ptr() as *const c_char,
+ cons,
+ ));
}
diff --git a/tests/napi/tests/napi_tests.rs b/tests/napi/tests/napi_tests.rs
index 1c9b1ba94..53d4258f9 100644
--- a/tests/napi/tests/napi_tests.rs
+++ b/tests/napi/tests/napi_tests.rs
@@ -69,6 +69,7 @@ fn napi_tests() {
.arg("--allow-env")
.arg("--allow-ffi")
.arg("--allow-run")
+ .arg("--v8-flags=--expose-gc")
.arg("--config")
.arg(deno_config_path())
.arg("--no-lock")