From b984e4a719e9596b7afbe256f69d68f3e53dbe17 Mon Sep 17 00:00:00 2001 From: Trivikram Kamat <16024985+trivikr@users.noreply.github.com> Date: Fri, 31 Jul 2026 01:34:27 +0000 Subject: [PATCH] ffi: throw on fast calls to a closed library Close() marks every FFIFunction closed, but the generated fast-call path never sees it: V8 embeds the trampoline address when it compiles the call site, so an optimized call jumps into the library that dlclose() unloaded. Freeing the trampoline in Close() instead would leave compiled code calling unmapped memory. Give each library a one-byte flag that Close() sets and test it in the JavaScript wrapper before entering the trampoline. Fast signatures that need no argument conversion were handed to user code as the raw function, so they now get a guard-only wrapper too. Signed-off-by: Trivikram Kamat <16024985+trivikr@users.noreply.github.com> Assisted-by: claude:opus-5 --- doc/api/ffi.md | 4 +- lib/internal/ffi/fast-api.js | 47 +++++++++- src/env_properties.h | 1 + src/node_ffi.cc | 43 ++++++++- src/node_ffi.h | 5 ++ test/ffi/test-ffi-closed-fast-calls.js | 119 +++++++++++++++++++++++++ 6 files changed, 215 insertions(+), 4 deletions(-) create mode 100644 test/ffi/test-ffi-closed-fast-calls.js diff --git a/doc/api/ffi.md b/doc/api/ffi.md index 94dc9e42f03d..6982c2d81bde 100644 --- a/doc/api/ffi.md +++ b/doc/api/ffi.md @@ -322,7 +322,8 @@ Calling `library.close()` (or disposing the library) more than once is a no-op. After a library has been closed: -* Resolved function wrappers become invalid. +* Resolved function wrappers become invalid. Calling one throws + [`ERR_FFI_LIBRARY_CLOSED`][], including on optimized call sites. * Further symbol and function resolution throws. * Registered callbacks are invalidated. @@ -757,6 +758,7 @@ and keep callback and pointer lifetimes explicit on the native side. [Permission Model]: permissions.md#permission-model [`--allow-ffi`]: cli.md#--allow-ffi +[`ERR_FFI_LIBRARY_CLOSED`]: errors.md#err_ffi_library_closed [`ffi.toBuffer(pointer, length, copy)`]: #ffitobufferpointer-length-copy [`using`]: https://tc39.es/proposal-explicit-resource-management/#sec-using-declarations [type names]: #type-names diff --git a/lib/internal/ffi/fast-api.js b/lib/internal/ffi/fast-api.js index ebfaed92b27d..7b11758ede1b 100644 --- a/lib/internal/ffi/fast-api.js +++ b/lib/internal/ffi/fast-api.js @@ -2,6 +2,7 @@ const { ArrayPrototypeIncludes, + Error, NumberIsInteger, ObjectDefineProperty, ReflectApply, @@ -9,6 +10,8 @@ const { TypeError, } = primordials; +const assert = require('internal/assert'); + const { Buffer, } = require('buffer'); @@ -21,6 +24,7 @@ const { const { charIsSigned, getRawPointer, + kClosedFlag, kFastArguments, kFastBufferInvoke, } = internalBinding('ffi'); @@ -68,6 +72,31 @@ function throwFFIArgCountError(expected, actual) { `Invalid argument count: expected ${expected}, got ${actual}`); } +// Mirrors THROW_ERR_FFI_LIBRARY_CLOSED in src/node_errors.h. The native +// `closed` checks in InvokeFunction/InvokeFunctionSB cannot run on the fast-call +// path: V8 embeds the trampoline address in optimized code, so a tiered-up call +// site jumps straight into the library that `close()` unloaded. +function throwLibraryClosed() { + // eslint-disable-next-line no-restricted-syntax + const err = new Error('Library is closed'); + err.code = 'ERR_FFI_LIBRARY_CLOSED'; + throw err; +} + +// Guard-only wrappers for fast signatures that need no argument conversion. +// The flag is a one-byte Uint8Array, so the check is an element load that V8 +// cannot fold away across the call. +function wrapWithClosedGuard(rawFn, nargs, closedFlag) { + function wrapper(...args) { + if (args.length !== nargs) { + throwFFIArgCountError(nargs, args.length); + } + if (closedFlag[0] !== 0) throwLibraryClosed(); + return ReflectApply(rawFn, undefined, args); + } + return inheritMetadata(wrapper, rawFn, nargs); +} + function validateFastIntegerArg(type, value, index) { const info = fastIntegerTypeInfo[type]; if (info === undefined) return; @@ -202,7 +231,7 @@ function inheritMetadata(wrapper, rawFn, nargs) { return wrapper; } -function wrapWithRawPointerConversions(rawFn, argumentTypes, _owner) { +function wrapWithRawPointerConversions(rawFn, argumentTypes, owner) { if (rawFn === undefined || rawFn === null) { return rawFn; } @@ -213,9 +242,19 @@ function wrapWithRawPointerConversions(rawFn, argumentTypes, _owner) { return rawFn; } + // `kFastArguments` is attached by `CreateFunction` to every function that got + // a fast-call trampoline, so reaching here means the raw function must never + // be exposed unguarded: after `close()` the trampoline still points into the + // unloaded library and only a JavaScript-side check can stop the call. + // `DynamicLibrary::New` installs the flag on every instance, so a missing one + // means the native side and this wrapper are out of sync. + const closedFlag = owner?.[kClosedFlag]; + assert(closedFlag !== undefined, + 'FFI: fast function is missing the owning library closed flag'); + const indexes = getFastArgumentIndexes(argumentTypes); if (indexes === null) { - return rawFn; + return wrapWithClosedGuard(rawFn, argumentTypes.length, closedFlag); } const stringState = { @@ -236,6 +275,7 @@ function wrapWithRawPointerConversions(rawFn, argumentTypes, _owner) { if (arguments.length !== 1) { throwFFIArgCountError(1, arguments.length); } + if (closedFlag[0] !== 0) throwLibraryClosed(); validateFastIntegerArg(t0, a0, 0); let arg = a0; if (needsNullPointerConversion(t0) && @@ -265,6 +305,7 @@ function wrapWithRawPointerConversions(rawFn, argumentTypes, _owner) { if (arguments.length !== 2) { throwFFIArgCountError(2, arguments.length); } + if (closedFlag[0] !== 0) throwLibraryClosed(); const stringCall = (c0 && hasStringPointerArg(t0, a0)) || (c1 && hasStringPointerArg(t1, a1)); if (stringCall) enterStringConversion(stringState); @@ -286,6 +327,7 @@ function wrapWithRawPointerConversions(rawFn, argumentTypes, _owner) { if (arguments.length !== 3) { throwFFIArgCountError(3, arguments.length); } + if (closedFlag[0] !== 0) throwLibraryClosed(); const stringCall = (c0 && hasStringPointerArg(t0, a0)) || (c1 && hasStringPointerArg(t1, a1)) || (c2 && hasStringPointerArg(t2, a2)); @@ -303,6 +345,7 @@ function wrapWithRawPointerConversions(rawFn, argumentTypes, _owner) { if (args.length !== nargs) { throwFFIArgCountError(nargs, args.length); } + if (closedFlag[0] !== 0) throwLibraryClosed(); let stringCall = false; for (let i = 0; i < indexes.length; i++) { const index = indexes[i]; diff --git a/src/env_properties.h b/src/env_properties.h index e3179287dce4..bce03aa147d7 100644 --- a/src/env_properties.h +++ b/src/env_properties.h @@ -52,6 +52,7 @@ V(ffi_sb_return_symbol, "ffi_sb_return_symbol") \ V(ffi_fast_arguments_symbol, "ffi_fast_arguments_symbol") \ V(ffi_fast_buffer_invoke_symbol, "ffi_fast_buffer_invoke_symbol") \ + V(ffi_closed_flag_symbol, "ffi_closed_flag_symbol") \ V(constructor_key_symbol, "constructor_key_symbol") \ V(handle_onclose_symbol, "handle_onclose") \ V(no_message_symbol, "no_message_symbol") \ diff --git a/src/node_ffi.cc b/src/node_ffi.cc index b05d09270126..b3a07620b0aa 100644 --- a/src/node_ffi.cc +++ b/src/node_ffi.cc @@ -38,6 +38,7 @@ using v8::PropertyAttribute; using v8::ReadOnly; using v8::String; using v8::TryCatch; +using v8::Uint8Array; using v8::Value; namespace ffi { @@ -72,6 +73,8 @@ void DynamicLibrary::MemoryInfo(MemoryTracker* tracker) const { tracker->TrackFieldWithSize( "symbols", symbols_size, "std::unordered_map"); + tracker->TrackField("closed_flag", closed_flag_); + // FFIFunctionInfo instances and their sb_backing ArrayBuffers are // owned by V8 function wrappers and reachable only via weak references, // so they are deliberately not counted here. @@ -83,6 +86,16 @@ void DynamicLibrary::Close() { fn->ptr = nullptr; } + // Generated fast-call trampolines are not torn down here. V8 bakes the + // trampoline address into optimized code, so releasing it while a compiled + // call site still holds it would replace a call into the unloaded library + // with a call into unmapped memory. The trampoline stays mapped for the + // lifetime of its FFIFunctionInfo and is instead kept unreachable by the + // JavaScript-side check on this flag. + if (closed_flag_ != nullptr) { + *static_cast(closed_flag_->Data()) = 1; + } + // Closing the library invalidates all registered callbacks. Node.js does not // track or revoke callback pointers that have already been handed to native // code. If native code calls a callback pointer after `close()` or @@ -383,10 +396,16 @@ MaybeLocal DynamicLibrary::CreateFunction( } } - if (needs_fast_argument_wrapper || needs_fast_buffer_invoke) { + if (use_fast_api) { // Fast API wrappers need only the parameter type names. Result conversion // is still handled by V8's CFunction metadata, unlike the SharedBuffer path // which must also know how to read slot 0. + // + // These names are attached to every fast function, not just the ones that + // need `needs_fast_argument_wrapper` or `needs_fast_buffer_invoke` + // conversions, because their presence is also how the JavaScript side + // recognizes a function that can reach a trampoline and therefore needs the + // closed-library guard. Local arguments_arr; if (!ToV8Value(context, fn->arg_type_names, isolate) .ToLocal(&arguments_arr)) { @@ -479,6 +498,21 @@ void DynamicLibrary::New(const FunctionCallbackInfo& args) { THROW_ERR_FFI_CALL_FAILED(env, "dlopen failed: %s", uv_dlerror(&lib->lib_)); return; } + + // Publish the closed flag on the instance for the JS-side fast-call guard. + // The backing store is retained so `Close()` can still write the flag after + // the view is collected, and so it outlives any wrapper closure reading it. + Local flag = ArrayBuffer::New(env->isolate(), 1); + lib->closed_flag_ = flag->GetBackingStore(); + if (!args.This() + ->DefineOwnProperty( + env->context(), + env->ffi_closed_flag_symbol(), + Uint8Array::New(flag, 0, 1), + static_cast(ReadOnly | DontEnum | DontDelete)) + .FromMaybe(false)) { + lib->Close(); + } } void DynamicLibrary::Close(const FunctionCallbackInfo& args) { @@ -1323,6 +1357,13 @@ static void Initialize(Local target, FIXED_ONE_BYTE_STRING(isolate, "kFastBufferInvoke"), env->ffi_fast_buffer_invoke_symbol()) .Check(); + // Keys the per-library closed flag that fast wrappers test before calling a + // generated trampoline. + target + ->Set(context, + FIXED_ONE_BYTE_STRING(isolate, "kClosedFlag"), + env->ffi_closed_flag_symbol()) + .Check(); } } // namespace ffi diff --git a/src/node_ffi.h b/src/node_ffi.h index a55cb74fc619..c96d5792470d 100644 --- a/src/node_ffi.h +++ b/src/node_ffi.h @@ -145,6 +145,11 @@ class DynamicLibrary : public BaseObject { bool is_closed() const; uv_lib_t lib_ = {}; + // Single-byte flag shared with JavaScript, set to 1 by `Close()`. The + // `closed` checks in the C++ invokers cannot run once V8 has compiled a + // fast-call site, because the trampoline address is embedded in the + // optimized code, so the JS wrapper tests this flag before entering it. + std::shared_ptr closed_flag_; std::string path_; std::unordered_map symbols_; std::unordered_map> functions_; diff --git a/test/ffi/test-ffi-closed-fast-calls.js b/test/ffi/test-ffi-closed-fast-calls.js new file mode 100644 index 000000000000..ac6c7335e251 --- /dev/null +++ b/test/ffi/test-ffi-closed-fast-calls.js @@ -0,0 +1,119 @@ +// Flags: --experimental-ffi --allow-natives-syntax +'use strict'; + +// Regression test for calls that reach a generated fast-call trampoline after +// the owning library has been closed. +// +// `DynamicLibrary::Close()` sets `FFIFunction::closed` and clears +// `FFIFunction::ptr`, and both C++ invokers check them. Neither check can run on +// the fast-call path: the trampoline is created with the resolved symbol address +// and V8 embeds the trampoline address in optimized code, so a call site that +// has tiered up jumps straight into the unloaded library. + +const common = require('../common'); + +common.skipIfFFIMissing(); + +const assert = require('node:assert'); +const { test } = require('node:test'); +const ffi = require('node:ffi'); +const { cString, fixtureSymbols, libraryPath } = require('./ffi-test-common'); + +// Every function that has a trampoline is exposed as a JavaScript wrapper, and +// that wrapper holds the fast-call site, so it is the function that has to be +// optimized to put the trampoline in play. Optimizing only an outer caller does +// not reach it, and neither does calling after `close()`: a call site that +// always throws never tiers up, which is why a closed library used to survive a +// single call and then crash under sustained use. +function optimizeFastCall(fn, args) { + assert.doesNotMatch(fn.toString(), /\[native code\]/, + 'FFI functions with a fast-call trampoline must be ' + + 'exposed as JavaScript wrappers'); + eval('%PrepareFunctionForOptimization(fn)'); + fn(...args); + fn(...args); + eval('%OptimizeFunctionOnNextCall(fn)'); + fn(...args); +} + +// The guard must hold for repeated calls, not just the first one. +function assertAlwaysClosed(fn, args) { + assert.throws(() => fn(...args), { code: 'ERR_FFI_LIBRARY_CLOSED' }); + + for (let i = 0; i < 1000; i++) { + let code; + try { + fn(...args); + } catch (err) { + code = err.code; + } + assert.strictEqual(code, 'ERR_FFI_LIBRARY_CLOSED'); + } +} + +test('closed library throws from optimized scalar fast calls', () => { + const { lib, functions } = ffi.dlopen(libraryPath, fixtureSymbols); + const multiply = functions.multiply_f64; + + optimizeFastCall(multiply, [2, 3]); + assert.strictEqual(multiply(2, 3), 6); + + lib.close(); + assertAlwaysClosed(multiply, [2, 3]); +}); + +test('closed library throws from optimized integer fast calls', () => { + const { lib, functions } = ffi.dlopen(libraryPath, fixtureSymbols); + const add = functions.add_i32; + + optimizeFastCall(add, [20, 22]); + assert.strictEqual(add(20, 22), 42); + + lib.close(); + assertAlwaysClosed(add, [20, 22]); +}); + +test('closed library throws from optimized pointer fast calls', () => { + const { lib, functions } = ffi.dlopen(libraryPath, fixtureSymbols); + const strlen = functions.safe_strlen; + const buffer = cString('closed'); + + optimizeFastCall(strlen, [buffer]); + assert.strictEqual(strlen(buffer), 6); + + lib.close(); + assertAlwaysClosed(strlen, [buffer]); +}); + +test('closed library throws from optimized multi-argument fast calls', () => { + const { lib, functions } = ffi.dlopen(libraryPath, fixtureSymbols); + const sum = functions.sum_five_f64; + + optimizeFastCall(sum, [1, 2, 3, 4, 5]); + assert.strictEqual(sum(1, 2, 3, 4, 5), 15); + + lib.close(); + assertAlwaysClosed(sum, [1, 2, 3, 4, 5]); +}); + +test('closed library throws for functions from getFunction()', () => { + const lib = new ffi.DynamicLibrary(libraryPath); + const multiply = lib.getFunction('multiply_f64', fixtureSymbols.multiply_f64); + + optimizeFastCall(multiply, [2, 3]); + assert.strictEqual(multiply(2, 3), 6); + + lib.close(); + assertAlwaysClosed(multiply, [2, 3]); +}); + +test('disposing a library invalidates retained fast wrappers', () => { + let multiply; + { + using handle = ffi.dlopen(libraryPath, fixtureSymbols); + multiply = handle.functions.multiply_f64; + optimizeFastCall(multiply, [2, 3]); + } + + assertAlwaysClosed(multiply, [2, 3]); +});