From 9f56bc8b4c0bec37ec095cf1bbf67ee2fe55edff Mon Sep 17 00:00:00 2001
From: Nitzan Uziely <nitzan@testim.io>
Date: Thu, 11 Feb 2021 20:59:00 +0200
Subject: [PATCH] child_process: fix spawn and fork abort behavior

Fix AbortSignal in Spawn which doesn't actually abort
the process, and fork can emit an AbortError even if
the process was already exited. Add documentation
For killSignal.

Fixes: https://github.com/nodejs/node/issues/37273

PR-URL: https://github.com/nodejs/node/pull/37325
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
---
 doc/api/child_process.md                      | 10 +++
 lib/child_process.js                          | 33 ++++---
 ...rocess-exec-abortcontroller-promisified.js | 27 +++---
 .../test-child-process-fork-abort-signal.js   | 45 +++++++++-
 .../test-child-process-spawn-controller.js    | 90 ++++++++++++++++---
 5 files changed, 165 insertions(+), 40 deletions(-)

diff --git a/doc/api/child_process.md b/doc/api/child_process.md
index ee8f0b4f0bc939..b7fce216282012 100644
--- a/doc/api/child_process.md
+++ b/doc/api/child_process.md
@@ -351,6 +351,9 @@ controller.abort();
 <!-- YAML
 added: v0.5.0
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/37325
+    description: killSignal for AbortSignal was added.
   - version: v15.6.0
     pr-url: https://github.com/nodejs/node/pull/36603
     description: AbortSignal support was added.
@@ -383,6 +386,8 @@ changes:
     messages between processes. Possible values are `'json'` and `'advanced'`.
     See [Advanced serialization][] for more details. **Default:** `'json'`.
   * `signal` {AbortSignal} Allows closing the subprocess using an AbortSignal.
+  * `killSignal` {string} The signal value to be used when the spawned
+    process will be killed by the abort signal. **Default:** `'SIGTERM'`.
   * `silent` {boolean} If `true`, stdin, stdout, and stderr of the child will be
     piped to the parent, otherwise they will be inherited from the parent, see
     the `'pipe'` and `'inherit'` options for [`child_process.spawn()`][]'s
@@ -431,6 +436,9 @@ The `signal` option works exactly the same way it does in
 <!-- YAML
 added: v0.1.90
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/37325
+    description: killSignal for AbortSignal was added.
   - version: v15.5.0
     pr-url: https://github.com/nodejs/node/pull/36432
     description: AbortSignal support was added.
@@ -477,6 +485,8 @@ changes:
   * `windowsHide` {boolean} Hide the subprocess console window that would
     normally be created on Windows systems. **Default:** `false`.
   * `signal` {AbortSignal} allows aborting the execFile using an AbortSignal.
+  * `killSignal` {string} The signal value to be used when the spawned
+    process will be killed by the abort signal. **Default:** `'SIGTERM'`.
 
 * Returns: {ChildProcess}
 
diff --git a/lib/child_process.js b/lib/child_process.js
index e8fefce87d7671..57b5eaa71b1bb5 100644
--- a/lib/child_process.js
+++ b/lib/child_process.js
@@ -585,6 +585,18 @@ function normalizeSpawnArguments(file, args, options) {
   };
 }
 
+function abortChildProcess(child, killSignal) {
+  if (!child)
+    return;
+  try {
+    if (child.kill(killSignal)) {
+      child.emit('error', new AbortError());
+    }
+  } catch (err) {
+    child.emit('error', err);
+  }
+}
+
 
 function spawn(file, args, options) {
   const child = new ChildProcess();
@@ -594,21 +606,19 @@ function spawn(file, args, options) {
     const signal = options.signal;
     // Validate signal, if present
     validateAbortSignal(signal, 'options.signal');
-
+    const killSignal = sanitizeKillSignal(options.killSignal);
     // Do nothing and throw if already aborted
     if (signal.aborted) {
       onAbortListener();
     } else {
       signal.addEventListener('abort', onAbortListener, { once: true });
-      child.once('close',
+      child.once('exit',
                  () => signal.removeEventListener('abort', onAbortListener));
     }
 
     function onAbortListener() {
       process.nextTick(() => {
-        child?.kill?.(options.killSignal);
-
-        child.emit('error', new AbortError());
+        abortChildProcess(child, killSignal);
       });
     }
   }
@@ -764,19 +774,18 @@ function spawnWithSignal(file, args, options) {
   }
   const child = spawn(file, args, opts);
 
-  if (options && options.signal) {
+  if (options?.signal) {
+    const killSignal = sanitizeKillSignal(options.killSignal);
+
     function kill() {
-      if (child._handle) {
-        child._handle.kill(options.killSignal || 'SIGTERM');
-        child.emit('error', new AbortError());
-      }
+      abortChildProcess(child, killSignal);
     }
     if (options.signal.aborted) {
       process.nextTick(kill);
     } else {
-      options.signal.addEventListener('abort', kill);
+      options.signal.addEventListener('abort', kill, { once: true });
       const remove = () => options.signal.removeEventListener('abort', kill);
-      child.once('close', remove);
+      child.once('exit', remove);
     }
   }
   return child;
diff --git a/test/parallel/test-child-process-exec-abortcontroller-promisified.js b/test/parallel/test-child-process-exec-abortcontroller-promisified.js
index fa00f2d2177635..b818a00a2921d6 100644
--- a/test/parallel/test-child-process-exec-abortcontroller-promisified.js
+++ b/test/parallel/test-child-process-exec-abortcontroller-promisified.js
@@ -4,48 +4,47 @@ const assert = require('assert');
 const exec = require('child_process').exec;
 const { promisify } = require('util');
 
-let pwdcommand, dir;
 const execPromisifed = promisify(exec);
 const invalidArgTypeError = {
   code: 'ERR_INVALID_ARG_TYPE',
   name: 'TypeError'
 };
 
-
+let waitCommand = '';
 if (common.isWindows) {
-  pwdcommand = 'echo %cd%';
-  dir = 'c:\\windows';
+  waitCommand = 'TIMEOUT 120';
 } else {
-  pwdcommand = 'pwd';
-  dir = '/dev';
+  waitCommand = 'sleep 2m';
 }
 
-
 {
   const ac = new AbortController();
   const signal = ac.signal;
-  const promise = execPromisifed(pwdcommand, { cwd: dir, signal });
-  assert.rejects(promise, /AbortError/).then(common.mustCall());
+  const promise = execPromisifed(waitCommand, { signal });
+  assert.rejects(promise, /AbortError/, 'post aborted sync signal failed')
+        .then(common.mustCall());
   ac.abort();
 }
 
 {
   assert.throws(() => {
-    execPromisifed(pwdcommand, { cwd: dir, signal: {} });
+    execPromisifed(waitCommand, { signal: {} });
   }, invalidArgTypeError);
 }
 
 {
   function signal() {}
   assert.throws(() => {
-    execPromisifed(pwdcommand, { cwd: dir, signal });
+    execPromisifed(waitCommand, { signal });
   }, invalidArgTypeError);
 }
 
 {
   const ac = new AbortController();
-  const signal = (ac.abort(), ac.signal);
-  const promise = execPromisifed(pwdcommand, { cwd: dir, signal });
+  const { signal } = ac;
+  ac.abort();
+  const promise = execPromisifed(waitCommand, { signal });
 
-  assert.rejects(promise, /AbortError/).then(common.mustCall());
+  assert.rejects(promise, /AbortError/, 'pre aborted signal failed')
+        .then(common.mustCall());
 }
diff --git a/test/parallel/test-child-process-fork-abort-signal.js b/test/parallel/test-child-process-fork-abort-signal.js
index 63367479234d7f..93a968366270cc 100644
--- a/test/parallel/test-child-process-fork-abort-signal.js
+++ b/test/parallel/test-child-process-fork-abort-signal.js
@@ -1,6 +1,6 @@
 'use strict';
 
-const { mustCall } = require('../common');
+const { mustCall, mustNotCall } = require('../common');
 const { strictEqual } = require('assert');
 const fixtures = require('../common/fixtures');
 const { fork } = require('child_process');
@@ -12,7 +12,10 @@ const { fork } = require('child_process');
   const cp = fork(fixtures.path('child-process-stay-alive-forever.js'), {
     signal
   });
-  cp.on('exit', mustCall());
+  cp.on('exit', mustCall((code, killSignal) => {
+    strictEqual(code, null);
+    strictEqual(killSignal, 'SIGTERM');
+  }));
   cp.on('error', mustCall((err) => {
     strictEqual(err.name, 'AbortError');
   }));
@@ -26,8 +29,44 @@ const { fork } = require('child_process');
   const cp = fork(fixtures.path('child-process-stay-alive-forever.js'), {
     signal
   });
-  cp.on('exit', mustCall());
+  cp.on('exit', mustCall((code, killSignal) => {
+    strictEqual(code, null);
+    strictEqual(killSignal, 'SIGTERM');
+  }));
+  cp.on('error', mustCall((err) => {
+    strictEqual(err.name, 'AbortError');
+  }));
+}
+
+{
+  // Test passing a different kill signal
+  const ac = new AbortController();
+  const { signal } = ac;
+  ac.abort();
+  const cp = fork(fixtures.path('child-process-stay-alive-forever.js'), {
+    signal,
+    killSignal: 'SIGKILL',
+  });
+  cp.on('exit', mustCall((code, killSignal) => {
+    strictEqual(code, null);
+    strictEqual(killSignal, 'SIGKILL');
+  }));
   cp.on('error', mustCall((err) => {
     strictEqual(err.name, 'AbortError');
   }));
 }
+
+{
+  // Test aborting a cp before close but after exit
+  const ac = new AbortController();
+  const { signal } = ac;
+  const cp = fork(fixtures.path('child-process-stay-alive-forever.js'), {
+    signal
+  });
+  cp.on('exit', mustCall(() => {
+    ac.abort();
+  }));
+  cp.on('error', mustNotCall());
+
+  setTimeout(() => cp.kill(), 1);
+}
diff --git a/test/parallel/test-child-process-spawn-controller.js b/test/parallel/test-child-process-spawn-controller.js
index c25307907c1b26..36c4112cbe00da 100644
--- a/test/parallel/test-child-process-spawn-controller.js
+++ b/test/parallel/test-child-process-spawn-controller.js
@@ -2,20 +2,25 @@
 
 const common = require('../common');
 const assert = require('assert');
-const cp = require('child_process');
+const { spawn } = require('child_process');
+const fixtures = require('../common/fixtures');
 
+const aliveScript = fixtures.path('child-process-stay-alive-forever.js');
 {
   // Verify that passing an AbortSignal works
   const controller = new AbortController();
   const { signal } = controller;
 
-  const echo = cp.spawn('echo', ['fun'], {
-    encoding: 'utf8',
-    shell: true,
-    signal
+  const cp = spawn(process.execPath, [aliveScript], {
+    signal,
   });
 
-  echo.on('error', common.mustCall((e) => {
+  cp.on('exit', common.mustCall((code, killSignal) => {
+    assert.strictEqual(code, null);
+    assert.strictEqual(killSignal, 'SIGTERM');
+  }));
+
+  cp.on('error', common.mustCall((e) => {
     assert.strictEqual(e.name, 'AbortError');
   }));
 
@@ -29,13 +34,76 @@ const cp = require('child_process');
 
   controller.abort();
 
-  const echo = cp.spawn('echo', ['fun'], {
-    encoding: 'utf8',
-    shell: true,
-    signal
+  const cp = spawn(process.execPath, [aliveScript], {
+    signal,
+  });
+  cp.on('exit', common.mustCall((code, killSignal) => {
+    assert.strictEqual(code, null);
+    assert.strictEqual(killSignal, 'SIGTERM');
+  }));
+
+  cp.on('error', common.mustCall((e) => {
+    assert.strictEqual(e.name, 'AbortError');
+  }));
+}
+
+{
+  // Verify that waiting a bit and closing works
+  const controller = new AbortController();
+  const { signal } = controller;
+
+  const cp = spawn(process.execPath, [aliveScript], {
+    signal,
+  });
+
+  cp.on('exit', common.mustCall((code, killSignal) => {
+    assert.strictEqual(code, null);
+    assert.strictEqual(killSignal, 'SIGTERM');
+  }));
+
+  cp.on('error', common.mustCall((e) => {
+    assert.strictEqual(e.name, 'AbortError');
+  }));
+
+  setTimeout(() => controller.abort(), 1);
+}
+
+{
+  // Test passing a different killSignal
+  const controller = new AbortController();
+  const { signal } = controller;
+
+  const cp = spawn(process.execPath, [aliveScript], {
+    signal,
+    killSignal: 'SIGKILL',
   });
 
-  echo.on('error', common.mustCall((e) => {
+  cp.on('exit', common.mustCall((code, killSignal) => {
+    assert.strictEqual(code, null);
+    assert.strictEqual(killSignal, 'SIGKILL');
+  }));
+
+  cp.on('error', common.mustCall((e) => {
     assert.strictEqual(e.name, 'AbortError');
   }));
+
+  setTimeout(() => controller.abort(), 1);
+}
+
+{
+  // Test aborting a cp before close but after exit
+  const controller = new AbortController();
+  const { signal } = controller;
+
+  const cp = spawn(process.execPath, [aliveScript], {
+    signal,
+  });
+
+  cp.on('exit', common.mustCall(() => {
+    controller.abort();
+  }));
+
+  cp.on('error', common.mustNotCall());
+
+  setTimeout(() => cp.kill(), 1);
 }