Skip to content

Commit d8e5d05

Browse files
ChuckLangfordChuckLangford
committed
child_process: validate execFile & fork arguments
The argument parsing for execFile and fork are inconsistent. execFile throws on one invalid argument but not others. fork has similar logic but the implementation is very different. This update implements consistency for both functions. Fixes: #2681
1 parent eeb6fdc commit d8e5d05

File tree

2 files changed

+116
-33
lines changed

2 files changed

+116
-33
lines changed

‎lib/child_process.js

+84-30
Original file line numberDiff line numberDiff line change
@@ -20,15 +20,10 @@ exports.fork = function(modulePath /*, args, options*/) {
2020

2121
// Get options and args arguments.
2222
var options, args, execArgv;
23-
if (Array.isArray(arguments[1])) {
24-
args = arguments[1];
25-
options = util._extend({}, arguments[2]);
26-
} else if (arguments[1] && typeof arguments[1] !== 'object') {
27-
throw new TypeError('Incorrect value of args option');
28-
} else {
29-
args = [];
30-
options = util._extend({}, arguments[1]);
31-
}
23+
24+
var opts = normalizeForkArgs(arguments, options);
25+
args = opts.args;
26+
options = opts.options;
3227

3328
// Prepare arguments for fork:
3429
execArgv = options.execArgv || process.execArgv;
@@ -114,6 +109,82 @@ exports.exec = function(command /*, options, callback*/) {
114109
opts.callback);
115110
};
116111

112+
function normalizeArgsOptions(allArgs, defaultOption) {
113+
var pos = 1, arrayPos = -1, objectPos = -1, args, options;
114+
if (pos < allArgs.length && Array.isArray(allArgs[pos])) {
115+
arrayPos = pos++;
116+
args = allArgs[arrayPos];
117+
} else if (pos < allArgs.length && allArgs[pos] == null) {
118+
arrayPos = pos++;
119+
}
120+
121+
if (pos < allArgs.length && typeof allArgs[pos] === 'object') {
122+
objectPos = pos++;
123+
defaultOption = util._extend(defaultOption, allArgs[objectPos]);
124+
} else if (pos < allArgs.length && allArgs[pos] == null) {
125+
objectPos = pos++;
126+
}
127+
128+
return {
129+
pos: pos,
130+
args: args || [],
131+
options: defaultOption,
132+
arrayPos: arrayPos,
133+
objectPos: objectPos
134+
};
135+
}
136+
137+
function normalizeForkArgs(allArgs, defaultOption) {
138+
if (!defaultOption) {
139+
defaultOption = {};
140+
}
141+
142+
var opts = normalizeArgsOptions(allArgs, defaultOption);
143+
144+
if (opts.pos === 2 &&
145+
allArgs.length > 2 &&
146+
opts.arrayPos > -1) {
147+
throw new TypeError('Incorrect value of options option');
148+
}
149+
150+
if (opts.pos === 1 && allArgs.length > 1) {
151+
throw new TypeError('Incorrect value of args option');
152+
}
153+
154+
return opts;
155+
}
156+
157+
function normalizeExecFileArgs(allArgs, defaultOption) {
158+
var opts = normalizeArgsOptions(allArgs, defaultOption);
159+
160+
if (opts.pos < allArgs.length && typeof allArgs[opts.pos] === 'function') {
161+
opts.callback = allArgs[opts.pos++];
162+
}
163+
164+
if (opts.pos === 3 &&
165+
allArgs.length > 3 &&
166+
typeof opts.callback !== 'function' &&
167+
allArgs[3] !== undefined &&
168+
allArgs[3] !== null) {
169+
throw new TypeError('Incorrect value of callback option');
170+
}
171+
172+
if (opts.pos === 2 && allArgs.length > 2) {
173+
if (opts.arrayPos > -1) {
174+
throw new TypeError('Incorrect value of options option');
175+
} else if (opts.objectPos > -1 &&
176+
allArgs[opts.pos] !== null &&
177+
allArgs[opts.pos] !== undefined) {
178+
throw new TypeError('Incorrect value of callback option');
179+
}
180+
}
181+
182+
if (opts.pos === 1 && allArgs.length > 1) {
183+
throw new TypeError('Incorrect value of args option');
184+
}
185+
186+
return opts;
187+
}
117188

118189
exports.execFile = function(file /*, args, options, callback*/) {
119190
var args = [], callback;
@@ -126,27 +197,10 @@ exports.execFile = function(file /*, args, options, callback*/) {
126197
env: null
127198
};
128199

129-
// Parse the optional positional parameters.
130-
var pos = 1;
131-
if (pos < arguments.length && Array.isArray(arguments[pos])) {
132-
args = arguments[pos++];
133-
} else if (pos < arguments.length && arguments[pos] == null) {
134-
pos++;
135-
}
136-
137-
if (pos < arguments.length && typeof arguments[pos] === 'object') {
138-
options = util._extend(options, arguments[pos++]);
139-
} else if (pos < arguments.length && arguments[pos] == null) {
140-
pos++;
141-
}
142-
143-
if (pos < arguments.length && typeof arguments[pos] === 'function') {
144-
callback = arguments[pos++];
145-
}
146-
147-
if (pos === 1 && arguments.length > 1) {
148-
throw new TypeError('Incorrect value of args option');
149-
}
200+
var opts = normalizeExecFileArgs(arguments, options);
201+
args = opts.args;
202+
options = opts.options;
203+
callback = opts.callback;
150204

151205
var child = spawn(file, args, {
152206
cwd: options.cwd,

‎test/parallel/test-child-process-spawn-typeerror.js

+32-3
Original file line numberDiff line numberDiff line change
@@ -111,13 +111,36 @@ assert.doesNotThrow(function() { execFile(cmd, a, o, u); });
111111
assert.doesNotThrow(function() { execFile(cmd, n, o, c); });
112112
assert.doesNotThrow(function() { execFile(cmd, a, n, c); });
113113
assert.doesNotThrow(function() { execFile(cmd, a, o, n); });
114+
assert.doesNotThrow(function() { execFile(cmd, u, u, u); });
115+
assert.doesNotThrow(function() { execFile(cmd, u, u, c); });
116+
assert.doesNotThrow(function() { execFile(cmd, u, o, u); });
117+
assert.doesNotThrow(function() { execFile(cmd, a, u, u); });
118+
assert.doesNotThrow(function() { execFile(cmd, n, n, n); });
119+
assert.doesNotThrow(function() { execFile(cmd, n, n, c); });
120+
assert.doesNotThrow(function() { execFile(cmd, n, o, n); });
121+
assert.doesNotThrow(function() { execFile(cmd, a, n, n); });
122+
assert.doesNotThrow(function() { execFile(cmd, a, u); });
123+
assert.doesNotThrow(function() { execFile(cmd, a, n); });
124+
assert.doesNotThrow(function() { execFile(cmd, o, u); });
125+
assert.doesNotThrow(function() { execFile(cmd, o, n); });
126+
assert.doesNotThrow(function() { execFile(cmd, c, u); });
127+
assert.doesNotThrow(function() { execFile(cmd, c, n); });
114128

115129
// string is invalid in arg position (this may seem strange, but is
116130
// consistent across node API, cf. `net.createServer('not options', 'not
117131
// callback')`
118132
assert.throws(function() { execFile(cmd, s, o, c); }, TypeError);
119-
assert.doesNotThrow(function() { execFile(cmd, a, s, c); });
120-
assert.doesNotThrow(function() { execFile(cmd, a, o, s); });
133+
assert.throws(function() { execFile(cmd, a, s, c); }, TypeError);
134+
assert.throws(function() { execFile(cmd, a, o, s); }, TypeError);
135+
assert.throws(function() { execFile(cmd, a, s); }, TypeError);
136+
assert.throws(function() { execFile(cmd, o, s); }, TypeError);
137+
assert.throws(function() { execFile(cmd, u, u, s); }, TypeError);
138+
assert.throws(function() { execFile(cmd, n, n, s); }, TypeError);
139+
assert.throws(function() { execFile(cmd, a, u, s); }, TypeError);
140+
assert.throws(function() { execFile(cmd, a, n, s); }, TypeError);
141+
assert.throws(function() { execFile(cmd, u, o, s); }, TypeError);
142+
assert.throws(function() { execFile(cmd, n, o, s); }, TypeError);
143+
assert.doesNotThrow(function() { execFile(cmd, c, s); });
121144

122145

123146
// verify that fork has same argument parsing behaviour as spawn
@@ -131,6 +154,12 @@ assert.doesNotThrow(function() { fork(empty); });
131154
assert.doesNotThrow(function() { fork(empty, a); });
132155
assert.doesNotThrow(function() { fork(empty, a, o); });
133156
assert.doesNotThrow(function() { fork(empty, o); });
157+
assert.doesNotThrow(function() { fork(empty, u, u); });
158+
assert.doesNotThrow(function() { fork(empty, u, o); });
159+
assert.doesNotThrow(function() { fork(empty, a, u); });
160+
assert.doesNotThrow(function() { fork(empty, n, n); });
161+
assert.doesNotThrow(function() { fork(empty, n, o); });
162+
assert.doesNotThrow(function() { fork(empty, a, n); });
134163

135164
assert.throws(function() { fork(empty, s); }, TypeError);
136-
assert.doesNotThrow(function() { fork(empty, a, s); }, TypeError);
165+
assert.throws(function() { fork(empty, a, s); }, TypeError);

0 commit comments

Comments
 (0)