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]); +});