From 4101a7352c659d540ba615b94c1267d00b3c3b1e Mon Sep 17 00:00:00 2001 From: agape1225 <49804691+agape1225@users.noreply.github.com> Date: Sat, 3 Oct 2026 13:59:11 +0900 Subject: [PATCH] vm: fix breakOnSigint race in Module.evaluate() vm.Script's runInContext()/runInThisContext() temporarily remove the process's own SIGINT listeners while running with breakOnSigint: true, since the native watchdog installed for that call consumes the signal to interrupt execution, and the process's own listeners would race with it. vm.Module.prototype.evaluate() accepts the same breakOnSigint option and installs the same native watchdog, but never got the matching guard. Move the existing helper into a shared location and apply it to Module.prototype.evaluate() too. Verified the new test fails against the unfixed code (child process killed by raw SIGINT) and passes with the fix. Signed-off-by: agape1225 <49804691+agape1225@users.noreply.github.com> --- lib/internal/vm.js | 31 +++++++ lib/internal/vm/module.js | 6 +- lib/vm.js | 19 +---- .../test-vm-module-sigint-existing-handler.js | 85 +++++++++++++++++++ 4 files changed, 122 insertions(+), 19 deletions(-) create mode 100644 test/parallel/test-vm-module-sigint-existing-handler.js diff --git a/lib/internal/vm.js b/lib/internal/vm.js index 9f1c33b0ccfe..e6c5f4aaed69 100644 --- a/lib/internal/vm.js +++ b/lib/internal/vm.js @@ -1,7 +1,9 @@ 'use strict'; const { + ArrayPrototypeForEach, FunctionPrototypeCall, + ReflectApply, Symbol, } = primordials; @@ -111,6 +113,34 @@ function registerImportModuleDynamically(referrer, importModuleDynamically) { }); } +/** + * Temporarily removes all SIGINT listeners before invoking `fn`, re-attaching + * them afterwards. Used to run code with `breakOnSigint: true`, where a + * native SigintWatchdog is installed to terminate execution on SIGINT: if the + * process's own SIGINT listeners stayed attached, they would race with that + * watchdog over the same signal instead of running normally once the + * watchdog is gone. + * @param {Function} fn - The function to invoke with SIGINT listeners removed. + * @param {object} thisArg - The `this` value to invoke `fn` with. + * @param {Array} argsArray - The arguments to invoke `fn` with. + * @returns {any} + */ +function sigintHandlersWrap(fn, thisArg, argsArray) { + const sigintListeners = process.rawListeners('SIGINT'); + + process.removeAllListeners('SIGINT'); + + try { + return ReflectApply(fn, thisArg, argsArray); + } finally { + // Add using the public methods so that the `newListener` handler of + // process can re-attach the listeners. + ArrayPrototypeForEach(sigintListeners, (listener) => { + process.addListener('SIGINT', listener); + }); + } +} + /** * Compiles a function from the given code string. * @param {string} code - The code string to compile. @@ -234,4 +264,5 @@ module.exports = { makeContextifyScript, registerImportModuleDynamically, runScriptInThisContext, + sigintHandlersWrap, }; diff --git a/lib/internal/vm/module.js b/lib/internal/vm/module.js index ce2350f6cd16..464aa4d28031 100644 --- a/lib/internal/vm/module.js +++ b/lib/internal/vm/module.js @@ -92,7 +92,7 @@ const kContext = Symbol('kContext'); const kPerContextModuleId = Symbol('kPerContextModuleId'); const kLink = Symbol('kLink'); -const { isContext } = require('internal/vm'); +const { isContext, sigintHandlersWrap } = require('internal/vm'); function isModule(object) { if (typeof object !== 'object' || object === null || !ObjectPrototypeHasOwnProperty(object, kWrap)) { @@ -230,6 +230,10 @@ class Module { 'must be one of linked, evaluated, or errored', ); } + if (breakOnSigint && process.listenerCount('SIGINT') > 0) { + return sigintHandlersWrap( + this[kWrap].evaluate, this[kWrap], [timeout, breakOnSigint]); + } return this[kWrap].evaluate(timeout, breakOnSigint); } catch (e) { return PromiseReject(e); diff --git a/lib/vm.js b/lib/vm.js index 2c7446af6076..913c85a679df 100644 --- a/lib/vm.js +++ b/lib/vm.js @@ -63,6 +63,7 @@ const { internalCompileFunction, isContext: _isContext, registerImportModuleDynamically, + sigintHandlersWrap, } = require('internal/vm'); const { vm_dynamic_import_main_context_default, @@ -270,24 +271,6 @@ function createScript(code, options) { return new Script(code, options); } -// Remove all SIGINT listeners and re-attach them after the wrapped function -// has executed, so that caught SIGINT are handled by the listeners again. -function sigintHandlersWrap(fn, thisArg, argsArray) { - const sigintListeners = process.rawListeners('SIGINT'); - - process.removeAllListeners('SIGINT'); - - try { - return ReflectApply(fn, thisArg, argsArray); - } finally { - // Add using the public methods so that the `newListener` handler of - // process can re-attach the listeners. - ArrayPrototypeForEach(sigintListeners, (listener) => { - process.addListener('SIGINT', listener); - }); - } -} - function runInContext(code, contextifiedObject, options) { validateContext(contextifiedObject); if (typeof options === 'string') { diff --git a/test/parallel/test-vm-module-sigint-existing-handler.js b/test/parallel/test-vm-module-sigint-existing-handler.js new file mode 100644 index 000000000000..7d4a126cbdc0 --- /dev/null +++ b/test/parallel/test-vm-module-sigint-existing-handler.js @@ -0,0 +1,85 @@ +'use strict'; +const common = require('../common'); +if (common.isWindows) { + // No way to send CTRL_C_EVENT to processes from JS right now. + common.skip('platform not supported'); +} + +// Tests that vm.Module.prototype.evaluate({ breakOnSigint: true }) does not +// race with the process's own pre-existing SIGINT listeners, the same way +// vm.Script's runInThisContext()/runInContext() already don't (see +// test-vm-sigint-existing-handler.js). + +const assert = require('assert'); +const vm = require('vm'); +const spawn = require('child_process').spawn; + +if (process.argv[2] === 'child') { + let firstHandlerCalled = 0; + process.on('SIGINT', common.mustCall(() => { + firstHandlerCalled++; + // Handler attached _before_ execution. + }, 2)); + + let onceHandlerCalled = 0; + process.once('SIGINT', common.mustCall(() => { + onceHandlerCalled++; + // Handler attached _before_ execution. + })); + + (async () => { + const context = vm.createContext({ process }); + const mod = new vm.SourceTextModule( + 'process.send("ready"); while (true) {}', + { context }); + await mod.link(() => {}); + + await assert.rejects( + mod.evaluate({ breakOnSigint: true }), + { code: 'ERR_SCRIPT_EXECUTION_INTERRUPTED' }, + ); + assert.strictEqual(firstHandlerCalled, 0); + assert.strictEqual(onceHandlerCalled, 0); + + // Keep the process alive for a while so the second SIGINT can be caught. + const timeout = setTimeout(() => {}, 1000); + + let afterHandlerCalled = 0; + process.on('SIGINT', common.mustCall(() => { + // Handler attached _after_ execution. + if (afterHandlerCalled++ === 0) { + // The first time it just bounces back to check that the `once()` + // handler is not called the second time. + assert.strictEqual(firstHandlerCalled, 1); + assert.strictEqual(onceHandlerCalled, 1); + process.send('again'); + return; + } + + assert.strictEqual(onceHandlerCalled, 1); + assert.strictEqual(firstHandlerCalled, 2); + timeout.unref(); + }, 2)); + + process.send('again'); + })().then(common.mustCall()); + + return; +} + +const child = spawn(process.execPath, [ + '--experimental-vm-modules', __filename, 'child', +], { + stdio: [null, 'inherit', 'inherit', 'ipc'], +}); + +child.on('message', common.mustCall(() => { + // First kill() breaks the while(true) loop, second one invokes the real + // signal handlers. + process.kill(child.pid, 'SIGINT'); +}, 3)); + +child.on('close', common.mustCall((code, signal) => { + assert.strictEqual(signal, null); + assert.strictEqual(code, 0); +}));