child_process: do not extend result for *Sync()

PR-URL: https://github.com/nodejs/node/pull/13601
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

lib/child_process.js

@@ -28,13 +28,11 @@ const { createPromise,
 const debug = util.debuglog('child_process');
 
 const uv = process.binding('uv');
-const spawn_sync = process.binding('spawn_sync');
 const Buffer = require('buffer').Buffer;
 const Pipe = process.binding('pipe_wrap').Pipe;
 const { isUint8Array } = process.binding('util');
 const child_process = require('internal/child_process');
 
-const errnoException = util._errnoException;
 const _validateStdio = child_process._validateStdio;
 const setupChannel = child_process.setupChannel;
 const ChildProcess = exports.ChildProcess = child_process.ChildProcess;
@@ -508,8 +506,6 @@ function spawnSync(/*file, args, options*/) {
 
   var options = opts.options;
 
-  var i;
-
   debug('spawnSync', opts.args, options);
 
   // Validate the timeout, if present.
@@ -533,7 +529,7 @@ function spawnSync(/*file, args, options*/) {
   }
 
   // We may want to pass data in on any given fd, ensure it is a valid buffer
-  for (i = 0; i < options.stdio.length; i++) {
+  for (var i = 0; i < options.stdio.length; i++) {
     var input = options.stdio[i] && options.stdio[i].input;
     if (input != null) {
       var pipe = options.stdio[i] = util._extend({}, options.stdio[i]);
@@ -549,50 +545,27 @@ function spawnSync(/*file, args, options*/) {
     }
   }
 
-  var result = spawn_sync.spawn(options);
-
-  if (result.output && options.encoding && options.encoding !== 'buffer') {
-    for (i = 0; i < result.output.length; i++) {
-      if (!result.output[i])
-        continue;
-      result.output[i] = result.output[i].toString(options.encoding);
-    }
-  }
-
-  result.stdout = result.output && result.output[1];
-  result.stderr = result.output && result.output[2];
-
-  if (result.error) {
-    result.error = errnoException(result.error, 'spawnSync ' + opts.file);
-    result.error.path = opts.file;
-    result.error.spawnargs = opts.args.slice(1);
-  }
-
-  util._extend(result, opts);
-
-  return result;
+  return child_process.spawnSync(opts);
 }
 exports.spawnSync = spawnSync;
 
 
-function checkExecSyncError(ret) {
-  if (ret.error || ret.status !== 0) {
-    var err = ret.error;
-    ret.error = null;
-
-    if (!err) {
-      var msg = 'Command failed: ';
-      msg += ret.cmd || ret.args.join(' ');
-      if (ret.stderr && ret.stderr.length > 0)
-        msg += '\n' + ret.stderr.toString();
-      err = new Error(msg);
-    }
-
-    util._extend(err, ret);
-    return err;
+function checkExecSyncError(ret, args, cmd) {
+  var err;
+  if (ret.error) {
+    err = ret.error;
+  } else if (ret.status !== 0) {
+    var msg = 'Command failed: ';
+    msg += cmd || args.join(' ');
+    if (ret.stderr && ret.stderr.length > 0)
+      msg += '\n' + ret.stderr.toString();
+    err = new Error(msg);
   }
-
-  return false;
+  if (err) {
+    err.status = ret.status < 0 ? uv.errname(ret.status) : ret.status;
+    err.signal = ret.signal;
+  }
+  return err;
 }
 
 
@@ -605,7 +578,7 @@ function execFileSync(/*command, args, options*/) {
   if (inheritStderr && ret.stderr)
     process.stderr.write(ret.stderr);
 
-  var err = checkExecSyncError(ret);
+  var err = checkExecSyncError(ret, opts.args, undefined);
 
   if (err)
     throw err;
@@ -620,12 +593,11 @@ function execSync(command /*, options*/) {
   var inheritStderr = !opts.options.stdio;
 
   var ret = spawnSync(opts.file, opts.options);
-  ret.cmd = command;
 
   if (inheritStderr && ret.stderr)
     process.stderr.write(ret.stderr);
 
-  var err = checkExecSyncError(ret);
+  var err = checkExecSyncError(ret, opts.args, command);
 
   if (err)
     throw err;

lib/internal/child_process.js

@@ -18,6 +18,7 @@ const UDP = process.binding('udp_wrap').UDP;
 const SocketList = require('internal/socket_list');
 const { isUint8Array } = process.binding('util');
 const { convertToValidSignal } = require('internal/util');
+const spawn_sync = process.binding('spawn_sync');
 
 const errnoException = util._errnoException;
 const SocketListSend = SocketList.SocketListSend;
@@ -898,9 +899,34 @@ function maybeClose(subprocess) {
   }
 }
 
+function spawnSync(opts) {
+  var options = opts.options;
+  var result = spawn_sync.spawn(options);
+
+  if (result.output && options.encoding && options.encoding !== 'buffer') {
+    for (var i = 0; i < result.output.length; i++) {
+      if (!result.output[i])
+        continue;
+      result.output[i] = result.output[i].toString(options.encoding);
+    }
+  }
+
+  result.stdout = result.output && result.output[1];
+  result.stderr = result.output && result.output[2];
+
+  if (result.error) {
+    result.error = errnoException(result.error, 'spawnSync ' + opts.file);
+    result.error.path = opts.file;
+    result.error.spawnargs = opts.args.slice(1);
+  }
+
+  return result;
+}
+
 module.exports = {
   ChildProcess,
   setupChannel,
   _validateStdio,
-  getSocketList
+  getSocketList,
+  spawnSync
 };

test/parallel/test-child-process-custom-fds.js

@@ -1,6 +1,9 @@
+// Flags: --expose_internals
 'use strict';
 const common = require('../common');
 const assert = require('assert');
+const internalCp = require('internal/child_process');
+const oldSpawnSync = internalCp.spawnSync;
 
 // Verify that customFds is used if stdio is not provided.
 {
@@ -9,25 +12,29 @@ const assert = require('assert');
   common.expectWarning('DeprecationWarning', msg);
 
   const customFds = [-1, process.stdout.fd, process.stderr.fd];
-  const child = common.spawnSyncPwd({ customFds });
-
-  assert.deepStrictEqual(child.options.customFds, customFds);
-  assert.deepStrictEqual(child.options.stdio, [
-    { type: 'pipe', readable: true, writable: false },
-    { type: 'fd', fd: process.stdout.fd },
-    { type: 'fd', fd: process.stderr.fd }
-  ]);
+  internalCp.spawnSync = common.mustCall(function(opts) {
+    assert.deepStrictEqual(opts.options.customFds, customFds);
+    assert.deepStrictEqual(opts.options.stdio, [
+      { type: 'pipe', readable: true, writable: false },
+      { type: 'fd', fd: process.stdout.fd },
+      { type: 'fd', fd: process.stderr.fd }
+    ]);
+  });
+  common.spawnSyncPwd({ customFds });
+  internalCp.spawnSync = oldSpawnSync;
 }
 
 // Verify that customFds is ignored when stdio is present.
 {
   const customFds = [0, 1, 2];
-  const child = common.spawnSyncPwd({ customFds, stdio: 'pipe' });
-
-  assert.deepStrictEqual(child.options.customFds, customFds);
-  assert.deepStrictEqual(child.options.stdio, [
-    { type: 'pipe', readable: true, writable: false },
-    { type: 'pipe', readable: false, writable: true },
-    { type: 'pipe', readable: false, writable: true }
-  ]);
+  internalCp.spawnSync = common.mustCall(function(opts) {
+    assert.deepStrictEqual(opts.options.customFds, customFds);
+    assert.deepStrictEqual(opts.options.stdio, [
+      { type: 'pipe', readable: true, writable: false },
+      { type: 'pipe', readable: false, writable: true },
+      { type: 'pipe', readable: false, writable: true }
+    ]);
+  });
+  common.spawnSyncPwd({ customFds, stdio: 'pipe' });
+  internalCp.spawnSync = oldSpawnSync;
 }

test/parallel/test-child-process-spawnsync-kill-signal.js

@@ -1,3 +1,4 @@
+// Flags: --expose_internals
 'use strict';
 const common = require('../common');
 const assert = require('assert');
@@ -6,13 +7,22 @@ const cp = require('child_process');
 if (process.argv[2] === 'child') {
   setInterval(common.noop, 1000);
 } else {
+  const internalCp = require('internal/child_process');
+  const oldSpawnSync = internalCp.spawnSync;
   const { SIGKILL } = process.binding('constants').os.signals;
 
-  function spawn(killSignal) {
+  function spawn(killSignal, beforeSpawn) {
+    if (beforeSpawn) {
+      internalCp.spawnSync = common.mustCall(function(opts) {
+        beforeSpawn(opts);
+        return oldSpawnSync(opts);
+      });
+    }
     const child = cp.spawnSync(process.execPath,
                                [__filename, 'child'],
                                {killSignal, timeout: 100});
-
+    if (beforeSpawn)
+      internalCp.spawnSync = oldSpawnSync;
     assert.strictEqual(child.status, null);
     assert.strictEqual(child.error.code, 'ETIMEDOUT');
     return child;
@@ -25,26 +35,30 @@ if (process.argv[2] === 'child') {
 
   // Verify that the default kill signal is SIGTERM.
   {
-    const child = spawn();
+    const child = spawn(undefined, (opts) => {
+      assert.strictEqual(opts.options.killSignal, undefined);
+    });
 
     assert.strictEqual(child.signal, 'SIGTERM');
-    assert.strictEqual(child.options.killSignal, undefined);
   }
 
   // Verify that a string signal name is handled properly.
   {
-    const child = spawn('SIGKILL');
+    const child = spawn('SIGKILL', (opts) => {
+      assert.strictEqual(opts.options.killSignal, SIGKILL);
+    });
 
     assert.strictEqual(child.signal, 'SIGKILL');
-    assert.strictEqual(child.options.killSignal, SIGKILL);
   }
 
   // Verify that a numeric signal is handled properly.
   {
-    const child = spawn(SIGKILL);
-
     assert.strictEqual(typeof SIGKILL, 'number');
+
+    const child = spawn(SIGKILL, (opts) => {
+      assert.strictEqual(opts.options.killSignal, SIGKILL);
+    });
+
     assert.strictEqual(child.signal, 'SIGKILL');
-    assert.strictEqual(child.options.killSignal, SIGKILL);
   }
 }

test/parallel/test-child-process-spawnsync-shell.js

@@ -1,7 +1,10 @@
+// Flags: --expose_internals
 'use strict';
 const common = require('../common');
 const assert = require('assert');
 const cp = require('child_process');
+const internalCp = require('internal/child_process');
+const oldSpawnSync = internalCp.spawnSync;
 
 // Verify that a shell is, in fact, executed
 const doesNotExist = cp.spawnSync('does-not-exist', {shell: true});
@@ -16,10 +19,14 @@ else
   assert.strictEqual(doesNotExist.status, 127);  // Exit code of /bin/sh
 
 // Verify that passing arguments works
+internalCp.spawnSync = common.mustCall(function(opts) {
+  assert.strictEqual(opts.args[opts.args.length - 1].replace(/"/g, ''),
+                     'echo foo');
+  return oldSpawnSync(opts);
+});
 const echo = cp.spawnSync('echo', ['foo'], {shell: true});
+internalCp.spawnSync = oldSpawnSync;
 
-assert.strictEqual(echo.args[echo.args.length - 1].replace(/"/g, ''),
-                   'echo foo');
 assert.strictEqual(echo.stdout.toString().trim(), 'foo');
 
 // Verify that shell features can be used
@@ -52,16 +59,18 @@ assert.strictEqual(env.stdout.toString().trim(), 'buzz');
     const shellFlags = platform === 'win32' ? ['/d', '/s', '/c'] : ['-c'];
     const outputCmd = platform === 'win32' ? `"${cmd}"` : cmd;
     const windowsVerbatim = platform === 'win32' ? true : undefined;
-    const result = cp.spawnSync(cmd, { shell });
-
-    assert.strictEqual(result.file, shellOutput);
-    assert.deepStrictEqual(result.args,
-                           [shellOutput, ...shellFlags, outputCmd]);
-    assert.strictEqual(result.options.shell, shell);
-    assert.strictEqual(result.options.file, result.file);
-    assert.deepStrictEqual(result.options.args, result.args);
-    assert.strictEqual(result.options.windowsVerbatimArguments,
-                       windowsVerbatim);
+    internalCp.spawnSync = common.mustCall(function(opts) {
+      assert.strictEqual(opts.file, shellOutput);
+      assert.deepStrictEqual(opts.args,
+                             [shellOutput, ...shellFlags, outputCmd]);
+      assert.strictEqual(opts.options.shell, shell);
+      assert.strictEqual(opts.options.file, opts.file);
+      assert.deepStrictEqual(opts.options.args, opts.args);
+      assert.strictEqual(opts.options.windowsVerbatimArguments,
+                         windowsVerbatim);
+    });
+    cp.spawnSync(cmd, { shell });
+    internalCp.spawnSync = oldSpawnSync;
   }
 
   // Test Unix platforms with the default shell.