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: spawn() and spawnSync() validation #8312

Merged
merged 3 commits into from
Dec 25, 2016
Merged

child_process: spawn() and spawnSync() validation #8312

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b374ee8
src: add handle check to spawn_sync
cjihrig Aug 28, 2016
fc7b0dd
child_process: improve input validation
cjihrig Aug 28, 2016
45c9ca7
src: remove redundant spawn/spawnSync type checks
cjihrig Oct 18, 2016
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
66 changes: 65 additions & 1 deletion lib/child_process.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,9 @@ function _convertCustomFds(options) {
function normalizeSpawnArguments(file /*, args, options*/) {
var args, options;

if (typeof file !== 'string' || file.length === 0)
throw new TypeError('"file" argument must be a non-empty string');

if (Array.isArray(arguments[1])) {
args = arguments[1].slice(0);
options = arguments[2];
Expand All @@ -331,6 +334,47 @@ function normalizeSpawnArguments(file /*, args, options*/) {
else if (options === null || typeof options !== 'object')
throw new TypeError('"options" argument must be an object');

// Validate the cwd, if present.
if (options.cwd != null &&
typeof options.cwd !== 'string') {
throw new TypeError('"cwd" must be a string');
}

// Validate detached, if present.
if (options.detached != null &&
typeof options.detached !== 'boolean') {
throw new TypeError('"detached" must be a boolean');
}

// Validate the uid, if present.
if (options.uid != null && !Number.isInteger(options.uid)) {
throw new TypeError('"uid" must be an integer');
}

// Validate the gid, if present.
if (options.gid != null && !Number.isInteger(options.gid)) {
throw new TypeError('"gid" must be an integer');
}

// Validate the shell, if present.
if (options.shell != null &&
typeof options.shell !== 'boolean' &&
typeof options.shell !== 'string') {
throw new TypeError('"shell" must be a boolean or string');
}

// Validate argv0, if present.
if (options.argv0 != null &&
typeof options.argv0 !== 'string') {
throw new TypeError('"argv0" must be a string');
}

// Validate windowsVerbatimArguments, if present.
if (options.windowsVerbatimArguments != null &&
typeof options.windowsVerbatimArguments !== 'boolean') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this have to be, literally, boolean? I would expect it to have to be truthy/falsy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could probably be coerced to a Boolean, but the extra strictness is good IMO. Not that it matters much, but windowsVerbatimArguments isn't documented anyway, IIRC.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, its internal-only, so maybe not so important, and better to start strict. I don't agree strictness is generally good in this case, btw. It creates a bizarre API wherein truthy is sometimes true/false, sometimes null/undefined/0/"" are also false, sometimes not, and sometimes function/[]/{}/non-zero/"strings" are also true, and sometimes null or undefined are a "third way", neither true or false but some kind of other behaviour. Ouch. Anyhow, as a semi-private (but used by modules on npmjs.org none-the-less) API, I'm OK with this restriction.

throw new TypeError('"windowsVerbatimArguments" must be a boolean');
}

// Make a shallow copy so we don't clobber the user's options object.
options = Object.assign({}, options);

Expand Down Expand Up @@ -420,13 +464,33 @@ function spawnSync(/*file, args, options*/) {

debug('spawnSync', opts.args, options);

// Validate the timeout, if present.
if (options.timeout != null &&
!(Number.isInteger(options.timeout) && options.timeout >= 0)) {
throw new TypeError('"timeout" must be an unsigned integer');
}

// Validate maxBuffer, if present.
if (options.maxBuffer != null &&
!(Number.isInteger(options.maxBuffer) && options.maxBuffer >= 0)) {
throw new TypeError('"maxBuffer" must be an unsigned integer');
}

options.file = opts.file;
options.args = opts.args;
options.envPairs = opts.envPairs;

if (options.killSignal)
// Validate the kill signal, if present.
if (typeof options.killSignal === 'string' ||
typeof options.killSignal === 'number') {
options.killSignal = lookupSignal(options.killSignal);

if (options.killSignal === 0)
throw new RangeError('"killSignal" cannot be 0');
} else if (options.killSignal != null) {
throw new TypeError('"killSignal" must be a string or number');
}

options.stdio = _validateStdio(options.stdio || 'pipe', true).stdio;

if (options.input) {
Expand Down
20 changes: 7 additions & 13 deletions src/process_wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,35 +121,29 @@ class ProcessWrap : public HandleWrap {

// options.uid
Local<Value> uid_v = js_options->Get(env->uid_string());
if (uid_v->IsInt32()) {
if (!uid_v->IsUndefined() && !uid_v->IsNull()) {
CHECK(uid_v->IsInt32());
const int32_t uid = uid_v->Int32Value(env->context()).FromJust();
options.flags |= UV_PROCESS_SETUID;
options.uid = static_cast<uv_uid_t>(uid);
} else if (!uid_v->IsUndefined() && !uid_v->IsNull()) {
return env->ThrowTypeError("options.uid should be a number");
}

// options.gid
Local<Value> gid_v = js_options->Get(env->gid_string());
if (gid_v->IsInt32()) {
if (!gid_v->IsUndefined() && !gid_v->IsNull()) {
CHECK(gid_v->IsInt32());
const int32_t gid = gid_v->Int32Value(env->context()).FromJust();
options.flags |= UV_PROCESS_SETGID;
options.gid = static_cast<uv_gid_t>(gid);
} else if (!gid_v->IsUndefined() && !gid_v->IsNull()) {
return env->ThrowTypeError("options.gid should be a number");
}

// TODO(bnoordhuis) is this possible to do without mallocing ?

// options.file
Local<Value> file_v = js_options->Get(env->file_string());
node::Utf8Value file(env->isolate(),
file_v->IsString() ? file_v : Local<Value>());
if (file.length() > 0) {
options.file = *file;
} else {
return env->ThrowTypeError("Bad argument");
}
CHECK(file_v->IsString());
node::Utf8Value file(env->isolate(), file_v);
options.file = *file;

// options.args
Local<Value> argv_v = js_options->Get(env->args_string());
Expand Down
26 changes: 11 additions & 15 deletions src/spawn_sync.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -501,7 +501,12 @@ void SyncProcessRunner::CloseHandlesAndDeleteLoop() {
// Close the process handle when ExitCallback was not called.
uv_handle_t* uv_process_handle =
reinterpret_cast<uv_handle_t*>(&uv_process_);
if (!uv_is_closing(uv_process_handle))

// Close the process handle if it is still open. The handle type also
// needs to be checked because TryInitializeAndRunLoop() won't spawn a
// process if input validation fails.
if (uv_process_handle->type == UV_PROCESS &&
!uv_is_closing(uv_process_handle))
uv_close(uv_process_handle, nullptr);

// Give closing watchers a chance to finish closing and get their close
Expand Down Expand Up @@ -729,17 +734,15 @@ int SyncProcessRunner::ParseOptions(Local<Value> js_value) {
}
Local<Value> js_uid = js_options->Get(env()->uid_string());
if (IsSet(js_uid)) {
if (!js_uid->IsInt32())
return UV_EINVAL;
CHECK(js_uid->IsInt32());
const int32_t uid = js_uid->Int32Value(env()->context()).FromJust();
uv_process_options_.uid = static_cast<uv_uid_t>(uid);
uv_process_options_.flags |= UV_PROCESS_SETUID;
}

Local<Value> js_gid = js_options->Get(env()->gid_string());
if (IsSet(js_gid)) {
if (!js_gid->IsInt32())
return UV_EINVAL;
CHECK(js_gid->IsInt32());
const int32_t gid = js_gid->Int32Value(env()->context()).FromJust();
uv_process_options_.gid = static_cast<uv_gid_t>(gid);
uv_process_options_.flags |= UV_PROCESS_SETGID;
Expand All @@ -755,28 +758,21 @@ int SyncProcessRunner::ParseOptions(Local<Value> js_value) {

Local<Value> js_timeout = js_options->Get(env()->timeout_string());
if (IsSet(js_timeout)) {
if (!js_timeout->IsNumber())
return UV_EINVAL;
CHECK(js_timeout->IsNumber());
int64_t timeout = js_timeout->IntegerValue();
if (timeout < 0)
return UV_EINVAL;
timeout_ = static_cast<uint64_t>(timeout);
}

Local<Value> js_max_buffer = js_options->Get(env()->max_buffer_string());
if (IsSet(js_max_buffer)) {
if (!js_max_buffer->IsUint32())
return UV_EINVAL;
CHECK(js_max_buffer->IsUint32());
max_buffer_ = js_max_buffer->Uint32Value();
}

Local<Value> js_kill_signal = js_options->Get(env()->kill_signal_string());
if (IsSet(js_kill_signal)) {
if (!js_kill_signal->IsInt32())
return UV_EINVAL;
CHECK(js_kill_signal->IsInt32());
kill_signal_ = js_kill_signal->Int32Value();
if (kill_signal_ == 0)
return UV_EINVAL;
}

Local<Value> js_stdio = js_options->Get(env()->stdio_string());
Expand Down
13 changes: 12 additions & 1 deletion test/parallel/test-child-process-spawn-typeerror.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ 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 invalidFileMsg =
/^TypeError: "file" argument must be a non-empty string$/;
const empty = common.fixturesDir + '/empty.js';

assert.throws(function() {
Expand Down Expand Up @@ -36,7 +38,16 @@ assert.doesNotThrow(function() {
// verify that invalid argument combinations throw
assert.throws(function() {
spawn();
}, /Bad argument/);
}, invalidFileMsg);

assert.throws(function() {
spawn('');
}, invalidFileMsg);

assert.throws(function() {
const file = { toString() { throw new Error('foo'); } };
spawn(file);
}, invalidFileMsg);

assert.throws(function() {
spawn(cmd, null);
Expand Down
Loading