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>

Author: agape1225

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,
 };

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);

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') {

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);
+}));