Fix some of the unsoundness in Buffer

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<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,27 +114,31 @@ 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) }
   }
 }
 
 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) }
   }
 }
 

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));
   }
 }