Merge pull request #1331 from napi-rs/pass-in-buffer-leak

fix(napi): should also delete the reference while dropping the Buffer

.github/workflows/msrv.yml

@@ -63,8 +63,3 @@ jobs:
           yarn test --verbose
         env:
           RUST_BACKTRACE: 1
-
-      - name: Clear the cargo caches
-        run: |
-          cargo install cargo-cache --no-default-features --features ci-autoclean
-          cargo-cache

crates/napi/src/bindgen_runtime/js_values/arraybuffer.rs

@@ -67,6 +67,11 @@ macro_rules! impl_typed_array {
             crate::check_status_or_throw!(
               env,
               unsafe { sys::napi_reference_unref(env, ref_, &mut 0) },
+              "Failed to unref Buffer reference in drop"
+            );
+            crate::check_status_or_throw!(
+              env,
+              unsafe { sys::napi_delete_reference(env, ref_) },
               "Failed to delete Buffer reference in drop"
             );
             return;

crates/napi/src/bindgen_runtime/js_values/buffer.rs

@@ -25,23 +25,27 @@ pub struct Buffer {
   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) ref_count: 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 {
-      unsafe { Vec::from_raw_parts(self.inner.as_ptr(), self.len, self.capacity) };
+    if Arc::strong_count(&self.ref_count) == 1 {
+      if let Some((ref_, env)) = self.raw {
+        let mut ref_count = 0;
+        check_status_or_throw!(
+          env,
+          unsafe { sys::napi_reference_unref(env, ref_, &mut ref_count) },
+          "Failed to unref Buffer reference in drop"
+        );
+        check_status_or_throw!(
+          env,
+          unsafe { sys::napi_delete_reference(env, ref_) },
+          "Failed to delete Buffer reference in drop"
+        );
+      } else {
+        unsafe { Vec::from_raw_parts(self.inner.as_ptr(), self.len, self.capacity) };
+      }
     }
   }
 }
@@ -50,24 +54,15 @@ impl Drop for Buffer {
 // without synchronization. Also see the docs for the `AsMut` impl.
 unsafe impl Send for Buffer {}
 
-impl Buffer {
-  pub fn clone(&mut self, env: &Env) -> Result<Self> {
-    let raw = if let Some((ref_, _)) = self.raw {
-      check_status!(
-        unsafe { sys::napi_reference_ref(env.0, ref_, &mut 0) },
-        "Failed to ref Buffer reference in Buffer::clone"
-      )?;
-      Some((ref_, env.0))
-    } else {
-      None
-    };
-    Ok(Self {
+impl Clone for Buffer {
+  fn clone(&self) -> Self {
+    Self {
       inner: self.inner,
       len: self.len,
       capacity: self.capacity,
-      raw,
-      drop_in_vm: self.drop_in_vm.clone(),
-    })
+      raw: self.raw,
+      ref_count: self.ref_count.clone(),
+    }
   }
 }
 
@@ -95,7 +90,7 @@ impl From<Vec<u8>> for Buffer {
       len,
       capacity,
       raw: None,
-      drop_in_vm: Arc::new(()),
+      ref_count: Arc::new(()),
     }
   }
 }
@@ -184,7 +179,7 @@ impl FromNapiValue for Buffer {
       len,
       capacity: len,
       raw: Some((ref_, env)),
-      drop_in_vm: Arc::new(()),
+      ref_count: Arc::new(()),
     })
   }
 }
@@ -227,9 +222,16 @@ impl ToNapiValue for Buffer {
   }
 }
 
+impl ToNapiValue for &Buffer {
+  unsafe fn to_napi_value(env: sys::napi_env, val: Self) -> Result<sys::napi_value> {
+    let buf = val.clone();
+    unsafe { ToNapiValue::to_napi_value(env, buf) }
+  }
+}
+
 impl ToNapiValue for &mut Buffer {
   unsafe fn to_napi_value(env: sys::napi_env, val: Self) -> Result<sys::napi_value> {
-    let buf = val.clone(&Env::from(env))?;
+    let buf = val.clone();
     unsafe { ToNapiValue::to_napi_value(env, buf) }
   }
 }

crates/napi/src/bindgen_runtime/mod.rs

@@ -73,7 +73,7 @@ pub unsafe extern "C" fn raw_finalize_unchecked<T: ObjectFinalize>(
 #[doc(hidden)]
 pub unsafe extern "C" fn drop_buffer(
   _env: sys::napi_env,
-  finalize_data: *mut c_void,
+  #[allow(unused)] finalize_data: *mut c_void,
   finalize_hint: *mut c_void,
 ) {
   #[cfg(debug_assertions)]

examples/napi/src/typed_array.rs

@@ -68,8 +68,6 @@ impl Task for AsyncBuffer {
 }
 
 #[napi]
-fn async_reduce_buffer(mut buf: Buffer, env: Env) -> Result<AsyncTask<AsyncBuffer>> {
-  Ok(AsyncTask::new(AsyncBuffer {
-    buf: buf.clone(&env)?,
-  }))
+fn async_reduce_buffer(buf: Buffer) -> Result<AsyncTask<AsyncBuffer>> {
+  Ok(AsyncTask::new(AsyncBuffer { buf: buf.clone() }))
 }

memory-testing/buffer.mjs

@@ -10,16 +10,18 @@ const require = createRequire(import.meta.url)
 const api = require(`./index.node`)
 
 let i = 1
+const FIXTURE = Buffer.allocUnsafe(1000 * 1000 * 20)
 // eslint-disable-next-line no-constant-condition
 while (true) {
   api.bufferLen()
   api.arrayBufferLen()
-  api.bufferConvert(Buffer.from(Array.from({ length: 1024 * 10240 }).fill(1)))
-  api.arrayBufferConvert(
-    Uint8Array.from(Array.from({ length: 1024 * 10240 }).fill(1)),
-  )
+  api.bufferConvert(Buffer.from(FIXTURE))
+  api.arrayBufferConvert(Uint8Array.from(FIXTURE))
+  api.bufferPassThrough(Buffer.from(FIXTURE))
+  api.arrayBufferPassThrough(Uint8Array.from(FIXTURE))
   if (i % 10 === 0) {
-    await setTimeout(100)
+    await setTimeout(1000)
+    global?.gc?.()
     displayMemoryUsageFromNode(initialMemoryUsage)
   }
   i++

memory-testing/src/lib.rs

@@ -161,3 +161,13 @@ pub fn array_buffer_convert(array_buffer: Uint8Array) -> Uint8Array {
 pub fn array_buffer_len() -> u32 {
   Uint8Array::new(vec![1; 1024 * 10240]).len() as u32
 }
+
+#[napi]
+pub fn buffer_pass_through(buffer: Buffer) -> Buffer {
+  buffer
+}
+
+#[napi]
+pub fn array_buffer_pass_through(array_buffer: Uint8Array) -> Uint8Array {
+  array_buffer
+}

memory-testing/test-util.mjs

@@ -10,7 +10,7 @@ const sleep = promisify(setTimeout)
 const client = new Dockerode()
 
 export async function createSuite(testFile, maxMemoryUsage) {
-  console.info(chalk.cyanBright('Create container'))
+  console.info(chalk.cyanBright(`Create container to test ${testFile}`))
 
   const container = await client.createContainer({
     Image: 'node:lts-slim',
@@ -74,6 +74,10 @@ export async function createSuite(testFile, maxMemoryUsage) {
 
   shouldAssertMemoryUsage = true
 
-  await container.stop()
-  await container.remove()
+  try {
+    await container.stop()
+    await container.remove()
+  } catch (e) {
+    console.error(e)
+  }
 }