fix(napi): some of the unsoundness in Buffer (#1294)
* 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. * fix multiple sources of UB in `Buffer` - `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<u8>` 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. * 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.
This commit is contained in:
Dennis Duda
GitHub
parent
26f6c926d3
commit
fc63ba8b52
2 changed files with 55 additions and 63 deletions
|
|
@ -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<u8>,
|
||||
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<AtomicBool>,
|
||||
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<Vec<u8>> 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<Buffer> for Vec<u8> {
|
||||
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<u8>"
|
||||
"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<sys::napi_value> {
|
||||
// From Node.js value, not from `Vec<u8>`
|
||||
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) }
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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));
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue