From 8f6c69fc4c60029391d3f5828e3c60e3c55ce7e9 Mon Sep 17 00:00:00 2001 From: Trivikram Kamat <16024985+trivikr@users.noreply.github.com> Date: Mon, 17 Aug 2026 09:26:47 -0700 Subject: [PATCH 1/6] ffi: keep FFI functions non-constructible Use concise method functions for Fast API and shared-buffer wrappers, and create native fallback functions with ConstructorBehavior::kThrow, so FFI functions remain non-constructible on all invocation paths. Signed-off-by: Kamat, Trivikram <16024985+trivikr@users.noreply.github.com> Assisted-by: codex:gpt-5.6-sol PR-URL: https://github.com/nodejs/node/pull/65184 Fixes: https://github.com/nodejs/node/issues/65183 Reviewed-By: Paolo Insogna --- lib/internal/ffi-shared-buffer.js | 70 ++++++++++++++-------------- lib/internal/ffi/fast-api.js | 18 +++---- src/node_ffi.cc | 11 +++-- test/ffi/test-ffi-dynamic-library.js | 21 +++++++++ 4 files changed, 75 insertions(+), 45 deletions(-) diff --git a/lib/internal/ffi-shared-buffer.js b/lib/internal/ffi-shared-buffer.js index c8b48d2aad4c..94764bdf0db7 100644 --- a/lib/internal/ffi-shared-buffer.js +++ b/lib/internal/ffi-shared-buffer.js @@ -198,6 +198,8 @@ function inheritMetadata(wrapper, rawFn, nargs) { // 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. +// Concise methods do not have [[Construct]], unlike function expressions, so +// use them below to match the native FFI functions' non-constructible behavior. function wrapWithSharedBuffer(rawFn, signature) { if (rawFn == null) return rawFn; const buffer = rawFn[kSbSharedBuffer]; @@ -254,7 +256,7 @@ function wrapWithSharedBuffer(rawFn, signature) { // so arity specialization wouldn't buy much here. assert(slowInvoke !== undefined, 'FFI: shared-buffer raw function with pointer arguments is missing kSbInvokeSlow'); - wrapper = function(...args) { + wrapper = { invoke(...args) { if (args.length !== nargs) { throwFFIArgCountError(nargs, args.length); } @@ -271,7 +273,7 @@ function wrapWithSharedBuffer(rawFn, signature) { } rawFn(); return retGetter === null ? undefined : retGetter(view, 0, true); - }; + } }.invoke; } else { // Arity specialization avoids the per-call `Array` allocation of // `...args`; the void/non-void split removes a per-call branch on @@ -295,42 +297,42 @@ function buildNumericWrapper( /* c8 ignore start */ if (nargs === 0) { if (retGetter === null) { - return function() { + return { invoke() { if (arguments.length !== 0) { throwFFIArgCountError(0, arguments.length); } rawFn(); - }; + } }.invoke; } - return function() { + return { invoke() { if (arguments.length !== 0) { throwFFIArgCountError(0, arguments.length); } rawFn(); return retGetter(view, 0, true); - }; + } }.invoke; } /* c8 ignore stop */ if (nargs === 1) { const i0 = argInfos[0]; const o0 = argOffsets[0]; if (retGetter === null) { - return function(a0) { + return { invoke(a0) { if (arguments.length !== 1) { throwFFIArgCountError(1, arguments.length); } writeNumericArg(view, i0, o0, a0, 0); rawFn(); - }; + } }.invoke; } - return function(a0) { + return { invoke(a0) { if (arguments.length !== 1) { throwFFIArgCountError(1, arguments.length); } writeNumericArg(view, i0, o0, a0, 0); rawFn(); return retGetter(view, 0, true); - }; + } }.invoke; } if (nargs === 2) { const i0 = argInfos[0]; @@ -338,16 +340,16 @@ function buildNumericWrapper( const o0 = argOffsets[0]; const o1 = argOffsets[1]; if (retGetter === null) { - return function(a0, a1) { + return { invoke(a0, a1) { if (arguments.length !== 2) { throwFFIArgCountError(2, arguments.length); } writeNumericArg(view, i0, o0, a0, 0); writeNumericArg(view, i1, o1, a1, 1); rawFn(); - }; + } }.invoke; } - return function(a0, a1) { + return { invoke(a0, a1) { if (arguments.length !== 2) { throwFFIArgCountError(2, arguments.length); } @@ -355,7 +357,7 @@ function buildNumericWrapper( writeNumericArg(view, i1, o1, a1, 1); rawFn(); return retGetter(view, 0, true); - }; + } }.invoke; } if (nargs === 3) { const i0 = argInfos[0]; @@ -365,7 +367,7 @@ function buildNumericWrapper( const o1 = argOffsets[1]; const o2 = argOffsets[2]; if (retGetter === null) { - return function(a0, a1, a2) { + return { invoke(a0, a1, a2) { if (arguments.length !== 3) { throwFFIArgCountError(3, arguments.length); } @@ -373,9 +375,9 @@ function buildNumericWrapper( writeNumericArg(view, i1, o1, a1, 1); writeNumericArg(view, i2, o2, a2, 2); rawFn(); - }; + } }.invoke; } - return function(a0, a1, a2) { + return { invoke(a0, a1, a2) { if (arguments.length !== 3) { throwFFIArgCountError(3, arguments.length); } @@ -384,7 +386,7 @@ function buildNumericWrapper( writeNumericArg(view, i2, o2, a2, 2); rawFn(); return retGetter(view, 0, true); - }; + } }.invoke; } if (nargs === 4) { const i0 = argInfos[0]; @@ -396,7 +398,7 @@ function buildNumericWrapper( const o2 = argOffsets[2]; const o3 = argOffsets[3]; if (retGetter === null) { - return function(a0, a1, a2, a3) { + return { invoke(a0, a1, a2, a3) { if (arguments.length !== 4) { throwFFIArgCountError(4, arguments.length); } @@ -405,9 +407,9 @@ function buildNumericWrapper( writeNumericArg(view, i2, o2, a2, 2); writeNumericArg(view, i3, o3, a3, 3); rawFn(); - }; + } }.invoke; } - return function(a0, a1, a2, a3) { + return { invoke(a0, a1, a2, a3) { if (arguments.length !== 4) { throwFFIArgCountError(4, arguments.length); } @@ -417,7 +419,7 @@ function buildNumericWrapper( writeNumericArg(view, i3, o3, a3, 3); rawFn(); return retGetter(view, 0, true); - }; + } }.invoke; } if (nargs === 5) { const i0 = argInfos[0]; @@ -431,7 +433,7 @@ function buildNumericWrapper( const o3 = argOffsets[3]; const o4 = argOffsets[4]; if (retGetter === null) { - return function(a0, a1, a2, a3, a4) { + return { invoke(a0, a1, a2, a3, a4) { if (arguments.length !== 5) { throwFFIArgCountError(5, arguments.length); } @@ -441,9 +443,9 @@ function buildNumericWrapper( writeNumericArg(view, i3, o3, a3, 3); writeNumericArg(view, i4, o4, a4, 4); rawFn(); - }; + } }.invoke; } - return function(a0, a1, a2, a3, a4) { + return { invoke(a0, a1, a2, a3, a4) { if (arguments.length !== 5) { throwFFIArgCountError(5, arguments.length); } @@ -454,7 +456,7 @@ function buildNumericWrapper( writeNumericArg(view, i4, o4, a4, 4); rawFn(); return retGetter(view, 0, true); - }; + } }.invoke; } if (nargs === 6) { const i0 = argInfos[0]; @@ -470,7 +472,7 @@ function buildNumericWrapper( const o4 = argOffsets[4]; const o5 = argOffsets[5]; if (retGetter === null) { - return function(a0, a1, a2, a3, a4, a5) { + return { invoke(a0, a1, a2, a3, a4, a5) { if (arguments.length !== 6) { throwFFIArgCountError(6, arguments.length); } @@ -481,9 +483,9 @@ function buildNumericWrapper( writeNumericArg(view, i4, o4, a4, 4); writeNumericArg(view, i5, o5, a5, 5); rawFn(); - }; + } }.invoke; } - return function(a0, a1, a2, a3, a4, a5) { + return { invoke(a0, a1, a2, a3, a4, a5) { if (arguments.length !== 6) { throwFFIArgCountError(6, arguments.length); } @@ -495,12 +497,12 @@ function buildNumericWrapper( writeNumericArg(view, i5, o5, a5, 5); rawFn(); return retGetter(view, 0, true); - }; + } }.invoke; } // 7+ args: further specialization is diminishing returns and bloats // this builder. if (retGetter === null) { - return function(...args) { + return { invoke(...args) { if (args.length !== nargs) { throwFFIArgCountError(nargs, args.length); } @@ -508,9 +510,9 @@ function buildNumericWrapper( writeNumericArg(view, argInfos[i], argOffsets[i], args[i], i); } rawFn(); - }; + } }.invoke; } - return function(...args) { + return { invoke(...args) { if (args.length !== nargs) { throwFFIArgCountError(nargs, args.length); } @@ -519,7 +521,7 @@ function buildNumericWrapper( } rawFn(); return retGetter(view, 0, true); - }; + } }.invoke; } module.exports = { diff --git a/lib/internal/ffi/fast-api.js b/lib/internal/ffi/fast-api.js index b93d061b00cd..4355ab1a992f 100644 --- a/lib/internal/ffi/fast-api.js +++ b/lib/internal/ffi/fast-api.js @@ -254,6 +254,8 @@ function throwIfFastLibraryClosed(state) { } } +// Concise methods do not have [[Construct]], unlike function expressions. +// Keep wrappers non-constructible to match the native FFI functions. function wrapWithRawPointerConversions(rawFn, argumentTypes, owner) { if (rawFn === undefined || rawFn === null) { return rawFn; @@ -287,7 +289,7 @@ function wrapWithRawPointerConversions(rawFn, argumentTypes, owner) { const memory0 = needsRawPointerConversion(t0) || string0; const fastBufferInvoke = needsPointerLikeConversion(t0) ? rawFn[kFastBufferInvoke] : undefined; - wrapper = function(a0) { + wrapper = { invoke(a0) { throwIfFastLibraryClosed(state); if (arguments.length !== 1) { throwFFIArgCountError(1, arguments.length); @@ -312,13 +314,13 @@ function wrapWithRawPointerConversions(rawFn, argumentTypes, owner) { arg = getRawPointerArg(arg, 0); } return rawFn(arg); - }; + } }.invoke; } else if (nargs === 2) { const c0 = ArrayPrototypeIncludes(indexes, 0); const c1 = ArrayPrototypeIncludes(indexes, 1); const t0 = argumentTypes[0]; const t1 = argumentTypes[1]; - wrapper = function(a0, a1) { + wrapper = { invoke(a0, a1) { throwIfFastLibraryClosed(state); if (arguments.length !== 2) { throwFFIArgCountError(2, arguments.length); @@ -332,7 +334,7 @@ function wrapWithRawPointerConversions(rawFn, argumentTypes, owner) { } finally { if (stringCall) exitStringConversion(stringState); } - }; + } }.invoke; } else if (nargs === 3) { const c0 = ArrayPrototypeIncludes(indexes, 0); const c1 = ArrayPrototypeIncludes(indexes, 1); @@ -340,7 +342,7 @@ function wrapWithRawPointerConversions(rawFn, argumentTypes, owner) { const t0 = argumentTypes[0]; const t1 = argumentTypes[1]; const t2 = argumentTypes[2]; - wrapper = function(a0, a1, a2) { + wrapper = { invoke(a0, a1, a2) { throwIfFastLibraryClosed(state); if (arguments.length !== 3) { throwFFIArgCountError(3, arguments.length); @@ -356,9 +358,9 @@ function wrapWithRawPointerConversions(rawFn, argumentTypes, owner) { } finally { if (stringCall) exitStringConversion(stringState); } - }; + } }.invoke; } else { - wrapper = function(...args) { + wrapper = { invoke(...args) { throwIfFastLibraryClosed(state); if (args.length !== nargs) { throwFFIArgCountError(nargs, args.length); @@ -382,7 +384,7 @@ function wrapWithRawPointerConversions(rawFn, argumentTypes, owner) { } finally { if (stringCall) exitStringConversion(stringState); } - }; + } }.invoke; } return inheritMetadata(wrapper, rawFn, nargs); diff --git a/src/node_ffi.cc b/src/node_ffi.cc index 42c62c829168..3bf80b142dfd 100644 --- a/src/node_ffi.cc +++ b/src/node_ffi.cc @@ -324,7 +324,9 @@ MaybeLocal DynamicLibrary::CreateFunction( maybe_ret = Function::New(context, use_sb ? DynamicLibrary::InvokeFunctionSB : DynamicLibrary::InvokeFunction, - info->object()); + info->object(), + 0, + v8::ConstructorBehavior::kThrow); } Local ret; @@ -377,8 +379,11 @@ MaybeLocal DynamicLibrary::CreateFunction( // (strings, Buffers, ArrayBuffers, and ArrayBufferViews). if (has_ptr_args) { Local slow_fn; - if (!Function::New( - context, DynamicLibrary::InvokeFunction, info->object()) + if (!Function::New(context, + DynamicLibrary::InvokeFunction, + info->object(), + 0, + v8::ConstructorBehavior::kThrow) .ToLocal(&slow_fn)) { return MaybeLocal(); } diff --git a/test/ffi/test-ffi-dynamic-library.js b/test/ffi/test-ffi-dynamic-library.js index 82400335a12f..d22cf48d77b1 100644 --- a/test/ffi/test-ffi-dynamic-library.js +++ b/test/ffi/test-ffi-dynamic-library.js @@ -67,6 +67,27 @@ test('dlopen resolves functions from definitions', () => { } }); +test('FFI functions are not constructible', () => { + const { lib, functions } = ffi.dlopen(libraryPath, { + add_i32: fixtureSymbols.add_i32, + multiply_f64: fixtureSymbols.multiply_f64, + }); + + try { + assert.strictEqual(Object.hasOwn(functions.add_i32, 'prototype'), false); + assert.strictEqual( + Object.hasOwn(functions.multiply_f64, 'prototype'), false); + assert.throws( + () => Reflect.construct(functions.add_i32, [20, 22]), + TypeError); + assert.throws( + () => Reflect.construct(functions.multiply_f64, [6, 7]), + TypeError); + } finally { + lib.close(); + } +}); + test('DynamicLibrary exposes functions and symbols', () => { const lib = new ffi.DynamicLibrary(libraryPath); From d95dba23d467a874c8e71078c9547151b2e52f5c Mon Sep 17 00:00:00 2001 From: Trivikram Kamat <16024985+trivikr@users.noreply.github.com> Date: Tue, 4 Aug 2026 03:54:39 +0000 Subject: [PATCH 2/6] ffi: remove dead null check in callback arguments InvokeCallback tested `args[i] == nullptr` and mapped the argument to JS `null`. `args` is libffi's avalue array, and libffi always points each slot at its own storage for the corresponding argument, so the slot pointers are never null and the branch never ran. The check also read as a guarantee the code does not provide: a NULL pointer argument surfaces as the BigInt `0n`, because ToJSArgument converts `ffi_type_pointer` values with BigInt::NewFromUnsigned. Drop the branch rather than reimplementing it in ToJSArgument, which would change behavior by making pointer parameters arrive as either a BigInt or `null`. Signed-off-by: Trivikram Kamat <16024985+trivikr@users.noreply.github.com> Assisted-by: claude:opus-5 PR-URL: https://github.com/nodejs/node/pull/64998 Reviewed-By: Paolo Insogna --- src/node_ffi.cc | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/node_ffi.cc b/src/node_ffi.cc index 3bf80b142dfd..7e3da95b958f 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::Null; using v8::Object; using v8::PropertyAttribute; using v8::ReadOnly; @@ -715,13 +714,11 @@ void DynamicLibrary::InvokeCallback(ffi_cif* cif, size_t expected_args = cb->args.size(); LocalVector callback_args(isolate, expected_args); + // libffi always points `args[i]` at its own storage for the value of + // argument `i`, so the slot pointers themselves are never null. A NULL + // pointer argument surfaces as the BigInt `0n` via ToJSArgument. for (size_t i = 0; i < expected_args; i++) { - if (args[i] == nullptr) { - callback_args[i] = Null(isolate); - continue; - } else { - callback_args[i] = ToJSArgument(isolate, cb->args[i], args[i]); - } + callback_args[i] = ToJSArgument(isolate, cb->args[i], args[i]); } TryCatch try_catch(isolate); From 89310c73d296d2bdc8677ad4fc9dc3fad563f063 Mon Sep 17 00:00:00 2001 From: avivkeller Date: Sat, 18 Jul 2026 18:52:13 -0700 Subject: [PATCH 3/6] repl: add benchmarks Signed-off-by: avivkeller PR-URL: https://github.com/nodejs/node/pull/64590 Reviewed-By: James M Snell --- benchmark/repl/completion.js | 53 +++++++++++++++++++++++++ benchmark/repl/creation.js | 39 ++++++++++++++++++ benchmark/repl/evaluate.js | 57 +++++++++++++++++++++++++++ benchmark/repl/process-lines.js | 52 ++++++++++++++++++++++++ benchmark/repl/reset-context.js | 26 ++++++++++++ test/benchmark/test-benchmark-repl.js | 8 ++++ 6 files changed, 235 insertions(+) create mode 100644 benchmark/repl/completion.js create mode 100644 benchmark/repl/creation.js create mode 100644 benchmark/repl/evaluate.js create mode 100644 benchmark/repl/process-lines.js create mode 100644 benchmark/repl/reset-context.js create mode 100644 test/benchmark/test-benchmark-repl.js diff --git a/benchmark/repl/completion.js b/benchmark/repl/completion.js new file mode 100644 index 000000000000..b9f55d8416b0 --- /dev/null +++ b/benchmark/repl/completion.js @@ -0,0 +1,53 @@ +'use strict'; + +const common = require('../common.js'); +const repl = require('node:repl'); +const { PassThrough, Writable } = require('node:stream'); + +const bench = common.createBenchmark(main, { + n: [5e3], + query: [ + 'cons', + 'console.lo', + 'Buffer.prototype.wri', + "require('f", + ], + useGlobal: [0, 1], +}); + +function main({ n, query, useGlobal }) { + const input = new PassThrough(); + const output = new Writable({ write(c, e, cb) { cb(); } }); + const server = new repl.REPLServer({ + input, + output, + terminal: false, + useGlobal: !!useGlobal, + }); + + // Inspector callbacks do not keep the event loop alive on their own. + const keepAlive = setInterval(() => {}, 0x7fffffff); + let remaining = n; + + function complete() { + server.complete(query, onComplete); + } + + function onComplete(err) { + if (err) { + throw err; + } + + if (--remaining === 0) { + bench.end(n); + clearInterval(keepAlive); + server.close(); + return; + } + + setImmediate(complete); + } + + bench.start(); + setImmediate(complete); +} diff --git a/benchmark/repl/creation.js b/benchmark/repl/creation.js new file mode 100644 index 000000000000..60e795063d19 --- /dev/null +++ b/benchmark/repl/creation.js @@ -0,0 +1,39 @@ +'use strict'; + +const common = require('../common.js'); +const repl = require('node:repl'); +const { PassThrough, Writable } = require('node:stream'); + +const bench = common.createBenchmark(main, { + n: [500], + preview: [0, 1], + terminal: [0, 1], + useGlobal: [0, 1], +}, { + combinationFilter: ({ preview, terminal }) => !!terminal || !preview, +}); + +function main({ n, preview, terminal, useGlobal }) { + const inputs = Array.from({ length: n }, () => new PassThrough()); + const outputs = Array.from( + { length: n }, + () => new Writable({ write(c, e, cb) { cb(); } }), + ); + const servers = new Array(n); + + bench.start(); + for (let i = 0; i < n; i++) { + servers[i] = new repl.REPLServer({ + input: inputs[i], + output: outputs[i], + preview: !!preview, + terminal: !!terminal, + useGlobal: !!useGlobal, + }); + } + bench.end(n); + + for (const server of servers) { + server.close(); + } +} diff --git a/benchmark/repl/evaluate.js b/benchmark/repl/evaluate.js new file mode 100644 index 000000000000..48376a2b48fe --- /dev/null +++ b/benchmark/repl/evaluate.js @@ -0,0 +1,57 @@ +'use strict'; + +const common = require('../common.js'); +const repl = require('node:repl'); +const { PassThrough, Writable } = require('node:stream'); + +const bench = common.createBenchmark(main, { + n: [2e4], + code: [ + '1 + 1', + '({ answer: 42 })', + 'Promise.resolve(42)', + 'await Promise.resolve(42)', + ], + mode: ['sloppy', 'strict'], + useGlobal: [0, 1], +}); + +function main({ n, code, mode, useGlobal }) { + const input = new PassThrough(); + const output = new Writable({ write(c, e, cb) { cb(); } }); + const server = new repl.REPLServer({ + input, + output, + replMode: mode === 'strict' ? + repl.REPL_MODE_STRICT : + repl.REPL_MODE_SLOPPY, + terminal: false, + useGlobal: !!useGlobal, + }); + + // Inspector callbacks do not keep the event loop alive on their own. + const keepAlive = setInterval(() => {}, 0x7fffffff); + let remaining = n; + + function evaluate() { + server.eval(`${code}\n`, server.context, 'repl', onEvaluate); + } + + function onEvaluate(err) { + if (err) { + throw err; + } + + if (--remaining === 0) { + bench.end(n); + clearInterval(keepAlive); + server.close(); + return; + } + + setImmediate(evaluate); + } + + bench.start(); + setImmediate(evaluate); +} diff --git a/benchmark/repl/process-lines.js b/benchmark/repl/process-lines.js new file mode 100644 index 000000000000..fd019512f9fc --- /dev/null +++ b/benchmark/repl/process-lines.js @@ -0,0 +1,52 @@ +'use strict'; + +const common = require('../common.js'); +const repl = require('node:repl'); +const { PassThrough, Writable } = require('node:stream'); + +const bench = common.createBenchmark(main, { + n: [1e4], + code: [ + '1 + 1\n', + 'Promise.resolve(42)\n', + ], + mode: ['sloppy', 'strict'], + terminal: [0, 1], + useGlobal: [0, 1], +}); + +function main({ n, code: inputCode, mode, terminal, useGlobal }) { + const input = new PassThrough(); + const output = new Writable({ write(c, e, cb) { cb(); } }); + const server = new repl.REPLServer({ + input, + output, + replMode: mode === 'strict' ? + repl.REPL_MODE_STRICT : + repl.REPL_MODE_SLOPPY, + terminal: !!terminal, + useGlobal: !!useGlobal, + }); + const originalEval = server.eval; + // TTY input dispatch can briefly have no other active event loop handles. + const keepAlive = setInterval(() => {}, 0x7fffffff); + let remaining = n; + + // eslint-disable-next-line node-core/func-name-matching + server.eval = function REPLEval(code, context, file, callback) { + originalEval(code, context, file, function onEvaluate() { + const result = Reflect.apply(callback, this, arguments); + if (--remaining === 0) { + bench.end(n); + clearInterval(keepAlive); + server.close(); + } else { + setImmediate(() => input.write(inputCode)); + } + return result; + }); + }; + + bench.start(); + input.write(inputCode); +} diff --git a/benchmark/repl/reset-context.js b/benchmark/repl/reset-context.js new file mode 100644 index 000000000000..ab96f92564f2 --- /dev/null +++ b/benchmark/repl/reset-context.js @@ -0,0 +1,26 @@ +'use strict'; + +const common = require('../common.js'); +const repl = require('node:repl'); +const { PassThrough, Writable } = require('node:stream'); + +const bench = common.createBenchmark(main, { + n: [1e3], +}); + +function main({ n }) { + const input = new PassThrough(); + const output = new Writable({ write(c, e, cb) { cb(); } }); + const server = new repl.REPLServer({ + input, + output, + terminal: false, + }); + + bench.start(); + for (let i = 0; i < n; i++) { + server.resetContext(); + } + bench.end(n); + server.close(); +} diff --git a/test/benchmark/test-benchmark-repl.js b/test/benchmark/test-benchmark-repl.js new file mode 100644 index 000000000000..045813039589 --- /dev/null +++ b/test/benchmark/test-benchmark-repl.js @@ -0,0 +1,8 @@ +'use strict'; + +const common = require('../common'); +const runBenchmark = require('../common/benchmark'); + +common.skipIfInspectorDisabled(); + +runBenchmark('repl', { NODEJS_BENCHMARK_ZERO_ALLOWED: 1 }); From 6456418ac1751774abacd7b3f05b5916732495a3 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sat, 8 Aug 2026 21:31:29 -0700 Subject: [PATCH 4/6] src: add v8::Local specialization for MaybeStackBuffer Long-term itch. Per v8 rules, we're not supposed to be heap allocating v8::Local's; instead we're supposed to be using v8::LocalVector. Create a specialization of MaybeStackBuffer that uses either a stack array of v8::Locals or v8::LocalVector with some additional utility improvements. Signed-off-by: James M Snell PR-URL: https://github.com/nodejs/node/pull/65159 Reviewed-By: Stephen Belanger --- src/api/callback.cc | 2 +- src/cares_wrap.cc | 4 +- src/crypto/crypto_tls.cc | 6 +-- src/js_stream.cc | 8 +--- src/js_udp_wrap.cc | 9 ++--- src/node_concepts.h | 7 ++++ src/node_dir.cc | 5 ++- src/node_env_var.cc | 7 +++- src/node_http2.cc | 17 ++++---- src/node_messaging.cc | 4 +- src/node_messaging.h | 2 +- src/node_v8.cc | 10 ++--- src/spawn_sync.cc | 5 +-- src/util-inl.h | 40 +++++++++++++++---- src/util.h | 84 +++++++++++++++++++++++++++++++++++++++- 15 files changed, 161 insertions(+), 49 deletions(-) diff --git a/src/api/callback.cc b/src/api/callback.cc index 0f458f470501..85da82ff84a5 100644 --- a/src/api/callback.cc +++ b/src/api/callback.cc @@ -245,7 +245,7 @@ MaybeLocal InternalMakeCallback(Environment* env, Local context = env->context(); if (use_async_hooks_trampoline) { - MaybeStackBuffer, 16> args(3 + argc); + MaybeStackBuffer args(env->isolate(), 3 + argc); args[0] = Number::New(env->isolate(), asyncContext.async_id); args[1] = resource; args[2] = callback; diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index bf47c239abbe..9aa77e5c8574 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -187,12 +187,12 @@ Local AddrTTLToArray( Environment* env, const T* addrttls, size_t naddrttls) { - MaybeStackBuffer, 8> ttls(naddrttls); + MaybeStackBuffer ttls(env->isolate(), naddrttls); for (size_t i = 0; i < naddrttls; i++) { ttls[i] = Integer::NewFromUnsigned(env->isolate(), addrttls[i].ttl); } - return Array::New(env->isolate(), ttls.out(), naddrttls); + return ttls.ToArray(); } // Parse the CSV produced by ares_get_servers_csv() back into (ip, port) diff --git a/src/crypto/crypto_tls.cc b/src/crypto/crypto_tls.cc index 936c201de99d..5a531209e232 100644 --- a/src/crypto/crypto_tls.cc +++ b/src/crypto/crypto_tls.cc @@ -41,7 +41,6 @@ using ncrypto::MarkPopErrorOnReturn; using ncrypto::SSLPointer; using ncrypto::SSLSessionPointer; using ncrypto::X509Pointer; -using v8::Array; using v8::ArrayBuffer; using v8::ArrayBufferView; using v8::BackingStore; @@ -1956,7 +1955,7 @@ void TLSWrap::GetSharedSigalgs(const FunctionCallbackInfo& args) { SSL* ssl = w->ssl_.get(); int nsig = SSL_get_shared_sigalgs(ssl, 0, nullptr, nullptr, nullptr, nullptr, nullptr); - MaybeStackBuffer, 16> ret_arr(nsig); + MaybeStackBuffer ret_arr(env->isolate(), nsig); for (int i = 0; i < nsig; i++) { int hash_nid; @@ -2023,8 +2022,7 @@ void TLSWrap::GetSharedSigalgs(const FunctionCallbackInfo& args) { ret_arr[i] = OneByteString(env->isolate(), sig_with_md); } - args.GetReturnValue().Set( - Array::New(env->isolate(), ret_arr.out(), ret_arr.length())); + args.GetReturnValue().Set(ret_arr.ToArray()); } void TLSWrap::ExportKeyingMaterial(const FunctionCallbackInfo& args) { diff --git a/src/js_stream.cc b/src/js_stream.cc index 0d6d6a7ac80a..3a3a1c943d25 100644 --- a/src/js_stream.cc +++ b/src/js_stream.cc @@ -11,7 +11,6 @@ namespace node { using errors::TryCatchScope; -using v8::Array; using v8::Context; using v8::FunctionCallbackInfo; using v8::FunctionTemplate; @@ -119,17 +118,14 @@ int JSStream::DoWrite(WriteWrap* w, int value_int = UV_EPROTO; - MaybeStackBuffer, 16> bufs_arr(count); + MaybeStackBuffer bufs_arr(env()->isolate(), count); for (size_t i = 0; i < count; i++) { if (!Buffer::Copy(env(), bufs[i].base, bufs[i].len).ToLocal(&bufs_arr[i])) { return value_int; } } - Local argv[] = { - w->object(), - Array::New(env()->isolate(), bufs_arr.out(), count) - }; + Local argv[] = {w->object(), bufs_arr.ToArray()}; TryCatchScope try_catch(env()); Local value; diff --git a/src/js_udp_wrap.cc b/src/js_udp_wrap.cc index 1648d162b8f7..83b91042023f 100644 --- a/src/js_udp_wrap.cc +++ b/src/js_udp_wrap.cc @@ -11,7 +11,6 @@ namespace node { using errors::TryCatchScope; -using v8::Array; using v8::Context; using v8::FunctionCallbackInfo; using v8::FunctionTemplate; @@ -98,7 +97,7 @@ ssize_t JSUDPWrap::Send(uv_buf_t* bufs, int64_t value_int = JS_EXCEPTION_PENDING; size_t total_len = 0; - MaybeStackBuffer, 16> buffers(nbufs); + MaybeStackBuffer buffers(env()->isolate(), nbufs); for (size_t i = 0; i < nbufs; i++) { if (!Buffer::Copy(env(), bufs[i].base, bufs[i].len).ToLocal(&buffers[i])) { return value_int; @@ -110,9 +109,9 @@ ssize_t JSUDPWrap::Send(uv_buf_t* bufs, if (!AddressToJS(env(), addr).ToLocal(&address)) return value_int; Local args[] = { - listener()->CreateSendWrap(total_len)->object(), - Array::New(env()->isolate(), buffers.out(), nbufs), - address, + listener()->CreateSendWrap(total_len)->object(), + buffers.ToArray(), + address, }; if (!MakeCallback(env()->onwrite_string(), arraysize(args), args) diff --git a/src/node_concepts.h b/src/node_concepts.h index 5dbce5c6f88a..3109e2ab83f8 100644 --- a/src/node_concepts.h +++ b/src/node_concepts.h @@ -3,6 +3,7 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS +#include #include #include #include @@ -32,6 +33,12 @@ concept StandardCharType = template concept IsCallable = std::is_function::value || requires { &T::operator(); }; +// Types that can reside on V8's managed heap (v8::Value, v8::Object, etc.). +// Used to select the MaybeStackBuffer specialization that holds handles in a +// v8::LocalVector instead of malloc'd memory. +template +concept V8Type = std::is_base_of_v; + } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/src/node_dir.cc b/src/node_dir.cc index 952161d9e2cf..eca5140b6423 100644 --- a/src/node_dir.cc +++ b/src/node_dir.cc @@ -206,7 +206,7 @@ static MaybeLocal DirentListToArray(Environment* env, uv_dirent_t* ents, int num, enum encoding encoding) { - MaybeStackBuffer, 64> entries(num * 2); + MaybeStackBuffer entries(env->isolate(), num * 2); // Return an array of all read filenames. int j = 0; @@ -222,7 +222,8 @@ static MaybeLocal DirentListToArray(Environment* env, entries[j++] = Integer::New(env->isolate(), ents[i].type); } - return Array::New(env->isolate(), entries.out(), j); + CHECK_EQ(j, num * 2); + return entries.ToArray(); } static void AfterDirRead(uv_fs_t* req) { diff --git a/src/node_env_var.cc b/src/node_env_var.cc index e94180cd659d..25e405be86d2 100644 --- a/src/node_env_var.cc +++ b/src/node_env_var.cc @@ -201,7 +201,7 @@ MaybeLocal RealEnvStore::Enumerate(Isolate* isolate) const { auto cleanup = OnScopeLeave([&]() { uv_os_free_environ(items, count); }); CHECK_EQ(uv_os_environ(&items, &count), 0); - MaybeStackBuffer, 256> env_v(count); + MaybeStackBuffer env_v(isolate, count); int env_v_index = 0; for (int i = 0; i < count; i++) { #ifdef _WIN32 @@ -216,7 +216,10 @@ MaybeLocal RealEnvStore::Enumerate(Isolate* isolate) const { env_v[env_v_index++] = str; } - return Array::New(isolate, env_v.out(), env_v_index); + // We're possibly not filling the entire buffer. + CHECK_LE(env_v_index, count); + env_v.SetLength(env_v_index); + return env_v.ToArray(); } std::shared_ptr KVStore::Clone(Isolate* isolate) const { diff --git a/src/node_http2.cc b/src/node_http2.cc index 04b2acca148d..58f12c5561b5 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -1609,8 +1609,8 @@ void Http2Session::HandleHeadersFrame(const nghttp2_frame* frame) { // this way for performance reasons (it's faster to generate and pass an // array than it is to generate and pass the object). - MaybeStackBuffer, 64> headers_v(stream->headers_count() * 2); - MaybeStackBuffer, 32> sensitive_v(stream->headers_count()); + MaybeStackBuffer headers_v(isolate, stream->headers_count() * 2); + MaybeStackBuffer sensitive_v(isolate, stream->headers_count()); size_t sensitive_count = 0; stream->TransferHeaders([&](const Http2Header& header, size_t i) { @@ -1627,13 +1627,14 @@ void Http2Session::HandleHeadersFrame(const nghttp2_frame* frame) { stream->retained_headers_length_ += stream->current_headers_length_; stream->current_headers_length_ = 0; + sensitive_v.SetLength(sensitive_count); Local args[] = { - stream->object(), - Integer::New(isolate, id), - Integer::New(isolate, stream->headers_category()), - Integer::New(isolate, frame->hd.flags), - Array::New(isolate, headers_v.out(), headers_v.length()), - Array::New(isolate, sensitive_v.out(), sensitive_count), + stream->object(), + Integer::New(isolate, id), + Integer::New(isolate, stream->headers_category()), + Integer::New(isolate, frame->hd.flags), + headers_v.ToArray(), + sensitive_v.ToArray(), }; MakeCallback(env()->http2session_on_headers_function(), arraysize(args), args); diff --git a/src/node_messaging.cc b/src/node_messaging.cc index f00ab803fef0..5a0c4962d3f7 100644 --- a/src/node_messaging.cc +++ b/src/node_messaging.cc @@ -1093,7 +1093,7 @@ void MessagePort::PostMessage(const FunctionCallbackInfo& args) { "MessagePort.postMessage"); } - TransferList transfer_list; + TransferList transfer_list(env->isolate()); if (!GetTransferList(env, context, args[1], &transfer_list)) { return; } @@ -1607,7 +1607,7 @@ static void StructuredClone(const FunctionCallbackInfo& args) { Local value = args[0]; - TransferList transfer_list; + TransferList transfer_list(isolate); Local options = args[1].As(); Local transfer_list_v; if (!options->Get(context, env->transfer_string()) diff --git a/src/node_messaging.h b/src/node_messaging.h index 3a838a39200a..0abd7da3bf02 100644 --- a/src/node_messaging.h +++ b/src/node_messaging.h @@ -17,7 +17,7 @@ namespace worker { class MessagePortData; class MessagePort; -typedef MaybeStackBuffer, 8> TransferList; +typedef MaybeStackBuffer TransferList; // Used to represent the in-flight structure of an object that is being // transferred or cloned using postMessage(). diff --git a/src/node_v8.cc b/src/node_v8.cc index 34e8460790b7..14ee626c9f3e 100644 --- a/src/node_v8.cc +++ b/src/node_v8.cc @@ -745,17 +745,17 @@ void Initialize(Local target, // Heap space names are extracted once and exposed to JavaScript to // avoid excessive creation of heap space name Strings. HeapSpaceStatistics s; - MaybeStackBuffer, 16> heap_spaces(number_of_heap_spaces); + MaybeStackBuffer heap_spaces(env->isolate(), + number_of_heap_spaces); for (size_t i = 0; i < number_of_heap_spaces; i++) { env->isolate()->GetHeapSpaceStatistics(&s, i); heap_spaces[i] = String::NewFromUtf8(env->isolate(), s.space_name()) .ToLocalChecked(); } target - ->Set( - context, - FIXED_ONE_BYTE_STRING(env->isolate(), "kHeapSpaces"), - Array::New(env->isolate(), heap_spaces.out(), number_of_heap_spaces)) + ->Set(context, + FIXED_ONE_BYTE_STRING(env->isolate(), "kHeapSpaces"), + heap_spaces.ToArray()) .Check(); SetMethod(context, diff --git a/src/spawn_sync.cc b/src/spawn_sync.cc index 2da2e18950ad..0358bb995fee 100644 --- a/src/spawn_sync.cc +++ b/src/spawn_sync.cc @@ -767,7 +767,7 @@ MaybeLocal SyncProcessRunner::BuildOutputArray() { CHECK(!stdio_pipes_.empty()); EscapableHandleScope scope(env()->isolate()); - MaybeStackBuffer, 8> js_output(stdio_pipes_.size()); + MaybeStackBuffer js_output(env()->isolate(), stdio_pipes_.size()); for (uint32_t i = 0; i < stdio_pipes_.size(); i++) { SyncProcessStdioPipe* h = stdio_pipes_[i].get(); @@ -781,8 +781,7 @@ MaybeLocal SyncProcessRunner::BuildOutputArray() { } } - return scope.Escape( - Array::New(env()->isolate(), js_output.out(), js_output.length())); + return scope.Escape(js_output.ToArray()); } Maybe SyncProcessRunner::ParseOptions(Local js_value) { diff --git a/src/util-inl.h b/src/util-inl.h index 0a5a90def488..7eb5ebabbfea 100644 --- a/src/util-inl.h +++ b/src/util-inl.h @@ -394,14 +394,13 @@ v8::MaybeLocal ToV8Value(v8::Local context, if (isolate == nullptr) isolate = v8::Isolate::GetCurrent(); v8::EscapableHandleScope handle_scope(isolate); - MaybeStackBuffer, 128> arr(vec.size()); - arr.SetLength(vec.size()); + MaybeStackBuffer arr(isolate, vec.size()); for (size_t i = 0; i < vec.size(); ++i) { if (!ToV8Value(context, vec[i], isolate).ToLocal(&arr[i])) return v8::MaybeLocal(); } - return handle_scope.Escape(v8::Array::New(isolate, arr.out(), arr.length())); + return handle_scope.Escape(arr.ToArray()); } template @@ -430,8 +429,7 @@ v8::MaybeLocal ToV8Value(v8::Local context, if (isolate == nullptr) isolate = v8::Isolate::GetCurrent(); v8::EscapableHandleScope handle_scope(isolate); - MaybeStackBuffer, 128> arr(vec.size()); - arr.SetLength(vec.size()); + MaybeStackBuffer arr(isolate, vec.size()); auto it = vec.begin(); for (size_t i = 0; i < vec.size(); ++i) { if (!ToV8Value(context, *it, isolate).ToLocal(&arr[i])) @@ -439,7 +437,7 @@ v8::MaybeLocal ToV8Value(v8::Local context, std::advance(it, 1); } - return handle_scope.Escape(v8::Array::New(isolate, arr.out(), arr.length())); + return handle_scope.Escape(arr.ToArray()); } template @@ -519,7 +517,14 @@ v8::Local ToV8ValuePrimitiveArray(v8::Local context, } SlicedArguments::SlicedArguments( - const v8::FunctionCallbackInfo& args, size_t start) { + const v8::FunctionCallbackInfo& args, size_t start) + : SlicedArguments(args.GetIsolate(), args, start) {} + +SlicedArguments::SlicedArguments( + v8::Isolate* isolate, + const v8::FunctionCallbackInfo& args, + size_t start) + : MaybeStackBuffer(isolate) { const size_t length = static_cast(args.Length()); if (start >= length) return; const size_t size = length - start; @@ -545,6 +550,27 @@ void MaybeStackBuffer::AllocateSufficientStorage( length_ = storage; } +template +void MaybeStackBuffer::AllocateSufficientStorage( + size_t storage) { + CHECK(!IsInvalidated()); + if (storage > capacity()) { + if (!local_vector_.has_value()) { + local_vector_.emplace(isolate_, storage); + // Copy existing stack data into the LocalVector. + for (size_t i = 0; i < length_; i++) { + (*local_vector_)[i] = buf_st_[i]; + } + } else { + local_vector_->resize(storage); + } + buf_ = local_vector_->data(); + capacity_ = storage; + } + + length_ = storage; +} + template requires(sizeof(T) == 1) ArrayBufferViewContents::ArrayBufferViewContents( diff --git a/src/util.h b/src/util.h index 0461a5ebb19f..621d2fbf4ee6 100644 --- a/src/util.h +++ b/src/util.h @@ -526,6 +526,85 @@ class MaybeStackBuffer { T buf_st_[kStackStorageSize]; }; +template +class MaybeStackBuffer { + public: + using V = v8::Local; + + MaybeStackBuffer(const MaybeStackBuffer&) = delete; + MaybeStackBuffer& operator=(const MaybeStackBuffer& other) = delete; + + const V* out() const { return buf_; } + V* out() { return buf_; } + + // operator* for compatibility with `v8::String::(Utf8)Value` + V* operator*() { return buf_; } + const V* operator*() const { return buf_; } + + V& operator[](size_t index) { + CHECK_LT(index, length()); + return buf_[index]; + } + + const V& operator[](size_t index) const { + CHECK_LT(index, length()); + return buf_[index]; + } + + size_t length() const { return length_; } + + // Current maximum capacity of the buffer with which SetLength() can be used + // without first calling AllocateSufficientStorage(). + size_t capacity() const { return capacity_; } + + // Make sure enough space for `storage` entries is available. + // This method can be called multiple times throughout the lifetime of the + // buffer, but once this has been called Invalidate() cannot be used. + // Content of the buffer in the range [0, length()) is preserved. + void AllocateSufficientStorage(size_t storage); + + void SetLength(size_t length) { + // capacity() returns how much memory is actually available. + CHECK_LE(length, capacity()); + length_ = length; + } + + // If the buffer is stored in a LocalVector rather than on the stack. + bool IsAllocated() const { return !IsInvalidated() && buf_ != buf_st_; } + + // If Invalidate() has been called. + bool IsInvalidated() const { return buf_ == nullptr; } + + explicit MaybeStackBuffer(v8::Isolate* isolate) + : isolate_(isolate), + length_(0), + capacity_(arraysize(buf_st_)), + buf_(buf_st_) { + // Default to a zero-length, null-terminated buffer. + buf_[0] = V(); + } + + MaybeStackBuffer(v8::Isolate* isolate, size_t storage) + : MaybeStackBuffer(isolate) { + AllocateSufficientStorage(storage); + } + + // LocalVector (via optional) handles cleanup automatically. + ~MaybeStackBuffer() = default; + + v8::Local ToArray() const { + return v8::Array::New(isolate_, buf_, length_); + } + + private: + v8::Isolate* isolate_; + size_t length_; + size_t capacity_; + V* buf_; + V buf_st_[kStackStorageSize]; + std::optional> local_vector_; +}; + // Provides access to an ArrayBufferView's storage, either the original, // or for small data, a copy of it. This object's lifetime is bound to the // original ArrayBufferView's lifetime. @@ -782,10 +861,13 @@ constexpr inline bool IsBigEndian() { static_assert(IsLittleEndian() || IsBigEndian(), "Node.js does not support mixed-endian systems"); -class SlicedArguments : public MaybeStackBuffer> { +class SlicedArguments : public MaybeStackBuffer { public: inline explicit SlicedArguments( const v8::FunctionCallbackInfo& args, size_t start = 0); + inline SlicedArguments(v8::Isolate* isolate, + const v8::FunctionCallbackInfo& args, + size_t start = 0); }; // Convert a v8::PersistentBase, e.g. v8::Global, to a Local, with an extra From 45d6d31279be840e2f4184d72dbe0f369c481c29 Mon Sep 17 00:00:00 2001 From: Shani Singh Date: Tue, 18 Aug 2026 00:31:56 +0530 Subject: [PATCH 5/6] http: fix keylog listener setup on existing agent sockets `maybeEnableKeylog()` runs as the agent's `'newListener'` handler and attaches the agent's keylog handler to the sockets the agent already owns. `agent.sockets` maps a name to an array of sockets, but the loop treated those arrays as sockets and called `.on()` on them. Adding a `'keylog'` listener to an agent that already owned a socket therefore threw `TypeError: sockets[i].on is not a function` out of `agent.on('keylog', ...)`. Since the throw happened inside the `'newListener'` handler it propagated before the listener was stored, so the caller got an exception and no listener. Sockets parked in `agent.freeSockets` were never visited at all. Walk both maps the way `Agent.prototype.destroy()` does. Signed-off-by: Shani Singh PR-URL: https://github.com/nodejs/node/pull/65066 Reviewed-By: Tim Perry --- lib/_http_agent.js | 15 ++-- ...test-http-agent-keylog-existing-sockets.js | 71 +++++++++++++++++++ 2 files changed, 82 insertions(+), 4 deletions(-) create mode 100644 test/parallel/test-http-agent-keylog-existing-sockets.js diff --git a/lib/_http_agent.js b/lib/_http_agent.js index f4da2ed246cd..3871c5b56a2f 100644 --- a/lib/_http_agent.js +++ b/lib/_http_agent.js @@ -280,10 +280,17 @@ function maybeEnableKeylog(eventName) { this[kOnKeylog] = function onkeylog(keylog) { agent.emit('keylog', keylog, this); }; - // Existing sockets will start listening on keylog now. - const sockets = ObjectValues(this.sockets); - for (let i = 0; i < sockets.length; i++) { - sockets[i].on('keylog', this[kOnKeylog]); + // Existing sockets will start listening on keylog now. Both maps hold + // arrays of sockets keyed by name, so each bucket has to be walked. + const sets = [this.freeSockets, this.sockets]; + for (let s = 0; s < sets.length; s++) { + const buckets = ObjectValues(sets[s]); + for (let b = 0; b < buckets.length; b++) { + const sockets = buckets[b]; + for (let n = 0; n < sockets.length; n++) { + sockets[n].on('keylog', this[kOnKeylog]); + } + } } } } diff --git a/test/parallel/test-http-agent-keylog-existing-sockets.js b/test/parallel/test-http-agent-keylog-existing-sockets.js new file mode 100644 index 000000000000..1a391c09b8be --- /dev/null +++ b/test/parallel/test-http-agent-keylog-existing-sockets.js @@ -0,0 +1,71 @@ +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const http = require('http'); + +// Adding a 'keylog' listener to an agent is wired up by maybeEnableKeylog(), +// which attaches the agent's keylog handler to the sockets the agent already +// owns. `agent.sockets` and `agent.freeSockets` map a name to an *array* of +// sockets, so each bucket has to be walked. Treating the buckets themselves as +// sockets threw a TypeError out of `agent.on('keylog', ...)`, which also meant +// the listener was never registered. + +// Two servers so the two sockets get different names, which keeps one parked +// in freeSockets instead of being reused by the second request. +const idleServer = http.createServer((req, res) => res.end('idle')); +const busyServer = http.createServer((req, res) => { + setTimeout(() => res.end('busy'), common.platformTimeout(200)); +}); + +function countSockets(agent) { + let free = 0; + let active = 0; + for (const bucket of Object.values(agent.freeSockets)) free += bucket.length; + for (const bucket of Object.values(agent.sockets)) active += bucket.length; + return { free, active }; +} + +idleServer.listen(0, common.mustCall(() => { + busyServer.listen(0, common.mustCall(() => { + const agent = new http.Agent({ keepAlive: true, maxSockets: 4 }); + + // First request finishes, so its socket is released into freeSockets. + http.get({ port: idleServer.address().port, agent }, common.mustCall((res) => { + res.resume(); + res.on('end', common.mustCall(() => { + // Second request is still in flight, so its socket is in sockets. + const req = http.get({ port: busyServer.address().port, agent }, + common.mustCall((res2) => { + res2.resume(); + res2.on('end', common.mustCall(() => { + agent.destroy(); + idleServer.close(); + busyServer.close(); + })); + })); + + req.on('socket', common.mustCall(() => { + setImmediate(common.mustCall(() => { + const { free, active } = countSockets(agent); + assert.strictEqual(free, 1); + assert.strictEqual(active, 1); + + // Used to throw `TypeError: sockets[i].on is not a function`. + agent.on('keylog', common.mustNotCall()); + assert.strictEqual(agent.listenerCount('keylog'), 1); + + // Every existing socket, idle or in use, is now listening. + for (const set of [agent.freeSockets, agent.sockets]) { + for (const bucket of Object.values(set)) { + for (const socket of bucket) { + assert.strictEqual(socket.listenerCount('keylog'), 1); + } + } + } + })); + })); + })); + })); + })); +})); From 8488e1324af0631105cfaf365e0e2673de295696 Mon Sep 17 00:00:00 2001 From: Y1D7NG Date: Tue, 18 Aug 2026 05:03:39 +0800 Subject: [PATCH 6/6] fs: fix close listener leak in FileHandle streams Fixes: https://github.com/nodejs/node/issues/64214 Signed-off-by: y1d7ng PR-URL: https://github.com/nodejs/node/pull/64227 Reviewed-By: Chemi Atlow Reviewed-By: Claudio Wunder --- lib/internal/fs/streams.js | 14 ++++++- .../test-fs-promises-file-handle-stream.js | 39 +++++++++++++++++++ 2 files changed, 52 insertions(+), 1 deletion(-) diff --git a/lib/internal/fs/streams.js b/lib/internal/fs/streams.js index 4c53f7ea23e1..882aa8e71fc9 100644 --- a/lib/internal/fs/streams.js +++ b/lib/internal/fs/streams.js @@ -158,7 +158,19 @@ function importFd(stream, options) { stream[kHandle] = options.fd; stream[kFs] = FileHandleOperations(stream[kHandle]); stream[kHandle][kRef](); - options.fd.on('close', FunctionPrototypeBind(stream.close, stream)); + + const onclose = FunctionPrototypeBind(stream.close, stream); + options.fd.on('close', onclose); + if (options.autoClose === false) { + function cleanup() { + options.fd.removeListener('close', onclose); + options.fd[kUnref](); + } + stream.once('end', cleanup); + stream.once('finish', cleanup); + stream.once('error', cleanup); + } + return options.fd.fd; } diff --git a/test/parallel/test-fs-promises-file-handle-stream.js b/test/parallel/test-fs-promises-file-handle-stream.js index 71f312b6f9d7..61d0b3ca2ec7 100644 --- a/test/parallel/test-fs-promises-file-handle-stream.js +++ b/test/parallel/test-fs-promises-file-handle-stream.js @@ -42,7 +42,46 @@ async function validateRead() { ); } +async function validateReusedCreateReadStream() { + const filePath = path.resolve(tmpDir, 'tmp-reused-stream.txt'); + fs.writeFileSync(filePath, Buffer.from('ab', 'utf8')); + + const fileHandle = await open(filePath, 'r'); + try { + await buffer(fileHandle.createReadStream({ + start: 0, + end: 0, + autoClose: false, + })); + assert.strictEqual(fileHandle.listenerCount('close'), 0); + + await buffer(fileHandle.createReadStream({ + start: 1, + end: 1, + autoClose: false, + })); + assert.strictEqual(fileHandle.listenerCount('close'), 0); + } finally { + await fileHandle.close(); + } +} + +async function validateReusedCreateWriteStream() { + const filePath = path.resolve(tmpDir, 'tmp-reused-write-stream.txt'); + const fileHandle = await open(filePath, 'w'); + try { + const stream = fileHandle.createWriteStream({ autoClose: false }); + stream.end('a'); + await finished(stream); + assert.strictEqual(fileHandle.listenerCount('close'), 0); + } finally { + await fileHandle.close(); + } +} + Promise.all([ validateWrite(), validateRead(), + validateReusedCreateReadStream(), + validateReusedCreateWriteStream(), ]).then(common.mustCall());