From f4027b3dc6eceecf09a5b12478019f05f083c9cf Mon Sep 17 00:00:00 2001 From: Bryan English Date: Tue, 21 Apr 2026 14:01:12 -0400 Subject: [PATCH 1/4] benchmark: add benchmarks for ffi module Adds microbenchmarks covering the common FFI call shapes so future changes to the invoker can be evaluated: - add-i32.js: 2-arg integer - add-f64.js: 2-arg float - many-args.js: 6-arg integer - pointer-bigint.js: 1-arg pointer (BigInt) - sum-buffer.js: pointer + length (Buffer) A `common.js` helper resolves the fixture-library path from `test/ffi/fixture_library` without pulling in the test harness, and throws a clear message if the fixture hasn't been built yet. Also adds `sum_6_i32` to the fixture library for the many-args case. --- benchmark/ffi/add-f64.js | 28 +++++++++++++++++ benchmark/ffi/add-i32.js | 28 +++++++++++++++++ benchmark/ffi/common.js | 24 +++++++++++++++ benchmark/ffi/many-args.js | 28 +++++++++++++++++ benchmark/ffi/pointer-bigint.js | 28 +++++++++++++++++ benchmark/ffi/sum-buffer.js | 33 +++++++++++++++++++++ test/ffi/fixture_library/ffi_test_library.c | 4 +++ 7 files changed, 173 insertions(+) create mode 100644 benchmark/ffi/add-f64.js create mode 100644 benchmark/ffi/add-i32.js create mode 100644 benchmark/ffi/common.js create mode 100644 benchmark/ffi/many-args.js create mode 100644 benchmark/ffi/pointer-bigint.js create mode 100644 benchmark/ffi/sum-buffer.js diff --git a/benchmark/ffi/add-f64.js b/benchmark/ffi/add-f64.js new file mode 100644 index 00000000000000..fab6457e33a09d --- /dev/null +++ b/benchmark/ffi/add-f64.js @@ -0,0 +1,28 @@ +'use strict'; + +const common = require('../common.js'); +const ffi = require('node:ffi'); +const { libraryPath, ensureFixtureLibrary } = require('./common.js'); + +const bench = common.createBenchmark(main, { + n: [1e7], +}, { + flags: ['--experimental-ffi'], +}); + +ensureFixtureLibrary(); + +const { lib, functions } = ffi.dlopen(libraryPath, { + add_f64: { result: 'f64', parameters: ['f64', 'f64'] }, +}); + +const add = functions.add_f64; + +function main({ n }) { + bench.start(); + for (let i = 0; i < n; ++i) + add(1.5, 2.5); + bench.end(n); + + lib.close(); +} diff --git a/benchmark/ffi/add-i32.js b/benchmark/ffi/add-i32.js new file mode 100644 index 00000000000000..bc8e2c4138080e --- /dev/null +++ b/benchmark/ffi/add-i32.js @@ -0,0 +1,28 @@ +'use strict'; + +const common = require('../common.js'); +const ffi = require('node:ffi'); +const { libraryPath, ensureFixtureLibrary } = require('./common.js'); + +const bench = common.createBenchmark(main, { + n: [1e7], +}, { + flags: ['--experimental-ffi'], +}); + +ensureFixtureLibrary(); + +const { lib, functions } = ffi.dlopen(libraryPath, { + add_i32: { result: 'i32', parameters: ['i32', 'i32'] }, +}); + +const add = functions.add_i32; + +function main({ n }) { + bench.start(); + for (let i = 0; i < n; ++i) + add(20, 22); + bench.end(n); + + lib.close(); +} diff --git a/benchmark/ffi/common.js b/benchmark/ffi/common.js new file mode 100644 index 00000000000000..6b1ed29cc4ef56 --- /dev/null +++ b/benchmark/ffi/common.js @@ -0,0 +1,24 @@ +'use strict'; + +const common = require('../common.js'); +const fs = require('node:fs'); +const path = require('node:path'); + +// Cannot use test/ffi/ffi-test-common.js because it requires test/common +// (the test harness module). Construct the path directly. +const libraryPath = path.join(__dirname, '..', '..', 'test', 'ffi', + 'fixture_library', 'build', common.buildType, + process.platform === 'win32' ? 'ffi_test_library.dll' : + process.platform === 'darwin' ? 'ffi_test_library.dylib' : + 'ffi_test_library.so'); + +function ensureFixtureLibrary() { + if (!fs.existsSync(libraryPath)) { + throw new Error( + `Missing FFI fixture library: ${libraryPath}. ` + + 'Build it with `tools/test.py test/ffi/test-ffi-calls.js` first.', + ); + } +} + +module.exports = { libraryPath, ensureFixtureLibrary }; diff --git a/benchmark/ffi/many-args.js b/benchmark/ffi/many-args.js new file mode 100644 index 00000000000000..bacd873df52d1b --- /dev/null +++ b/benchmark/ffi/many-args.js @@ -0,0 +1,28 @@ +'use strict'; + +const common = require('../common.js'); +const ffi = require('node:ffi'); +const { libraryPath, ensureFixtureLibrary } = require('./common.js'); + +const bench = common.createBenchmark(main, { + n: [1e7], +}, { + flags: ['--experimental-ffi'], +}); + +ensureFixtureLibrary(); + +const { lib, functions } = ffi.dlopen(libraryPath, { + sum_6_i32: { result: 'i32', parameters: ['i32', 'i32', 'i32', 'i32', 'i32', 'i32'] }, +}); + +const fn = functions.sum_6_i32; + +function main({ n }) { + bench.start(); + for (let i = 0; i < n; ++i) + fn(1, 2, 3, 4, 5, 6); + bench.end(n); + + lib.close(); +} diff --git a/benchmark/ffi/pointer-bigint.js b/benchmark/ffi/pointer-bigint.js new file mode 100644 index 00000000000000..c44ef7f1656a0c --- /dev/null +++ b/benchmark/ffi/pointer-bigint.js @@ -0,0 +1,28 @@ +'use strict'; + +const common = require('../common.js'); +const ffi = require('node:ffi'); +const { libraryPath, ensureFixtureLibrary } = require('./common.js'); + +const bench = common.createBenchmark(main, { + n: [1e7], +}, { + flags: ['--experimental-ffi'], +}); + +ensureFixtureLibrary(); + +const { lib, functions } = ffi.dlopen(libraryPath, { + pointer_to_usize: { result: 'u64', parameters: ['pointer'] }, +}); + +const fn = functions.pointer_to_usize; + +function main({ n }) { + bench.start(); + for (let i = 0; i < n; ++i) + fn(0xdeadbeefn); + bench.end(n); + + lib.close(); +} diff --git a/benchmark/ffi/sum-buffer.js b/benchmark/ffi/sum-buffer.js new file mode 100644 index 00000000000000..3117f61aaedabf --- /dev/null +++ b/benchmark/ffi/sum-buffer.js @@ -0,0 +1,33 @@ +'use strict'; + +const common = require('../common.js'); +const ffi = require('node:ffi'); +const { libraryPath, ensureFixtureLibrary } = require('./common.js'); + +const bench = common.createBenchmark(main, { + size: [64, 1024, 16384], + n: [1e6], +}, { + flags: ['--experimental-ffi'], +}); + +ensureFixtureLibrary(); + +const { lib, functions } = ffi.dlopen(libraryPath, { + sum_buffer: { result: 'u64', parameters: ['pointer', 'u64'] }, +}); + +function main({ n, size }) { + const buf = Buffer.alloc(size, 0x42); + const ptr = ffi.getRawPointer(buf); + const len = BigInt(size); + + const sum = functions.sum_buffer; + + bench.start(); + for (let i = 0; i < n; ++i) + sum(ptr, len); + bench.end(n); + + lib.close(); +} diff --git a/test/ffi/fixture_library/ffi_test_library.c b/test/ffi/fixture_library/ffi_test_library.c index 8ac19c1a6d1ae2..8ba2be4505c7d6 100644 --- a/test/ffi/fixture_library/ffi_test_library.c +++ b/test/ffi/fixture_library/ffi_test_library.c @@ -331,6 +331,10 @@ FFI_EXPORT double sum_five_f64( return a + b + c + d + e; } +FFI_EXPORT int32_t sum_6_i32(int32_t a, int32_t b, int32_t c, int32_t d, int32_t e, int32_t f) { + return a + b + c + d + e + f; +} + // Mixed parameter types. FFI_EXPORT double mixed_operation(int32_t i, float f, double d, uint32_t u) { From c839afa2590751184a1daafe14b935b2403da6d3 Mon Sep 17 00:00:00 2001 From: Bryan English Date: Tue, 21 Apr 2026 14:11:38 -0400 Subject: [PATCH 2/4] ffi: add shared-buffer fast path for numeric and pointer signatures Adds an ArrayBuffer-based invocation path for FFI functions whose signatures are composed entirely of numeric types (i8..i64, u8..u64, f32, f64, bool, char) and/or pointer types. The JS wrapper packs arguments directly into a per-function AB via primordial DataView setters and the C++ invoker (`InvokeFunctionSB`) reads them without going through V8's `FunctionCallbackInfo`. Results are returned the same way. Pointer arguments use runtime dispatch: BigInt, null, and undefined take the fast path, while Buffer, ArrayBuffer, ArrayBufferView, and String fall back transparently to the classic `InvokeFunction` path via a stashed `_invokeSlow` function. Signatures containing non-numeric/non-pointer types also bypass the fast path. The fast path is disabled on big-endian platforms. Callers do not opt in, and the fast path is transparent in every way users should rely on. One observable change: function wrappers returned by `library.getFunction`, `library.getFunctions`, and `library.functions` now have `.length` equal to the declared parameter count rather than `0`. Code that relied on the previous value will need to be updated. --- lib/ffi.js | 2 + lib/internal/ffi-shared-buffer.js | 617 ++++++++++++++++++ src/env_properties.h | 4 + src/ffi/types.cc | 128 +++- src/ffi/types.h | 27 + src/node_builtins.cc | 3 + src/node_ffi.cc | 253 +++++++- src/node_ffi.h | 6 + test/ffi/fixture_library/ffi_test_library.c | 35 +- test/ffi/test-ffi-dynamic-library.js | 6 +- test/ffi/test-ffi-shared-buffer.js | 661 ++++++++++++++++++++ 11 files changed, 1713 insertions(+), 29 deletions(-) create mode 100644 lib/internal/ffi-shared-buffer.js create mode 100644 test/ffi/test-ffi-shared-buffer.js diff --git a/lib/ffi.js b/lib/ffi.js index b276f4b29dfcdc..44aff307ad0f32 100644 --- a/lib/ffi.js +++ b/lib/ffi.js @@ -53,6 +53,8 @@ const { toArrayBuffer, } = internalBinding('ffi'); +require('internal/ffi-shared-buffer'); + function checkFFIPermission() { if (!permission.isEnabled() || permission.has('ffi')) { return; diff --git a/lib/internal/ffi-shared-buffer.js b/lib/internal/ffi-shared-buffer.js new file mode 100644 index 00000000000000..695b7a72f136fa --- /dev/null +++ b/lib/internal/ffi-shared-buffer.js @@ -0,0 +1,617 @@ +'use strict'; + +const { + DataView, + DataViewPrototypeGetBigInt64, + DataViewPrototypeGetBigUint64, + DataViewPrototypeGetFloat32, + DataViewPrototypeGetFloat64, + DataViewPrototypeGetInt16, + DataViewPrototypeGetInt32, + DataViewPrototypeGetInt8, + DataViewPrototypeGetUint16, + DataViewPrototypeGetUint32, + DataViewPrototypeGetUint8, + DataViewPrototypeSetBigInt64, + DataViewPrototypeSetBigUint64, + DataViewPrototypeSetFloat32, + DataViewPrototypeSetFloat64, + DataViewPrototypeSetInt16, + DataViewPrototypeSetInt32, + DataViewPrototypeSetInt8, + DataViewPrototypeSetUint16, + DataViewPrototypeSetUint32, + DataViewPrototypeSetUint8, + FunctionPrototypeCall, + NumberIsInteger, + ObjectDefineProperty, + ObjectGetOwnPropertyDescriptor, + ObjectKeys, + ReflectApply, + TypeError, +} = primordials; + +const { + codes: { + ERR_INTERNAL_ASSERTION, + }, +} = require('internal/errors'); + +const { + DynamicLibrary, + charIsSigned, + kSbInvokeSlow, + kSbParams, + kSbResult, + kSbSharedBuffer, + uintptrMax, +} = internalBinding('ffi'); + +// Validator fields (`min`, `max`, `label`) must mirror `ToFFIArgument` in +// `src/ffi/types.cc` so the fast and slow paths produce identical errors. +const sI8 = DataViewPrototypeSetInt8; +const gI8 = DataViewPrototypeGetInt8; +const sU8 = DataViewPrototypeSetUint8; +const gU8 = DataViewPrototypeGetUint8; +const sI16 = DataViewPrototypeSetInt16; +const gI16 = DataViewPrototypeGetInt16; +const sU16 = DataViewPrototypeSetUint16; +const gU16 = DataViewPrototypeGetUint16; +const sI32 = DataViewPrototypeSetInt32; +const gI32 = DataViewPrototypeGetInt32; +const sU32 = DataViewPrototypeSetUint32; +const gU32 = DataViewPrototypeGetUint32; +const sI64 = DataViewPrototypeSetBigInt64; +const gI64 = DataViewPrototypeGetBigInt64; +const sU64 = DataViewPrototypeSetBigUint64; +const gU64 = DataViewPrototypeGetBigUint64; +const sF32 = DataViewPrototypeSetFloat32; +const gF32 = DataViewPrototypeGetFloat32; +const sF64 = DataViewPrototypeSetFloat64; +const gF64 = DataViewPrototypeGetFloat64; + +const sbTypeInfo = { + __proto__: null, + i8: { set: sI8, get: gI8, kind: 'int', min: -128, max: 127, label: 'an int8' }, + int8: { set: sI8, get: gI8, kind: 'int', min: -128, max: 127, label: 'an int8' }, + char: charIsSigned ? + { set: sI8, get: gI8, kind: 'int', min: -128, max: 127, label: 'an int8' } : + { set: sU8, get: gU8, kind: 'int', min: 0, max: 255, label: 'a uint8' }, + u8: { set: sU8, get: gU8, kind: 'int', min: 0, max: 255, label: 'a uint8' }, + uint8: { set: sU8, get: gU8, kind: 'int', min: 0, max: 255, label: 'a uint8' }, + bool: { set: sU8, get: gU8, kind: 'int', min: 0, max: 255, label: 'a uint8' }, + i16: { set: sI16, get: gI16, kind: 'int', min: -32768, max: 32767, label: 'an int16' }, + int16: { set: sI16, get: gI16, kind: 'int', min: -32768, max: 32767, label: 'an int16' }, + u16: { set: sU16, get: gU16, kind: 'int', min: 0, max: 65535, label: 'a uint16' }, + uint16: { set: sU16, get: gU16, kind: 'int', min: 0, max: 65535, label: 'a uint16' }, + i32: { set: sI32, get: gI32, kind: 'int', min: -2147483648, max: 2147483647, label: 'an int32' }, + int32: { set: sI32, get: gI32, kind: 'int', min: -2147483648, max: 2147483647, label: 'an int32' }, + u32: { set: sU32, get: gU32, kind: 'int', min: 0, max: 4294967295, label: 'a uint32' }, + uint32: { set: sU32, get: gU32, kind: 'int', min: 0, max: 4294967295, label: 'a uint32' }, + i64: { set: sI64, get: gI64, kind: 'i64', label: 'an int64' }, + int64: { set: sI64, get: gI64, kind: 'i64', label: 'an int64' }, + u64: { set: sU64, get: gU64, kind: 'u64', label: 'a uint64' }, + uint64: { set: sU64, get: gU64, kind: 'u64', label: 'a uint64' }, + f32: { set: sF32, get: gF32, kind: 'float', label: 'a float' }, + float: { set: sF32, get: gF32, kind: 'float', label: 'a float' }, + float32: { set: sF32, get: gF32, kind: 'float', label: 'a float' }, + f64: { set: sF64, get: gF64, kind: 'float', label: 'a double' }, + double: { set: sF64, get: gF64, kind: 'float', label: 'a double' }, + float64: { set: sF64, get: gF64, kind: 'float', label: 'a double' }, + pointer: { set: sU64, get: gU64, kind: 'pointer' }, + ptr: { set: sU64, get: gU64, kind: 'pointer' }, + function: { set: sU64, get: gU64, kind: 'pointer' }, + buffer: { set: sU64, get: gU64, kind: 'pointer' }, + arraybuffer: { set: sU64, get: gU64, kind: 'pointer' }, + string: { set: sU64, get: gU64, kind: 'pointer' }, + str: { set: sU64, get: gU64, kind: 'pointer' }, +}; + +const U64_MAX = 0xFFFFFFFFFFFFFFFFn; +const I64_MAX = 0x7FFFFFFFFFFFFFFFn; +const I64_MIN = -0x8000000000000000n; + +// Builds the exact error shape the non-SB implementation (the native FFI +// invoker) produces. +function throwFFIArgError(msg) { + // eslint-disable-next-line no-restricted-syntax + const err = new TypeError(msg); + err.code = 'ERR_INVALID_ARG_VALUE'; + throw err; +} + +function throwFFIArgCountError(expected, actual) { + throwFFIArgError( + `Invalid argument count: expected ${expected}, got ${actual}`); +} + +// Validation and error messages must mirror `ToFFIArgument` in +// `src/ffi/types.cc`. +function writeNumericArg(view, info, offset, arg, index) { + const kind = info.kind; + if (kind === 'int') { + if (typeof arg !== 'number' || !NumberIsInteger(arg) || + arg < info.min || arg > info.max) { + throwFFIArgError(`Argument ${index} must be ${info.label}`); + } + info.set(view, offset, arg, true); + return; + } + if (kind === 'i64') { + if (typeof arg !== 'bigint' || arg < I64_MIN || arg > I64_MAX) { + throwFFIArgError(`Argument ${index} must be ${info.label}`); + } + sI64(view, offset, arg, true); + return; + } + if (kind === 'u64') { + if (typeof arg !== 'bigint' || arg < 0n || arg > U64_MAX) { + throwFFIArgError(`Argument ${index} must be ${info.label}`); + } + sU64(view, offset, arg, true); + return; + } + if (kind === 'float') { + if (typeof arg !== 'number') { + throwFFIArgError(`Argument ${index} must be ${info.label}`); + } + info.set(view, offset, arg, true); + return; + } + + // Unreachable: caller filters out non-numeric kinds. + throw new ERR_INTERNAL_ASSERTION( + `FFI: writeNumericArg reached with unexpected kind="${kind}"`); +} + +// Returns true on fast-path success, false when the caller must fall back +// to the slow path (strings, Buffers, ArrayBuffers, ArrayBufferViews). +function writePointerArg(view, offset, arg, index) { + if (typeof arg === 'bigint') { + // Bound by `uintptrMax` (not `U64_MAX`) to mirror the slow path: on + // 32-bit platforms a BigInt that doesn't fit in `uintptr_t` would be + // silently truncated by `ReadFFIArgFromBuffer`'s + // `memcpy(..., type->size, ...)`. + if (arg < 0n || arg > uintptrMax) { + throwFFIArgError( + `Argument ${index} must be a non-negative pointer bigint`); + } + sU64(view, offset, arg, true); + return true; + } + if (arg === null || arg === undefined) { + sU64(view, offset, 0n, true); + return true; + } + return false; +} + +// The `pointer` descriptor mirrors the raw function's so user code that +// reassigns `.pointer` keeps working through the wrapper. +function inheritMetadata(wrapper, rawFn, nargs) { + ObjectDefineProperty(wrapper, 'name', { + __proto__: null, value: rawFn.name, configurable: true, + }); + ObjectDefineProperty(wrapper, 'length', { + __proto__: null, value: nargs, configurable: true, + }); + ObjectDefineProperty(wrapper, 'pointer', { + __proto__: null, value: rawFn.pointer, + writable: true, configurable: true, enumerable: true, + }); + return wrapper; +} + +// Reentrancy: the ArrayBuffer is per-function, but `InvokeFunctionSB` copies +// arguments out of it into invocation-local storage before `ffi_call` and +// reads the return value back only after, so nested/reentrant calls into +// the same function are safe. +function wrapWithSharedBuffer(rawFn, parameters, resultType) { + if (rawFn === undefined || rawFn === null) return rawFn; + const buffer = rawFn[kSbSharedBuffer]; + if (buffer === undefined) return rawFn; + + // Callers without explicit signature info (the `functions` accessor + // patch below) rely on the `kSbParams` / `kSbResult` metadata attached + // by the native `CreateFunction`. + if (parameters === undefined) parameters = rawFn[kSbParams]; + if (resultType === undefined) resultType = rawFn[kSbResult]; + // `CreateFunction` always attaches these for SB-eligible functions. + // Missing here means the native side and this wrapper are out of sync. + if (parameters === undefined || resultType === undefined) { + throw new ERR_INTERNAL_ASSERTION( + 'FFI: shared-buffer raw function is missing kSbParams or kSbResult'); + } + + const slowInvoke = rawFn[kSbInvokeSlow]; + const view = new DataView(buffer); + let retGetter = null; + if (resultType !== 'void') { + const retInfo = sbTypeInfo[resultType]; + if (retInfo === undefined) { + throw new ERR_INTERNAL_ASSERTION( + `FFI: shared-buffer type table missing entry for result type "${resultType}"`); + } + retGetter = retInfo.get; + } + + const nargs = parameters.length; + const argInfos = []; + const argOffsets = []; + let anyPointer = false; + for (let i = 0; i < nargs; i++) { + const info = sbTypeInfo[parameters[i]]; + if (info === undefined) { + throw new ERR_INTERNAL_ASSERTION( + `FFI: shared-buffer type table missing entry for parameter type "${parameters[i]}"`); + } + // Push the `sbTypeInfo` entry directly (entries with the same `kind` + // share a shape, keeping `writeNumericArg`'s call sites + // low-polymorphism) and store offsets in a parallel array to avoid + // per-arg object cloning. + argInfos.push(info); + argOffsets.push(8 * (i + 1)); + if (info.kind === 'pointer') anyPointer = true; + } + + let wrapper; + if (anyPointer) { + // Pointer signatures need a per-arg runtime type check and fall back + // to the native slow-path invoker for non-BigInt pointer arguments, + // so arity specialization wouldn't buy much here. + if (slowInvoke === undefined) { + throw new ERR_INTERNAL_ASSERTION( + 'FFI: shared-buffer raw function with pointer arguments is ' + + 'missing kSbInvokeSlow'); + } + wrapper = function(...args) { + if (args.length !== nargs) { + throwFFIArgCountError(nargs, args.length); + } + for (let i = 0; i < nargs; i++) { + const info = argInfos[i]; + const offset = argOffsets[i]; + if (info.kind === 'pointer') { + if (!writePointerArg(view, offset, args[i], i)) { + return ReflectApply(slowInvoke, undefined, args); + } + } else { + writeNumericArg(view, info, offset, args[i], i); + } + } + rawFn(); + return retGetter === null ? undefined : retGetter(view, 0, true); + }; + } else { + // Arity specialization avoids the per-call `Array` allocation of + // `...args`; the void/non-void split removes a per-call branch on + // `retGetter`. + wrapper = buildNumericWrapper( + rawFn, view, argInfos, argOffsets, nargs, retGetter); + } + return inheritMetadata(wrapper, rawFn, nargs); +} + +// Specialized for nargs 0..6 with a generic rest-params fallback for 7+. +// Per-arg type info and offsets are captured into closure locals so the +// hot path reads a single variable per arg. This is admittedly pretty weird +// but it's the result of lots of perf-hunting. +function buildNumericWrapper( + rawFn, view, argInfos, argOffsets, nargs, retGetter) { + if (nargs === 0) { + if (retGetter === null) { + return function() { + if (arguments.length !== 0) { + throwFFIArgCountError(0, arguments.length); + } + rawFn(); + }; + } + return function() { + if (arguments.length !== 0) { + throwFFIArgCountError(0, arguments.length); + } + rawFn(); + return retGetter(view, 0, true); + }; + } + if (nargs === 1) { + const i0 = argInfos[0]; + const o0 = argOffsets[0]; + if (retGetter === null) { + return function(a0) { + if (arguments.length !== 1) { + throwFFIArgCountError(1, arguments.length); + } + writeNumericArg(view, i0, o0, a0, 0); + rawFn(); + }; + } + return function(a0) { + if (arguments.length !== 1) { + throwFFIArgCountError(1, arguments.length); + } + writeNumericArg(view, i0, o0, a0, 0); + rawFn(); + return retGetter(view, 0, true); + }; + } + if (nargs === 2) { + const i0 = argInfos[0]; + const i1 = argInfos[1]; + const o0 = argOffsets[0]; + const o1 = argOffsets[1]; + if (retGetter === null) { + return function(a0, a1) { + if (arguments.length !== 2) { + throwFFIArgCountError(2, arguments.length); + } + writeNumericArg(view, i0, o0, a0, 0); + writeNumericArg(view, i1, o1, a1, 1); + rawFn(); + }; + } + return function(a0, a1) { + if (arguments.length !== 2) { + throwFFIArgCountError(2, arguments.length); + } + writeNumericArg(view, i0, o0, a0, 0); + writeNumericArg(view, i1, o1, a1, 1); + rawFn(); + return retGetter(view, 0, true); + }; + } + if (nargs === 3) { + const i0 = argInfos[0]; + const i1 = argInfos[1]; + const i2 = argInfos[2]; + const o0 = argOffsets[0]; + const o1 = argOffsets[1]; + const o2 = argOffsets[2]; + if (retGetter === null) { + return function(a0, a1, a2) { + if (arguments.length !== 3) { + throwFFIArgCountError(3, arguments.length); + } + writeNumericArg(view, i0, o0, a0, 0); + writeNumericArg(view, i1, o1, a1, 1); + writeNumericArg(view, i2, o2, a2, 2); + rawFn(); + }; + } + return function(a0, a1, a2) { + if (arguments.length !== 3) { + throwFFIArgCountError(3, arguments.length); + } + writeNumericArg(view, i0, o0, a0, 0); + writeNumericArg(view, i1, o1, a1, 1); + writeNumericArg(view, i2, o2, a2, 2); + rawFn(); + return retGetter(view, 0, true); + }; + } + if (nargs === 4) { + const i0 = argInfos[0]; + const i1 = argInfos[1]; + const i2 = argInfos[2]; + const i3 = argInfos[3]; + const o0 = argOffsets[0]; + const o1 = argOffsets[1]; + const o2 = argOffsets[2]; + const o3 = argOffsets[3]; + if (retGetter === null) { + return function(a0, a1, a2, a3) { + if (arguments.length !== 4) { + throwFFIArgCountError(4, arguments.length); + } + writeNumericArg(view, i0, o0, a0, 0); + writeNumericArg(view, i1, o1, a1, 1); + writeNumericArg(view, i2, o2, a2, 2); + writeNumericArg(view, i3, o3, a3, 3); + rawFn(); + }; + } + return function(a0, a1, a2, a3) { + if (arguments.length !== 4) { + throwFFIArgCountError(4, arguments.length); + } + writeNumericArg(view, i0, o0, a0, 0); + writeNumericArg(view, i1, o1, a1, 1); + writeNumericArg(view, i2, o2, a2, 2); + writeNumericArg(view, i3, o3, a3, 3); + rawFn(); + return retGetter(view, 0, true); + }; + } + if (nargs === 5) { + const i0 = argInfos[0]; + const i1 = argInfos[1]; + const i2 = argInfos[2]; + const i3 = argInfos[3]; + const i4 = argInfos[4]; + const o0 = argOffsets[0]; + const o1 = argOffsets[1]; + const o2 = argOffsets[2]; + const o3 = argOffsets[3]; + const o4 = argOffsets[4]; + if (retGetter === null) { + return function(a0, a1, a2, a3, a4) { + if (arguments.length !== 5) { + throwFFIArgCountError(5, arguments.length); + } + writeNumericArg(view, i0, o0, a0, 0); + writeNumericArg(view, i1, o1, a1, 1); + writeNumericArg(view, i2, o2, a2, 2); + writeNumericArg(view, i3, o3, a3, 3); + writeNumericArg(view, i4, o4, a4, 4); + rawFn(); + }; + } + return function(a0, a1, a2, a3, a4) { + if (arguments.length !== 5) { + throwFFIArgCountError(5, arguments.length); + } + writeNumericArg(view, i0, o0, a0, 0); + writeNumericArg(view, i1, o1, a1, 1); + writeNumericArg(view, i2, o2, a2, 2); + writeNumericArg(view, i3, o3, a3, 3); + writeNumericArg(view, i4, o4, a4, 4); + rawFn(); + return retGetter(view, 0, true); + }; + } + if (nargs === 6) { + const i0 = argInfos[0]; + const i1 = argInfos[1]; + const i2 = argInfos[2]; + const i3 = argInfos[3]; + const i4 = argInfos[4]; + const i5 = argInfos[5]; + const o0 = argOffsets[0]; + const o1 = argOffsets[1]; + const o2 = argOffsets[2]; + const o3 = argOffsets[3]; + const o4 = argOffsets[4]; + const o5 = argOffsets[5]; + if (retGetter === null) { + return function(a0, a1, a2, a3, a4, a5) { + if (arguments.length !== 6) { + throwFFIArgCountError(6, arguments.length); + } + writeNumericArg(view, i0, o0, a0, 0); + writeNumericArg(view, i1, o1, a1, 1); + writeNumericArg(view, i2, o2, a2, 2); + writeNumericArg(view, i3, o3, a3, 3); + writeNumericArg(view, i4, o4, a4, 4); + writeNumericArg(view, i5, o5, a5, 5); + rawFn(); + }; + } + return function(a0, a1, a2, a3, a4, a5) { + if (arguments.length !== 6) { + throwFFIArgCountError(6, arguments.length); + } + writeNumericArg(view, i0, o0, a0, 0); + writeNumericArg(view, i1, o1, a1, 1); + writeNumericArg(view, i2, o2, a2, 2); + writeNumericArg(view, i3, o3, a3, 3); + writeNumericArg(view, i4, o4, a4, 4); + writeNumericArg(view, i5, o5, a5, 5); + rawFn(); + return retGetter(view, 0, true); + }; + } + // 7+ args: further specialization is diminishing returns and bloats + // this builder. + if (retGetter === null) { + return function(...args) { + if (args.length !== nargs) { + throwFFIArgCountError(nargs, args.length); + } + for (let i = 0; i < nargs; i++) { + writeNumericArg(view, argInfos[i], argOffsets[i], args[i], i); + } + rawFn(); + }; + } + return function(...args) { + if (args.length !== nargs) { + throwFFIArgCountError(nargs, args.length); + } + for (let i = 0; i < nargs; i++) { + writeNumericArg(view, argInfos[i], argOffsets[i], args[i], i); + } + rawFn(); + return retGetter(view, 0, true); + }; +} + +// Accept-set mirrors the native `ParseFunctionSignature` in +// `src/ffi/types.cc`. `ParseFunctionSignature` additionally throws when +// multiple aliases are set at once. The wrapper runs before the native +// call, so those conflicts still surface from the native side regardless +// of which alias we happen to read here. +function sigParams(sig) { + return sig.parameters ?? sig.arguments ?? []; +} + +function sigResult(sig) { + return sig.result ?? sig.return ?? sig.returns ?? 'void'; +} + +// The native invoker for SB-eligible symbols is `InvokeFunctionSB`, which +// reads arguments from the shared buffer populated by +// `wrapWithSharedBuffer`. These patches make sure every path that surfaces +// a raw SB-eligible function to user code (`getFunction`, `getFunctions`, +// and the `functions` accessor) returns the wrapper instead. +const rawGetFunction = DynamicLibrary.prototype.getFunction; +const rawGetFunctions = DynamicLibrary.prototype.getFunctions; + +DynamicLibrary.prototype.getFunction = function getFunction(name, sig) { + // Native `DynamicLibrary::GetFunction` validates `sig`, so by the time + // we have `raw` we know `sig` is a valid object. + const raw = FunctionPrototypeCall(rawGetFunction, this, name, sig); + return wrapWithSharedBuffer(raw, sigParams(sig), sigResult(sig)); +}; + +DynamicLibrary.prototype.getFunctions = function getFunctions(definitions) { + // Native `GetFunctions` switches on `args.Length() > 0`. Zero args + // returns every cached function, one arg requires an object. Forwarding + // `undefined` would fail the object check, so drop it when omitted. + const raw = definitions === undefined ? + FunctionPrototypeCall(rawGetFunctions, this) : + FunctionPrototypeCall(rawGetFunctions, this, definitions); + if (raw === undefined || raw === null) return raw; + const keys = ObjectKeys(raw); + const out = { __proto__: null }; + for (let i = 0; i < keys.length; i++) { + const name = keys[i]; + // No `definitions`: native side returned every cached function, so we + // wrap using each function's own `kSbParams` / `kSbResult` metadata + // (same fallback as the `functions` accessor). + if (definitions === undefined) { + out[name] = wrapWithSharedBuffer(raw[name]); + } else { + const sig = definitions[name]; + out[name] = wrapWithSharedBuffer(raw[name], sigParams(sig), sigResult(sig)); + } + } + return out; +}; + +{ + // The native side installs `functions` as an accessor returning raw + // functions. Rewrap each access so `lib.functions.foo(...)` goes through + // the SB wrapper instead of invoking the fast path against an + // uninitialized buffer. + const functionsDescriptor = + ObjectGetOwnPropertyDescriptor(DynamicLibrary.prototype, 'functions'); + if (functionsDescriptor === undefined || !functionsDescriptor.get) { + // Missing getter means the native and JS sides are out of sync; silently + // skipping the patch would expose the fast-path-against-uninitialized-buffer + // footgun this whole block exists to prevent. + throw new ERR_INTERNAL_ASSERTION( + 'FFI: DynamicLibrary.prototype.functions accessor not found or has no getter'); + } + const origGetter = functionsDescriptor.get; + ObjectDefineProperty(DynamicLibrary.prototype, 'functions', { + __proto__: null, + configurable: true, + enumerable: functionsDescriptor.enumerable, + get() { + const raw = FunctionPrototypeCall(origGetter, this); + if (raw === undefined || raw === null) return raw; + const wrapped = { __proto__: null }; + const keys = ObjectKeys(raw); + for (let i = 0; i < keys.length; i++) { + const name = keys[i]; + wrapped[name] = wrapWithSharedBuffer(raw[name]); + } + return wrapped; + }, + }); +} + +module.exports = { + wrapWithSharedBuffer, +}; diff --git a/src/env_properties.h b/src/env_properties.h index b95bdfa65e4d39..0fc7b2b66179e4 100644 --- a/src/env_properties.h +++ b/src/env_properties.h @@ -46,6 +46,10 @@ #define PER_ISOLATE_SYMBOL_PROPERTIES(V) \ V(fs_use_promises_symbol, "fs_use_promises_symbol") \ V(async_id_symbol, "async_id_symbol") \ + V(ffi_sb_shared_buffer_symbol, "ffi_sb_shared_buffer_symbol") \ + V(ffi_sb_invoke_slow_symbol, "ffi_sb_invoke_slow_symbol") \ + V(ffi_sb_params_symbol, "ffi_sb_params_symbol") \ + V(ffi_sb_result_symbol, "ffi_sb_result_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/ffi/types.cc b/src/ffi/types.cc index 11b653cdf4e0d6..7d86bc6b438d85 100644 --- a/src/ffi/types.cc +++ b/src/ffi/types.cc @@ -95,9 +95,6 @@ Maybe ParseFunctionSignature(Environment* env, bool has_parameters; bool has_arguments; - ffi_type* return_type; - std::vector args; - if (!signature->Has(context, returns_key).To(&has_returns) || !signature->Has(context, return_key).To(&has_return) || !signature->Has(context, result_key).To(&has_result) || @@ -125,7 +122,10 @@ Maybe ParseFunctionSignature(Environment* env, return {}; } - return_type = &ffi_type_void; + ffi_type* return_type = &ffi_type_void; + std::vector args; + std::string return_type_name = "void"; + std::vector arg_type_names; Isolate* isolate = env->isolate(); if (has_returns || has_return || has_result) { @@ -159,6 +159,7 @@ Maybe ParseFunctionSignature(Environment* env, if (!ToFFIType(env, return_type_str.ToStringView()).To(&return_type)) { return {}; } + return_type_name = *return_type_str; } if (has_arguments || has_parameters) { @@ -203,10 +204,14 @@ Maybe ParseFunctionSignature(Environment* env, } args.push_back(arg_type); + arg_type_names.emplace_back(*arg_str); } } - return Just(FunctionSignature{return_type, std::move(args)}); + return Just(FunctionSignature{return_type, + std::move(args), + std::move(return_type_name), + std::move(arg_type_names)}); } bool SignaturesMatch(const FFIFunction& fn, @@ -225,6 +230,102 @@ bool SignaturesMatch(const FFIFunction& fn, return true; } +bool IsSBEligibleFFIType(ffi_type* type) { + return type == &ffi_type_void || type == &ffi_type_sint8 || + type == &ffi_type_uint8 || type == &ffi_type_sint16 || + type == &ffi_type_uint16 || type == &ffi_type_sint32 || + type == &ffi_type_uint32 || type == &ffi_type_sint64 || + type == &ffi_type_uint64 || type == &ffi_type_float || + type == &ffi_type_double || type == &ffi_type_pointer; +} + +bool IsSBEligibleSignature(const FFIFunction& fn) { + // The JS wrapper writes and reads the shared buffer little-endian while + // the C++ side uses memcpy in host order. On big-endian hosts these + // disagree, so the fast path is disabled there. + if constexpr (IsBigEndian()) { + return false; + } + // Zero-argument functions gain nothing from the shared-buffer path + // (no argument packing to skip) and measurably lose on tight native + // calls like `uv_os_getpid` due to the wrapper's fixed overhead. + if (fn.args.empty()) return false; + if (!IsSBEligibleFFIType(fn.return_type)) return false; + for (ffi_type* arg : fn.args) { + if (!IsSBEligibleFFIType(arg)) return false; + } + return true; +} + +bool SignatureHasPointerArgs(const FFIFunction& fn) { + for (ffi_type* arg : fn.args) { + if (arg == &ffi_type_pointer) return true; + } + return false; +} + +void ReadFFIArgFromBuffer(ffi_type* type, + const uint8_t* buffer, + size_t offset, + void* out) { + CHECK(IsSBEligibleFFIType(type)); + CHECK_LE(type->size, sizeof(uint64_t)); + // memcpy avoids the strict-aliasing violation that a direct typed load + // from the raw uint8_t buffer would incur. + const uint8_t* src = buffer + offset; + std::memcpy(out, src, type->size); +} + +void WriteFFIReturnToBuffer(ffi_type* type, + const void* result, + uint8_t* buffer, + size_t offset) { + CHECK(IsSBEligibleFFIType(type)); + uint8_t* dst = buffer + offset; + std::memset(dst, 0, 8); + + if (type == &ffi_type_void) { + return; + } + + // libffi promotes small integer return values to ffi_arg size, so these + // branches read as ffi_arg or ffi_sarg and then truncate back down. + if (type == &ffi_type_sint8) { + int8_t tmp = static_cast(*static_cast(result)); + std::memcpy(dst, &tmp, sizeof(tmp)); + return; + } + if (type == &ffi_type_uint8) { + uint8_t tmp = static_cast(*static_cast(result)); + std::memcpy(dst, &tmp, sizeof(tmp)); + return; + } + if (type == &ffi_type_sint16) { + int16_t tmp = static_cast(*static_cast(result)); + std::memcpy(dst, &tmp, sizeof(tmp)); + return; + } + if (type == &ffi_type_uint16) { + uint16_t tmp = static_cast(*static_cast(result)); + std::memcpy(dst, &tmp, sizeof(tmp)); + return; + } + if (type == &ffi_type_sint32) { + int32_t tmp = static_cast(*static_cast(result)); + std::memcpy(dst, &tmp, sizeof(tmp)); + return; + } + if (type == &ffi_type_uint32) { + uint32_t tmp = static_cast(*static_cast(result)); + std::memcpy(dst, &tmp, sizeof(tmp)); + return; + } + + // Remaining SB-eligible types (sint64, uint64, float, double, pointer) + // are not promoted by libffi and can be copied as-is. + std::memcpy(dst, result, type->size); +} + v8::Maybe ToFFIType(Environment* env, std::string_view type_str) { if (type_str == "void") { return Just(&ffi_type_void); @@ -261,6 +362,10 @@ v8::Maybe ToFFIType(Environment* env, std::string_view type_str) { } } +// The JS fast path in lib/internal/ffi-shared-buffer.js mirrors the +// validation below. `writeNumericArg` matches the numeric branches and +// `writePointerArg` matches the pointer-BigInt branch. Error codes and +// messages must stay identical across all three sites. Maybe ToFFIArgument(Environment* env, unsigned int index, ffi_type* type, @@ -361,8 +466,8 @@ Maybe ToFFIArgument(Environment* env, if (arg->IsNullOrUndefined()) { *static_cast(ret) = reinterpret_cast(nullptr); } else if (arg->IsString()) { - // This will handled in Invoke so that we can free the copied string after - // the call + // String arguments are handled in Invoke so the UTF-8 copy can be + // freed after the call. return Just(FFIArgumentCategory::String); } else if (arg->IsArrayBufferView()) { // Pointer-like ArrayBufferView arguments borrow backing-store memory @@ -451,12 +556,11 @@ Local ToJSArgument(Isolate* isolate, ffi_type* type, void* data) { } else if (type == &ffi_type_double) { ret = Number::New(isolate, *static_cast(data)); } else if (type == &ffi_type_pointer) { - // All others are treated as pointer (and thus bigint), the user will use - // other helpers to convert + // Pointers surface as BigInt. Callers decode them further with the + // ffi helpers. ret = BigInt::NewFromUnsigned( isolate, reinterpret_cast(*static_cast(data))); } else { - // For anything else, return undefined to avoid problems ret = Undefined(isolate); } @@ -619,7 +723,9 @@ bool ToFFIReturnValue(Local result, ffi_type* type, void* ret) { *static_cast(ret) = pointer; } else { - // Note that strings, buffer or ArrayBuffer are ignored + // Strings, Buffers, and ArrayBuffers are not accepted as pointer + // return values from a JS callback. The slot is zeroed before the + // false return so the caller sees a defined null pointer. *static_cast(ret) = reinterpret_cast(nullptr); return false; } diff --git a/src/ffi/types.h b/src/ffi/types.h index 1b7802e5b52480..14f474a465aa4a 100644 --- a/src/ffi/types.h +++ b/src/ffi/types.h @@ -20,6 +20,8 @@ bool ThrowIfContainsNullBytes(Environment* env, struct FunctionSignature { ffi_type* return_type; std::vector args; + std::string return_type_name; + std::vector arg_type_names; }; v8::Maybe ParseFunctionSignature( Environment* env, std::string_view name, v8::Local signature); @@ -51,6 +53,31 @@ bool SignaturesMatch(const FFIFunction& fn, ffi_type* return_type, const std::vector& args); +// True if the FFI type can be read from / written to a raw byte buffer +// without needing V8 operations (conversion, allocation, etc.). +bool IsSBEligibleFFIType(ffi_type* type); + +// True if the signature's return type and all argument types are SB-eligible. +bool IsSBEligibleSignature(const FFIFunction& fn); + +// True if any argument is pointer-typed. For these, the JS wrapper must +// do a runtime type check to decide fast vs. slow path per call. +bool SignatureHasPointerArgs(const FFIFunction& fn); + +// Read a value of the given FFI type from buffer at the given byte offset +// into the output pointer (sized as uint64_t to hold any numeric type). +void ReadFFIArgFromBuffer(ffi_type* type, + const uint8_t* buffer, + size_t offset, + void* out); + +// Write a return value of the given FFI type from the ffi_call result +// into the buffer at the given byte offset. +void WriteFFIReturnToBuffer(ffi_type* type, + const void* result, + uint8_t* buffer, + size_t offset); + } // namespace node::ffi #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/src/node_builtins.cc b/src/node_builtins.cc index 39b027ed24f06a..b48b7f3e60903e 100644 --- a/src/node_builtins.cc +++ b/src/node_builtins.cc @@ -140,6 +140,9 @@ BuiltinLoader::BuiltinCategories BuiltinLoader::GetBuiltinCategories() const { "internal/quic/quic", "internal/quic/symbols", "internal/quic/stats", "internal/quic/state", #endif // !OPENSSL_NO_QUIC +#if !HAVE_FFI + "internal/ffi-shared-buffer", +#endif // !HAVE_FFI "ffi", // Experimental. "quic", // Experimental. "sqlite", // Experimental. diff --git a/src/node_ffi.cc b/src/node_ffi.cc index b1a8b54d38050f..82c74b51ded0a1 100644 --- a/src/node_ffi.cc +++ b/src/node_ffi.cc @@ -1,7 +1,10 @@ #if HAVE_FFI #include "node_ffi.h" +#include #include +#include +#include #include "base_object-inl.h" #include "env-inl.h" #include "ffi/data.h" @@ -11,9 +14,12 @@ namespace node { using v8::Array; +using v8::ArrayBuffer; using v8::BigInt; +using v8::Boolean; using v8::Context; using v8::DontDelete; +using v8::DontEnum; using v8::External; using v8::Function; using v8::FunctionCallbackInfo; @@ -26,6 +32,7 @@ using v8::Local; using v8::LocalVector; using v8::Maybe; using v8::MaybeLocal; +using v8::NewStringType; using v8::Null; using v8::Object; using v8::PropertyAttribute; @@ -59,6 +66,10 @@ void DynamicLibrary::MemoryInfo(MemoryTracker* tracker) const { tracker->TrackFieldWithSize( "symbols", symbols_size, "std::unordered_map"); + + // 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. } void DynamicLibrary::Close() { @@ -115,7 +126,8 @@ Maybe DynamicLibrary::PrepareFunction( if (!ParseFunctionSignature(env, name, signature).To(&parsed)) { return {}; } - auto [return_type, args] = parsed; + auto [return_type, args, return_type_name, arg_type_names] = + std::move(parsed); bool should_cache_symbol = false; bool should_cache_function = false; @@ -129,11 +141,14 @@ Maybe DynamicLibrary::PrepareFunction( should_cache_symbol = symbols_.find(name) == symbols_.end(); - fn = std::make_shared(FFIFunction{.closed = false, - .ptr = ptr, - .cif = {}, - .args = args, - .return_type = return_type}); + fn = std::make_shared( + FFIFunction{.closed = false, + .ptr = ptr, + .cif = {}, + .args = args, + .return_type = return_type, + .arg_type_names = std::move(arg_type_names), + .return_type_name = std::move(return_type_name)}); ffi_status status = ffi_prep_cif(&fn->cif, FFI_DEFAULT_ABI, @@ -188,13 +203,26 @@ MaybeLocal DynamicLibrary::CreateFunction( const std::string& name, const std::shared_ptr& fn) { Isolate* isolate = env->isolate(); + Local context = env->context(); - FFIFunctionInfo* info = new FFIFunctionInfo(); + // The info is held in a unique_ptr so early-return paths free it. Ownership + // moves to the weak callback via `release()` once `SetWeak` succeeds. + auto info = std::make_unique(); info->fn = fn; + + DCHECK_EQ(fn->args.size(), fn->arg_type_names.size()); + + bool use_sb = IsSBEligibleSignature(*fn); + bool has_ptr_args = use_sb && SignatureHasPointerArgs(*fn); + Local data = - External::New(isolate, info, v8::kExternalPointerTypeTagDefault); + External::New(isolate, info.get(), v8::kExternalPointerTypeTagDefault); + MaybeLocal maybe_ret = - Function::New(env->context(), DynamicLibrary::InvokeFunction, data); + Function::New(context, + use_sb ? DynamicLibrary::InvokeFunctionSB + : DynamicLibrary::InvokeFunction, + data); Local ret; if (!maybe_ret.ToLocal(&ret)) { return MaybeLocal(); @@ -204,13 +232,10 @@ MaybeLocal DynamicLibrary::CreateFunction( if (!ToV8Value(env->context(), name, isolate).ToLocal(&name_str)) { return MaybeLocal(); } - - info->self.Reset(isolate, ret); - info->self.SetWeak( - info, DynamicLibrary::CleanupFunctionInfo, WeakCallbackType::kParameter); ret->SetName(name_str.As()); + if (!ret->Set( - env->context(), + context, env->pointer_string(), BigInt::NewFromUnsigned( isolate, @@ -219,6 +244,94 @@ MaybeLocal DynamicLibrary::CreateFunction( return MaybeLocal(); } + // Internal properties are keyed by per-isolate Symbols (see + // `env_properties.h`) to keep them out of string-key reflection, and the + // `ReadOnly | DontEnum | DontDelete` attribute set blocks user code from + // reading, modifying, or deleting them. + PropertyAttribute internal_attrs = + static_cast(ReadOnly | DontEnum | DontDelete); + + if (use_sb) { + size_t sb_size = 8 * (fn->args.size() + 1); + Local ab = ArrayBuffer::New(isolate, sb_size); + // The shared_ptr to the backing store keeps the memory alive while + // FFIFunctionInfo still references it. + info->sb_backing = ab->GetBackingStore(); + info->sb_data = static_cast(info->sb_backing->Data()); + info->sb_size = sb_size; + + if (!ret->DefineOwnProperty( + context, env->ffi_sb_shared_buffer_symbol(), ab, internal_attrs) + .FromMaybe(false)) { + return MaybeLocal(); + } + + // Signatures with pointer args also expose a slow-path invoker bound + // to the same FFIFunctionInfo. The JS wrapper routes through it when a + // pointer argument is anything other than a BigInt, null, or undefined + // (strings, Buffers, ArrayBuffers, and ArrayBufferViews). + if (has_ptr_args) { + Local slow_fn; + if (!Function::New(context, DynamicLibrary::InvokeFunction, data) + .ToLocal(&slow_fn)) { + return MaybeLocal(); + } + if (!ret->DefineOwnProperty(context, + env->ffi_sb_invoke_slow_symbol(), + slow_fn, + internal_attrs) + .FromMaybe(false)) { + return MaybeLocal(); + } + } + + // Attach the original signature type names so the JS wrapper can + // rebuild the signature from a raw function when the caller did not + // pass parameters and result explicitly. The `lib.functions` accessor + // path relies on this. + Local params_arr = + Array::New(isolate, static_cast(fn->arg_type_names.size())); + for (size_t i = 0; i < fn->arg_type_names.size(); i++) { + Local s; + if (!String::NewFromUtf8( + isolate, fn->arg_type_names[i].c_str(), NewStringType::kNormal) + .ToLocal(&s)) { + return MaybeLocal(); + } + if (!params_arr->Set(context, static_cast(i), s) + .FromMaybe(false)) { + return MaybeLocal(); + } + } + if (!ret->DefineOwnProperty(context, + env->ffi_sb_params_symbol(), + params_arr, + internal_attrs) + .FromMaybe(false)) { + return MaybeLocal(); + } + + Local result_name; + if (!String::NewFromUtf8( + isolate, fn->return_type_name.c_str(), NewStringType::kNormal) + .ToLocal(&result_name)) { + return MaybeLocal(); + } + if (!ret->DefineOwnProperty(context, + env->ffi_sb_result_symbol(), + result_name, + internal_attrs) + .FromMaybe(false)) { + return MaybeLocal(); + } + } + + info->self.Reset(isolate, ret); + info->self.SetWeak(info.get(), + DynamicLibrary::CleanupFunctionInfo, + WeakCallbackType::kParameter); + info.release(); + return ret; } @@ -342,6 +455,65 @@ void DynamicLibrary::InvokeFunction(const FunctionCallbackInfo& args) { free(result); } +void DynamicLibrary::InvokeFunctionSB(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); + FFIFunctionInfo* info = + static_cast(args.Data().As()->Value()); + FFIFunction* fn = info->fn.get(); + + if (fn == nullptr || fn->closed || fn->ptr == nullptr) { + THROW_ERR_FFI_LIBRARY_CLOSED(env); + return; + } + + // Arguments reach the native invoker through the shared buffer, not + // through V8. The JS wrapper always calls the raw function as `rawFn()` + // so any non-zero argument count indicates that user code reached the + // raw SB function directly and is about to read stale buffer contents. + if (args.Length() != 0) { + THROW_ERR_INVALID_ARG_VALUE( + env, + "SB-invoked FFI functions receive arguments through the shared " + "buffer, not as JavaScript arguments"); + return; + } + + // A failure of either CHECK means the SB invoker ran against a function + // that `CreateFunction` did not set up for the fast path, which is a + // contract violation. They stay enabled in Release because each FFI call + // is already dominated by `ffi_call` itself. + CHECK_NOT_NULL(info->sb_data); + CHECK_EQ(info->sb_size, 8u * (info->fn->args.size() + 1)); + + uint8_t* buffer = info->sb_data; + unsigned int nargs = fn->args.size(); + + // Layout is 8 bytes per slot. The return value lives at offset 0 and + // argument i lives at offset 8*(i+1). + std::vector values(nargs, 0); + std::vector ffi_args(nargs, nullptr); + + for (unsigned int i = 0; i < nargs; i++) { + ReadFFIArgFromBuffer(fn->args[i], buffer, 8 * (i + 1), &values[i]); + ffi_args[i] = &values[i]; + } + + // The storage must cover both the ffi_arg width that libffi uses for + // promoted small integer returns and the 8 bytes needed for non-promoted + // SB-eligible returns like f64, i64, and u64. `sizeof(ffi_arg)` is only + // 4 on 32-bit ARM, so take the max. + constexpr size_t kSBResultStorageSize = + sizeof(ffi_arg) > 8 ? sizeof(ffi_arg) : 8; + alignas(8) uint8_t result_storage[kSBResultStorageSize] = {0}; + void* result = (fn->return_type != &ffi_type_void) ? result_storage : nullptr; + + ffi_call(&fn->cif, FFI_FN(fn->ptr), result, ffi_args.data()); + + if (result != nullptr) { + WriteFFIReturnToBuffer(fn->return_type, result, buffer, 0); + } +} + // This is the function that will be called by libffi when a callback // is invoked from a dlopen library. It converts the arguments to JavaScript // values and calls the original JavaScript callback function. @@ -914,11 +1086,15 @@ Local DynamicLibrary::GetConstructorTemplate( Local(), attributes); - tmpl->InstanceTemplate()->SetAccessorProperty( + // `functions` lives on the prototype template rather than the instance + // template so `lib/ffi.js` can replace it via `Object.defineProperty` + // on the prototype. The attribute set omits `DontDelete` for the same + // reason. + tmpl->PrototypeTemplate()->SetAccessorProperty( FIXED_ONE_BYTE_STRING(isolate, "functions"), FunctionTemplate::New(env->isolate(), DynamicLibrary::GetFunctions), Local(), - attributes); + static_cast(ReadOnly)); SetProtoMethod(isolate, tmpl, "close", DynamicLibrary::Close); SetProtoMethod(isolate, tmpl, "getFunction", DynamicLibrary::GetFunction); @@ -978,6 +1154,51 @@ static void Initialize(Local target, SetMethod(context, target, "setUint64", SetUint64); SetMethod(context, target, "setFloat32", SetFloat32); SetMethod(context, target, "setFloat64", SetFloat64); + + // ToFFIType maps `char` to sint8 or uint8 based on `CHAR_MIN < 0` at C++ + // build time. Exposing the same decision to JS lets the shared-buffer + // wrapper's range check match `ToFFIArgument` on every platform. + Isolate* isolate = env->isolate(); + target + ->Set(context, + FIXED_ONE_BYTE_STRING(isolate, "charIsSigned"), + Boolean::New(isolate, CHAR_MIN < 0)) + .Check(); + + // The shared-buffer fast path uses `uintptrMax` to reject pointer BigInts + // that would otherwise be silently truncated by `ReadFFIArgFromBuffer`'s + // `memcpy(..., type->size, ...)` on 32-bit platforms. The slow path + // rejects the same values through `ToFFIArgument`. + target + ->Set(context, + FIXED_ONE_BYTE_STRING(isolate, "uintptrMax"), + v8::BigInt::NewFromUnsigned( + isolate, + static_cast(std::numeric_limits::max()))) + .Check(); + + // Per-isolate Symbols used by `lib/internal/ffi-shared-buffer.js` to key + // shared-buffer internal state on raw FFI functions. + target + ->Set(context, + FIXED_ONE_BYTE_STRING(isolate, "kSbSharedBuffer"), + env->ffi_sb_shared_buffer_symbol()) + .Check(); + target + ->Set(context, + FIXED_ONE_BYTE_STRING(isolate, "kSbInvokeSlow"), + env->ffi_sb_invoke_slow_symbol()) + .Check(); + target + ->Set(context, + FIXED_ONE_BYTE_STRING(isolate, "kSbParams"), + env->ffi_sb_params_symbol()) + .Check(); + target + ->Set(context, + FIXED_ONE_BYTE_STRING(isolate, "kSbResult"), + env->ffi_sb_result_symbol()) + .Check(); } } // namespace ffi diff --git a/src/node_ffi.h b/src/node_ffi.h index 6482a31ae386a6..1c68042ae075f4 100644 --- a/src/node_ffi.h +++ b/src/node_ffi.h @@ -25,11 +25,16 @@ struct FFIFunction { ffi_cif cif; std::vector args; ffi_type* return_type; + std::vector arg_type_names; + std::string return_type_name; }; struct FFIFunctionInfo { std::shared_ptr fn; v8::Global self; + std::shared_ptr sb_backing; + uint8_t* sb_data = nullptr; + size_t sb_size = 0; }; struct FFICallback { @@ -75,6 +80,7 @@ class DynamicLibrary : public BaseObject { static void New(const v8::FunctionCallbackInfo& args); static void Close(const v8::FunctionCallbackInfo& args); static void InvokeFunction(const v8::FunctionCallbackInfo& args); + static void InvokeFunctionSB(const v8::FunctionCallbackInfo& args); static void InvokeCallback(ffi_cif* cif, void* ret, void** args, diff --git a/test/ffi/fixture_library/ffi_test_library.c b/test/ffi/fixture_library/ffi_test_library.c index 8ba2be4505c7d6..51679d5d22ced2 100644 --- a/test/ffi/fixture_library/ffi_test_library.c +++ b/test/ffi/fixture_library/ffi_test_library.c @@ -215,6 +215,7 @@ FFI_EXPORT int32_t logical_not(int32_t a) { // Void operations (side effects). static int32_t global_counter = 0; +static int32_t global_scratch = 0; FFI_EXPORT void increment_counter(void) { global_counter++; @@ -228,6 +229,19 @@ FFI_EXPORT void reset_counter(void) { global_counter = 0; } +FFI_EXPORT void store_sum_2_i32(int32_t a, int32_t b) { + global_scratch = a + b; +} + +FFI_EXPORT void store_sum_6_i32( + int32_t a, int32_t b, int32_t c, int32_t d, int32_t e, int32_t f) { + global_scratch = a + b + c + d + e + f; +} + +FFI_EXPORT int32_t get_scratch(void) { + return global_scratch; +} + // Callback operations. typedef int32_t (*IntCallback)(int32_t); @@ -331,10 +345,29 @@ FFI_EXPORT double sum_five_f64( return a + b + c + d + e; } -FFI_EXPORT int32_t sum_6_i32(int32_t a, int32_t b, int32_t c, int32_t d, int32_t e, int32_t f) { +FFI_EXPORT int32_t sum_3_i32(int32_t a, int32_t b, int32_t c) { + return a + b + c; +} + +FFI_EXPORT int32_t sum_4_i32(int32_t a, int32_t b, int32_t c, int32_t d) { + return a + b + c + d; +} + +FFI_EXPORT int32_t +sum_6_i32(int32_t a, int32_t b, int32_t c, int32_t d, int32_t e, int32_t f) { return a + b + c + d + e + f; } +FFI_EXPORT int32_t sum_7_i32(int32_t a, + int32_t b, + int32_t c, + int32_t d, + int32_t e, + int32_t f, + int32_t g) { + return a + b + c + d + e + f + g; +} + // Mixed parameter types. FFI_EXPORT double mixed_operation(int32_t i, float f, double d, uint32_t u) { diff --git a/test/ffi/test-ffi-dynamic-library.js b/test/ffi/test-ffi-dynamic-library.js index 8ff2e9de816317..ec67e4f34c40be 100644 --- a/test/ffi/test-ffi-dynamic-library.js +++ b/test/ffi/test-ffi-dynamic-library.js @@ -50,7 +50,11 @@ test('dlopen resolves functions from definitions', () => { assert.strictEqual(functions.add_f32(1.25, 2.75), 4); assert.strictEqual(functions.add_u64(20n, 22n), 42n); assert.strictEqual(functions.add_i32.name, 'add_i32'); - assert.strictEqual(functions.add_i32.length, 0); + // Shared-buffer wrapper sets `length` to the FFI signature's arity + // (see `inheritMetadata` in lib/internal/ffi-shared-buffer.js). The raw + // native function has length 0, but the wrapper exposes the parameter + // count so `fn.length` is useful for introspection. + assert.strictEqual(functions.add_i32.length, 2); assert.strictEqual(typeof functions.add_i32.pointer, 'bigint'); assert.strictEqual(Object.getPrototypeOf(functions), null); } finally { diff --git a/test/ffi/test-ffi-shared-buffer.js b/test/ffi/test-ffi-shared-buffer.js new file mode 100644 index 00000000000000..f65e62737eac49 --- /dev/null +++ b/test/ffi/test-ffi-shared-buffer.js @@ -0,0 +1,661 @@ +// Flags: --experimental-ffi --expose-internals +'use strict'; +const common = require('../common'); +common.skipIfFFIMissing(); + +const assert = require('node:assert'); +const { test } = require('node:test'); + +// Capture the unpatched DynamicLibrary.prototype.getFunction BEFORE loading +// `node:ffi`, which patches it. The SB-metadata test below uses the raw +// method to inspect Symbol-keyed internals that `inheritMetadata` +// deliberately does not forward onto the wrapper. +const { internalBinding } = require('internal/test/binding'); +const ffiBinding = internalBinding('ffi'); +const { + kSbInvokeSlow, + kSbParams, + kSbResult, + kSbSharedBuffer, +} = ffiBinding; +const rawGetFunctionUnpatched = ffiBinding.DynamicLibrary.prototype.getFunction; + +const ffi = require('node:ffi'); +const { libraryPath } = require('./ffi-test-common'); + +test('numeric-only i32 function uses SB path', () => { + const { lib, functions } = ffi.dlopen(libraryPath, { + add_i32: { result: 'i32', parameters: ['i32', 'i32'] }, + }); + try { + assert.strictEqual(functions.add_i32(20, 22), 42); + assert.strictEqual(functions.add_i32(-10, 10), 0); + assert.strictEqual(functions.add_i32(0, 0), 0); + assert.strictEqual(functions.add_i32(2147483647, 0), 2147483647); + } finally { + lib.close(); + } +}); + +test('i8/u8/i16/u16 round-trip', () => { + const { lib, functions } = ffi.dlopen(libraryPath, { + add_i8: { result: 'i8', parameters: ['i8', 'i8'] }, + add_u8: { result: 'u8', parameters: ['u8', 'u8'] }, + add_i16: { result: 'i16', parameters: ['i16', 'i16'] }, + add_u16: { result: 'u16', parameters: ['u16', 'u16'] }, + }); + try { + assert.strictEqual(functions.add_i8(10, 20), 30); + assert.strictEqual(functions.add_u8(100, 155), 255); + assert.strictEqual(functions.add_i16(1000, 2000), 3000); + assert.strictEqual(functions.add_u16(30000, 35535), 65535); + } finally { + lib.close(); + } +}); + +test('f32/f64 round-trip', () => { + const { lib, functions } = ffi.dlopen(libraryPath, { + add_f32: { result: 'f32', parameters: ['f32', 'f32'] }, + add_f64: { result: 'f64', parameters: ['f64', 'f64'] }, + }); + try { + // 1.25 and 2.75 are exactly representable in float32, so the sum is exact. + assert.strictEqual(functions.add_f32(1.25, 2.75), 4.0); + assert.strictEqual(functions.add_f64(1.5, 2.5), 4.0); + } finally { + lib.close(); + } +}); + +test('i64/u64 BigInt round-trip', () => { + const { lib, functions } = ffi.dlopen(libraryPath, { + add_i64: { result: 'i64', parameters: ['i64', 'i64'] }, + add_u64: { result: 'u64', parameters: ['u64', 'u64'] }, + }); + try { + assert.strictEqual(functions.add_i64(10n, 20n), 30n); + assert.strictEqual(functions.add_u64(10n, 20n), 30n); + } finally { + lib.close(); + } +}); + +test('zero-arg function', () => { + const { lib, functions } = ffi.dlopen(null, { + uv_os_getpid: { result: 'i32', parameters: [] }, + }); + try { + const pid = functions.uv_os_getpid(); + assert.strictEqual(typeof pid, 'number'); + assert.ok(pid > 0); + } finally { + lib.close(); + } +}); + +test('6-arg numeric function', () => { + const { lib, functions } = ffi.dlopen(libraryPath, { + sum_6_i32: { result: 'i32', parameters: ['i32', 'i32', 'i32', 'i32', 'i32', 'i32'] }, + }); + try { + assert.strictEqual(functions.sum_6_i32(1, 2, 3, 4, 5, 6), 21); + } finally { + lib.close(); + } +}); + +test('pointer args: fast path (BigInt/null) and slow-path fallback (Buffer/ArrayBuffer)', () => { + const { lib, functions } = ffi.dlopen(libraryPath, { + identity_pointer: { result: 'pointer', parameters: ['pointer'] }, + pointer_to_usize: { result: 'u64', parameters: ['pointer'] }, + }); + try { + assert.strictEqual(functions.identity_pointer(0n), 0n); + assert.strictEqual(functions.identity_pointer(0x1234n), 0x1234n); + assert.strictEqual(functions.identity_pointer(null), 0n); + assert.strictEqual(functions.identity_pointer(undefined), 0n); + assert.strictEqual(functions.pointer_to_usize(0x42n), 0x42n); + + const buf = Buffer.from('hello'); + const bufPtr = functions.identity_pointer(buf); + assert.strictEqual(typeof bufPtr, 'bigint'); + assert.strictEqual(bufPtr, ffi.getRawPointer(buf)); + + const abPtr = functions.identity_pointer(new ArrayBuffer(16)); + assert.strictEqual(typeof abPtr, 'bigint'); + assert.ok(abPtr !== 0n); + } finally { + lib.close(); + } +}); + +test('string pointer uses slow-path fallback', () => { + const { lib, functions } = ffi.dlopen(libraryPath, { + string_length: { result: 'u64', parameters: ['pointer'] }, + }); + try { + assert.strictEqual(functions.string_length('hello'), 5n); + // strlen(NULL) is UB, so use a NUL-terminated Buffer for the fast path. + assert.strictEqual(functions.string_length(Buffer.from('world\0')), 5n); + } finally { + lib.close(); + } +}); + +test('non-SB-eligible signature falls back to raw function', () => { + const { lib, functions } = ffi.dlopen(libraryPath, { + string_duplicate: { result: 'pointer', parameters: ['pointer'] }, + free_string: { result: 'void', parameters: ['pointer'] }, + }); + try { + const dup = functions.string_duplicate('round-trip'); + assert.strictEqual(typeof dup, 'bigint'); + assert.ok(dup !== 0n); + functions.free_string(dup); + } finally { + lib.close(); + } +}); + +test('reentrancy across two FFI symbols', () => { + // A JS callback invoked by one FFI function reenters a different FFI + // function. Each has its own ArrayBuffer; neither may clobber the other. + const { lib, functions } = ffi.dlopen(libraryPath, { + call_int_callback: { result: 'i32', parameters: ['pointer', 'i32'] }, + add_i32: { result: 'i32', parameters: ['i32', 'i32'] }, + }); + + let callDepth = 0; + let innerResult = -1; + const callback = lib.registerCallback( + { result: 'i32', parameters: ['i32'] }, + (x) => { + callDepth++; + if (callDepth === 1) innerResult = functions.add_i32(x, 100); + return x * 2; + }, + ); + + try { + const outer = functions.call_int_callback(callback, 7); + assert.strictEqual(innerResult, 107); + assert.strictEqual(outer, 14); + } finally { + lib.unregisterCallback(callback); + lib.close(); + } +}); + +test('arity mismatch throws ERR_INVALID_ARG_VALUE', () => { + const { lib, functions } = ffi.dlopen(libraryPath, { + add_i32: { result: 'i32', parameters: ['i32', 'i32'] }, + }); + try { + assert.throws(() => functions.add_i32(1), { + code: 'ERR_INVALID_ARG_VALUE', + message: /Invalid argument count: expected 2, got 1/, + }); + assert.throws(() => functions.add_i32(1, 2, 3), { + code: 'ERR_INVALID_ARG_VALUE', + message: /Invalid argument count: expected 2, got 3/, + }); + } finally { + lib.close(); + } +}); + +test('arity 7+ uses the generic rest-params branch', () => { + const { lib, functions } = ffi.dlopen(libraryPath, { + sum_7_i32: { + result: 'i32', + parameters: ['i32', 'i32', 'i32', 'i32', 'i32', 'i32', 'i32'], + }, + }); + try { + assert.strictEqual(functions.sum_7_i32(1, 2, 3, 4, 5, 6, 7), 28); + assert.throws( + () => functions.sum_7_i32(1, 2, 3, 4, 5, 6), + { code: 'ERR_INVALID_ARG_VALUE', message: /expected 7, got 6/ }, + ); + } finally { + lib.close(); + } +}); + +test('wrappers preserve name/length/pointer and the functions accessor returns wrappers', () => { + const { lib, functions } = ffi.dlopen(libraryPath, { + add_i32: { result: 'i32', parameters: ['i32', 'i32'] }, + identity_pointer: { result: 'pointer', parameters: ['pointer'] }, + }); + try { + assert.strictEqual(functions.add_i32.name, 'add_i32'); + assert.strictEqual(typeof functions.add_i32.pointer, 'bigint'); + assert.ok(functions.add_i32.pointer !== 0n); + + assert.strictEqual(functions.identity_pointer.name, 'identity_pointer'); + assert.strictEqual(typeof functions.identity_pointer.pointer, 'bigint'); + assert.ok(functions.identity_pointer.pointer !== 0n); + + // `lib.functions.*` must also go through the SB wrapper. + assert.strictEqual(typeof lib.functions.add_i32, 'function'); + assert.strictEqual(lib.functions.add_i32(20, 22), 42); + assert.strictEqual(lib.functions.identity_pointer(0x1234n), 0x1234n); + } finally { + lib.close(); + } +}); + +test('integer boundaries for i8/u8/i16/u16/i32/u32', () => { + const { lib, functions } = ffi.dlopen(libraryPath, { + add_i8: { result: 'i8', parameters: ['i8', 'i8'] }, + add_u8: { result: 'u8', parameters: ['u8', 'u8'] }, + add_i16: { result: 'i16', parameters: ['i16', 'i16'] }, + add_u16: { result: 'u16', parameters: ['u16', 'u16'] }, + add_i32: { result: 'i32', parameters: ['i32', 'i32'] }, + add_u32: { result: 'u32', parameters: ['u32', 'u32'] }, + }); + + try { + assert.strictEqual(functions.add_i8(127, 0), 127); + assert.strictEqual(functions.add_i8(-128, 0), -128); + assert.strictEqual(functions.add_u8(255, 0), 255); + assert.strictEqual(functions.add_u8(0, 0), 0); + assert.strictEqual(functions.add_i16(32767, 0), 32767); + assert.strictEqual(functions.add_i16(-32768, 0), -32768); + assert.strictEqual(functions.add_u16(65535, 0), 65535); + assert.strictEqual(functions.add_i32(2147483647, 0), 2147483647); + assert.strictEqual(functions.add_i32(-2147483648, 0), -2147483648); + assert.strictEqual(functions.add_u32(4294967295, 0), 4294967295); + assert.strictEqual(functions.add_u32(0, 0), 0); + + const expect = { code: 'ERR_INVALID_ARG_VALUE' }; + assert.throws(() => functions.add_i8(128, 0), expect); + assert.throws(() => functions.add_i8(-129, 0), expect); + assert.throws(() => functions.add_u8(256, 0), expect); + assert.throws(() => functions.add_u8(-1, 0), expect); + assert.throws(() => functions.add_i16(32768, 0), expect); + assert.throws(() => functions.add_i16(-32769, 0), expect); + assert.throws(() => functions.add_u16(65536, 0), expect); + assert.throws(() => functions.add_u16(-1, 0), expect); + assert.throws(() => functions.add_i32(2147483648, 0), expect); + assert.throws(() => functions.add_i32(-2147483649, 0), expect); + assert.throws(() => functions.add_u32(4294967296, 0), expect); + assert.throws(() => functions.add_u32(-1, 0), expect); + + assert.throws(() => functions.add_i32(1.5, 0), expect); + assert.throws(() => functions.add_i32(NaN, 0), expect); + assert.throws(() => functions.add_i32(Infinity, 0), expect); + assert.throws(() => functions.add_i32('1', 0), expect); + } finally { + lib.close(); + } +}); + +test('i64/u64 BigInt boundaries and Number/BigInt type mismatches', () => { + const { lib, functions } = ffi.dlopen(libraryPath, { + add_i64: { result: 'i64', parameters: ['i64', 'i64'] }, + add_u64: { result: 'u64', parameters: ['u64', 'u64'] }, + }); + + try { + const I64_MAX = (1n << 63n) - 1n; + const I64_MIN = -(1n << 63n); + const U64_MAX = (1n << 64n) - 1n; + + assert.strictEqual(functions.add_i64(I64_MAX, 0n), I64_MAX); + assert.strictEqual(functions.add_i64(I64_MIN, 0n), I64_MIN); + assert.strictEqual(functions.add_u64(U64_MAX, 0n), U64_MAX); + assert.strictEqual(functions.add_u64(0n, 0n), 0n); + + const expect = { code: 'ERR_INVALID_ARG_VALUE' }; + assert.throws(() => functions.add_i64(I64_MAX + 1n, 0n), expect); + assert.throws(() => functions.add_i64(I64_MIN - 1n, 0n), expect); + assert.throws(() => functions.add_u64(U64_MAX + 1n, 0n), expect); + assert.throws(() => functions.add_u64(-1n, 0n), expect); + + assert.throws(() => functions.add_i64(1, 2n), expect); + assert.throws(() => functions.add_i64(1n, '2'), expect); + } finally { + lib.close(); + } +}); + +test('char type picks signed/unsigned range based on host ABI', () => { + const { lib, functions } = ffi.dlopen(libraryPath, { + char_is_signed: { result: 'i32', parameters: [] }, + identity_char: { result: 'char', parameters: ['char'] }, + }); + + try { + const isSigned = functions.char_is_signed() !== 0; + const expect = { code: 'ERR_INVALID_ARG_VALUE' }; + + assert.strictEqual(functions.identity_char(65), 65); + + if (isSigned) { + assert.strictEqual(functions.identity_char(-128), -128); + assert.strictEqual(functions.identity_char(127), 127); + assert.throws(() => functions.identity_char(128), expect); + assert.throws(() => functions.identity_char(-129), expect); + } else { + assert.strictEqual(functions.identity_char(255), 255); + assert.strictEqual(functions.identity_char(0), 0); + assert.throws(() => functions.identity_char(256), expect); + assert.throws(() => functions.identity_char(-1), expect); + } + } finally { + lib.close(); + } +}); + +test('SB metadata is Symbol-keyed, attribute-hardened, and not leaked onto the wrapper', () => { + const rawLib = new ffiBinding.DynamicLibrary(libraryPath); + try { + const rawFn = rawGetFunctionUnpatched.call( + rawLib, 'add_i32', { result: 'i32', parameters: ['i32', 'i32'] }); + + for (const [name, sym] of [ + ['kSbSharedBuffer', kSbSharedBuffer], + ['kSbInvokeSlow', kSbInvokeSlow], + ['kSbParams', kSbParams], + ['kSbResult', kSbResult], + ]) { + assert.strictEqual(typeof sym, 'symbol', `${name} must be a Symbol`); + } + + // Numeric-only signature: kSbInvokeSlow absent; the rest present and hardened. + for (const [name, sym] of [ + ['kSbSharedBuffer', kSbSharedBuffer], + ['kSbParams', kSbParams], + ['kSbResult', kSbResult], + ]) { + const desc = Object.getOwnPropertyDescriptor(rawFn, sym); + assert.ok(desc !== undefined, `${name} missing on pure-numeric SB function`); + assert.strictEqual(desc.enumerable, false); + assert.strictEqual(desc.configurable, false); + assert.strictEqual(desc.writable, false); + } + assert.strictEqual( + Object.getOwnPropertyDescriptor(rawFn, kSbInvokeSlow), undefined); + + // Pointer signature: kSbInvokeSlow must exist (and be hardened). + const rawPtrFn = rawGetFunctionUnpatched.call( + rawLib, 'identity_pointer', { result: 'pointer', parameters: ['pointer'] }); + const slowDesc = Object.getOwnPropertyDescriptor(rawPtrFn, kSbInvokeSlow); + assert.ok(slowDesc !== undefined); + assert.strictEqual(slowDesc.enumerable, false); + assert.strictEqual(slowDesc.configurable, false); + assert.strictEqual(slowDesc.writable, false); + + assert.deepStrictEqual(Object.keys(rawFn), ['pointer']); + const ownSyms = Object.getOwnPropertySymbols(rawFn); + assert.ok(ownSyms.includes(kSbSharedBuffer)); + assert.ok(ownSyms.includes(kSbParams)); + assert.ok(ownSyms.includes(kSbResult)); + + // Internals must not be forwarded by `inheritMetadata`. + const { lib, functions } = ffi.dlopen(libraryPath, { + add_i32: { result: 'i32', parameters: ['i32', 'i32'] }, + }); + try { + assert.strictEqual(functions.add_i32[kSbSharedBuffer], undefined); + assert.strictEqual(functions.add_i32[kSbInvokeSlow], undefined); + assert.strictEqual(functions.add_i32[kSbParams], undefined); + assert.strictEqual(functions.add_i32[kSbResult], undefined); + } finally { + lib.close(); + } + } finally { + rawLib.close(); + } +}); + +test('pointer fast-path range check: [0, 2^64 - 1]', () => { + const { lib, functions } = ffi.dlopen(libraryPath, { + identity_pointer: { result: 'pointer', parameters: ['pointer'] }, + }); + try { + assert.strictEqual(functions.identity_pointer(0n), 0n); + assert.strictEqual(functions.identity_pointer((1n << 64n) - 1n), (1n << 64n) - 1n); + + const expect = { code: 'ERR_INVALID_ARG_VALUE' }; + assert.throws(() => functions.identity_pointer(-1n), expect); + assert.throws(() => functions.identity_pointer(1n << 64n), expect); + } finally { + lib.close(); + } +}); + +test('self-recursive reentrancy: a single function\'s ArrayBuffer survives a nested call', () => { + // Stricter invariant than the two-symbol case: `InvokeFunctionSB` must + // copy args out of the ArrayBuffer to stack before `ffi_call` so a recursive + // call can reuse the same buffer without clobbering the outer frame. + const { lib, functions } = ffi.dlopen(libraryPath, { + call_binary_int_callback: { + result: 'i32', + parameters: ['function', 'i32', 'i32'], + }, + }); + + try { + let depth = 0; + const callback = lib.registerCallback( + { result: 'i32', parameters: ['i32', 'i32'] }, + common.mustCall((a, b) => { + depth++; + if (depth === 1) { + const inner = functions.call_binary_int_callback(callback, 100, 200); + assert.strictEqual(inner, 300); + } + return a + b; + }, 2), + ); + + try { + assert.strictEqual(functions.call_binary_int_callback(callback, 10, 20), 30); + } finally { + lib.unregisterCallback(callback); + } + } finally { + lib.close(); + } +}); + +test('void-return 0-arg wrapper branch', () => { + const { lib, functions } = ffi.dlopen(libraryPath, { + reset_counter: { result: 'void', parameters: [] }, + increment_counter: { result: 'void', parameters: [] }, + get_counter: { result: 'i32', parameters: [] }, + }); + try { + assert.strictEqual(functions.reset_counter(), undefined); + assert.strictEqual(functions.get_counter(), 0); + + functions.increment_counter(); + functions.increment_counter(); + functions.increment_counter(); + assert.strictEqual(functions.get_counter(), 3); + + assert.strictEqual(functions.reset_counter(), undefined); + assert.strictEqual(functions.get_counter(), 0); + } finally { + lib.close(); + } +}); + +test('void-return wrapper at arity 2 and 6 observes side effects', () => { + // The arity ladder has a separate void-return closure for each arity. + // A wiring bug in a mid-arity void specialization would not be caught + // by the 0-arg void test above, so exercise the side effects directly. + const { lib, functions } = ffi.dlopen(libraryPath, { + store_sum_2_i32: { result: 'void', parameters: ['i32', 'i32'] }, + store_sum_6_i32: { + result: 'void', + parameters: ['i32', 'i32', 'i32', 'i32', 'i32', 'i32'], + }, + get_scratch: { result: 'i32', parameters: [] }, + }); + try { + assert.strictEqual(functions.store_sum_2_i32(10, 32), undefined); + assert.strictEqual(functions.get_scratch(), 42); + + // Powers-of-two summands detect a dropped or duplicated slot. + assert.strictEqual( + functions.store_sum_6_i32(1, 2, 4, 8, 16, 32), undefined); + assert.strictEqual(functions.get_scratch(), 63); + + // Validation still runs on the void-return branch. + assert.throws( + () => functions.store_sum_2_i32(1.5, 2), + { code: 'ERR_INVALID_ARG_VALUE' }); + assert.throws( + () => functions.store_sum_6_i32(1, 2, 3, 4, 5), + { code: 'ERR_INVALID_ARG_VALUE' }); + } finally { + lib.close(); + } +}); + +test('mid-arity wrappers (1, 3, 4, 5)', () => { + const { lib, functions } = ffi.dlopen(libraryPath, { + logical_not: { result: 'i32', parameters: ['i32'] }, + sum_3_i32: { result: 'i32', parameters: ['i32', 'i32', 'i32'] }, + sum_4_i32: { result: 'i32', parameters: ['i32', 'i32', 'i32', 'i32'] }, + sum_five_i32: { result: 'i32', parameters: ['i32', 'i32', 'i32', 'i32', 'i32'] }, + }); + try { + assert.strictEqual(functions.logical_not(0), 1); + assert.strictEqual(functions.logical_not(42), 0); + + // Powers-of-two summands: a dropped or duplicated slot would change the total. + assert.strictEqual(functions.sum_3_i32(1, 2, 4), 7); + assert.strictEqual(functions.sum_4_i32(1, 2, 4, 8), 15); + assert.strictEqual(functions.sum_five_i32(1, 2, 4, 8, 16), 31); + } finally { + lib.close(); + } +}); + +test('float specials: NaN, ±Infinity, -0 round-trip bit-exact', () => { + const { lib, functions } = ffi.dlopen(libraryPath, { + add_f64: { result: 'f64', parameters: ['f64', 'f64'] }, + multiply_f64: { result: 'f64', parameters: ['f64', 'f64'] }, + }); + try { + assert.ok(Number.isNaN(functions.add_f64(NaN, 1.0))); + assert.strictEqual(functions.add_f64(Infinity, 1.0), Infinity); + assert.strictEqual(functions.add_f64(-Infinity, 1.0), -Infinity); + assert.ok(Object.is(functions.multiply_f64(-0, 1.0), -0)); + } finally { + lib.close(); + } +}); + +test('arity-7+ branch still runs per-arg validation', () => { + const { lib, functions } = ffi.dlopen(libraryPath, { + sum_7_i32: { result: 'i32', parameters: ['i32', 'i32', 'i32', 'i32', 'i32', 'i32', 'i32'] }, + }); + try { + assert.throws( + () => functions.sum_7_i32(1, 2, 3, 1.5, 5, 6, 7), + { code: 'ERR_INVALID_ARG_VALUE' }, + ); + } finally { + lib.close(); + } +}); + +test('mixed-kind signature (i32, f32, f64, u32) dispatches the right writer per slot', () => { + // Four distinct `sbTypeInfo.kind` values (int, float, float, int) — a + // wiring bug that reused one writer across slots would surface here. + const { lib, functions } = ffi.dlopen(libraryPath, { + mixed_operation: { parameters: ['i32', 'f32', 'f64', 'u32'], result: 'f64' }, + }); + + try { + assert.strictEqual(functions.mixed_operation(10, 2.5, 3.5, 4), 20); + assert.strictEqual(functions.mixed_operation(-1, 0.25, 0.75, 0), 0); + + const expect = { code: 'ERR_INVALID_ARG_VALUE' }; + // -1 on u32 slot: distinguishes u32 writer from i32 (i32 accepts -1). + assert.throws(() => functions.mixed_operation(0, 0.0, 0.0, -1), expect); + // 2^31 on i32 slot: distinguishes i32 writer from u32 (u32 accepts it). + assert.throws(() => functions.mixed_operation(2147483648, 0.0, 0.0, 0), expect); + // Float slots reject BigInt / string (the int/float writers both gate on `typeof`). + assert.throws(() => functions.mixed_operation(0, 1n, 0.0, 0), expect); + assert.throws(() => functions.mixed_operation(0, 0.0, 'x', 0), expect); + } finally { + lib.close(); + } +}); + +test('lib.getFunctions() with no arguments wraps every cached function', () => { + // Regression: the no-args branch previously returned raw native functions + // whose shared buffer was uninitialized, producing garbage numeric results. + // Mix SB-eligible signatures with one that is not (`string_length` takes a + // string, which bypasses the fast path) so the no-args branch has to walk + // the early-return path in `wrapWithSharedBuffer` alongside the wrapped + // branch. + const { lib } = ffi.dlopen(libraryPath, { + add_i32: { result: 'i32', parameters: ['i32', 'i32'] }, + add_f64: { result: 'f64', parameters: ['f64', 'f64'] }, + mixed_operation: { parameters: ['i32', 'f32', 'f64', 'u32'], result: 'f64' }, + identity_pointer: { result: 'pointer', parameters: ['pointer'] }, + string_length: { result: 'u64', parameters: ['string'] }, + }); + + try { + const all = lib.getFunctions(); + assert.strictEqual(Object.getPrototypeOf(all), null); + + // SB-eligible entries go through the shared-buffer wrapper. + assert.strictEqual(all.add_i32(20, 22), 42); + assert.strictEqual(all.add_f64(1.5, 2.5), 4.0); + assert.strictEqual(all.identity_pointer(0x42n), 0x42n); + + // Non-eligible entry returns its raw native wrapper unchanged; it still + // has to be callable from the object returned by `getFunctions()`. + assert.strictEqual(all.string_length('hello'), 5n); + + assert.deepStrictEqual( + Object.keys(all).sort(), + ['add_f64', 'add_i32', 'identity_pointer', + 'mixed_operation', 'string_length']); + + assert.throws(() => all.add_i32(1), { code: 'ERR_INVALID_ARG_VALUE' }); + assert.throws(() => all.add_i32(1.5, 0), { code: 'ERR_INVALID_ARG_VALUE' }); + + assert.strictEqual(typeof all.add_i32.pointer, 'bigint'); + assert.ok(all.add_i32.pointer !== 0n); + + // The wrapper object is no longer frozen; nothing in the SB design + // requires it. + assert.ok(!Object.isFrozen(all)); + } finally { + lib.close(); + } +}); + +test('mixed pointer + numeric signature uses the pointer-dispatch wrapper', () => { + const { lib, functions } = ffi.dlopen(libraryPath, { + call_int_callback: { result: 'i32', parameters: ['pointer', 'i32'] }, + }); + + try { + const cb = lib.registerCallback( + { result: 'i32', parameters: ['i32'] }, + (x) => x * 2, + ); + try { + assert.strictEqual(functions.call_int_callback(cb, 7), 14); + // Negative i32 must land in the numeric writer (not the pointer writer, + // which would reject a negative BigInt). + assert.strictEqual(functions.call_int_callback(cb, -5), -10); + } finally { + lib.unregisterCallback(cb); + } + } finally { + lib.close(); + } +}); From 31182b2280a3907b2375f25ff92c7eb27e84085e Mon Sep 17 00:00:00 2001 From: Bryan English Date: Fri, 24 Apr 2026 11:16:01 -0400 Subject: [PATCH 3/4] fixup! ffi: add shared-buffer fast path for numeric and pointer signatures Raise JS coverage on `lib/internal/ffi-shared-buffer.js` from 83.14% to 99.84% so the nodejs/node `coverage-linux-without-intl` workflow's 95% threshold is met (the workflow was failing at 94.98%). Two kinds of changes: * Mark genuinely-unreachable defensive blocks with `/* c8 ignore */`. These are `ERR_INTERNAL_ASSERTION` throws that trigger only when the native and JS sides disagree on the shared-buffer ABI, plus the `nargs === 0` branch in `buildNumericWrapper` which is dead today because `IsSBEligibleSignature` rejects 0-arg signatures on the native side. * Add fixtures and tests to exercise every arity of the void-return specialization ladder (1, 3, 4, 5, plus the 7+ rest-params fallback; 2 and 6 were already covered), and to hit the `throwFFIArgCountError` branch in each value-return specialization (1..6) and in the pointer-dispatch wrapper. No behavior change in the FFI runtime path itself. --- lib/internal/ffi-shared-buffer.js | 18 +++ src/node_builtins.cc | 4 +- test/ffi/fixture_library/ffi_test_library.c | 28 ++++ test/ffi/test-ffi-shared-buffer.js | 153 +++++++++++++++++++- 4 files changed, 197 insertions(+), 6 deletions(-) diff --git a/lib/internal/ffi-shared-buffer.js b/lib/internal/ffi-shared-buffer.js index 695b7a72f136fa..bce51fd79959dd 100644 --- a/lib/internal/ffi-shared-buffer.js +++ b/lib/internal/ffi-shared-buffer.js @@ -159,9 +159,11 @@ function writeNumericArg(view, info, offset, arg, index) { return; } + /* c8 ignore start */ // Unreachable: caller filters out non-numeric kinds. throw new ERR_INTERNAL_ASSERTION( `FFI: writeNumericArg reached with unexpected kind="${kind}"`); + /* c8 ignore stop */ } // Returns true on fast-path success, false when the caller must fall back @@ -218,20 +220,24 @@ function wrapWithSharedBuffer(rawFn, parameters, resultType) { if (resultType === undefined) resultType = rawFn[kSbResult]; // `CreateFunction` always attaches these for SB-eligible functions. // Missing here means the native side and this wrapper are out of sync. + /* c8 ignore start */ if (parameters === undefined || resultType === undefined) { throw new ERR_INTERNAL_ASSERTION( 'FFI: shared-buffer raw function is missing kSbParams or kSbResult'); } + /* c8 ignore stop */ const slowInvoke = rawFn[kSbInvokeSlow]; const view = new DataView(buffer); let retGetter = null; if (resultType !== 'void') { const retInfo = sbTypeInfo[resultType]; + /* c8 ignore start */ if (retInfo === undefined) { throw new ERR_INTERNAL_ASSERTION( `FFI: shared-buffer type table missing entry for result type "${resultType}"`); } + /* c8 ignore stop */ retGetter = retInfo.get; } @@ -241,10 +247,12 @@ function wrapWithSharedBuffer(rawFn, parameters, resultType) { let anyPointer = false; for (let i = 0; i < nargs; i++) { const info = sbTypeInfo[parameters[i]]; + /* c8 ignore start */ if (info === undefined) { throw new ERR_INTERNAL_ASSERTION( `FFI: shared-buffer type table missing entry for parameter type "${parameters[i]}"`); } + /* c8 ignore stop */ // Push the `sbTypeInfo` entry directly (entries with the same `kind` // share a shape, keeping `writeNumericArg`'s call sites // low-polymorphism) and store offsets in a parallel array to avoid @@ -259,11 +267,13 @@ function wrapWithSharedBuffer(rawFn, parameters, resultType) { // Pointer signatures need a per-arg runtime type check and fall back // to the native slow-path invoker for non-BigInt pointer arguments, // so arity specialization wouldn't buy much here. + /* c8 ignore start */ if (slowInvoke === undefined) { throw new ERR_INTERNAL_ASSERTION( 'FFI: shared-buffer raw function with pointer arguments is ' + 'missing kSbInvokeSlow'); } + /* c8 ignore stop */ wrapper = function(...args) { if (args.length !== nargs) { throwFFIArgCountError(nargs, args.length); @@ -298,6 +308,11 @@ function wrapWithSharedBuffer(rawFn, parameters, resultType) { // but it's the result of lots of perf-hunting. function buildNumericWrapper( rawFn, view, argInfos, argOffsets, nargs, retGetter) { + // `IsSBEligibleSignature` on the native side rejects 0-arg signatures, + // so this branch is unreachable today. It's kept as defense-in-depth + // for when that filter changes or for programmatic callers that hand a + // 0-arg signature through `wrapWithSharedBuffer` directly. + /* c8 ignore start */ if (nargs === 0) { if (retGetter === null) { return function() { @@ -315,6 +330,7 @@ function buildNumericWrapper( return retGetter(view, 0, true); }; } + /* c8 ignore stop */ if (nargs === 1) { const i0 = argInfos[0]; const o0 = argOffsets[0]; @@ -586,6 +602,7 @@ DynamicLibrary.prototype.getFunctions = function getFunctions(definitions) { // uninitialized buffer. const functionsDescriptor = ObjectGetOwnPropertyDescriptor(DynamicLibrary.prototype, 'functions'); + /* c8 ignore start */ if (functionsDescriptor === undefined || !functionsDescriptor.get) { // Missing getter means the native and JS sides are out of sync; silently // skipping the patch would expose the fast-path-against-uninitialized-buffer @@ -593,6 +610,7 @@ DynamicLibrary.prototype.getFunctions = function getFunctions(definitions) { throw new ERR_INTERNAL_ASSERTION( 'FFI: DynamicLibrary.prototype.functions accessor not found or has no getter'); } + /* c8 ignore stop */ const origGetter = functionsDescriptor.get; ObjectDefineProperty(DynamicLibrary.prototype, 'functions', { __proto__: null, diff --git a/src/node_builtins.cc b/src/node_builtins.cc index b48b7f3e60903e..b098a41cca9ea4 100644 --- a/src/node_builtins.cc +++ b/src/node_builtins.cc @@ -139,10 +139,10 @@ BuiltinLoader::BuiltinCategories BuiltinLoader::GetBuiltinCategories() const { #ifndef OPENSSL_NO_QUIC "internal/quic/quic", "internal/quic/symbols", "internal/quic/stats", "internal/quic/state", -#endif // !OPENSSL_NO_QUIC +#endif // !OPENSSL_NO_QUIC #if !HAVE_FFI "internal/ffi-shared-buffer", -#endif // !HAVE_FFI +#endif // !HAVE_FFI "ffi", // Experimental. "quic", // Experimental. "sqlite", // Experimental. diff --git a/test/ffi/fixture_library/ffi_test_library.c b/test/ffi/fixture_library/ffi_test_library.c index 51679d5d22ced2..4a57b9c9970a9a 100644 --- a/test/ffi/fixture_library/ffi_test_library.c +++ b/test/ffi/fixture_library/ffi_test_library.c @@ -233,11 +233,39 @@ FFI_EXPORT void store_sum_2_i32(int32_t a, int32_t b) { global_scratch = a + b; } +FFI_EXPORT void store_i32(int32_t a) { + global_scratch = a; +} + +FFI_EXPORT void store_sum_3_i32(int32_t a, int32_t b, int32_t c) { + global_scratch = a + b + c; +} + +FFI_EXPORT void store_sum_4_i32(int32_t a, int32_t b, int32_t c, int32_t d) { + global_scratch = a + b + c + d; +} + +FFI_EXPORT void store_sum_5_i32( + int32_t a, int32_t b, int32_t c, int32_t d, int32_t e) { + global_scratch = a + b + c + d + e; +} + FFI_EXPORT void store_sum_6_i32( int32_t a, int32_t b, int32_t c, int32_t d, int32_t e, int32_t f) { global_scratch = a + b + c + d + e + f; } +FFI_EXPORT void store_sum_8_i32(int32_t a, + int32_t b, + int32_t c, + int32_t d, + int32_t e, + int32_t f, + int32_t g, + int32_t h) { + global_scratch = a + b + c + d + e + f + g + h; +} + FFI_EXPORT int32_t get_scratch(void) { return global_scratch; } diff --git a/test/ffi/test-ffi-shared-buffer.js b/test/ffi/test-ffi-shared-buffer.js index f65e62737eac49..5acf0fd8eae240 100644 --- a/test/ffi/test-ffi-shared-buffer.js +++ b/test/ffi/test-ffi-shared-buffer.js @@ -484,34 +484,179 @@ test('void-return 0-arg wrapper branch', () => { } }); -test('void-return wrapper at arity 2 and 6 observes side effects', () => { +test('void-return wrapper at every specialized arity observes side effects', () => { // The arity ladder has a separate void-return closure for each arity. // A wiring bug in a mid-arity void specialization would not be caught - // by the 0-arg void test above, so exercise the side effects directly. + // by the 0-arg void test above, so exercise the side effects directly + // at every arity the ladder specializes (1..6) plus the 7+ rest-params + // fallback. const { lib, functions } = ffi.dlopen(libraryPath, { + store_i32: { result: 'void', parameters: ['i32'] }, store_sum_2_i32: { result: 'void', parameters: ['i32', 'i32'] }, + store_sum_3_i32: { result: 'void', parameters: ['i32', 'i32', 'i32'] }, + store_sum_4_i32: { + result: 'void', + parameters: ['i32', 'i32', 'i32', 'i32'], + }, + store_sum_5_i32: { + result: 'void', + parameters: ['i32', 'i32', 'i32', 'i32', 'i32'], + }, store_sum_6_i32: { result: 'void', parameters: ['i32', 'i32', 'i32', 'i32', 'i32', 'i32'], }, + store_sum_8_i32: { + result: 'void', + parameters: ['i32', 'i32', 'i32', 'i32', 'i32', 'i32', 'i32', 'i32'], + }, get_scratch: { result: 'i32', parameters: [] }, }); try { + // Powers-of-two summands detect a dropped or duplicated slot at each + // arity. + assert.strictEqual(functions.store_i32(7), undefined); + assert.strictEqual(functions.get_scratch(), 7); + assert.strictEqual(functions.store_sum_2_i32(10, 32), undefined); assert.strictEqual(functions.get_scratch(), 42); - // Powers-of-two summands detect a dropped or duplicated slot. + assert.strictEqual(functions.store_sum_3_i32(1, 2, 4), undefined); + assert.strictEqual(functions.get_scratch(), 7); + + assert.strictEqual(functions.store_sum_4_i32(1, 2, 4, 8), undefined); + assert.strictEqual(functions.get_scratch(), 15); + + assert.strictEqual(functions.store_sum_5_i32(1, 2, 4, 8, 16), undefined); + assert.strictEqual(functions.get_scratch(), 31); + assert.strictEqual( functions.store_sum_6_i32(1, 2, 4, 8, 16, 32), undefined); assert.strictEqual(functions.get_scratch(), 63); - // Validation still runs on the void-return branch. + // 7+ args takes the generic rest-params void branch rather than a + // per-arity specialization. + assert.strictEqual( + functions.store_sum_8_i32(1, 2, 4, 8, 16, 32, 64, 128), undefined); + assert.strictEqual(functions.get_scratch(), 255); + + // Validation still runs on every void-return branch, including the + // rest-params fallback. + assert.throws( + () => functions.store_i32(1.5), + { code: 'ERR_INVALID_ARG_VALUE' }); assert.throws( () => functions.store_sum_2_i32(1.5, 2), { code: 'ERR_INVALID_ARG_VALUE' }); + assert.throws( + () => functions.store_sum_3_i32(1, 1.5, 3), + { code: 'ERR_INVALID_ARG_VALUE' }); + assert.throws( + () => functions.store_sum_4_i32(1, 2, 1.5, 4), + { code: 'ERR_INVALID_ARG_VALUE' }); + assert.throws( + () => functions.store_sum_5_i32(1, 2, 3, 1.5, 5), + { code: 'ERR_INVALID_ARG_VALUE' }); assert.throws( () => functions.store_sum_6_i32(1, 2, 3, 4, 5), { code: 'ERR_INVALID_ARG_VALUE' }); + assert.throws( + () => functions.store_sum_8_i32(1, 2, 3, 4, 5, 6, 7, 1.5), + { code: 'ERR_INVALID_ARG_VALUE' }); + + // Wrong arity hits the `throwFFIArgCountError` branch inside each + // specialization (1..6 and the 7+ rest-params fallback). + for (const [name, expected, badArgs] of [ + ['store_i32', 1, []], + ['store_sum_2_i32', 2, [1]], + ['store_sum_3_i32', 3, [1, 2]], + ['store_sum_4_i32', 4, [1, 2, 3]], + ['store_sum_5_i32', 5, [1, 2, 3, 4]], + ['store_sum_6_i32', 6, [1, 2, 3, 4, 5]], + ['store_sum_8_i32', 8, [1, 2, 3, 4, 5, 6, 7]], + ]) { + assert.throws( + () => functions[name](...badArgs), + { + code: 'ERR_INVALID_ARG_VALUE', + message: new RegExp(`expected ${expected}, got ${badArgs.length}`), + }); + } + } finally { + lib.close(); + } +}); + +test('value-return wrapper arity mismatch hits every specialized branch', () => { + // `sum_7_i32` already exercises the 7+ rest-params branch elsewhere; + // this test targets the per-arity `throwFFIArgCountError` call in the + // value-return closures for arities 1..6 so each specialization's + // argument-count guard runs at least once. + const { lib, functions } = ffi.dlopen(libraryPath, { + logical_not: { result: 'i32', parameters: ['i32'] }, + add_i32: { result: 'i32', parameters: ['i32', 'i32'] }, + sum_3_i32: { result: 'i32', parameters: ['i32', 'i32', 'i32'] }, + sum_4_i32: { result: 'i32', parameters: ['i32', 'i32', 'i32', 'i32'] }, + sum_five_i32: { + result: 'i32', + parameters: ['i32', 'i32', 'i32', 'i32', 'i32'], + }, + sum_6_i32: { + result: 'i32', + parameters: ['i32', 'i32', 'i32', 'i32', 'i32', 'i32'], + }, + }); + try { + for (const [name, expected, badArgs] of [ + ['logical_not', 1, []], + ['add_i32', 2, [1]], + ['sum_3_i32', 3, [1, 2]], + ['sum_4_i32', 4, [1, 2, 3]], + ['sum_five_i32', 5, [1, 2, 3, 4]], + ['sum_6_i32', 6, [1, 2, 3, 4, 5]], + ]) { + assert.throws( + () => functions[name](...badArgs), + { + code: 'ERR_INVALID_ARG_VALUE', + message: new RegExp(`expected ${expected}, got ${badArgs.length}`), + }); + } + + // Sanity-check that a correct call still returns a value at each + // arity — a bug that swallowed the return on the value-return path + // would be caught here. + assert.strictEqual(functions.logical_not(0), 1); + assert.strictEqual(functions.add_i32(1, 2), 3); + assert.strictEqual(functions.sum_3_i32(1, 2, 4), 7); + assert.strictEqual(functions.sum_4_i32(1, 2, 4, 8), 15); + assert.strictEqual(functions.sum_five_i32(1, 2, 4, 8, 16), 31); + assert.strictEqual(functions.sum_6_i32(1, 2, 4, 8, 16, 32), 63); + } finally { + lib.close(); + } +}); + +test('pointer-dispatch wrapper rejects wrong-arity calls', () => { + // Pointer signatures share a single rest-params wrapper rather than the + // per-arity ladder, but it still has its own `throwFFIArgCountError` + // branch that needs to be exercised. + const { lib, functions } = ffi.dlopen(libraryPath, { + identity_pointer: { result: 'pointer', parameters: ['pointer'] }, + }); + try { + assert.throws( + () => functions.identity_pointer(), + { + code: 'ERR_INVALID_ARG_VALUE', + message: /expected 1, got 0/, + }); + assert.throws( + () => functions.identity_pointer(0n, 0n), + { + code: 'ERR_INVALID_ARG_VALUE', + message: /expected 1, got 2/, + }); } finally { lib.close(); } From 844973a7f83da0fe164f96173c19a7dd4cc5de4c Mon Sep 17 00:00:00 2001 From: Bryan English Date: Fri, 24 Apr 2026 11:16:01 -0400 Subject: [PATCH 4/4] fixup! fixup! ffi: add shared-buffer fast path for numeric and pointer signatures --- src/ffi/types.cc | 4 ++-- src/node_ffi.cc | 33 +++++++++------------------------ src/node_ffi.h | 2 -- 3 files changed, 11 insertions(+), 28 deletions(-) diff --git a/src/ffi/types.cc b/src/ffi/types.cc index 7d86bc6b438d85..87503e90f1f322 100644 --- a/src/ffi/types.cc +++ b/src/ffi/types.cc @@ -159,7 +159,7 @@ Maybe ParseFunctionSignature(Environment* env, if (!ToFFIType(env, return_type_str.ToStringView()).To(&return_type)) { return {}; } - return_type_name = *return_type_str; + return_type_name = return_type_str.ToString(); } if (has_arguments || has_parameters) { @@ -204,7 +204,7 @@ Maybe ParseFunctionSignature(Environment* env, } args.push_back(arg_type); - arg_type_names.emplace_back(*arg_str); + arg_type_names.emplace_back(arg_str.ToString()); } } diff --git a/src/node_ffi.cc b/src/node_ffi.cc index 82c74b51ded0a1..89018c8e6e4b32 100644 --- a/src/node_ffi.cc +++ b/src/node_ffi.cc @@ -32,7 +32,6 @@ using v8::Local; using v8::LocalVector; using v8::Maybe; using v8::MaybeLocal; -using v8::NewStringType; using v8::Null; using v8::Object; using v8::PropertyAttribute; @@ -257,8 +256,6 @@ MaybeLocal DynamicLibrary::CreateFunction( // The shared_ptr to the backing store keeps the memory alive while // FFIFunctionInfo still references it. info->sb_backing = ab->GetBackingStore(); - info->sb_data = static_cast(info->sb_backing->Data()); - info->sb_size = sb_size; if (!ret->DefineOwnProperty( context, env->ffi_sb_shared_buffer_symbol(), ab, internal_attrs) @@ -289,19 +286,9 @@ MaybeLocal DynamicLibrary::CreateFunction( // rebuild the signature from a raw function when the caller did not // pass parameters and result explicitly. The `lib.functions` accessor // path relies on this. - Local params_arr = - Array::New(isolate, static_cast(fn->arg_type_names.size())); - for (size_t i = 0; i < fn->arg_type_names.size(); i++) { - Local s; - if (!String::NewFromUtf8( - isolate, fn->arg_type_names[i].c_str(), NewStringType::kNormal) - .ToLocal(&s)) { - return MaybeLocal(); - } - if (!params_arr->Set(context, static_cast(i), s) - .FromMaybe(false)) { - return MaybeLocal(); - } + Local params_arr; + if (!ToV8Value(context, fn->arg_type_names, isolate).ToLocal(¶ms_arr)) { + return MaybeLocal(); } if (!ret->DefineOwnProperty(context, env->ffi_sb_params_symbol(), @@ -311,9 +298,8 @@ MaybeLocal DynamicLibrary::CreateFunction( return MaybeLocal(); } - Local result_name; - if (!String::NewFromUtf8( - isolate, fn->return_type_name.c_str(), NewStringType::kNormal) + Local result_name; + if (!ToV8Value(context, fn->return_type_name, isolate) .ToLocal(&result_name)) { return MaybeLocal(); } @@ -327,10 +313,9 @@ MaybeLocal DynamicLibrary::CreateFunction( } info->self.Reset(isolate, ret); - info->self.SetWeak(info.get(), + info->self.SetWeak(info.release(), DynamicLibrary::CleanupFunctionInfo, WeakCallbackType::kParameter); - info.release(); return ret; } @@ -482,10 +467,10 @@ void DynamicLibrary::InvokeFunctionSB(const FunctionCallbackInfo& args) { // that `CreateFunction` did not set up for the fast path, which is a // contract violation. They stay enabled in Release because each FFI call // is already dominated by `ffi_call` itself. - CHECK_NOT_NULL(info->sb_data); - CHECK_EQ(info->sb_size, 8u * (info->fn->args.size() + 1)); + CHECK(info->sb_backing); + CHECK_EQ(info->sb_backing->ByteLength(), 8u * (info->fn->args.size() + 1)); - uint8_t* buffer = info->sb_data; + uint8_t* buffer = static_cast(info->sb_backing->Data()); unsigned int nargs = fn->args.size(); // Layout is 8 bytes per slot. The return value lives at offset 0 and diff --git a/src/node_ffi.h b/src/node_ffi.h index 1c68042ae075f4..14362963f288ce 100644 --- a/src/node_ffi.h +++ b/src/node_ffi.h @@ -33,8 +33,6 @@ struct FFIFunctionInfo { std::shared_ptr fn; v8::Global self; std::shared_ptr sb_backing; - uint8_t* sb_data = nullptr; - size_t sb_size = 0; }; struct FFICallback {