From 5aa61c214255cc49eb4f5b1cadc02dd15a946b89 Mon Sep 17 00:00:00 2001 From: LongYinan Date: Tue, 26 Apr 2022 15:14:37 +0800 Subject: [PATCH] fix(napi): use create_buffer/arrary_buffer if provided data is empty --- crates/backend/src/typegen/fn.rs | 2 +- .../bindgen_runtime/js_values/arraybuffer.rs | 34 ++++---- .../src/bindgen_runtime/js_values/buffer.rs | 32 +++---- crates/napi/src/env.rs | 83 ++++++++++--------- .../napi-compat-mode/__test__/buffer.spec.ts | 8 +- examples/napi/__test__/values.spec.ts | 10 +++ 6 files changed, 93 insertions(+), 76 deletions(-) diff --git a/crates/backend/src/typegen/fn.rs b/crates/backend/src/typegen/fn.rs index 91f65407..3cae752c 100644 --- a/crates/backend/src/typegen/fn.rs +++ b/crates/backend/src/typegen/fn.rs @@ -66,7 +66,7 @@ impl ToTypeDef for NapiFn { .ts_generic_types .as_ref() .map(|g| format!("<{}>", g)) - .unwrap_or("".to_string()), + .unwrap_or_else(|| "".to_string()), args = self .ts_args_type .clone() diff --git a/crates/napi/src/bindgen_runtime/js_values/arraybuffer.rs b/crates/napi/src/bindgen_runtime/js_values/arraybuffer.rs index 99bdfe54..af1bc3ba 100644 --- a/crates/napi/src/bindgen_runtime/js_values/arraybuffer.rs +++ b/crates/napi/src/bindgen_runtime/js_values/arraybuffer.rs @@ -177,22 +177,24 @@ macro_rules! impl_typed_array { val.finalizer_notify, ))); check_status!( - unsafe { - sys::napi_create_external_arraybuffer( - env, - if length == 0 { - // Rust uses 0x1 as the data pointer for empty buffers, - // but NAPI/V8 only allows multiple buffers to have - // the same data pointer if it's 0x0. - ptr::null_mut() - } else { - val.data as *mut c_void - }, - length, - Some(finalizer::<$rust_type>), - hint_ptr as *mut c_void, - &mut arraybuffer_value, - ) + if length == 0 { + // Rust uses 0x1 as the data pointer for empty buffers, + // but NAPI/V8 only allows multiple buffers to have + // the same data pointer if it's 0x0. + unsafe { + sys::napi_create_arraybuffer(env, length, ptr::null_mut(), &mut arraybuffer_value) + } + } else { + unsafe { + sys::napi_create_external_arraybuffer( + env, + val.data as *mut c_void, + length, + Some(finalizer::<$rust_type>), + hint_ptr as *mut c_void, + &mut arraybuffer_value, + ) + } }, "Create external arraybuffer failed" )?; diff --git a/crates/napi/src/bindgen_runtime/js_values/buffer.rs b/crates/napi/src/bindgen_runtime/js_values/buffer.rs index a3135890..15c2c916 100644 --- a/crates/napi/src/bindgen_runtime/js_values/buffer.rs +++ b/crates/napi/src/bindgen_runtime/js_values/buffer.rs @@ -172,22 +172,22 @@ impl ToNapiValue for Buffer { let len = val.inner.len(); let mut ret = ptr::null_mut(); check_status!( - unsafe { - sys::napi_create_external_buffer( - env, - len, - if len == 0 { - // Rust uses 0x1 as the data pointer for empty buffers, - // but NAPI/V8 only allows multiple buffers to have - // the same data pointer if it's 0x0. - ptr::null_mut() - } else { - val.inner.as_mut_ptr() as *mut _ - }, - Some(drop_buffer), - Box::into_raw(Box::new((len, val.capacity))) as *mut _, - &mut ret, - ) + if len == 0 { + // Rust uses 0x1 as the data pointer for empty buffers, + // but NAPI/V8 only allows multiple buffers to have + // the same data pointer if it's 0x0. + unsafe { sys::napi_create_buffer(env, len, ptr::null_mut(), &mut ret) } + } else { + unsafe { + sys::napi_create_external_buffer( + env, + len, + val.inner.as_mut_ptr() as *mut _, + Some(drop_buffer), + Box::into_raw(Box::new((len, val.capacity))) as *mut _, + &mut ret, + ) + } }, "Failed to create napi buffer" )?; diff --git a/crates/napi/src/env.rs b/crates/napi/src/env.rs index 97c552e7..1c8a38cb 100644 --- a/crates/napi/src/env.rs +++ b/crates/napi/src/env.rs @@ -35,6 +35,8 @@ use serde::Serialize; pub type Callback = unsafe extern "C" fn(sys::napi_env, sys::napi_callback_info) -> sys::napi_value; +pub(crate) static EMPTY_VEC: Vec = vec![]; + #[derive(Clone, Copy)] /// `Env` is used to represent a context that the underlying N-API implementation can use to persist VM-specific state. /// @@ -261,21 +263,21 @@ impl Env { let mut raw_value = ptr::null_mut(); let data_ptr = data.as_mut_ptr(); check_status!(unsafe { - sys::napi_create_external_buffer( - self.0, - length, - if length == 0 { - // Rust uses 0x1 as the data pointer for empty buffers, - // but NAPI/V8 only allows multiple buffers to have - // the same data pointer if it's 0x0. - ptr::null_mut() - } else { - data_ptr as *mut c_void - }, - Some(drop_buffer), - Box::into_raw(Box::new((length, data.capacity()))) as *mut c_void, - &mut raw_value, - ) + if length == 0 { + // Rust uses 0x1 as the data pointer for empty buffers, + // but NAPI/V8 only allows multiple buffers to have + // the same data pointer if it's 0x0. + sys::napi_create_buffer(self.0, length, ptr::null_mut(), &mut raw_value) + } else { + sys::napi_create_external_buffer( + self.0, + length, + data_ptr as *mut c_void, + Some(drop_buffer), + Box::into_raw(Box::new((length, data.capacity()))) as *mut c_void, + &mut raw_value, + ) + } })?; Ok(JsBufferValue::new( JsBuffer(Value { @@ -304,18 +306,17 @@ impl Env { Finalize: FnOnce(Hint, Env), { let mut raw_value = ptr::null_mut(); + if data.is_null() || data == EMPTY_VEC.as_ptr() { + return Err(Error::new( + Status::InvalidArg, + "Borrowed data should not be null".to_owned(), + )); + } check_status!(unsafe { sys::napi_create_external_buffer( self.0, length, - if length == 0 { - // Rust uses 0x1 as the data pointer for empty buffers, - // but NAPI/V8 only allows multiple buffers to have - // the same data pointer if it's 0x0. - ptr::null_mut() - } else { - data as *mut c_void - }, + data as *mut c_void, Some( raw_finalize_with_custom_callback:: as unsafe extern "C" fn( @@ -393,26 +394,26 @@ impl Env { )) } - pub fn create_arraybuffer_with_data(&self, data: Vec) -> Result { + pub fn create_arraybuffer_with_data(&self, mut data: Vec) -> Result { let length = data.len(); let mut raw_value = ptr::null_mut(); - let data_ptr = data.as_ptr(); + let data_ptr = data.as_mut_ptr(); check_status!(unsafe { - sys::napi_create_external_arraybuffer( - self.0, - if length == 0 { - // Rust uses 0x1 as the data pointer for empty buffers, - // but NAPI/V8 only allows multiple buffers to have - // the same data pointer if it's 0x0. - ptr::null_mut() - } else { - data_ptr as *mut c_void - }, - length, - Some(drop_buffer), - Box::into_raw(Box::new((length, data.capacity()))) as *mut c_void, - &mut raw_value, - ) + if length == 0 { + // Rust uses 0x1 as the data pointer for empty buffers, + // but NAPI/V8 only allows multiple buffers to have + // the same data pointer if it's 0x0. + sys::napi_create_arraybuffer(self.0, length, ptr::null_mut(), &mut raw_value) + } else { + sys::napi_create_external_arraybuffer( + self.0, + data_ptr as *mut c_void, + length, + Some(drop_buffer), + Box::into_raw(Box::new((length, data.capacity()))) as *mut c_void, + &mut raw_value, + ) + } })?; mem::forget(data); @@ -609,7 +610,7 @@ impl Env { // `&'static dyn Fn…` in Rust parlance, in that thanks to `Box::into_raw()` // we are sure the context won't be freed, and thus the callback may use // it to call the actual method thanks to the trampoline… - // But we thus have a data leak: there is nothing yet reponsible for + // But we thus have a data leak: there is nothing yet responsible for // running the `drop(Box::from_raw(…))` cleanup code. // // To solve that, according to the docs, we need to attach a finalizer: diff --git a/examples/napi-compat-mode/__test__/buffer.spec.ts b/examples/napi-compat-mode/__test__/buffer.spec.ts index 506ba4c8..a286c228 100644 --- a/examples/napi-compat-mode/__test__/buffer.spec.ts +++ b/examples/napi-compat-mode/__test__/buffer.spec.ts @@ -34,8 +34,12 @@ test('should create borrowed buffer with finalize', (t) => { }) test('should create empty borrowed buffer with finalize', (t) => { - t.is(bindings.createEmptyBorrowedBufferWithFinalize().toString(), '') - t.is(bindings.createEmptyBorrowedBufferWithFinalize().toString(), '') + t.throws(() => bindings.createEmptyBorrowedBufferWithFinalize().toString(), { + message: 'Borrowed data should not be null', + }) + t.throws(() => bindings.createEmptyBorrowedBufferWithFinalize().toString(), { + message: 'Borrowed data should not be null', + }) }) test('should create empty buffer', (t) => { diff --git a/examples/napi/__test__/values.spec.ts b/examples/napi/__test__/values.spec.ts index 98c5aa51..cac38f37 100644 --- a/examples/napi/__test__/values.spec.ts +++ b/examples/napi/__test__/values.spec.ts @@ -377,6 +377,16 @@ test('buffer', (t) => { t.is(b.toString(), '') }) +test('reset empty buffer', (t) => { + const empty = getEmptyBuffer() + + const shared = new ArrayBuffer(0) + const buffer = Buffer.from(shared) + t.notThrows(() => { + buffer.set(empty) + }) +}) + test('convert typedarray to vec', (t) => { const input = new Uint32Array([1, 2, 3, 4, 5]) t.deepEqual(convertU32Array(input), Array.from(input))