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

src/runtime/ffi/FFIObject.rs

@@ -23,9 +23,12 @@ unsafe fn deallocator_from_addr(addr: usize) -> jsc::JSTypedArrayBytesDeallocato
     unsafe { core::mem::transmute::<usize, jsc::JSTypedArrayBytesDeallocator>(addr) }
 }
 
+/// Frees nothing. `JSBuffer__bufferFromPointerAndLengthAndDeinit` asserts a non-null
+/// deallocator for `len > 0`, so a borrowed view supplies this instead of `None`.
+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 passes the caller's deallocator through so FFI bytes are only freed when asked.
 #[allow(non_snake_case)]
 #[inline]
 fn create_buffer_with_ctx(
@@ -43,8 +46,8 @@ 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` stays valid for the Buffer's lifetime.
+    // `callback` controls disposal (a no-op when the storage stays caller-owned).
     unsafe {
         JSBuffer__bufferFromPointerAndLengthAndDeinit(
             global,
@@ -502,15 +505,8 @@ fn ptr_(global_this: &JSGlobalObject, value: JSValue, byte_offset: Option<JSValu
     JSValue::from_ptr_address(addr)
 }
 
-/// `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.
+/// `Slice` carries a raw (ptr, len) pointing at caller-owned FFI memory; consumers
+/// borrow it (or delegate disposal to a supplied finalizer), never free it from Rust.
 enum ValueOrError {
     Err(JSValue),
     Slice(*mut u8, usize),
@@ -740,20 +736,16 @@ fn to_buffer(
                 }
             }
 
-            // SAFETY: ptr/len came from get_ptr_slice; FFI-owned memory.
+            // SAFETY: ptr/len came from get_ptr_slice; caller-owned FFI memory.
             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))
+            // No finalizer means borrow: the noop deallocator keeps GC from freeing
+            // caller-owned storage (oven-sh/bun#35405).
+            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", () => {