bun:ffi: throw a TypeError when a symbol is called after its library's close()

packages/bun-types/ffi.d.ts

@@ -536,7 +536,11 @@ declare module "bun:ffi" {
      *
      * Once called, the library is no longer usable.
      *
-     * Calling a function from a library that has been closed is undefined behavior.
+     * A symbol of a closed {@link dlopen} or {@link linkSymbols} library throws a `TypeError` when called.
+     * Calling a symbol of a closed {@link cc} library is undefined behavior.
+     *
+     * `close()` keeps a {@link dlopen} library loaded when a symbol of the library is running, for
+     * example when a {@link JSCallback} that the library calls closes the library. That call then finishes.
      */
     close(): void;
   }

scripts/build/deps/webkit.ts

@@ -3,7 +3,7 @@
  * for local mode. Override via `--webkit-version=<hash>` to test a branch.
  * From https://github.com/oven-sh/WebKit releases.
  */
-export const WEBKIT_VERSION = "f20ce7744553c910bcf16a33faf976af208de091";
+export const WEBKIT_VERSION = "autobuild-preview-pr-714-d4f2d385";
 
 /**
  * WebKit (JavaScriptCore) — the JS engine.

src/jsc/bindings/JSCFFIBridge.cpp

@@ -8,10 +8,12 @@
 #include <JavaScriptCore/JSFFICallback.h>
 #include <JavaScriptCore/JSFFIFunction.h>
 #include "ScriptExecutionContext.h"
+#include <JavaScriptCore/JSArray.h>
 #include <JavaScriptCore/JSCJSValueInlines.h>
 #include <JavaScriptCore/JSCast.h>
 #include <JavaScriptCore/JSObject.h>
 
+#include "ZigGeneratedClasses.h"
 #include "ZigGlobalObject.h"
 #include "headers-handwritten.h"
 
@@ -21,6 +23,13 @@ static_assert(static_cast<uint8_t>(JSC::FFI::Type::JSValue) == 19, "FFI::Type ta
 static_assert(static_cast<uint8_t>(JSC::FFI::Type::Buffer) == 20, "FFI::Type tag drift");
 static_assert(static_cast<uint8_t>(JSC::FFI::Type::BufferLength) == 21, "FFI::Type tag drift");
 
+// The functions a library made, which close() closes. `symbols` cannot be that list: the user can change it.
+static JSC::JSArray* functionsOfLibrary(WebCore::JSFFI* library)
+{
+    JSC::JSValue functions = library->m_functionsValue.get();
+    return functions ? dynamicDowncast<JSC::JSArray>(functions) : nullptr;
+}
+
 extern "C" JSC::EncodedJSValue Bun__CreateJSCFFIFunction(
     Zig::GlobalObject* globalObject,
     const EncodedSlice* symbolName,
@@ -52,9 +61,43 @@ extern "C" JSC::EncodedJSValue Bun__CreateJSCFFIFunction(
     if (!function)
         RELEASE_AND_RETURN(scope, {});
 
+    if (auto* library = dynamicDowncast<WebCore::JSFFI>(owner)) {
+        auto* functions = functionsOfLibrary(library);
+        if (!functions) {
+            functions = JSC::constructEmptyArray(globalObject, nullptr);
+            RETURN_IF_EXCEPTION(scope, {});
+            library->m_functionsValue.set(vm, library, functions);
+        }
+        // Not push(): that is a [[Set]], and a setter on a prototype would be handed this array.
+        functions->putDirectIndex(globalObject, functions->length(), function);
+        RETURN_IF_EXCEPTION(scope, {});
+    }
+
     RELEASE_AND_RETURN(scope, JSC::JSValue::encode(function));
 }
 
+// Returns true if one of the functions is running. Then the caller must keep the library loaded.
+extern "C" bool Bun__JSCFFILibraryCloseFunctions(Zig::GlobalObject* globalObject, JSC::EncodedJSValue libraryValue)
+{
+    auto& vm = JSC::getVM(globalObject);
+    auto* library = dynamicDowncast<WebCore::JSFFI>(JSC::JSValue::decode(libraryValue));
+    if (!library)
+        return false;
+    auto* functions = functionsOfLibrary(library);
+    if (!functions)
+        return false;
+    bool isRunning = false;
+    for (unsigned i = 0, length = functions->length(); i < length; ++i) {
+        JSC::JSValue value = functions->tryGetIndexQuickly(i);
+        if (auto* function = value ? dynamicDowncast<JSC::JSFFIFunction>(value) : nullptr) {
+            function->close(vm);
+            isRunning = isRunning || function->isRunning(vm);
+        }
+    }
+    library->m_functionsValue.clear();
+    return isRunning;
+}
+
 static void Bun__jscFFIThreadsafeDispatch(JSC::FFI::ThreadsafeInvocation& invocation)
 {
     static_assert(sizeof(WebCore::ScriptExecutionContextIdentifier) <= sizeof(void*));

src/runtime/ffi/ffi.classes.ts

@@ -18,6 +18,6 @@ export default [
         getter: "getSymbols",
       },
     },
-    values: ["symbolsValue"],
+    values: ["symbolsValue", "functionsValue"],
   }),
 ];

src/runtime/ffi/ffi_body.rs

@@ -113,6 +113,8 @@ unsafe extern "C" {
         return_type: u8,
         threadsafe: bool,
     ) -> JSValue;
+
+    fn Bun__JSCFFILibraryCloseFunctions(global: *const JSGlobalObject, library: JSValue) -> bool;
 }
 
 /// `Ok(JSValue::ZERO)` is the C++ side's "could not create" without a
@@ -1309,8 +1311,19 @@ impl FFI {
     }
 
     #[bun_jsc::host_fn(method)]
-    pub(crate) fn close(&self, _global_this: &JSGlobalObject, _: &CallFrame) -> JsResult<JSValue> {
+    pub(crate) fn close(
+        &self,
+        global_this: &JSGlobalObject,
+        callframe: &CallFrame,
+    ) -> JsResult<JSValue> {
         jsc::mark_binding();
+        // Before `do_close` unloads the code the functions point into.
+        // SAFETY: thin FFI wrapper; the C++ side type-checks the cell (dynamicDowncast) before use.
+        let is_running = unsafe { Bun__JSCFFILibraryCloseFunctions(global_this, callframe.this()) };
+        if is_running {
+            // The running call returns into the library, so `do_close` must find none to unload.
+            self.dylib.set(None);
+        }
         self.do_close();
         Ok(JSValue::UNDEFINED)
     }

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

@@ -161,3 +161,6 @@ FFI_EXPORT int64_t cb_identity_neg_42_int64_t(int64_t (*cb)()) { return cb(); }
 
 FFI_EXPORT uint64_t bl_echo_len(const void *buf, uint64_t len) { (void)buf; return len; }
 FFI_EXPORT uint32_t bl_last_byte(const uint8_t *buf, uint64_t len) { return len ? buf[len - 1] : 999u; }
+
+// Not a tail call: the callback returns into this function.
+FFI_EXPORT int32_t cb_result_plus_int32_t(int32_t (*cb)(), int32_t a) { return cb() + a; }

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

@@ -1,7 +1,8 @@
 import { afterAll, describe, expect, it } from "bun:test";
-import { existsSync } from "fs";
+import { copyFileSync, existsSync } from "fs";
 import { bunEnv, bunExe, compileFixture, isDebug, isGlibcVersionAtLeast, isWindows, tempDir } from "harness";
 import { platform } from "os";
+import { join } from "path";
 
 import {
   cc,
@@ -968,6 +969,209 @@ it.skipIf(!FFI_FIXTURE_PATH)("ptr argument: ArrayBuffer cells through an FTL-com
   expect({ stdout, stderr, exitCode }).toEqual({ stdout: "mismatches 0\n", stderr: "", exitCode: 0 });
 });
 
+// close() unloads the library, so a call after it used to jump to an unmapped address. The function
+// objects outlive close() (a destructured symbol, a caller the JIT compiled with the address baked in),
+// so each of them has to throw. Runs in a child process: without the fix the first call after close()
+// is a segfault.
+it.skipIf(!FFI_FIXTURE_PATH)("a symbol called after its library's close() throws a TypeError", async () => {
+  await using proc = Bun.spawn({
+    cmd: [
+      bunExe(),
+      "-e",
+      `import { dlopen, linkSymbols } from "bun:ffi";
+      const definitions = {
+        add_int32_t: { args: ["i32", "i32"], returns: "i32" },
+        returns_true: { args: [], returns: "bool" },
+        identity_ptr: { args: ["ptr"], returns: "ptr" },
+      };
+      const open = () => dlopen(process.env.FFI_FIXTURE_PATH, definitions);
+      const results = {};
+      const attempt = (name, fn) => {
+        try {
+          results[name] = "returned " + fn();
+        } catch (e) {
+          results[name] = e.name + ": " + e.message;
+        }
+      };
+
+      {
+        const lib = open();
+        const { add_int32_t, returns_true } = lib.symbols;
+        function hot(a, b) { return add_int32_t(a, b); }
+        let sum = 0;
+        for (let i = 0; i < 200_000; i++) sum += hot(i, 1);
+        results["before close"] = sum;
+
+        lib.close();
+        lib.close();
+        attempt("optimized caller", () => hot(1, 2));
+        attempt("destructured symbol", () => add_int32_t(1, 2));
+        attempt("through lib.symbols", () => lib.symbols.returns_true());
+        attempt("Function.prototype.call", () => add_int32_t.call(null, 1, 2));
+        attempt("arguments that do not convert", () => add_int32_t("x", Symbol()));
+      }
+
+      {
+        // close() does not find the functions through lib.symbols, which the user can change.
+        const lib = open();
+        const { add_int32_t } = lib.symbols;
+        delete lib.symbols.add_int32_t;
+        lib.close();
+        attempt("symbol deleted from lib.symbols before close", () => add_int32_t(1, 2));
+      }
+
+      {
+        // An argument conversion can run JS, so close() can happen after the call has started.
+        const lib = open();
+        const { add_int32_t } = lib.symbols;
+        let armed = false;
+        const closer = { valueOf() { if (armed) lib.close(); return 1; } };
+        function hot(a, b) { return add_int32_t(a, b); }
+        for (let i = 0; i < 20_000; i++) hot(i & 1 ? closer : 1, 1);
+        armed = true;
+        attempt("close inside valueOf, optimized caller", () => hot(closer, 2));
+      }
+
+      {
+        const lib = open();
+        const { add_int32_t } = lib.symbols;
+        attempt("close inside valueOf", () => add_int32_t({ valueOf() { lib.close(); return 1; } }, 2));
+      }
+
+      {
+        // close() is per handle.
+        const first = open();
+        const second = open();
+        first.close();
+        attempt("closed handle of a file that is open twice", () => first.symbols.add_int32_t(40, 2));
+        results["open handle of a file that is open twice"] = second.symbols.add_int32_t(40, 2);
+
+        // The native callee would call the closed function's address.
+        attempt("closed function as a pointer argument", () => second.symbols.identity_ptr(first.symbols.add_int32_t));
+
+        const linked = linkSymbols({ sum: { ...definitions.add_int32_t, ptr: second.symbols.add_int32_t.ptr } });
+        const { sum } = linked.symbols;
+        results["linkSymbols before close"] = sum(40, 2);
+        linked.close();
+        attempt("linkSymbols", () => sum(40, 2));
+      }
+
+      {
+        // Nor can a setter on a prototype reach the functions close() has to find. Last: an indexed
+        // accessor on a prototype switches every array in the realm to slow puts.
+        let setterCalls = 0;
+        Object.defineProperty(Object.prototype, 0, { get() {}, set(v) { setterCalls++; }, configurable: true });
+        const lib = open();
+        delete Object.prototype[0];
+        const { add_int32_t } = lib.symbols;
+        lib.close();
+        attempt("indexed setter on Object.prototype during dlopen", () => add_int32_t(1, 2));
+        results["indexed setter calls"] = setterCalls;
+      }
+
+      console.log(JSON.stringify(results, null, 2));`,
+    ],
+    env: { ...bunEnv, FFI_FIXTURE_PATH },
+    stdout: "pipe",
+    stderr: "pipe",
+  });
+  const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+  const closed = name => `TypeError: bun:ffi: cannot call '${name}' because its library was closed`;
+  expect({ results: JSON.parse(stdout || "null"), stderr, exitCode }).toEqual({
+    results: {
+      "before close": 20000100000,
+      "optimized caller": closed("add_int32_t"),
+      "destructured symbol": closed("add_int32_t"),
+      "through lib.symbols": closed("returns_true"),
+      "Function.prototype.call": closed("add_int32_t"),
+      "arguments that do not convert": closed("add_int32_t"),
+      "symbol deleted from lib.symbols before close": closed("add_int32_t"),
+      "close inside valueOf, optimized caller": closed("add_int32_t"),
+      "close inside valueOf": closed("add_int32_t"),
+      "closed handle of a file that is open twice": closed("add_int32_t"),
+      "open handle of a file that is open twice": 42,
+      "closed function as a pointer argument":
+        "TypeError: bun:ffi: cannot pass 'add_int32_t' as a pointer because its library was closed",
+      "linkSymbols before close": 42,
+      "linkSymbols": closed("sum"),
+      "indexed setter on Object.prototype during dlopen": closed("add_int32_t"),
+      "indexed setter calls": 0,
+    },
+    stderr: "",
+    exitCode: 0,
+  });
+});
+
+// The library calls a JSCallback, and the callback closes the library. That call returns into the
+// library, so the library has to stay loaded. Without that the child dies when the callback returns.
+it.skipIf(!FFI_FIXTURE_PATH)("close() inside a call of the library lets the call finish", async () => {
+  // Each case has a copy of the library to itself: a library that stays loaded would hide a later unload.
+  using dir = tempDir("ffi-close-inside-a-call", {});
+  const copies = ["cold", "warm", "tail-call"].map(name => {
+    const copy = join(String(dir), name + "." + suffix);
+    copyFileSync(FFI_FIXTURE_PATH, copy);
+    return copy;
+  });
+  await using proc = Bun.spawn({
+    cmd: [
+      bunExe(),
+      "-e",
+      `import { dlopen, JSCallback } from "bun:ffi";
+      import { noInline } from "bun:jsc";
+      const results = {};
+      for (const [what, warmUp, copy] of [
+        ["cold", 0, process.env.FFI_FIXTURE_COPY_1],
+        ["warm", ${isDebug ? 5_000 : 20_000}, process.env.FFI_FIXTURE_COPY_2],
+        ["warm, tail call", ${isDebug ? 5_000 : 20_000}, process.env.FFI_FIXTURE_COPY_3],
+      ]) {
+        const lib = dlopen(copy, { cb_result_plus_int32_t: { args: ["callback", "i32"], returns: "i32" } });
+        const { cb_result_plus_int32_t } = lib.symbols;
+        let armed = false;
+        const closeIfArmed = () => { if (armed) lib.close(); };
+        noInline(closeIfArmed);
+        // A module is strict code, so a function that ends in a call makes a tail call. Optimized
+        // code inlines finish() in place of the callback's frame, and the stack then has no frame
+        // that the library called.
+        const finish = () => { closeIfArmed(); return 40; };
+        const callback = new JSCallback(
+          what.includes("tail call") ? () => finish() : () => { closeIfArmed(); return 40; },
+          { args: [], returns: "i32" },
+        );
+        function hot() { return cb_result_plus_int32_t(callback, 2); }
+        for (let i = 0; i < warmUp; i++) hot();
+        armed = true;
+        results[what] = { callThatCloses: hot() };
+        try {
+          results[what].nextCall = "returned " + hot();
+        } catch (e) {
+          results[what].nextCall = e.name + ": " + e.message;
+        }
+        callback.close();
+      }
+      console.log(JSON.stringify(results, null, 2));`,
+    ],
+    env: {
+      ...bunEnv,
+      FFI_FIXTURE_PATH,
+      FFI_FIXTURE_COPY_1: copies[0],
+      FFI_FIXTURE_COPY_2: copies[1],
+      FFI_FIXTURE_COPY_3: copies[2],
+    },
+    stdout: "pipe",
+    stderr: "pipe",
+  });
+  const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+  const expected = {
+    callThatCloses: 42,
+    nextCall: "TypeError: bun:ffi: cannot call 'cb_result_plus_int32_t' because its library was closed",
+  };
+  expect({ results: JSON.parse(stdout || "null"), stderr, exitCode }).toEqual({
+    results: { "cold": expected, "warm": expected, "warm, tail call": expected },
+    stderr: "",
+    exitCode: 0,
+  });
+});
+
 it("worker teardown drops queued threadsafe JSCallback invocations without crashing", async () => {
   using dir = tempDir("ffi-jscallback-terminate-queued", {
     "main.js": `