From 368eb9073bff776b8bb49480b98ca4628ebdc7cd Mon Sep 17 00:00:00 2001 From: Nathan Whitaker <17734409+nathanwhit@users.noreply.github.com> Date: Thu, 13 Jun 2024 15:31:42 -0700 Subject: [PATCH] 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. --- cli/napi/js_native_api.rs | 6 ++- tests/napi/object_wrap_test.js | 8 ++++ tests/napi/src/object_wrap.rs | 79 +++++++++++++++++++++++++++++++--- tests/napi/tests/napi_tests.rs | 1 + 4 files changed, 87 insertions(+), 7 deletions(-) diff --git a/cli/napi/js_native_api.rs b/cli/napi/js_native_api.rs index fad13ba62e..fe6535446f 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 de6391fb1c..ee6d4af86b 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 8c29caec53..63e9e2e232 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> = 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 1c9b1ba94d..53d4258f93 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")