ffi: fix handle leak in fast buffer arguments

src/ffi/fast.cc

@@ -245,7 +245,13 @@ extern "C" uintptr_t node_ffi_fast_buffer_data(v8::Local<v8::Value> value,
   // returns zero after throwing, preventing the native target from seeing an
   // invalid pointer value.
   constexpr uintptr_t kInvalidBuffer = std::numeric_limits<uintptr_t>::max();
-  v8::Isolate* isolate = options != nullptr ? options->isolate : nullptr;
+  // The trampoline always passes the FastApiCallbackOptions declared in the
+  // CFunctionInfo. No HandleScope is active during a Fast API call, and both
+  // ArrayBufferView::Buffer() and the error paths below create handles, so
+  // open one for the whole helper. Otherwise every call leaks handles into
+  // the caller's scope.
+  v8::Isolate* isolate = options->isolate;
+  v8::HandleScope scope(isolate);
 
   // Accept only the memory-backed JS values supported by ToFFIArgument in the
   // native helper. Other pointer conversions, including strings and direct
@@ -254,41 +260,26 @@ extern "C" uintptr_t node_ffi_fast_buffer_data(v8::Local<v8::Value> value,
   if (value->IsArrayBufferView()) {
     v8::Local<v8::ArrayBufferView> view = value.As<v8::ArrayBufferView>();
     if (view->Buffer()->WasDetached()) {
-      if (isolate != nullptr) {
-        // No HandleScope is active during a Fast API call, so open one before
-        // creating the error object.
-        v8::HandleScope scope(isolate);
-        THROW_ERR_INVALID_ARG_VALUE(
-            isolate,
-            "Argument %u is an ArrayBufferView backed by a detached "
-            "ArrayBuffer",
-            index);
-      }
+      THROW_ERR_INVALID_ARG_VALUE(
+          isolate,
+          "Argument %u is an ArrayBufferView backed by a detached "
+          "ArrayBuffer",
+          index);
       return kInvalidBuffer;
     }
     return PointerFromValue(value);
   }
   if (value->IsArrayBuffer()) {
     if (value.As<v8::ArrayBuffer>()->WasDetached()) {
-      if (isolate != nullptr) {
-        // No HandleScope is active during a Fast API call, so open one before
-        // creating the error object.
-        v8::HandleScope scope(isolate);
-        THROW_ERR_INVALID_ARG_VALUE(
-            isolate, "Argument %u is a detached ArrayBuffer", index);
-      }
+      THROW_ERR_INVALID_ARG_VALUE(
+          isolate, "Argument %u is a detached ArrayBuffer", index);
       return kInvalidBuffer;
     }
     return PointerFromValue(value);
   }
 
-  if (isolate != nullptr) {
-    // No HandleScope is active during a Fast API call, so open one before
-    // creating the error object.
-    v8::HandleScope scope(isolate);
-    THROW_ERR_INVALID_ARG_VALUE(
-        isolate, "Argument %u must be a buffer or an ArrayBuffer", index);
-  }
+  THROW_ERR_INVALID_ARG_VALUE(
+      isolate, "Argument %u must be a buffer or an ArrayBuffer", index);
   return kInvalidBuffer;
 }
 

test/ffi/test-ffi-fast-buffer.js

@@ -6,6 +6,7 @@ common.skipIfFFIMissing();
 
 const assert = require('node:assert');
 const { test } = require('node:test');
+const { queryObjects } = require('node:v8');
 const { internalBinding } = require('internal/test/binding');
 const {
   kSbSharedBuffer,
@@ -100,6 +101,36 @@ test('optimized pointer arguments reject direct SharedArrayBuffers', () => {
   }
 });
 
+test('optimized buffer calls do not retain handles', () => {
+  const { lib, functions } = ffi.dlopen(libraryPath, {
+    first_byte: { arguments: ['buffer'], return: 'u8' },
+  });
+  // A leaked handle to the backing store keeps it alive until the current
+  // synchronous run ends, so count live backing stores after the calls.
+  class TrackedArrayBuffer extends ArrayBuffer {}
+
+  function callFirstByte(view) {
+    return functions.first_byte(view);
+  }
+
+  try {
+    const warm = new Uint8Array(1);
+    eval('%PrepareFunctionForOptimization(callFirstByte)');
+    for (let i = 0; i < 1000; i++) callFirstByte(warm);
+    eval('%OptimizeFunctionOnNextCall(callFirstByte)');
+    callFirstByte(warm);
+
+    for (let i = 0; i < 100; i++) {
+      callFirstByte(new Uint8Array(new TrackedArrayBuffer(1)));
+    }
+    assert.strictEqual(
+      queryObjects(TrackedArrayBuffer, { format: 'count' }), 0);
+  } finally {
+    eval('%WaitForBackgroundOptimization()');
+    lib.close();
+  }
+});
+
 test('fast FFI string buffers survive reentrant callbacks', {
   // Bundled libffi callbacks crash on SmartOS.
   skip: common.isSunOS,