diff --git a/src/jsc/bindings/JSFFIFunction.cpp b/src/jsc/bindings/JSFFIFunction.cpp index bbf982c37c78..e742d97c715c 100644 --- a/src/jsc/bindings/JSFFIFunction.cpp +++ b/src/jsc/bindings/JSFFIFunction.cpp @@ -104,6 +104,20 @@ extern "C" void Bun__FFIFunction_setDataPtr(JSC::EncodedJSValue jsValue, void* p function->dataPtr = ptr; } +// Called from FFI.Function.deinit (ffi.zig) before the per-function TCC state +// is deleted. Nulls out the native trampoline pointer so that any JS +// references to this function which are invoked after the owning library is +// closed will throw a TypeError instead of jumping into freed JIT memory. +extern "C" void Bun__FFIFunction_setClosed(JSC::EncodedJSValue jsValue) +{ + Zig::JSFFIFunction* function = dynamicDowncast(JSC::JSValue::decode(jsValue)); + if (!function) + return; + + function->setFunction(nullptr); + function->symbolFromDynamicLibrary = nullptr; +} + extern "C" JSC::EncodedJSValue Bun__CreateFFIFunctionValue(Zig::GlobalObject* globalObject, const ZigString* symbolName, unsigned argCount, Zig::FFIFunction functionPointer, bool addPtrField, void* symbolFromDynamicLibrary) { if (addPtrField) { @@ -158,23 +172,25 @@ JSFFIFunction* JSFFIFunction::create(VM& vm, Zig::GlobalObject* globalObject, un return function; } -#if OS(WINDOWS) - JSC_DEFINE_HOST_FUNCTION(JSFFIFunction::trampoline, (JSC::JSGlobalObject * globalObject, JSC::CallFrame* callFrame)) { const auto* function = uncheckedDowncast(callFrame->jsCallee()); - return function->function()(globalObject, callFrame); + auto native = function->function(); + if (!native) [[unlikely]] { + auto& vm = JSC::getVM(globalObject); + auto scope = DECLARE_THROW_SCOPE(vm); + return throwVMTypeError(globalObject, scope, "Cannot call an FFI function after the library has been closed"_s); + } + return native(globalObject, callFrame); } -#endif - JSFFIFunction* JSFFIFunction::createForFFI(VM& vm, Zig::GlobalObject* globalObject, unsigned length, const String& name, CFFIFunction FFIFunction) { -#if OS(WINDOWS) + // Always route through the static trampoline so the TinyCC-compiled + // function pointer lives in the mutable m_function field instead of being + // baked into a NativeExecutable. This lets us safely detach the native + // code when the library is closed (see Bun__FFIFunction_setClosed). NativeExecutable* executable = vm.getHostFunction(trampoline, ImplementationVisibility::Public, NoIntrinsic, trampoline, nullptr, name); -#else - NativeExecutable* executable = vm.getHostFunction(FFIFunction, ImplementationVisibility::Public, NoIntrinsic, FFIFunction, nullptr, name); -#endif Structure* structure = globalObject->FFIFunctionStructure(); JSFFIFunction* function = new (NotNull, allocateCell(vm)) JSFFIFunction(vm, executable, globalObject, structure, reinterpret_cast(WTF::move(FFIFunction))); function->finishCreation(vm, executable, length, name); diff --git a/src/jsc/bindings/JSFFIFunction.h b/src/jsc/bindings/JSFFIFunction.h index 6eccae7b6174..c6c9f54e244e 100644 --- a/src/jsc/bindings/JSFFIFunction.h +++ b/src/jsc/bindings/JSFFIFunction.h @@ -79,13 +79,14 @@ class JSFFIFunction final : public JSC::JSFunction { } const CFFIFunction function() const { return m_function; } + void setFunction(CFFIFunction function) { m_function = function; } -#if OS(WINDOWS) - + // All calls to FFI functions created via createForFFI() route through this + // trampoline, which dispatches to m_function. This indirection lets us + // null out m_function when the owning library is closed so that calling a + // symbol after close() throws instead of jumping into freed JIT memory. static JSC::EncodedJSValue JSC_HOST_CALL_ATTRIBUTES trampoline(JSGlobalObject* globalObject, CallFrame* callFrame); -#endif - void* dataPtr; void* symbolFromDynamicLibrary { nullptr }; diff --git a/src/runtime/ffi/ffi.zig b/src/runtime/ffi/ffi.zig index 59baaf42aab9..d369809c11ab 100644 --- a/src/runtime/ffi/ffi.zig +++ b/src/runtime/ffi/ffi.zig @@ -544,8 +544,12 @@ pub const FFI = struct { const SymbolsMap = struct { map: bun.StringArrayHashMapUnmanaged(Function) = .{}, pub fn deinit(this: *SymbolsMap) void { - for (this.map.keys()) |key| { - bun.default_allocator.free(@constCast(key)); + // Each map key aliases the Function's base_name allocation, which + // Function.deinit frees along with the per-symbol TCC state, + // arg_types, and (on the dlopen/linkSymbols/cc paths) the + // protect()ed JSFFIFunction root. + for (this.map.values()) |*value| { + value.deinitWithoutGlobal(); } this.map.clearAndFree(bun.default_allocator); } @@ -810,6 +814,9 @@ pub const FFI = struct { function.symbol_from_dynamic_library, ); compiled.js_function = cb; + // Rooted so Function.deinit can safely detach it on + // close() even if it was removed from the symbols object. + cb.protect(); obj.put(globalThis, &str, cb); }, } @@ -1110,8 +1117,7 @@ pub const FFI = struct { const resolved_symbol = dylib.lookup(*anyopaque, function_name) orelse { const ret = global.toInvalidArguments("Symbol \"{s}\" not found in \"{s}\"", .{ bun.asByteSlice(function_name), name }); for (symbols.values()) |*value| { - bun.default_allocator.free(@constCast(bun.asByteSlice(value.base_name.?))); - value.arg_types.clearAndFree(bun.default_allocator); + value.deinit(global); } symbols.clearAndFree(bun.default_allocator); dylib.close(); @@ -1136,11 +1142,10 @@ pub const FFI = struct { }; switch (function.step) { .failed => |err| { - defer for (symbols.values()) |*other_function| { - other_function.deinit(global); - }; - const res = ZigString.init(err.msg).toErrorInstance(global); + for (symbols.values()) |*other_function| { + other_function.deinit(global); + } symbols.clearAndFree(bun.default_allocator); dylib.close(); return res; @@ -1164,6 +1169,9 @@ pub const FFI = struct { function.symbol_from_dynamic_library, ); compiled.js_function = cb; + // Rooted so Function.deinit can safely detach it on + // close() even if it was removed from the symbols object. + cb.protect(); obj.put(global, &str, cb); }, } @@ -1223,8 +1231,7 @@ pub const FFI = struct { if (function.symbol_from_dynamic_library == null) { const ret = global.toInvalidArguments("Symbol \"{s}\" is missing a \"ptr\" field. When using linkSymbols() or CFunction(), you must provide a \"ptr\" field with the memory address of the native function.", .{bun.asByteSlice(function_name)}); for (symbols.values()) |*value| { - allocator.free(@constCast(bun.asByteSlice(value.base_name.?))); - value.arg_types.clearAndFree(allocator); + value.deinit(global); } symbols.clearAndFree(allocator); return ret; @@ -1243,20 +1250,16 @@ pub const FFI = struct { }; switch (function.step) { .failed => |err| { + const res = ZigString.init(err.msg).toErrorInstance(global); for (symbols.values()) |*value| { - allocator.free(@constCast(bun.asByteSlice(value.base_name.?))); - value.arg_types.clearAndFree(allocator); + value.deinit(global); } - - const res = ZigString.init(err.msg).toErrorInstance(global); - function.deinit(global); symbols.clearAndFree(allocator); return res; }, .pending => { for (symbols.values()) |*value| { - allocator.free(@constCast(bun.asByteSlice(value.base_name.?))); - value.arg_types.clearAndFree(allocator); + value.deinit(global); } symbols.clearAndFree(allocator); return ZigString.static("Failed to compile (nothing happend!)").toErrorInstance(global); @@ -1273,6 +1276,9 @@ pub const FFI = struct { function.symbol_from_dynamic_library, ); compiled.js_function = cb; + // Rooted so Function.deinit can safely detach it on + // close() even if it was removed from the symbols object. + cb.protect(); obj.put(global, name, cb); }, @@ -1459,8 +1465,13 @@ pub const FFI = struct { } extern "c" fn FFICallbackFunctionWrapper_destroy(*anyopaque) void; + extern "c" fn Bun__FFIFunction_setClosed(JSValue) void; + + pub fn deinit(val: *Function, _: *jsc.JSGlobalObject) void { + val.deinitWithoutGlobal(); + } - pub fn deinit(val: *Function, globalThis: *jsc.JSGlobalObject) void { + pub fn deinitWithoutGlobal(val: *Function) void { jsc.markBinding(@src()); if (val.base_name) |base_name| { @@ -1471,14 +1482,24 @@ pub const FFI = struct { val.arg_types.clearAndFree(val.allocator); - if (val.state) |state| { - state.deinit(); - val.state = null; - } - if (val.step == .compiled) { if (val.step.compiled.js_function != .zero) { - _ = globalThis; + // On the dlopen/linkSymbols/cc paths, js_function is the + // JSFFIFunction we created for this symbol (rooted via + // protect() at creation time) whose native trampoline + // lives in the TCC state about to be freed. Detach it so + // calling it throws instead of jumping into freed JIT + // memory, then release the GC root. + // + // On the JSCallback path (ffi_callback_function_wrapper + // != null), js_function is the user's callback which we + // merely borrowed — it is rooted via the wrapper's + // Strong and may itself be an unrelated FFI + // symbol, so it must not be detached here. + if (val.step.compiled.ffi_callback_function_wrapper == null) { + Bun__FFIFunction_setClosed(val.step.compiled.js_function); + val.step.compiled.js_function.unprotect(); + } val.step.compiled.js_function = .zero; } @@ -1488,6 +1509,11 @@ pub const FFI = struct { } } + if (val.state) |state| { + state.deinit(); + val.state = null; + } + if (val.step == .failed and val.step.failed.allocated) { val.allocator.free(val.step.failed.msg); } diff --git a/test/js/bun/ffi/ffi.test.js b/test/js/bun/ffi/ffi.test.js index 48b6c5c67910..809ba66ebc7d 100644 --- a/test/js/bun/ffi/ffi.test.js +++ b/test/js/bun/ffi/ffi.test.js @@ -1,6 +1,6 @@ import { afterAll, describe, expect, it } from "bun:test"; import { existsSync } from "fs"; -import { isGlibcVersionAtLeast } from "harness"; +import { isArm64, isGlibcVersionAtLeast, isWindows } from "harness"; import { platform } from "os"; import { @@ -8,6 +8,7 @@ import { CFunction, CString, JSCallback, + linkSymbols, ptr, read, suffix, @@ -970,3 +971,138 @@ describe.if(!!libPath)("can open more than 63 symbols via", () => { }); } }); + +// TinyCC (which compiles the FFI trampolines) is disabled on Windows ARM64. +describe.skipIf(isWindows && isArm64)("calling an FFI symbol after close()", () => { + // Regression test: calling a captured FFI symbol after its library has been + // closed must throw instead of jumping into the freed TinyCC JIT pages. + it("linkSymbols: throws instead of calling freed code", () => { + const cb = new JSCallback(x => x + 1, { args: ["i32"], returns: "i32" }); + try { + const lib = linkSymbols({ + inc: { args: ["i32"], returns: "i32", ptr: cb.ptr }, + }); + const inc = lib.symbols.inc; + const native = inc.native; + expect(inc(41)).toBe(42); + expect(native(41)).toBe(42); + + lib.close(); + + expect(() => inc(1)).toThrow(TypeError); + expect(() => inc(1)).toThrow("Cannot call an FFI function after the library has been closed"); + expect(() => native(1)).toThrow(TypeError); + + // close() is idempotent + lib.close(); + expect(() => inc(1)).toThrow(TypeError); + } finally { + cb.close(); + } + }); + + it("linkSymbols: zero-arg function throws instead of calling freed code", () => { + const cb = new JSCallback(() => 7, { returns: "i32" }); + try { + const lib = linkSymbols({ + seven: { returns: "i32", ptr: cb.ptr }, + }); + // zero-arg functions are not wrapped by FFIBuilder, so the symbol IS + // the native JSFFIFunction itself. + const seven = lib.symbols.seven; + expect(seven()).toBe(7); + + lib.close(); + + expect(() => seven()).toThrow(TypeError); + expect(() => seven()).toThrow("Cannot call an FFI function after the library has been closed"); + } finally { + cb.close(); + } + }); + + it("close() after symbol was deleted from lib.symbols and GC'd does not read a freed cell", () => { + // compiled.js_function is stored in the Zig heap; it must be rooted so + // that close() can safely detach it even if the user removed it from + // the (mutable) symbols object and a GC ran in between. + const cb = new JSCallback(() => 7, { returns: "i32" }); + try { + const lib = linkSymbols({ seven: { returns: "i32", ptr: cb.ptr } }); + const seven = lib.symbols.seven; + expect(seven()).toBe(7); + + delete lib.symbols.seven; + Bun.gc(true); + + // Must not read a freed/reused GC cell. + lib.close(); + + // `seven` was kept alive by our local reference and must have been + // detached by close(). + expect(() => seven()).toThrow(TypeError); + } finally { + cb.close(); + } + }); + + it("mid-loop failure unroots and detaches already-compiled symbols", () => { + // If linkSymbols fails on symbol N after symbols 0..N-1 have been + // compiled + protected, the cleanup path must run Function.deinit on + // each earlier symbol (unprotect + setClosed) rather than only freeing + // base_name/arg_types. Otherwise each earlier JSFFIFunction becomes a + // permanent GC root and its TCC state leaks. + const cb = new JSCallback(() => 7, { returns: "i32" }); + try { + let captured; + expect(() => { + captured = linkSymbols({ + good: { returns: "i32", ptr: cb.ptr }, + // missing `ptr` triggers the mid-loop error after `good` compiled + bad: { returns: "i32" }, + }); + }).toThrow(/"bad".*ptr/); + expect(captured).toBeUndefined(); + // No observable side effects; primary regression signal is no ASAN + // leak report and (once the process exits) a balanced gcProtect table. + } finally { + cb.close(); + } + }); + + it("JSCallback.close() does not detach an FFI symbol passed as the callback", () => { + // On the JSCallback path, Function.step.compiled.js_function holds the + // user's callback (which may itself be a JSFFIFunction from another + // still-open library). Closing the JSCallback must not null that + // symbol's trampoline. + const target = new JSCallback(() => 7, { returns: "i32" }); + try { + const lib = linkSymbols({ seven: { returns: "i32", ptr: target.ptr } }); + const seven = lib.symbols.seven; // raw JSFFIFunction + expect(seven()).toBe(7); + + const wrapper = new JSCallback(seven, { returns: "i32" }); + wrapper.close(); + + // lib is still open; seven must still work. + expect(seven()).toBe(7); + + lib.close(); + expect(() => seven()).toThrow(TypeError); + } finally { + target.close(); + } + }); + + it.if(!!libPath)("dlopen: throws instead of calling freed code", () => { + const lib = _dlopen(libPath, { strlen: { args: ["ptr"], returns: "usize" } }); + const strlen = lib.symbols.strlen; + const native = strlen.native; + expect(strlen(Buffer.from("bun\0"))).toBe(3n); + + lib.close(); + + expect(() => strlen(Buffer.from("bun\0"))).toThrow(TypeError); + expect(() => strlen(Buffer.from("bun\0"))).toThrow("Cannot call an FFI function after the library has been closed"); + expect(() => native(Buffer.from("bun\0"))).toThrow(TypeError); + }); +});