Skip to content

Commit

Permalink
child_process: spawn ignores options in case args is undefined
Browse files Browse the repository at this point in the history
spawn method ignores 3-d argument 'options' in case
the second one 'args' equals to 'undefined'.

Fixes: nodejs#24912

PR-URL: nodejs#24913
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
  • Loading branch information
eduardbme authored and Trott committed Dec 16, 2018
1 parent 02b66b5 commit 2077007
Show file tree
Hide file tree
Showing 4 changed files with 103 additions and 8 deletions.
5 changes: 3 additions & 2 deletions lib/child_process.js
Original file line number Diff line number Diff line change
Expand Up @@ -403,8 +403,9 @@ function normalizeSpawnArguments(file, args, options) {

if (Array.isArray(args)) {
args = args.slice(0);
} else if (args !== undefined &&
(args === null || typeof args !== 'object')) {
} else if (args == null) {
args = [];
} else if (typeof args !== 'object') {
throw new ERR_INVALID_ARG_TYPE('args', 'object', args);
} else {
options = args;
Expand Down
53 changes: 53 additions & 0 deletions test/parallel/test-child-process-spawn-args.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
'use strict';

// This test confirms that `undefined`, `null`, and `[]`
// can be used as a placeholder for the second argument (`args`) of `spawn()`.
// Previously, there was a bug where using `undefined` for the second argument
// caused the third argument (`options`) to be ignored.
// See https://github.com/nodejs/node/issues/24912.

const assert = require('assert');
const { spawn } = require('child_process');

const common = require('../common');
const tmpdir = require('../common/tmpdir');

const command = common.isWindows ? 'cd' : 'pwd';
const options = { cwd: tmpdir.path };

if (common.isWindows) {
// This test is not the case for Windows based systems
// unless the `shell` options equals to `true`

options.shell = true;
}

const testCases = [
undefined,
null,
[],
];

const expectedResult = tmpdir.path.trim().toLowerCase();

(async () => {
const results = await Promise.all(
testCases.map((testCase) => {
return new Promise((resolve) => {
const subprocess = spawn(command, testCase, options);

let accumulatedData = Buffer.alloc(0);

subprocess.stdout.on('data', common.mustCall((data) => {
accumulatedData = Buffer.concat([accumulatedData, data]);
}));

subprocess.stdout.on('end', () => {
resolve(accumulatedData.toString().trim().toLowerCase());
});
});
})
);

assert.deepStrictEqual([...new Set(results)], [expectedResult]);
})();
8 changes: 2 additions & 6 deletions test/parallel/test-child-process-spawn-typeerror.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ const invalidArgValueError =
common.expectsError({ code: 'ERR_INVALID_ARG_VALUE', type: TypeError }, 14);

const invalidArgTypeError =
common.expectsError({ code: 'ERR_INVALID_ARG_TYPE', type: TypeError }, 13);
common.expectsError({ code: 'ERR_INVALID_ARG_TYPE', type: TypeError }, 11);

assert.throws(function() {
spawn(invalidcmd, 'this is not an array');
Expand All @@ -59,10 +59,6 @@ assert.throws(function() {
spawn(file);
}, invalidArgTypeError);

assert.throws(function() {
spawn(cmd, null);
}, invalidArgTypeError);

assert.throws(function() {
spawn(cmd, true);
}, invalidArgTypeError);
Expand Down Expand Up @@ -103,9 +99,9 @@ spawn(cmd, o);

// Variants of undefined as explicit 'no argument' at a position.
spawn(cmd, u, o);
spawn(cmd, n, o);
spawn(cmd, a, u);

assert.throws(function() { spawn(cmd, n, o); }, invalidArgTypeError);
assert.throws(function() { spawn(cmd, a, n); }, invalidArgTypeError);

assert.throws(function() { spawn(cmd, s); }, invalidArgTypeError);
Expand Down
45 changes: 45 additions & 0 deletions test/parallel/test-child-process-spawnsync-args.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
'use strict';

// This test confirms that `undefined`, `null`, and `[]` can be used
// as a placeholder for the second argument (`args`) of `spawnSync()`.
// Previously, there was a bug where using `undefined` for the second argument
// caused the third argument (`options`) to be ignored.
// See https://github.com/nodejs/node/issues/24912.

const assert = require('assert');
const { spawnSync } = require('child_process');

const common = require('../common');
const tmpdir = require('../common/tmpdir');

const command = common.isWindows ? 'cd' : 'pwd';
const options = { cwd: tmpdir.path };

if (common.isWindows) {
// This test is not the case for Windows based systems
// unless the `shell` options equals to `true`

options.shell = true;
}

const testCases = [
undefined,
null,
[],
];

const expectedResult = tmpdir.path.trim().toLowerCase();

const results = testCases.map((testCase) => {
const { stdout, stderr } = spawnSync(
command,
testCase,
options
);

assert.deepStrictEqual(stderr, Buffer.alloc(0));

return stdout.toString().trim().toLowerCase();
});

assert.deepStrictEqual([...new Set(results)], [expectedResult]);

0 comments on commit 2077007

Please sign in to comment.