Fix leaking .ptr (#4181)

Co-authored-by: Jarred Sumner <709451+Jarred-Sumner@users.noreply.github.com>

5 files changed, 31 insertions, 11 deletions

diff --git a/src/bun.js/api/ffi.zig b/src/bun.js/api/ffi.zig
index 8d02f6672..3966d8287 100644
--- a/src/bun.js/api/ffi.zig
+++ b/src/bun.js/api/ffi.zig
@@ -386,6 +386,7 @@ pub const FFI = struct {
                         @as(u32, @intCast(function.arg_types.items.len)),
                         bun.cast(JSC.JSHostFunctionPtr, compiled.ptr),
                         false,
+                        true,
                     );
                     compiled.js_function = cb;
                     obj.put(global, &str, cb);
@@ -482,6 +483,7 @@ pub const FFI = struct {
                         @as(u32, @intCast(function.arg_types.items.len)),
                         bun.cast(JSC.JSHostFunctionPtr, compiled.ptr),
                         false,
+                        true,
                     );
                     compiled.js_function = cb;
 
diff --git a/src/bun.js/bindings/JSFFIFunction.cpp b/src/bun.js/bindings/JSFFIFunction.cpp
index a0cd83ba6..71a53be4c 100644
--- a/src/bun.js/bindings/JSFFIFunction.cpp
+++ b/src/bun.js/bindings/JSFFIFunction.cpp
@@ -113,10 +113,17 @@ extern "C" void Bun__untrackFFIFunction(Zig::GlobalObject* globalObject, JSC::En
 {
     globalObject->untrackFFIFunction(JSC::jsCast<JSC::JSFunction*>(JSC::JSValue::decode(function)));
 }
-extern "C" JSC::EncodedJSValue Bun__CreateFFIFunctionValue(Zig::GlobalObject* globalObject, const ZigString* symbolName, unsigned argCount, Zig::FFIFunction functionPointer, bool strong);
-extern "C" JSC::EncodedJSValue Bun__CreateFFIFunctionValue(Zig::GlobalObject* globalObject, const ZigString* symbolName, unsigned argCount, Zig::FFIFunction functionPointer, bool strong)
+extern "C" JSC::EncodedJSValue Bun__CreateFFIFunctionValue(Zig::GlobalObject* globalObject, const ZigString* symbolName, unsigned argCount, Zig::FFIFunction functionPointer, bool strong, bool addPtrField)
 {
-    return JSC::JSValue::encode(JSC::JSValue(Bun__CreateFFIFunction(globalObject, symbolName, argCount, functionPointer, strong)));
+    auto* function = Bun__CreateFFIFunction(globalObject, symbolName, argCount, functionPointer, strong);
+    if (addPtrField) {
+        auto& vm = globalObject->vm();
+        // We should only expose the "ptr" field when it's a JSCallback for bun:ffi.
+        // Not for internal usages of this function type.
+        // We should also consider a separate JSFunction type for our usage to not have this branch in the first place...
+        function->putDirect(vm, JSC::Identifier::fromString(vm, String(MAKE_STATIC_STRING_IMPL("ptr"))), JSC::jsNumber(bitwise_cast<double>(functionPointer)), JSC::PropertyAttribute::ReadOnly | 0);
+    }
+    return JSC::JSValue::encode(function);
 }
 
 namespace Zig {
@@ -145,7 +152,6 @@ DEFINE_VISIT_CHILDREN(JSFFIFunction);
 void JSFFIFunction::finishCreation(VM& vm, NativeExecutable* executable, unsigned length, const String& name)
 {
     Base::finishCreation(vm, executable, length, name);
-    this->putDirect(vm, JSC::Identifier::fromString(vm, String(MAKE_STATIC_STRING_IMPL("ptr"))), jsNumber(bitwise_cast<double>(this->m_function)), JSC::PropertyAttribute::ReadOnly | 0);
     ASSERT(inherits(info()));
 }
 
diff --git a/src/bun.js/bindings/bindings.zig b/src/bun.js/bindings/bindings.zig
index edaa67a26..529315a59 100644
--- a/src/bun.js/bindings/bindings.zig
+++ b/src/bun.js/bindings/bindings.zig
@@ -5357,6 +5357,7 @@ const private = struct {
         argCount: u32,
         functionPointer: JSHostFunctionPtr,
         strong: bool,
+        add_ptr_field: bool,
     ) JSValue;
 
     pub extern fn Bun__untrackFFIFunction(
@@ -5380,7 +5381,7 @@ pub fn NewFunction(
     comptime functionPointer: JSHostFunctionType,
     strong: bool,
 ) JSValue {
-    return NewRuntimeFunction(globalObject, symbolName, argCount, &functionPointer, strong);
+    return NewRuntimeFunction(globalObject, symbolName, argCount, &functionPointer, strong, false);
 }
 
 pub fn NewRuntimeFunction(
@@ -5389,9 +5390,10 @@ pub fn NewRuntimeFunction(
     argCount: u32,
     functionPointer: JSHostFunctionPtr,
     strong: bool,
+    add_ptr_property: bool,
 ) JSValue {
     JSC.markBinding(@src());
-    return private.Bun__CreateFFIFunctionValue(globalObject, symbolName, argCount, functionPointer, strong);
+    return private.Bun__CreateFFIFunctionValue(globalObject, symbolName, argCount, functionPointer, strong, add_ptr_property);
 }
 
 pub fn getFunctionData(function: JSValue) ?*anyopaque {
diff --git a/src/bun.js/test/jest.zig b/src/bun.js/test/jest.zig
index 6813af74a..5600f5338 100644
--- a/src/bun.js/test/jest.zig
+++ b/src/bun.js/test/jest.zig
@@ -425,22 +425,22 @@ pub const Jest = struct {
         module.put(
             globalObject,
             ZigString.static("beforeAll"),
-            JSC.NewRuntimeFunction(globalObject, ZigString.static("beforeAll"), 1, DescribeScope.beforeAll, false),
+            JSC.NewRuntimeFunction(globalObject, ZigString.static("beforeAll"), 1, DescribeScope.beforeAll, false, false),
         );
         module.put(
             globalObject,
             ZigString.static("beforeEach"),
-            JSC.NewRuntimeFunction(globalObject, ZigString.static("beforeEach"), 1, DescribeScope.beforeEach, false),
+            JSC.NewRuntimeFunction(globalObject, ZigString.static("beforeEach"), 1, DescribeScope.beforeEach, false, false),
         );
         module.put(
             globalObject,
             ZigString.static("afterAll"),
-            JSC.NewRuntimeFunction(globalObject, ZigString.static("afterAll"), 1, DescribeScope.afterAll, false),
+            JSC.NewRuntimeFunction(globalObject, ZigString.static("afterAll"), 1, DescribeScope.afterAll, false, false),
         );
         module.put(
             globalObject,
             ZigString.static("afterEach"),
-            JSC.NewRuntimeFunction(globalObject, ZigString.static("afterEach"), 1, DescribeScope.afterEach, false),
+            JSC.NewRuntimeFunction(globalObject, ZigString.static("afterEach"), 1, DescribeScope.afterEach, false, false),
         );
         module.put(
             globalObject,
diff --git a/test/js/bun/ffi/ffi.test.js b/test/js/bun/ffi/ffi.test.js
index 5cefafaf8..80367e555 100644
--- a/test/js/bun/ffi/ffi.test.js
+++ b/test/js/bun/ffi/ffi.test.js
@@ -580,7 +580,8 @@ function ffiRunner(fast) {
 }
 
 it("read", () => {
-  const buffer = new BigInt64Array(16);
+  // The usage of globalThis is a GC thing we should really fix
+  globalThis.buffer = new BigInt64Array(16);
   const dataView = new DataView(buffer.buffer);
   const addr = ptr(buffer);
 
@@ -607,6 +608,8 @@ it("read", () => {
     expect(read.u32(addr, i)).toBe(dataView.getUint32(i, true));
     expect(read.f32(addr, i)).toBe(dataView.getFloat32(i, true));
   }
+
+  delete globalThis.buffer;
 });
 
 if (ok) {
@@ -631,3 +634,10 @@ it("dlopen throws an error instead of returning it", () => {
 it('suffix does not start with a "."', () => {
   expect(suffix).not.toMatch(/^\./);
 });
+
+it(".ptr is not leaked", () => {
+  for (let fn of [Bun.password.hash, Bun.password.verify, it]) {
+    expect(fn).not.toHaveProperty("ptr");
+    expect(fn.ptr).toBeUndefined();
+  }
+});