From 455b5920508193e367916615008ab3ad10c7d742 Mon Sep 17 00:00:00 2001 From: Dennis Duda Date: Thu, 1 Sep 2022 23:17:54 +0200 Subject: [PATCH 1/3] fix leaked napi refcount in `Buffer` when cloning Cloning unconditionally increased the refcount in `Buffer::clone`, but only called `napi_reference_unref` on dropping the last Buffer (the one with `strong_count == 1`). This means that the refcount will never drop back to zero after cloning, leaking the Buffer. This commit changes it to also unconditionally unref the buffer. --- .../src/bindgen_runtime/js_values/buffer.rs | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/crates/napi/src/bindgen_runtime/js_values/buffer.rs b/crates/napi/src/bindgen_runtime/js_values/buffer.rs index 07157955a2..b5fd1cca41 100644 --- a/crates/napi/src/bindgen_runtime/js_values/buffer.rs +++ b/crates/napi/src/bindgen_runtime/js_values/buffer.rs @@ -31,15 +31,16 @@ pub struct Buffer { 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] From 88585b60a11853fbb7cf986b303fe774aca72541 Mon Sep 17 00:00:00 2001 From: Dennis Duda Date: Fri, 2 Sep 2022 00:26:56 +0200 Subject: [PATCH 2/3] fix multiple sources of UB in `Buffer` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - `slice::from_raw_parts` may never be created with a null pointer, but `napi_get_buffer_info` was not sufficiently checked → UB when passing an empty Buffer - `&'static mut [u8],` is invalid, as it certainly doesn't live for `'static` Switching to `NonNull` and a `len` field fixes both of these. - I also don't really understand how the `impl ToNapiValue for &mut Buffer` could have been sound. It creates an entirely new `Arc`, but reuses the same `Vec` allocation, leading to... a double free of the `Vec` on drop? I have replaced it with a simple call to `clone` instead. --- .../src/bindgen_runtime/js_values/buffer.rs | 74 +++++++++++-------- crates/napi/src/bindgen_runtime/mod.rs | 2 +- 2 files changed, 43 insertions(+), 33 deletions(-) diff --git a/crates/napi/src/bindgen_runtime/js_values/buffer.rs b/crates/napi/src/bindgen_runtime/js_values/buffer.rs index b5fd1cca41..c8b244d094 100644 --- a/crates/napi/src/bindgen_runtime/js_values/buffer.rs +++ b/crates/napi/src/bindgen_runtime/js_values/buffer.rs @@ -3,7 +3,7 @@ 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; @@ -22,7 +22,8 @@ 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 @@ -48,12 +49,14 @@ impl Drop for Buffer { // 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 { @@ -68,7 +71,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(), @@ -93,7 +97,11 @@ 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)), @@ -103,7 +111,7 @@ impl From> for Buffer { impl From for Vec { fn from(buf: Buffer) -> Self { - buf.inner.to_vec() + buf.as_ref().to_vec() } } @@ -115,13 +123,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) } } } @@ -129,13 +141,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() } } @@ -160,11 +172,25 @@ 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)), @@ -183,7 +209,7 @@ impl ToNapiValue for Buffer { )?; return Ok(buf); } - let len = val.inner.len(); + let len = val.len; val.drop_in_vm.store(true, Ordering::Release); let mut ret = ptr::null_mut(); check_status!( @@ -193,12 +219,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, @@ -214,23 +239,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..1185d917f7 100644 --- a/crates/napi/src/bindgen_runtime/mod.rs +++ b/crates/napi/src/bindgen_runtime/mod.rs @@ -90,7 +90,7 @@ pub unsafe extern "C" fn drop_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.len, buf.capacity, )); } From b04f6e5023f5e7d45dd36af6c44a10be47fd6352 Mon Sep 17 00:00:00 2001 From: Dennis Duda Date: Fri, 2 Sep 2022 00:41:54 +0200 Subject: [PATCH 3/3] remove overcomplicated bool and drop impl As far as I can tell, by just removing the bool and letting the drop code do its thing we clean up correctly in all cases. Because `napi_create_external_buffer` gets an owned `Buffer` attached to it via the Box, we can rely on `from_raw` retrieving it in the `drop_buffer` function. --- .../src/bindgen_runtime/js_values/buffer.rs | 18 ++++-------------- crates/napi/src/bindgen_runtime/mod.rs | 11 +---------- 2 files changed, 5 insertions(+), 24 deletions(-) diff --git a/crates/napi/src/bindgen_runtime/js_values/buffer.rs b/crates/napi/src/bindgen_runtime/js_values/buffer.rs index c8b244d094..7fc0611905 100644 --- a/crates/napi/src/bindgen_runtime/js_values/buffer.rs +++ b/crates/napi/src/bindgen_runtime/js_values/buffer.rs @@ -5,7 +5,6 @@ use std::mem; use std::ops::{Deref, DerefMut}; 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; @@ -27,7 +26,7 @@ pub struct Buffer { 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 { @@ -42,15 +41,7 @@ impl Drop for Buffer { } if Arc::strong_count(&self.drop_in_vm) == 1 { - // 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_ptr(), self.len, self.capacity) }; - } + unsafe { Vec::from_raw_parts(self.inner.as_ptr(), self.len, self.capacity) }; } } } @@ -104,7 +95,7 @@ impl From> for Buffer { len, capacity, raw: None, - drop_in_vm: Arc::new(AtomicBool::new(false)), + drop_in_vm: Arc::new(()), } } } @@ -193,7 +184,7 @@ impl FromNapiValue for Buffer { len, capacity: len, raw: Some((ref_, env)), - drop_in_vm: Arc::new(AtomicBool::new(true)), + drop_in_vm: Arc::new(()), }) } } @@ -210,7 +201,6 @@ impl ToNapiValue for Buffer { return Ok(buf); } let len = val.len; - val.drop_in_vm.store(true, Ordering::Release); let mut ret = ptr::null_mut(); check_status!( if len == 0 { diff --git a/crates/napi/src/bindgen_runtime/mod.rs b/crates/napi/src/bindgen_runtime/mod.rs index 1185d917f7..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.len, - buf.capacity, - )); - } + drop(Box::from_raw(finalize_hint as *mut Buffer)); } }