fix(napi): Read reference ownership before calling finalizer to avoid…

… crash (denoland#24203)

Fixes denoland#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.

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(),

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();
+});

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,
+ ));
 }

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")