Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

child_process: check execFile and fork args #2667

Closed
wants to merge 2 commits into from
Closed

child_process: check execFile and fork args #2667

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
56bb942
child_process: check execFile and fork args
jasnell Sep 2, 2015
7e5710a
test: fix style nits in test-child-process-spawn-typeerror.js
jasnell Sep 2, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 20 additions & 10 deletions lib/child_process.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ exports.fork = function(modulePath /*, args, options*/) {
if (Array.isArray(arguments[1])) {
args = arguments[1];
options = util._extend({}, arguments[2]);
} else if (arguments[1] && typeof arguments[1] !== 'object') {
throw new TypeError('Incorrect value of args option');
} else {
args = [];
options = util._extend({}, arguments[1]);
Expand Down Expand Up @@ -104,7 +106,7 @@ exports.exec = function(command /*, options, callback*/) {


exports.execFile = function(file /*, args, options, callback*/) {
var args, callback;
var args = [], callback;
var options = {
encoding: 'utf8',
timeout: 0,
Expand All @@ -114,18 +116,26 @@ exports.execFile = function(file /*, args, options, callback*/) {
env: null
};

// Parse the parameters.
// Parse the optional positional parameters.
var pos = 1;
if (pos < arguments.length && Array.isArray(arguments[pos])) {
args = arguments[pos++];
} else if (pos < arguments.length && arguments[pos] == null) {
pos++;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible that both of these fail? If neither is supposed to then mind adding a final else with something like throw new Error('UNREACHABLE'); or the like?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the fall through on this particular one is required for the next check. Overall, the currently behavior in v0.12 (and this port of that code) is really just a half measure. We're not actually doing a complete check. For instance, execFile('ls',{a:1},'test') goes through without a throw even tho a string is passed in for the callback. Internally, execFile just acts as if the callback wasn't provided at all. Likewise, execFile('ls',[], 'test', function() {}) does not throw either. Nor does execFile('ls',[],'test','test'). The only type that is actually checked is the args, so that if you pass in execFile('ls', 'test'), a TypeError will throw.

Given that this is exactly how it behaves in v0.12 currently (for better or worse), I'd be more inclined to land it as is so we can close the convergence loop first, then make additional improvements separately.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation. Sounds like a good plan. Maybe put a todo comment in the code, or we can open an issue for what you've explained and land the fix in a future master. From how you've described it, there is ambiguity with argument parsing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we pull the common pos < arguments.length check outside?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure it's worth it to add the nested conditional. performance-wise there's no measurable gain.

so basically, I don't think it's necessary but I'm also not opposed to the change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but it will improve the readability and reduce redundant checks. It's just a suggestion I am okay without that as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that we'll need to revisit this anyway later. Let's leave it as is
for now and do any additional cleanups in the next round.
On Sep 2, 2015 10:29 PM, "thefourtheye" notifications@github.com wrote:

In lib/child_process.js
#2667 (comment):

@@ -118,18 +120,26 @@ exports.execFile = function(file /, args, options, callback/) {
env: null
};

  • // Parse the parameters.
  • // Parse the optional positional parameters.
  • var pos = 1;
  • if (pos < arguments.length && Array.isArray(arguments[pos])) {
  • args = arguments[pos++];
  • } else if (pos < arguments.length && arguments[pos] == null) {
  • pos++;
  • }

Right, but it will improve the readability and reduce redundant checks.
It's just a suggestion I am okay without that as well.

—
Reply to this email directly or view it on GitHub
https://github.com/nodejs/node/pull/2667/files#r38614155.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. Since we introduced new exception, this would be semver-major?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically. But this is already in v0.12 and is part of the convergence work. It will land in master and will be cherry picked to v4.x


if (typeof arguments[arguments.length - 1] === 'function') {
callback = arguments[arguments.length - 1];
if (pos < arguments.length && typeof arguments[pos] === 'object') {
options = util._extend(options, arguments[pos++]);
} else if (pos < arguments.length && arguments[pos] == null) {
pos++;
}

if (Array.isArray(arguments[1])) {
args = arguments[1];
options = util._extend(options, arguments[2]);
} else {
args = [];
options = util._extend(options, arguments[1]);
if (pos < arguments.length && typeof arguments[pos] === 'function') {
callback = arguments[pos++];
}

if (pos === 1 && arguments.length > 1) {
throw new TypeError('Incorrect value of args option');
}

var child = spawn(file, args, {
Expand Down
103 changes: 94 additions & 9 deletions test/parallel/test-child-process-spawn-typeerror.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,19 @@
'use strict';
var assert = require('assert');
var child_process = require('child_process');
var spawn = child_process.spawn;
var cmd = require('../common').isWindows ? 'rundll32' : 'ls';
var invalidArgsMsg = /Incorrect value of args option/;
var invalidOptionsMsg = /options argument must be an object/;

// verify that args argument must be an array
const assert = require('assert');
const child_process = require('child_process');
const spawn = child_process.spawn;
const fork = child_process.fork;
const execFile = child_process.execFile;
const common = require('../common');
const cmd = common.isWindows ? 'rundll32' : 'ls';
const invalidcmd = 'hopefully_you_dont_have_this_on_your_machine';
const invalidArgsMsg = /Incorrect value of args option/;
const invalidOptionsMsg = /options argument must be an object/;
const empty = common.fixturesDir + '/empty.js';

assert.throws(function() {
spawn(cmd, 'this is not an array');
var child = spawn(invalidcmd, 'this is not an array');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const child

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not seeing this edit as being critical to land this. can do this in the next pass.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. No problem. We can actually remove the variable and directly attach the even handler.

child.on('error', assert.fail);
}, TypeError);

// verify that valid argument combinations do not throw
Expand Down Expand Up @@ -49,3 +54,83 @@ assert.throws(function() {
spawn(cmd, [], 1);
}, invalidOptionsMsg);

// Argument types for combinatorics
const a = [];
const o = {};
const c = function callback() {};
const s = 'string';
const u = undefined;
const n = null;

// function spawn(file=f [,args=a] [, options=o]) has valid combinations:
// (f)
// (f, a)
// (f, a, o)
// (f, o)
assert.doesNotThrow(function() { spawn(cmd); });
assert.doesNotThrow(function() { spawn(cmd, a); });
assert.doesNotThrow(function() { spawn(cmd, a, o); });
assert.doesNotThrow(function() { spawn(cmd, o); });

// Variants of undefined as explicit 'no argument' at a position
assert.doesNotThrow(function() { spawn(cmd, u, o); });
assert.doesNotThrow(function() { spawn(cmd, a, u); });

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

assert.throws(function() { spawn(cmd, s); }, TypeError);
assert.throws(function() { spawn(cmd, a, s); }, TypeError);


// verify that execFile has same argument parsing behaviour as spawn
//
// function execFile(file=f [,args=a] [, options=o] [, callback=c]) has valid
// combinations:
// (f)
// (f, a)
// (f, a, o)
// (f, a, o, c)
// (f, a, c)
// (f, o)
// (f, o, c)
// (f, c)
assert.doesNotThrow(function() { execFile(cmd); });
assert.doesNotThrow(function() { execFile(cmd, a); });
assert.doesNotThrow(function() { execFile(cmd, a, o); });
assert.doesNotThrow(function() { execFile(cmd, a, o, c); });
assert.doesNotThrow(function() { execFile(cmd, a, c); });
assert.doesNotThrow(function() { execFile(cmd, o); });
assert.doesNotThrow(function() { execFile(cmd, o, c); });
assert.doesNotThrow(function() { execFile(cmd, c); });

// Variants of undefined as explicit 'no argument' at a position
assert.doesNotThrow(function() { execFile(cmd, u, o, c); });
assert.doesNotThrow(function() { execFile(cmd, a, u, c); });
assert.doesNotThrow(function() { execFile(cmd, a, o, u); });
assert.doesNotThrow(function() { execFile(cmd, n, o, c); });
assert.doesNotThrow(function() { execFile(cmd, a, n, c); });
assert.doesNotThrow(function() { execFile(cmd, a, o, n); });

// string is invalid in arg position (this may seem strange, but is
// consistent across node API, cf. `net.createServer('not options', 'not
// callback')`
assert.throws(function() { execFile(cmd, s, o, c); }, TypeError);
assert.doesNotThrow(function() { execFile(cmd, a, s, c); });
assert.doesNotThrow(function() { execFile(cmd, a, o, s); });


// verify that fork has same argument parsing behaviour as spawn
//
// function fork(file=f [,args=a] [, options=o]) has valid combinations:
// (f)
// (f, a)
// (f, a, o)
// (f, o)
assert.doesNotThrow(function() { fork(empty); });
assert.doesNotThrow(function() { fork(empty, a); });
assert.doesNotThrow(function() { fork(empty, a, o); });
assert.doesNotThrow(function() { fork(empty, o); });

assert.throws(function() { fork(empty, s); }, TypeError);
assert.doesNotThrow(function() { fork(empty, a, s); }, TypeError);