Skip to content

Commit

Permalink
fix: misalignment crash (#259)
Browse files Browse the repository at this point in the history
resolves #255 

The underlying issue was that the reference to wrap was removed just after the constructor was called.  The code was there probably due to a workaround bug in the previous version of node. 

May need to clean up reference to wrap when finalizer is invoked
  • Loading branch information
sehz committed Sep 26, 2023
1 parent 4088fe6 commit deee8c4
Showing 12 changed files with 61 additions and 59 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
@@ -60,7 +60,7 @@ jobs:
fail-fast: false
matrix:
# os: [ubuntu-latest, windows-latest, macOS-latest]
os: [ubuntu-latest, macOS-latest]
os: [ubuntu-latest, macOS-13]
rust: [stable]
node-version: [ '16', '18', '20' ]
steps:
2 changes: 1 addition & 1 deletion examples/async-cb/src/lib.rs
Original file line number Diff line number Diff line change
@@ -16,4 +16,4 @@ async fn hello<F: Fn(f64, String)>(seconds: i32, cb: F) {
// println!("woke from time");

cb(10.0, "hello world".to_string());
}
}
1 change: 1 addition & 0 deletions examples/class-simple/test.js
Original file line number Diff line number Diff line change
@@ -7,6 +7,7 @@ let obj = new addon.MyObject(10,2);
assert.equal(obj.value,10,"verify value works");
assert.equal(obj.plusOne(),11);
assert.equal(obj.value2,2);
//console.print("first");

obj.changeValue(100);
assert.equal(obj.value,100);
1 change: 0 additions & 1 deletion examples/js-env/src/lib.rs
Original file line number Diff line number Diff line change
@@ -21,4 +21,3 @@ impl TryIntoJs for EnvInterceptor {
js_env.create_double(self.0 * 2.0)
}
}

28 changes: 11 additions & 17 deletions examples/json/src/lib.rs
Original file line number Diff line number Diff line change
@@ -12,12 +12,12 @@ use serde_json::map::Map;
#[node_bindgen]
struct StandardJson {
some_name: String,
a_number: i64
a_number: i64,
}

#[node_bindgen]
struct Outer {
val: Inner
val: Inner,
}

#[node_bindgen]
@@ -29,19 +29,17 @@ struct UnitStruct;
#[node_bindgen]
enum ErrorType {
WithMessage(String, usize),
WithFields {
val: usize
},
UnitError
WithFields { val: usize },
UnitError,
}

#[node_bindgen]
struct WithSerdeJson {
val: Value
val: Value,
}

struct CustomJson {
val: f64
val: f64,
}

impl TryIntoJs for CustomJson {
@@ -67,14 +65,14 @@ fn custom_json() -> CustomJson {
fn standard_json() -> StandardJson {
StandardJson {
some_name: "John".to_owned(),
a_number: 1337
a_number: 1337,
}
}

#[node_bindgen]
fn multilevel_json() -> Outer {
Outer {
val: Inner("hello".to_owned())
val: Inner("hello".to_owned()),
}
}

@@ -90,9 +88,7 @@ fn with_message() -> ErrorType {

#[node_bindgen]
fn with_fields() -> ErrorType {
ErrorType::WithFields {
val: 123
}
ErrorType::WithFields { val: 123 }
}

#[node_bindgen]
@@ -102,9 +98,7 @@ fn with_unit() -> ErrorType {

#[node_bindgen]
fn failed_result_with_fields() -> Result<(), ErrorType> {
Err(ErrorType::WithFields {
val: 987
})
Err(ErrorType::WithFields { val: 987 })
}

#[node_bindgen]
@@ -119,6 +113,6 @@ fn with_serde_json() -> WithSerdeJson {
map.insert("second".to_owned(), Value::String("hello".to_owned()));

WithSerdeJson {
val: Value::Object(map)
val: Value::Object(map),
}
}
34 changes: 24 additions & 10 deletions nj-core/src/basic.rs
Original file line number Diff line number Diff line change
@@ -264,6 +264,7 @@ impl JsEnv {

/// get callback information
#[allow(clippy::not_unsafe_ptr_arg_deref)]
#[instrument]
pub fn get_cb_info(
&self,
info: napi_callback_info,
@@ -284,27 +285,24 @@ impl JsEnv {
ptr::null_mut()
))?;

trace!(argc, "actual argc");
// truncate arg to actual received count
args.resize(argc, ptr::null_mut());

Ok(JsCallback::new(JsEnv::new(self.0), this, args))
}

/// define classes
#[instrument(skip(properties))]
pub fn define_class(
&self,
name: &str,
constructor: napi_callback_raw,
properties: PropertiesBuilder,
) -> Result<napi_value, NjError> {
debug!(?properties, "defining class",);
let mut js_constructor = ptr::null_mut();
let mut raw_properties = properties.as_raw_properties();

debug!(
"defining class: {} with {} properties",
name,
raw_properties.len()
);
napi_call_result!(crate::sys::napi_define_class(
self.0,
name.as_ptr() as *const ::std::os::raw::c_char,
@@ -340,14 +338,16 @@ impl JsEnv {
}

#[allow(clippy::not_unsafe_ptr_arg_deref)]
#[instrument]
pub fn get_new_target(&self, info: napi_callback_info) -> Result<napi_value, NjError> {
let mut result = ptr::null_mut();
napi_call_result!(crate::sys::napi_get_new_target(self.0, info, &mut result))?;

debug!(?result, "got new target");
Ok(result)
}

#[allow(clippy::not_unsafe_ptr_arg_deref)]
#[instrument]
pub fn wrap(
&self,
js_object: napi_value,
@@ -356,6 +356,7 @@ impl JsEnv {
) -> Result<napi_ref, NjError> {
let mut result = ptr::null_mut();

debug!("napi wrap");
napi_call_result!(crate::sys::napi_wrap(
self.0,
js_object,
@@ -369,23 +370,34 @@ impl JsEnv {
}

#[allow(clippy::not_unsafe_ptr_arg_deref)]
#[instrument]
pub fn unwrap<T>(&self, js_this: napi_value) -> Result<&'static T, NjError> {
let mut result: *mut ::std::os::raw::c_void = ptr::null_mut();
napi_call_result!(crate::sys::napi_unwrap(self.0, js_this, &mut result))?;

Ok(unsafe {
debug!(?result, "got back raw pointer");
if result.is_null() {
return Err(NjError::Other("unwrap got null pointer".to_string()));
}
let rust_ref: &T = &mut *(result as *mut T);
rust_ref
})
}

#[allow(clippy::not_unsafe_ptr_arg_deref)]
#[instrument]
pub fn unwrap_mut<T>(&self, js_this: napi_value) -> Result<&'static mut T, NjError> {
let mut result: *mut ::std::os::raw::c_void = ptr::null_mut();
debug!(env = ?self.0,"napi unwrap");
napi_call_result!(crate::sys::napi_unwrap(self.0, js_this, &mut result))?;

Ok(unsafe {
let rust_ref: &mut T = &mut *(result as *mut T);
debug!(?result, "got back raw pointer");
if result.is_null() {
return Err(NjError::Other("unwrap mut null pointer".to_string()));
}
let ptr = result as *mut T;
let rust_ref: &mut T = &mut *(ptr);
rust_ref
})
}
@@ -396,7 +408,7 @@ impl JsEnv {
constructor: napi_value,
mut args: Vec<napi_value>,
) -> Result<napi_value, NjError> {
trace!("napi new instance: {}", args.len());
trace!(args = args.len(), "napi new instance");
let mut result = ptr::null_mut();
napi_call_result!(crate::sys::napi_new_instance(
self.0,
@@ -760,6 +772,7 @@ impl JsCallback {
}
}

#[instrument]
pub fn unwrap_mut<T>(&self) -> Result<&'static mut T, NjError> {
Ok(self
.env
@@ -886,6 +899,7 @@ impl ExtractArgFromJs<'_> for JsEnv {
}
}

#[derive(Debug)]
pub struct JsExports {
inner: napi_value,
env: JsEnv,
41 changes: 15 additions & 26 deletions nj-core/src/class.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use std::ptr;

use tracing::debug;
use tracing::instrument;

use crate::sys::napi_value;
use crate::sys::napi_env;
@@ -34,10 +35,11 @@ where
{
/// wrap myself in the JS instance
/// and saved the reference
#[instrument(skip(self))]
fn wrap(self, js_env: &JsEnv, js_cb: JsCallback) -> Result<napi_value, NjError> {
let boxed_self = Box::new(self);
let raw_ptr = Box::into_raw(boxed_self); // rust no longer manages this struct

debug!(?raw_ptr, "box into raw");
let wrap = js_env.wrap(js_cb.this(), raw_ptr as *mut u8, T::js_finalize)?;

unsafe {
@@ -46,15 +48,6 @@ where
rust_ref.wrapper = wrap;
}

// Finally, remove the reference in response to finalize callback
// See footnote on `napi_wrap` documentation: https://nodejs.org/api/n-api.html#n_api_napi_wrap
//
// "Caution: The optional returned reference (if obtained) should be deleted via napi_delete_reference
// ONLY in response to the finalize callback invocation. If it is deleted before then,
// then the finalize callback may never be invoked. Therefore, when obtaining a reference a
// finalize callback is also required in order to enable correct disposal of the reference."
js_env.delete_reference(wrap)?;

Ok(js_cb.this_owned())
}
}
@@ -73,13 +66,15 @@ pub trait JSClass: Sized {
fn get_constructor() -> napi_ref;

/// new instance
#[instrument]
fn new_instance(js_env: &JsEnv, js_args: Vec<napi_value>) -> Result<napi_value, NjError> {
debug!("new instance with args: {:#?}", js_args);
debug!("new instance");
let constructor = js_env.get_reference_value(Self::get_constructor())?;
js_env.new_instance(constructor, js_args)
}

/// given instance, return my object
#[instrument]
fn unwrap_mut(js_env: &JsEnv, instance: napi_value) -> Result<&'static mut Self, NjError> {
Ok(js_env
.unwrap_mut::<JSObjectWrapper<Self>>(instance)?
@@ -95,14 +90,18 @@ pub trait JSClass: Sized {
}

/// define class and properties under exports
#[instrument]
fn js_init(js_exports: &mut JsExports) -> Result<(), NjError> {
let js_constructor =
js_exports
.env()
.define_class(Self::CLASS_NAME, Self::js_new, Self::properties())?;

debug!(?js_constructor, "class defined with constructor");

// save the constructor reference, we need this later in order to instantiate
let js_ref = js_exports.env().create_reference(js_constructor, 1)?;
debug!(?js_constructor, "created reference to constructor");
Self::set_constructor(js_ref);

js_exports.set_name_property(Self::CLASS_NAME, js_constructor)?;
@@ -111,23 +110,23 @@ pub trait JSClass: Sized {

/// call when Javascript class constructor is called
/// For example: new Car(...)
#[instrument]
extern "C" fn js_new(env: napi_env, info: napi_callback_info) -> napi_value {
let js_env = JsEnv::new(env);

let result: Result<napi_value, NjError> = (|| {
debug!(
"Class constructor called: {:#?}",
std::any::type_name::<Self>()
);
debug!(clas = std::any::type_name::<Self>(), "getting new target");

let target = js_env.get_new_target(info)?;

if target.is_null() {
debug!("no target");
Err(NjError::NoPlainConstructor)
} else {
debug!("invoked as constructor");
debug!(?target, "invoked as constructor");

let (rust_obj, js_cb) = Self::create_from_js(&js_env, info)?;
debug!(?js_cb, "created rust object");
let my_obj = JSObjectWrapper {
inner: rust_obj,
wrapper: ptr::null_mut(),
@@ -140,16 +139,6 @@ pub trait JSClass: Sized {
result.into_js(&js_env)
}

/*
/// convert my self as JS object
fn as_js_instance(self,js_env: &JsEnv,js_args: Vec<napi_value>) -> napi_value {
let new_instance = Self::new_instance(js_env,args);
// unwrap the actual inner
}
*/

extern "C" fn js_finalize(
_env: napi_env,
finalize_data: *mut ::std::os::raw::c_void,
2 changes: 1 addition & 1 deletion nj-core/src/lib.rs
Original file line number Diff line number Diff line change
@@ -41,7 +41,7 @@ pub mod val {
}

pub mod log {
pub use ::tracing::*;
pub use tracing::*;
}

/// call napi and assert
2 changes: 1 addition & 1 deletion nj-core/src/property.rs
Original file line number Diff line number Diff line change
@@ -53,7 +53,7 @@ impl Property {
}
}

#[derive(Default)]
#[derive(Default, Debug)]
pub struct PropertiesBuilder(Vec<Property>);

impl PropertiesBuilder {
3 changes: 2 additions & 1 deletion nj-derive/src/generator/class/mod.rs
Original file line number Diff line number Diff line change
@@ -68,8 +68,8 @@ fn generate_class_helper(class: Class) -> TokenStream {
impl node_bindgen::core::JSClass for #impl_for_block {
const CLASS_NAME: &'static str = #class_type_lit;


fn set_constructor(constructor: node_bindgen::sys::napi_ref) {
node_bindgen::core::log::trace!("set constructor");
unsafe {
CLASS_CONSTRUCTOR = constructor;
}
@@ -97,6 +97,7 @@ fn generate_class_helper(class: Class) -> TokenStream {

#[node_bindgen::core::ctor]
fn register_class() {
node_bindgen::core::log::debug!(class = stringify!(#type_name),"registering class");
submit_register_callback(#type_name::js_init);
}
}
Loading

0 comments on commit deee8c4

Please sign in to comment.