diff --git a/crates/napi/src/bindgen_runtime/js_values/buffer.rs b/crates/napi/src/bindgen_runtime/js_values/buffer.rs index 07157955a2..7fc0611905 100644 --- a/crates/napi/src/bindgen_runtime/js_values/buffer.rs +++ b/crates/napi/src/bindgen_runtime/js_values/buffer.rs @@ -3,9 +3,8 @@ use std::collections::HashSet; use std::ffi::c_void; use std::mem; use std::ops::{Deref, DerefMut}; -use std::ptr; +use std::ptr::{self, NonNull}; use std::slice; -use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::Arc; #[cfg(debug_assertions)] use std::sync::Mutex; @@ -22,37 +21,33 @@ thread_local! { /// So it is safe to use it in `async fn`, the `&[u8]` under the hood will not be dropped until the `drop` called. /// Clone will create a new `Reference` to the same underlying `JavaScript Buffer`. pub struct Buffer { - pub(crate) inner: &'static mut [u8], + pub(crate) inner: NonNull, + pub(crate) len: usize, pub(crate) capacity: usize, raw: Option<(sys::napi_ref, sys::napi_env)>, // use it as ref count - pub(crate) drop_in_vm: Arc, + pub(crate) drop_in_vm: Arc<()>, } impl Drop for Buffer { fn drop(&mut self) { + if let Some((ref_, env)) = self.raw { + check_status_or_throw!( + env, + unsafe { sys::napi_reference_unref(env, ref_, &mut 0) }, + "Failed to unref Buffer reference in drop" + ); + return; + } + if Arc::strong_count(&self.drop_in_vm) == 1 { - if let Some((ref_, env)) = self.raw { - check_status_or_throw!( - env, - unsafe { sys::napi_reference_unref(env, ref_, &mut 0) }, - "Failed to unref Buffer reference in drop" - ); - return; - } - // Drop in Rust side - // ```rust - // #[napi] - // fn buffer_len() -> u32 { - // Buffer::from(vec![1, 2, 3]).len() as u32 - // } - if !self.drop_in_vm.load(Ordering::Acquire) { - unsafe { Vec::from_raw_parts(self.inner.as_mut_ptr(), self.inner.len(), self.capacity) }; - } + unsafe { Vec::from_raw_parts(self.inner.as_ptr(), self.len, self.capacity) }; } } } +// SAFETY: This is undefined behavior, as the JS side may always modify the underlying buffer, +// without synchronization. Also see the docs for the `AsMut` impl. unsafe impl Send for Buffer {} impl Buffer { @@ -67,7 +62,8 @@ impl Buffer { None }; Ok(Self { - inner: unsafe { slice::from_raw_parts_mut(self.inner.as_mut_ptr(), self.inner.len()) }, + inner: self.inner, + len: self.len, capacity: self.capacity, raw, drop_in_vm: self.drop_in_vm.clone(), @@ -92,17 +88,21 @@ impl From> for Buffer { let capacity = data.capacity(); mem::forget(data); Buffer { - inner: unsafe { slice::from_raw_parts_mut(inner_ptr, len) }, + // SAFETY: `Vec`'s docs guarantee that its pointer is never null (it's a dangling ptr if not + // allocated): + // > The pointer will never be null, so this type is null-pointer-optimized. + inner: unsafe { NonNull::new_unchecked(inner_ptr) }, + len, capacity, raw: None, - drop_in_vm: Arc::new(AtomicBool::new(false)), + drop_in_vm: Arc::new(()), } } } impl From for Vec { fn from(buf: Buffer) -> Self { - buf.inner.to_vec() + buf.as_ref().to_vec() } } @@ -114,13 +114,17 @@ impl From<&[u8]> for Buffer { impl AsRef<[u8]> for Buffer { fn as_ref(&self) -> &[u8] { - self.inner + // SAFETY: the pointer is guaranteed to be non-null, and guaranteed to be valid if `len` is not 0. + unsafe { slice::from_raw_parts(self.inner.as_ptr(), self.len) } } } impl AsMut<[u8]> for Buffer { fn as_mut(&mut self) -> &mut [u8] { - self.inner + // SAFETY: This is literally undefined behavior. `Buffer::clone` allows you to create shared + // access to the underlying data, but `as_mut` and `deref_mut` allow unsynchronized mutation of + // that data (not to speak of the JS side having write access as well, at the same time). + unsafe { slice::from_raw_parts_mut(self.inner.as_ptr(), self.len) } } } @@ -128,13 +132,13 @@ impl Deref for Buffer { type Target = [u8]; fn deref(&self) -> &Self::Target { - self.inner + self.as_ref() } } impl DerefMut for Buffer { fn deref_mut(&mut self) -> &mut Self::Target { - self.inner + self.as_mut() } } @@ -159,14 +163,28 @@ impl FromNapiValue for Buffer { )?; check_status!( unsafe { sys::napi_get_buffer_info(env, napi_val, &mut buf, &mut len as *mut usize) }, - "Failed to convert napi buffer into rust Vec" + "Failed to get Buffer pointer and length" )?; + // From the docs of `napi_get_buffer_info`: + // > [out] data: The underlying data buffer of the node::Buffer. If length is 0, this may be + // > NULL or any other pointer value. + // + // In order to guarantee that `slice::from_raw_parts` is sound, the pointer must be non-null, so + // let's make sure it always is, even in the case of `napi_get_buffer_info` returning a null + // ptr. + let buf = NonNull::new(buf as *mut u8); + let inner = match buf { + Some(buf) if len != 0 => buf, + _ => NonNull::dangling(), + }; + Ok(Self { - inner: unsafe { slice::from_raw_parts_mut(buf as *mut _, len) }, + inner, + len, capacity: len, raw: Some((ref_, env)), - drop_in_vm: Arc::new(AtomicBool::new(true)), + drop_in_vm: Arc::new(()), }) } } @@ -182,8 +200,7 @@ impl ToNapiValue for Buffer { )?; return Ok(buf); } - let len = val.inner.len(); - val.drop_in_vm.store(true, Ordering::Release); + let len = val.len; let mut ret = ptr::null_mut(); check_status!( if len == 0 { @@ -192,12 +209,11 @@ impl ToNapiValue for Buffer { // the same data pointer if it's 0x0. unsafe { sys::napi_create_buffer(env, len, ptr::null_mut(), &mut ret) } } else { - let val_ptr = val.inner.as_mut_ptr(); unsafe { sys::napi_create_external_buffer( env, len, - val_ptr as *mut c_void, + val.inner.as_ptr() as *mut c_void, Some(drop_buffer), Box::into_raw(Box::new(val)) as *mut c_void, &mut ret, @@ -213,23 +229,8 @@ impl ToNapiValue for Buffer { impl ToNapiValue for &mut Buffer { unsafe fn to_napi_value(env: sys::napi_env, val: Self) -> Result { - // From Node.js value, not from `Vec` - if let Some((ref_, _)) = val.raw { - let mut buf = ptr::null_mut(); - check_status!( - unsafe { sys::napi_get_reference_value(env, ref_, &mut buf) }, - "Failed to get Buffer value from reference" - )?; - Ok(buf) - } else { - let buf = Buffer { - inner: unsafe { slice::from_raw_parts_mut(val.inner.as_mut_ptr(), val.capacity) }, - capacity: val.capacity, - raw: None, - drop_in_vm: Arc::new(AtomicBool::new(true)), - }; - unsafe { ToNapiValue::to_napi_value(env, buf) } - } + let buf = val.clone(&Env::from(env))?; + unsafe { ToNapiValue::to_napi_value(env, buf) } } } diff --git a/crates/napi/src/bindgen_runtime/mod.rs b/crates/napi/src/bindgen_runtime/mod.rs index b007201ea3..9d40dbfde6 100644 --- a/crates/napi/src/bindgen_runtime/mod.rs +++ b/crates/napi/src/bindgen_runtime/mod.rs @@ -1,7 +1,5 @@ use std::ffi::c_void; -use std::mem; use std::rc::Rc; -use std::sync::Arc; pub use callback_info::*; pub use ctor::ctor; @@ -86,13 +84,6 @@ pub unsafe extern "C" fn drop_buffer( }); } unsafe { - let buf = Box::from_raw(finalize_hint as *mut Buffer); - if Arc::strong_count(&buf.drop_in_vm) == 1 { - mem::drop(Vec::from_raw_parts( - finalize_data as *mut u8, - buf.inner.len(), - buf.capacity, - )); - } + drop(Box::from_raw(finalize_hint as *mut Buffer)); } }