From d4a75ce947867d93f47d9d9c2573c07f88eec07e Mon Sep 17 00:00:00 2001 From: "Kamat, Trivikram" <16024985+trivikr@users.noreply.github.com> Date: Mon, 20 Jul 2026 20:34:23 -0700 Subject: [PATCH] ffi: fix optimized buffer conversions Preserve pointer-like argument conversions for buffer and arraybuffer signatures after Fast API optimization. Keep memory-backed arguments on the specialized native fast path. Signed-off-by: Kamat, Trivikram <16024985+trivikr@users.noreply.github.com> Assisted-by: openai:gpt-5.6-sol --- lib/ffi.js | 2 - lib/internal/ffi/fast-api.js | 65 ++++++++------------------------ src/ffi/fast.cc | 22 ++++++++++- src/ffi/fast.h | 2 + src/node_ffi.cc | 3 +- test/ffi/test-ffi-fast-buffer.js | 38 +++++++++++++++++++ 6 files changed, 79 insertions(+), 53 deletions(-) diff --git a/lib/ffi.js b/lib/ffi.js index f35ebae3e312ab..01c330953ac8a0 100644 --- a/lib/ffi.js +++ b/lib/ffi.js @@ -68,7 +68,6 @@ const { } = require('internal/ffi-shared-buffer'); const { - initializeFastBufferMetadata, wrapWithRawPointerConversions, } = require('internal/ffi/fast-api'); @@ -90,7 +89,6 @@ function wrapFFIFunction(rawFn, owner) { returnType = rawFn[kSbReturn]; } } - initializeFastBufferMetadata(rawFn, argumentTypes); const wrapped = wrapWithSharedBuffer( rawFn, argumentTypes === undefined ? undefined : makeSignature(argumentTypes, returnType)); diff --git a/lib/internal/ffi/fast-api.js b/lib/internal/ffi/fast-api.js index c897f1baf9e61c..497304f59a362d 100644 --- a/lib/internal/ffi/fast-api.js +++ b/lib/internal/ffi/fast-api.js @@ -6,7 +6,6 @@ const { ObjectDefineProperty, ReflectApply, StringPrototypeIncludes, - Symbol, TypeError, } = primordials; @@ -24,11 +23,8 @@ const { getRawPointer, kFastArguments, kFastBufferInvoke, - kSbSharedBuffer, } = internalBinding('ffi'); -const kFastBuffer = Symbol('kFastBuffer'); - const U64_MAX = 0xFFFFFFFFFFFFFFFFn; const I64_MAX = 0x7FFFFFFFFFFFFFFFn; const I64_MIN = -0x8000000000000000n; @@ -79,16 +75,13 @@ function validateFastIntegerArg(type, value, index) { } } -function needsRawPointerConversion(type, rawFn) { - if (rawFn !== undefined && rawFn[kFastBuffer] === true && - (type === 'buffer' || type === 'arraybuffer')) { - return false; - } +function needsRawPointerConversion(type) { return type === 'buffer' || type === 'arraybuffer'; } function needsPointerLikeConversion(type) { - return type === 'pointer' || type === 'ptr' || type === 'function'; + return type === 'pointer' || type === 'ptr' || type === 'function' || + type === 'buffer' || type === 'arraybuffer'; } function needsStringPointerConversion(type) { @@ -100,12 +93,8 @@ function needsNullPointerConversion(type) { needsRawPointerConversion(type); } -function needsPointerConversion(type, rawFn) { - if (rawFn !== undefined && rawFn[kFastBuffer] === true && - (type === 'buffer' || type === 'arraybuffer')) { - return false; - } - return needsRawPointerConversion(type, rawFn) || +function needsPointerConversion(type) { + return needsRawPointerConversion(type) || needsNullPointerConversion(type) || needsStringPointerConversion(type); } @@ -174,11 +163,11 @@ function convertPointerArg(type, value, stringState, index) { return value; } -function getFastArgumentIndexes(argumentsTypes, rawFn) { +function getFastArgumentIndexes(argumentsTypes) { let indexes = null; for (let i = 0; i < argumentsTypes.length; i++) { if (fastIntegerTypeInfo[argumentsTypes[i]] === undefined && - !needsPointerConversion(argumentsTypes[i], rawFn)) { + !needsPointerConversion(argumentsTypes[i])) { continue; } if (indexes === null) { @@ -189,31 +178,12 @@ function getFastArgumentIndexes(argumentsTypes, rawFn) { return indexes; } -function convertFastArg(type, value, rawFn, stringState, index) { +function convertFastArg(type, value, stringState, index) { validateFastIntegerArg(type, value, index); - return needsPointerConversion(type, rawFn) ? + return needsPointerConversion(type) ? convertPointerArg(type, value, stringState, index) : value; } -function initializeFastBufferMetadata(rawFn, argumentTypes) { - if (rawFn === undefined || rawFn === null || argumentTypes === undefined) { - return; - } - if (rawFn[kSbSharedBuffer] !== undefined) { - return; - } - - if (rawFn[kFastArguments] !== undefined) { - for (let i = 0; i < argumentTypes.length; i++) { - const type = argumentTypes[i]; - if (type === 'buffer' || type === 'arraybuffer') { - rawFn[kFastBuffer] = true; - break; - } - } - } -} - function inheritMetadata(wrapper, rawFn, nargs) { ObjectDefineProperty(wrapper, 'name', { __proto__: null, value: rawFn.name, configurable: true, @@ -239,7 +209,7 @@ function wrapWithRawPointerConversions(rawFn, argumentTypes, _owner) { return rawFn; } - const indexes = getFastArgumentIndexes(argumentTypes, rawFn); + const indexes = getFastArgumentIndexes(argumentTypes); if (indexes === null) { return rawFn; } @@ -295,9 +265,8 @@ function wrapWithRawPointerConversions(rawFn, argumentTypes, _owner) { (c1 && hasStringPointerArg(t1, a1)); if (stringCall) enterStringConversion(stringState); try { - return rawFn(c0 ? - convertFastArg(t0, a0, rawFn, stringState, 0) : a0, - c1 ? convertFastArg(t1, a1, rawFn, stringState, 1) : a1); + return rawFn(c0 ? convertFastArg(t0, a0, stringState, 0) : a0, + c1 ? convertFastArg(t1, a1, stringState, 1) : a1); } finally { if (stringCall) exitStringConversion(stringState); } @@ -318,10 +287,9 @@ function wrapWithRawPointerConversions(rawFn, argumentTypes, _owner) { (c2 && hasStringPointerArg(t2, a2)); if (stringCall) enterStringConversion(stringState); try { - return rawFn(c0 ? - convertFastArg(t0, a0, rawFn, stringState, 0) : a0, - c1 ? convertFastArg(t1, a1, rawFn, stringState, 1) : a1, - c2 ? convertFastArg(t2, a2, rawFn, stringState, 2) : a2); + return rawFn(c0 ? convertFastArg(t0, a0, stringState, 0) : a0, + c1 ? convertFastArg(t1, a1, stringState, 1) : a1, + c2 ? convertFastArg(t2, a2, stringState, 2) : a2); } finally { if (stringCall) exitStringConversion(stringState); } @@ -344,7 +312,7 @@ function wrapWithRawPointerConversions(rawFn, argumentTypes, _owner) { for (let i = 0; i < indexes.length; i++) { const index = indexes[i]; args[index] = convertFastArg( - argumentTypes[index], args[index], rawFn, stringState, index); + argumentTypes[index], args[index], stringState, index); } return ReflectApply(rawFn, undefined, args); } finally { @@ -360,6 +328,5 @@ module.exports = { convertPointerArg, hasPointerMemoryArg, hasStringPointerArg, - initializeFastBufferMetadata, wrapWithRawPointerConversions, }; diff --git a/src/ffi/fast.cc b/src/ffi/fast.cc index 8c4420761fec1d..cb753993c6776d 100644 --- a/src/ffi/fast.cc +++ b/src/ffi/fast.cc @@ -178,12 +178,32 @@ bool IsPointerTypeName(const std::string& name) { return name == "pointer" || name == "ptr" || name == "function"; } +bool IsBufferTypeName(const std::string& name) { + return name == "buffer" || name == "arraybuffer"; +} + bool SignatureNeedsFastBufferInvoke(const FFIFunction& fn) { // The secondary buffer invoke is only generated for the hot monomorphic case // where a single pointer-like argument can be satisfied by a Buffer or // ArrayBuffer without allocating or caching a BigInt pointer in JS. return fn.arg_type_names.size() == 1 && - IsPointerTypeName(fn.arg_type_names[0]); + (IsPointerTypeName(fn.arg_type_names[0]) || + IsBufferTypeName(fn.arg_type_names[0])); +} + +std::shared_ptr CloneWithRawPointerArgNames( + const std::shared_ptr& fn) { + // The primary Fast API entrypoint receives pointer-compatible values as + // BigInts after the JS wrapper has converted strings, nullish values, and + // memory-backed objects. A secondary entrypoint handles the monomorphic + // memory-backed case without extracting the pointer in JS. + auto clone = std::make_shared(*fn); + for (std::string& name : clone->arg_type_names) { + if (IsBufferTypeName(name)) { + name = "pointer"; + } + } + return clone; } std::shared_ptr CloneWithFastBufferArgNames( diff --git a/src/ffi/fast.h b/src/ffi/fast.h index 539df26da79bf4..b85191aade135c 100644 --- a/src/ffi/fast.h +++ b/src/ffi/fast.h @@ -61,6 +61,8 @@ bool SignatureNeedsRawPointerConversions(const FFIFunction& fn); bool SignatureNeedsFastIntegerValidation(const FFIFunction& fn); bool IsPointerTypeName(const std::string& name); bool SignatureNeedsFastBufferInvoke(const FFIFunction& fn); +std::shared_ptr CloneWithRawPointerArgNames( + const std::shared_ptr& fn); std::shared_ptr CloneWithFastBufferArgNames( const std::shared_ptr& fn); std::unique_ptr CreateFastFFIMetadata(const FFIFunction& fn); diff --git a/src/node_ffi.cc b/src/node_ffi.cc index 23c58e8ea12813..b05d09270126e9 100644 --- a/src/node_ffi.cc +++ b/src/node_ffi.cc @@ -249,7 +249,8 @@ MaybeLocal DynamicLibrary::CreateFunction( // Try the generated Fast API path first. If metadata creation rejects the // signature, fall back to SharedBuffer for supported scalar shapes, then to // the generic libffi invoker. - info->fast_metadata = CreateFastFFIMetadata(*fn); + std::shared_ptr fast_fn = CloneWithRawPointerArgNames(fn); + info->fast_metadata = CreateFastFFIMetadata(*fast_fn); bool use_fast_api = info->fast_metadata != nullptr; bool use_sb = !use_fast_api && IsSBEligibleSignature(*fn); bool has_ptr_args = use_sb && SignatureHasPointerArgs(*fn); diff --git a/test/ffi/test-ffi-fast-buffer.js b/test/ffi/test-ffi-fast-buffer.js index 596ae83d1888bd..97d8a3c9b85294 100644 --- a/test/ffi/test-ffi-fast-buffer.js +++ b/test/ffi/test-ffi-fast-buffer.js @@ -95,3 +95,41 @@ test('fast FFI string buffers survive reentrant callbacks', { lib.close(); } }); + +test('optimized buffer signatures preserve pointer-like conversions', () => { + const lib = new ffi.DynamicLibrary(libraryPath); + const asBuffer = lib.getFunction('pointer_to_usize', { + arguments: ['buffer'], + return: 'u64', + }); + const asArrayBuffer = lib.getFunction('pointer_to_usize', { + arguments: ['arraybuffer'], + return: 'u64', + }); + + function callBuffer(value) { + return asBuffer(value); + } + + function callArrayBuffer(value) { + return asArrayBuffer(value); + } + + try { + for (let i = 0; i < 100_000; i++) { + assert.strictEqual(callBuffer(0n), 0n); + assert.strictEqual(callArrayBuffer(0n), 0n); + } + + for (const call of [callBuffer, callArrayBuffer]) { + assert.strictEqual(call(null), 0n); + assert.strictEqual(call(undefined), 0n); + assert.notStrictEqual(call('ffi'), 0n); + + const bytes = Buffer.alloc(1); + assert.strictEqual(call(bytes), ffi.getRawPointer(bytes)); + } + } finally { + lib.close(); + } +});