From ca7a27b0e9e1f5ca07f3ef14b02ff9171c695111 Mon Sep 17 00:00:00 2001 From: "Kamat, Trivikram" <16024985+trivikr@users.noreply.github.com> Date: Thu, 16 Jul 2026 20:20:07 -0700 Subject: [PATCH 1/2] ffi: preserve strings during reentrant calls Cache temporary string conversion buffers by wrapper and active call depth. This prevents nested FFI calls from overwriting or replacing buffers still in use by an outer native call. Signed-off-by: Kamat, Trivikram <16024985+trivikr@users.noreply.github.com> Assisted-by: openai:gpt-5.6-sol --- lib/internal/ffi/fast-api.js | 95 +++++++++++++++------ test/ffi/fixture_library/ffi_test_library.c | 9 ++ test/ffi/test-ffi-fast-buffer.js | 23 +++++ 3 files changed, 100 insertions(+), 27 deletions(-) diff --git a/lib/internal/ffi/fast-api.js b/lib/internal/ffi/fast-api.js index 11e8c005dee98c..1f0d7305f751af 100644 --- a/lib/internal/ffi/fast-api.js +++ b/lib/internal/ffi/fast-api.js @@ -28,7 +28,6 @@ const { } = internalBinding('ffi'); const kFastBuffer = Symbol('kFastBuffer'); -const kStringConversionBuffer = Symbol('kStringConversionBuffer'); const U64_MAX = 0xFFFFFFFFFFFFFFFFn; const I64_MAX = 0x7FFFFFFFFFFFFFFFn; @@ -119,19 +118,20 @@ function hasPointerMemoryArg(type, value) { (isArrayBufferView(value) || isAnyArrayBuffer(value)); } -function getStringConversionPointer(owner, value, index) { - const size = value.length * 3 + 1; - let buffers = owner[kStringConversionBuffer]; - if (buffers === undefined) { - buffers = []; - ObjectDefineProperty(owner, kStringConversionBuffer, { - __proto__: null, - configurable: false, - enumerable: false, - writable: false, - value: buffers, - }); +function enterStringConversion(state) { + if (state.buffers[state.depth] === undefined) { + state.buffers[state.depth] = []; } + state.depth++; +} + +function exitStringConversion(state) { + state.depth--; +} + +function getStringConversionPointer(state, value, index) { + const size = value.length * 3 + 1; + const buffers = state.buffers[state.depth - 1]; let entry = buffers[index]; if (entry !== undefined && entry.string === value) { return entry.pointer; @@ -157,13 +157,13 @@ function getStringConversionPointer(owner, value, index) { return entry.pointer; } -function convertPointerArg(type, value, owner, index) { +function convertPointerArg(type, value, stringState, index) { if (needsNullPointerConversion(type) && (value === null || value === undefined)) { return 0n; } if (hasStringPointerArg(type, value)) { - return getStringConversionPointer(owner, value, index); + return getStringConversionPointer(stringState, value, index); } if (hasPointerMemoryArg(type, value)) { return getRawPointer(value); @@ -189,10 +189,10 @@ function getFastArgumentIndexes(argumentsTypes, rawFn) { return indexes; } -function convertFastArg(type, value, rawFn, owner, index) { +function convertFastArg(type, value, rawFn, stringState, index) { validateFastIntegerArg(type, value, index); return needsPointerConversion(type, rawFn) ? - convertPointerArg(type, value, owner, index) : value; + convertPointerArg(type, value, stringState, index) : value; } function initializeFastBufferMetadata(rawFn, argumentTypes) { @@ -228,7 +228,7 @@ function inheritMetadata(wrapper, rawFn, nargs) { return wrapper; } -function wrapWithRawPointerConversions(rawFn, argumentTypes, owner) { +function wrapWithRawPointerConversions(rawFn, argumentTypes, _owner) { if (rawFn === undefined || rawFn === null) { return rawFn; } @@ -244,6 +244,12 @@ function wrapWithRawPointerConversions(rawFn, argumentTypes, owner) { return rawFn; } + const stringState = { + __proto__: null, + buffers: [], + depth: 0, + }; + const nargs = argumentTypes.length; let wrapper; if (nargs === 1 && indexes.length === 1 && indexes[0] === 0) { @@ -262,7 +268,12 @@ function wrapWithRawPointerConversions(rawFn, argumentTypes, owner) { (arg === null || arg === undefined)) { arg = 0n; } else if (string0 && typeof arg === 'string') { - arg = getStringConversionPointer(owner, arg, 0); + enterStringConversion(stringState); + try { + return rawFn(getStringConversionPointer(stringState, arg, 0)); + } finally { + exitStringConversion(stringState); + } } else if (memory0 && (isArrayBufferView(arg) || isAnyArrayBuffer(arg))) { if (fastBufferInvoke !== undefined) { return fastBufferInvoke(arg); @@ -280,8 +291,16 @@ function wrapWithRawPointerConversions(rawFn, argumentTypes, owner) { if (arguments.length !== 2) { throwFFIArgCountError(2, arguments.length); } - return rawFn(c0 ? convertFastArg(t0, a0, rawFn, owner, 0) : a0, - c1 ? convertFastArg(t1, a1, rawFn, owner, 1) : a1); + const stringCall = (c0 && hasStringPointerArg(t0, a0)) || + (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); + } finally { + if (stringCall) exitStringConversion(stringState); + } }; } else if (nargs === 3) { const c0 = ArrayPrototypeIncludes(indexes, 0); @@ -294,21 +313,43 @@ function wrapWithRawPointerConversions(rawFn, argumentTypes, owner) { if (arguments.length !== 3) { throwFFIArgCountError(3, arguments.length); } - return rawFn(c0 ? convertFastArg(t0, a0, rawFn, owner, 0) : a0, - c1 ? convertFastArg(t1, a1, rawFn, owner, 1) : a1, - c2 ? convertFastArg(t2, a2, rawFn, owner, 2) : a2); + const stringCall = (c0 && hasStringPointerArg(t0, a0)) || + (c1 && hasStringPointerArg(t1, a1)) || + (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); + } finally { + if (stringCall) exitStringConversion(stringState); + } }; } else { wrapper = function(...args) { if (args.length !== nargs) { throwFFIArgCountError(nargs, args.length); } + let stringCall = false; for (let i = 0; i < indexes.length; i++) { const index = indexes[i]; - args[index] = convertFastArg( - argumentTypes[index], args[index], rawFn, owner, index); + if (hasStringPointerArg(argumentTypes[index], args[index])) { + stringCall = true; + break; + } + } + if (stringCall) enterStringConversion(stringState); + try { + for (let i = 0; i < indexes.length; i++) { + const index = indexes[i]; + args[index] = convertFastArg( + argumentTypes[index], args[index], rawFn, stringState, index); + } + return ReflectApply(rawFn, undefined, args); + } finally { + if (stringCall) exitStringConversion(stringState); } - return ReflectApply(rawFn, undefined, args); }; } diff --git a/test/ffi/fixture_library/ffi_test_library.c b/test/ffi/fixture_library/ffi_test_library.c index c58a6536469dc8..10d2ed9f66c95d 100644 --- a/test/ffi/fixture_library/ffi_test_library.c +++ b/test/ffi/fixture_library/ffi_test_library.c @@ -333,6 +333,15 @@ FFI_EXPORT void call_void_callback(VoidCallback callback) { } } +FFI_EXPORT int32_t string_survives_callback(const char* str, + VoidCallback callback) { + if (callback) { + callback(); + } + + return str && strcmp(str, "outer string") == 0; +} + FFI_EXPORT void call_string_callback(StringCallback callback, const char* str) { if (callback) { callback(str); diff --git a/test/ffi/test-ffi-fast-buffer.js b/test/ffi/test-ffi-fast-buffer.js index ceca89b7c43285..f869a9615819f5 100644 --- a/test/ffi/test-ffi-fast-buffer.js +++ b/test/ffi/test-ffi-fast-buffer.js @@ -69,3 +69,26 @@ test('fast FFI buffer arguments reject invalid values', () => { lib.close(); } }); + +test('fast FFI string buffers survive reentrant callbacks', () => { + const { lib, functions } = ffi.dlopen(libraryPath, { + safe_strlen: { arguments: ['string'], return: 'i32' }, + string_survives_callback: { + arguments: ['string', 'pointer'], + return: 'i32', + }, + }); + let nestedLength; + const callback = lib.registerCallback(() => { + nestedLength = functions.safe_strlen('inner string'); + }); + + try { + assert.strictEqual( + functions.string_survives_callback('outer string', callback), 1); + assert.strictEqual(nestedLength, 12); + } finally { + lib.unregisterCallback(callback); + lib.close(); + } +}); From 414f6029da46ea27367d6b1055731db91d67847b Mon Sep 17 00:00:00 2001 From: "Kamat, Trivikram" <16024985+trivikr@users.noreply.github.com> Date: Sat, 18 Jul 2026 12:31:00 -0700 Subject: [PATCH 2/2] test: skip FFI reentrant callback test on SmartOS --- lib/internal/ffi/fast-api.js | 6 +++--- test/ffi/test-ffi-fast-buffer.js | 5 ++++- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/lib/internal/ffi/fast-api.js b/lib/internal/ffi/fast-api.js index 1f0d7305f751af..c897f1baf9e61c 100644 --- a/lib/internal/ffi/fast-api.js +++ b/lib/internal/ffi/fast-api.js @@ -297,7 +297,7 @@ function wrapWithRawPointerConversions(rawFn, argumentTypes, _owner) { try { return rawFn(c0 ? convertFastArg(t0, a0, rawFn, stringState, 0) : a0, - c1 ? convertFastArg(t1, a1, rawFn, stringState, 1) : a1); + c1 ? convertFastArg(t1, a1, rawFn, stringState, 1) : a1); } finally { if (stringCall) exitStringConversion(stringState); } @@ -320,8 +320,8 @@ function wrapWithRawPointerConversions(rawFn, argumentTypes, _owner) { 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); + c1 ? convertFastArg(t1, a1, rawFn, stringState, 1) : a1, + c2 ? convertFastArg(t2, a2, rawFn, stringState, 2) : a2); } finally { if (stringCall) exitStringConversion(stringState); } diff --git a/test/ffi/test-ffi-fast-buffer.js b/test/ffi/test-ffi-fast-buffer.js index f869a9615819f5..596ae83d1888bd 100644 --- a/test/ffi/test-ffi-fast-buffer.js +++ b/test/ffi/test-ffi-fast-buffer.js @@ -70,7 +70,10 @@ test('fast FFI buffer arguments reject invalid values', () => { } }); -test('fast FFI string buffers survive reentrant callbacks', () => { +test('fast FFI string buffers survive reentrant callbacks', { + // Bundled libffi callbacks crash on SmartOS. + skip: common.isSunOS, +}, () => { const { lib, functions } = ffi.dlopen(libraryPath, { safe_strlen: { arguments: ['string'], return: 'i32' }, string_survives_callback: {