ffi: don't free caller-owned memory in toBuffer without a finalizer

src/runtime/ffi/FFIObject.rs

@@ -23,9 +23,19 @@ unsafe fn deallocator_from_addr(addr: usize) -> jsc::JSTypedArrayBytesDeallocato
     unsafe { core::mem::transmute::<usize, jsc::JSTypedArrayBytesDeallocator>(addr) }
 }
 
+/// Bytes deallocator that frees nothing, for a borrowed FFI pointer.
+/// `toBuffer(ptr, offset, len)` without an explicit finalizer views caller-owned
+/// memory it must NOT free, but the underlying
+/// `JSBuffer__bufferFromPointerAndLengthAndDeinit` requires a non-null deallocator
+/// for non-empty storage. A deallocator that does nothing satisfies that while
+/// leaving the storage caller-owned: GC releases only JSC's view, so no bad-free.
+/// Disposal is delegated only when the caller supplies a finalizer.
+unsafe extern "C" fn noop_bytes_deallocator(_ptr: *mut c_void, _ctx: *mut c_void) {}
+
 /// Unlike `JSValue::create_buffer` (which hard-codes `MarkedArrayBuffer_deallocator`),
-/// this variant passes the caller's (possibly null) deallocator through, so FFI-owned
-/// memory is only freed by the user-supplied callback.
+/// this variant passes the selected deallocator through: `to_buffer` supplies either
+/// the caller's finalizer or `noop_bytes_deallocator` for borrowed storage, so the
+/// bytes are only freed when the caller asked for that.
 #[allow(non_snake_case)]
 #[inline]
 fn create_buffer_with_ctx(
@@ -43,8 +53,9 @@ fn create_buffer_with_ctx(
             deallocator: jsc::JSTypedArrayBytesDeallocator,
         ) -> JSValue;
     }
-    // SAFETY: `global` is live; slice describes FFI-owned memory whose
-    // ownership transfers to JSC (freed via `callback`, or never if None).
+    // SAFETY: `global` is live; `slice` describes caller-provided memory that stays
+    // valid for the Buffer's lifetime. `callback` controls disposal, and may be a
+    // no-op when the storage remains caller-owned (JSC then owns only the view).
     unsafe {
         JSBuffer__bufferFromPointerAndLengthAndDeinit(
             global,
@@ -505,12 +516,9 @@ fn ptr_(global_this: &JSGlobalObject, value: JSValue, byte_offset: Option<JSValu
 /// `union(enum)` → Rust enum.
 /// `Slice` carries a raw (ptr, len) because it points at caller-owned FFI memory
 /// of unknown lifetime.
-// Consumer audit: `new_cstring` copies the bytes into a JS string;
-// `to_array_buffer` wraps the pointer with the caller's optional finalizer and
-// never frees it from Rust; `to_buffer` does the same when a finalizer is
-// given, but WITHOUT one it falls back to `JSValue::create_buffer`, which
-// installs `MarkedArrayBuffer_deallocator` and `mi_free`s the caller-owned
-// slice on GC — free-foreign-memory footgun, see PR #31753.
+// Consumer audit: `new_cstring` copies the bytes into a JS string; `to_array_buffer`
+// and `to_buffer` both borrow the pointer when no finalizer is supplied and never
+// free it from Rust. A supplied finalizer controls disposal.
 enum ValueOrError {
     Err(JSValue),
     Slice(*mut u8, usize),
@@ -740,20 +748,21 @@ fn to_buffer(
                 }
             }
 
-            // SAFETY: ptr/len came from get_ptr_slice; FFI-owned memory.
+            // SAFETY: ptr/len came from get_ptr_slice; a caller-supplied finalizer
+            // controls disposal, otherwise the storage stays caller-owned and borrowed.
             let slice = unsafe { core::slice::from_raw_parts_mut(ptr, len) };
-            if callback.is_some() || ctx.is_some() {
-                return Ok(create_buffer_with_ctx(
-                    global_this,
-                    slice,
-                    ctx.unwrap_or(core::ptr::null_mut()),
-                    callback,
-                ));
-            }
-
-            // `JSValue::create_buffer` installs `MarkedArrayBuffer_deallocator` so
-            // the slice is `mi_free`d on GC (including the free-foreign-memory footgun).
-            Ok(JSValue::create_buffer(global_this, slice))
+            // Without an explicit finalizer the pointer stays caller-owned (e.g. it
+            // came from `ptr(buffer)`), so this must not install Bun's allocator
+            // deallocator: freeing it on GC frees storage this Buffer does not own
+            // (ASAN bad-free / SIGSEGV in `mi_free`), which is what the previous
+            // `JSValue::create_buffer` fall-back did. The no-op deallocator makes the
+            // Buffer a borrowed zero-copy view instead.
+            Ok(create_buffer_with_ctx(
+                global_this,
+                slice,
+                ctx.unwrap_or(core::ptr::null_mut()),
+                callback.or(Some(noop_bytes_deallocator)),
+            ))
         }
     }
 }

test/js/bun/ffi/ffi-test.c

@@ -118,14 +118,10 @@ uint32_t add_uint32_t(uint32_t a, uint32_t b) { return a + b; }
 uint64_t add_uint64_t(uint64_t a, uint64_t b) { return a + b; }
 
 FFI_EXPORT void *ptr_should_point_to_42_as_int32_t();
-FFI_EXPORT void *getNoopDeallocatorCallback();
 
 static int32_t ffi_static_42 = 42;
 void *ptr_should_point_to_42_as_int32_t() { return &ffi_static_42; }
 
-static void noop_deallocator(void *ptr, void *ctx) { (void)ptr; (void)ctx; }
-void *getNoopDeallocatorCallback() { return &noop_deallocator; }
-
 static uint8_t buffer_with_deallocator[128];
 static int deallocatorCalled;
 FFI_EXPORT void deallocator(void *ptr, void *userData) { deallocatorCalled++; }

test/js/bun/ffi/ffi.test.js

@@ -325,10 +325,6 @@ function getTypes(fast) {
       returns: "ptr",
       args: [],
     },
-    getNoopDeallocatorCallback: {
-      returns: "ptr",
-      args: [],
-    },
     getDeallocatorBuffer: {
       returns: "ptr",
       args: [],
@@ -382,7 +378,6 @@ function ffiRunner(fast) {
         is_null,
         does_pointer_equal_42_as_int32_t,
         ptr_should_point_to_42_as_int32_t,
-        getNoopDeallocatorCallback,
         cb_identity_true,
         cb_identity_false,
         cb_identity_42_char,
@@ -493,11 +488,12 @@ function ffiRunner(fast) {
       expect(cptr != 0).toBe(true);
       expect(typeof cptr === "number").toBe(true);
       expect(does_pointer_equal_42_as_int32_t(cptr)).toBe(true);
-      const noopDeallocator = getNoopDeallocatorCallback();
       {
-        const buffer = toBuffer(cptr, 0, 4, noopDeallocator);
+        // No finalizer: both views borrow `cptr` (static storage in the fixture),
+        // so the GC below must not free it. See oven-sh/bun#35405.
+        const buffer = toBuffer(cptr, 0, 4);
         expect(buffer.readInt32(0)).toBe(42);
-        expect(new DataView(toArrayBuffer(cptr, 0, 4, noopDeallocator), 0, 4).getInt32(0, true)).toBe(42);
+        expect(new DataView(toArrayBuffer(cptr, 0, 4), 0, 4).getInt32(0, true)).toBe(42);
         expect(ptr(buffer)).toBe(cptr);
       }
       Bun.gc(true);
@@ -1358,6 +1354,137 @@ describe.if(!!libPath)("can open more than 63 symbols via", () => {
   }
 });
 
+// `toBuffer(ptr, offset, len)` without an explicit finalizer used to adopt the
+// caller's pointer as owned and install Bun's allocator deallocator, so GC would
+// `mi_free` caller-owned memory — an ASAN bad-free / release SIGSEGV.
+// These run in a subprocess because the bug crashes the process on unpatched Bun.
+describe("toBuffer borrowed-pointer ownership (no bad-free on GC)", () => {
+  async function runsClean(script) {
+    await using proc = Bun.spawn({
+      cmd: [bunExe(), "-e", script],
+      env: bunEnv,
+      stdout: "pipe",
+      stderr: "pipe",
+    });
+    const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+    return { stdout, stderr, exitCode };
+  }
+
+  const gcLoop = `for (let i = 0; i < 20; i++) { Bun.gc(true); Buffer.alloc(1024 * 1024); }`;
+
+  // Forcing GC repeatedly in a subprocess costs a few seconds on a debug build, so
+  // the default 5s budget is too tight to be reliable. (On an unpatched Bun the child
+  // SIGSEGVs and then wedges in the crash handler, so there the red signal is this
+  // timeout rather than the exit-code assertion.)
+  const GC_TIMEOUT = 20_000;
+
+  // Post-condition on the ACTUAL caller memory the bad-free targets: drop only the
+  // adopted Buffer, then confirm `original[index]` (the storage `ptr(...)` pointed
+  // at) is still readable and writable — proving its backing was NOT freed. Then
+  // drop `original` too, exercising the owner's normal disposal after the borrowed
+  // view was collected — on an unpatched build the earlier invalid free has already
+  // corrupted that ownership path. That turns "the process didn't abort" into "the
+  // caller memory survived".
+  const originalSurvives = (index, expected) => `
+      adopted = null;
+      ${gcLoop}
+      if (original[${index}] !== ${expected}) throw new Error("caller memory corrupted after adopted GC: " + original[${index}]);
+      original[${index}] = 0x55;
+      if (original[${index}] !== 0x55) throw new Error("caller memory not writable after adopted GC");
+      original = null;
+      ${gcLoop}
+  `;
+
+  it(
+    "toBuffer(ptr(buffer)) does not free caller-owned memory on GC",
+    async () => {
+      expect(
+        await runsClean(`
+      import { ptr, toBuffer } from "bun:ffi";
+      let original = Buffer.alloc(64, 0x41);
+      let adopted = toBuffer(ptr(original), 0, 64);
+      if (adopted[0] !== 0x41) throw new Error("expected a zero-copy view");
+      adopted[0] = 0x42;
+      if (original[0] !== 0x42) throw new Error("expected an aliasing view");
+      ${originalSurvives(0, "0x42")}
+      console.log("survived-gc");
+    `),
+      ).toEqual({ stdout: "survived-gc\n", stderr: "", exitCode: 0 });
+    },
+    GC_TIMEOUT,
+  );
+
+  it(
+    "toBuffer(ptr(buffer), offset) does not free an interior pointer on GC",
+    async () => {
+      // The adopted view starts at original[8], so both the aliasing check and the
+      // post-GC survival check must inspect that byte, not original[0].
+      expect(
+        await runsClean(`
+      import { ptr, toBuffer } from "bun:ffi";
+      let original = Buffer.alloc(64, 0x41);
+      let adopted = toBuffer(ptr(original), 8, 48);
+      if (adopted[0] !== 0x41) throw new Error("expected a zero-copy view");
+      adopted[0] = 0x42;
+      if (original[8] !== 0x42) throw new Error("expected a view aliasing original[8]");
+      ${originalSurvives(8, "0x42")}
+      console.log("survived-gc");
+    `),
+      ).toEqual({ stdout: "survived-gc\n", stderr: "", exitCode: 0 });
+    },
+    GC_TIMEOUT,
+  );
+
+  it(
+    "toBuffer(ptr(typedArray)) does not free caller-owned memory on GC",
+    async () => {
+      expect(
+        await runsClean(`
+      import { ptr, toBuffer } from "bun:ffi";
+      let original = new Uint8Array(64).fill(0x41);
+      let adopted = toBuffer(ptr(original), 0, 64);
+      ${originalSurvives(0, "0x41")}
+      console.log("survived-gc");
+    `),
+      ).toEqual({ stdout: "survived-gc\n", stderr: "", exitCode: 0 });
+    },
+    GC_TIMEOUT,
+  );
+
+  // Regression (the #1 risk): the bad-free fix must NOT change the explicit-finalizer
+  // path. When the caller supplies a finalizer, it still controls disposal and the
+  // deallocator is invoked exactly once on GC. Uses the compiled fixture's
+  // deallocator-counter helpers (getDeallocatorBuffer/getDeallocatorCallback both
+  // reset the counter, so the count is isolated to this test).
+  it.skipIf(!FFI_FIXTURE_PATH)(
+    "toBuffer with an explicit finalizer calls the deallocator exactly once on GC",
+    () => {
+      const {
+        symbols: { getDeallocatorCallback, getDeallocatorBuffer, getDeallocatorCalledCount },
+      } = dlopen(FFI_FIXTURE_PATH, {
+        getDeallocatorCallback: { args: [], returns: "ptr" },
+        getDeallocatorBuffer: { args: [], returns: "ptr" },
+        getDeallocatorCalledCount: { args: [], returns: "int" },
+      });
+      const bufPtr = getDeallocatorBuffer();
+      let buf = toBuffer(bufPtr, 0, 128, getDeallocatorCallback());
+      expect(buf.length).toBe(128);
+      expect(getDeallocatorCalledCount()).toBe(0); // not called during construction
+      buf = null;
+      // Await the collection rather than assuming one pass suffices: a single
+      // Bun.gc(true) can still see `buf` conservatively from the stack.
+      for (let i = 0; i < 20 && getDeallocatorCalledCount() === 0; i++) {
+        Bun.gc(true);
+        Buffer.alloc(1024 * 1024);
+      }
+      expect(getDeallocatorCalledCount()).toBe(1); // called exactly once on GC
+      Bun.gc(true);
+      expect(getDeallocatorCalledCount()).toBe(1); // not called again
+    },
+    GC_TIMEOUT,
+  );
+});
+
 describe.skipIf(!FFI_FIXTURE_PATH)("engine-native FFI (single implementation)", () => {
   const lib = FFI_FIXTURE_PATH;
   it("linkSymbols() binds and calls symbols from raw pointers", () => {