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: internal cleanup and input validation improvements #12348

Merged
merged 3 commits into from
Apr 17, 2017
Merged

child_process: internal cleanup and input validation improvements #12348

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
a9111f9
child_process: minor cleanup of internals
cjihrig Apr 11, 2017
59c6230
test: cleanup test-child-process-constructor.js
cjihrig Apr 11, 2017
97a7728
child_process: improve ChildProcess validation
cjihrig Apr 11, 2017
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
103 changes: 53 additions & 50 deletions lib/internal/child_process.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,8 +157,6 @@ const handleConversion = {
function ChildProcess() {
EventEmitter.call(this);

var self = this;

this._closesNeeded = 1;
this._closesGot = 0;
this.connected = false;
Expand All @@ -171,41 +169,31 @@ function ChildProcess() {
this._handle = new Process();
this._handle.owner = this;

this._handle.onexit = function(exitCode, signalCode) {
//
// follow 0.4.x behaviour:
//
// - normally terminated processes don't touch this.signalCode
// - signaled processes don't touch this.exitCode
//
// new in 0.9.x:
//
// - spawn failures are reported with exitCode < 0
//
var syscall = self.spawnfile ? 'spawn ' + self.spawnfile : 'spawn';
var err = (exitCode < 0) ? errnoException(exitCode, syscall) : null;

this._handle.onexit = (exitCode, signalCode) => {
if (signalCode) {
self.signalCode = signalCode;
this.signalCode = signalCode;
} else {
self.exitCode = exitCode;
this.exitCode = exitCode;
}

if (self.stdin) {
self.stdin.destroy();
if (this.stdin) {
this.stdin.destroy();
}

self._handle.close();
self._handle = null;
this._handle.close();
this._handle = null;

if (exitCode < 0) {
if (self.spawnfile)
err.path = self.spawnfile;
var syscall = this.spawnfile ? 'spawn ' + this.spawnfile : 'spawn';
Copy link
Member

Choose a reason for hiding this comment

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

can this be a const too?

Copy link
Member

Choose a reason for hiding this comment

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

I thought I saw that const could trigger deopts in functions, cc/ @mscdex

Copy link
Contributor

@mscdex mscdex Apr 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

It can but it's not always immediately obvious if it actually will or not without checking during runtime (e.g. by adding --trace-opt and grepping for 'aborted' and/or 'disabled' in the resulting output).

const err = errnoException(exitCode, syscall);

err.spawnargs = self.spawnargs.slice(1);
self.emit('error', err);
if (this.spawnfile)
err.path = this.spawnfile;

err.spawnargs = this.spawnargs.slice(1);
this.emit('error', err);
} else {
self.emit('exit', self.exitCode, self.signalCode);
this.emit('exit', this.exitCode, this.signalCode);
}

// if any of the stdio streams have not been touched,
Expand All @@ -214,9 +202,9 @@ function ChildProcess() {
// Do it on nextTick so that the user has one last chance
// to consume the output, if for example they only want to
// start reading the data once the process exits.
process.nextTick(flushStdio, self);
process.nextTick(flushStdio, this);

maybeClose(self);
maybeClose(this);
};
}
util.inherits(ChildProcess, EventEmitter);
Expand Down Expand Up @@ -262,10 +250,13 @@ function getHandleWrapType(stream) {


ChildProcess.prototype.spawn = function(options) {
const self = this;
var ipc;
var ipcFd;
var i;

if (options === null || typeof options !== 'object')
throw new TypeError('"options" must be an object');

// If no `stdio` option was given - use default
var stdio = options.stdio || 'pipe';

Expand All @@ -277,12 +268,25 @@ ChildProcess.prototype.spawn = function(options) {

if (ipc !== undefined) {
// Let child process know about opened IPC channel
options.envPairs = options.envPairs || [];
if (options.envPairs === undefined)
options.envPairs = [];
else if (!Array.isArray(options.envPairs))
throw new TypeError('"envPairs" must be an array');

options.envPairs.push('NODE_CHANNEL_FD=' + ipcFd);
}

this.spawnfile = options.file;
this.spawnargs = options.args;
if (typeof options.file === 'string')
this.spawnfile = options.file;
else
throw new TypeError('"file" must be a string');

if (Array.isArray(options.args))
this.spawnargs = options.args;
else if (options.args === undefined)
this.spawnargs = [];
else
throw new TypeError('"args" must be an array');

var err = this._handle.spawn(options);

Expand All @@ -291,7 +295,7 @@ ChildProcess.prototype.spawn = function(options) {
err === uv.UV_EMFILE ||
err === uv.UV_ENFILE ||
err === uv.UV_ENOENT) {
process.nextTick(onErrorNT, self, err);
process.nextTick(onErrorNT, this, err);
// There is no point in continuing when we've hit EMFILE or ENFILE
// because we won't be able to set up the stdio file descriptors.
// It's kind of silly that the de facto spec for ENOENT (the test suite)
Expand Down Expand Up @@ -319,20 +323,20 @@ ChildProcess.prototype.spawn = function(options) {
if (stream.type === 'ignore') continue;

if (stream.ipc) {
self._closesNeeded++;
this._closesNeeded++;
continue;
}

if (stream.handle) {
// when i === 0 - we're dealing with stdin
// (which is the only one writable pipe)
stream.socket = createSocket(self.pid !== 0 ?
stream.socket = createSocket(this.pid !== 0 ?
stream.handle : null, i > 0);

if (i > 0 && self.pid !== 0) {
self._closesNeeded++;
stream.socket.on('close', function() {
maybeClose(self);
if (i > 0 && this.pid !== 0) {
this._closesNeeded++;
stream.socket.on('close', () => {
maybeClose(this);
});
}
}
Expand All @@ -345,9 +349,10 @@ ChildProcess.prototype.spawn = function(options) {
this.stderr = stdio.length >= 3 && stdio[2].socket !== undefined ?
stdio[2].socket : null;

this.stdio = stdio.map(function(stdio) {
return stdio.socket === undefined ? null : stdio.socket;
});
this.stdio = [];

for (i = 0; i < stdio.length; i++)
this.stdio.push(stdio[i].socket === undefined ? null : stdio[i].socket);

// Add .send() method and start listening for IPC data
if (ipc !== undefined) setupChannel(this, ipc);
Expand Down Expand Up @@ -778,12 +783,10 @@ function _validateStdio(stdio, sync) {
// (i.e. PipeWraps or fds)
stdio = stdio.reduce(function(acc, stdio, i) {
function cleanup() {
acc.filter(function(stdio) {
return stdio.type === 'pipe' || stdio.type === 'ipc';
}).forEach(function(stdio) {
if (stdio.handle)
stdio.handle.close();
});
for (var i = 0; i < acc.length; i++) {
if ((acc[i].type === 'pipe' || acc[i].type === 'ipc') && acc[i].handle)
acc[i].handle.close();
}
}

// Defaults
Expand Down Expand Up @@ -860,7 +863,7 @@ function _validateStdio(stdio, sync) {
return acc;
}, []);

return {stdio: stdio, ipc: ipc, ipcFd: ipcFd};
return { stdio, ipc, ipcFd };
}


Expand Down
52 changes: 48 additions & 4 deletions test/parallel/test-child-process-constructor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,53 @@

require('../common');
const assert = require('assert');
const child_process = require('child_process');
const ChildProcess = child_process.ChildProcess;
const { ChildProcess } = require('child_process');
assert.strictEqual(typeof ChildProcess, 'function');

{
// Verify that invalid options to spawn() throw.
const child = new ChildProcess();

[undefined, null, 'foo', 0, 1, NaN, true, false].forEach((options) => {
assert.throws(() => {
child.spawn(options);
}, /^TypeError: "options" must be an object$/);
});
}

{
// Verify that spawn throws if file is not a string.
const child = new ChildProcess();

[undefined, null, 0, 1, NaN, true, false, {}].forEach((file) => {
assert.throws(() => {
child.spawn({ file });
}, /^TypeError: "file" must be a string$/);
});
}

{
// Verify that spawn throws if envPairs is not an array or undefined.
const child = new ChildProcess();

[null, 0, 1, NaN, true, false, {}, 'foo'].forEach((envPairs) => {
assert.throws(() => {
child.spawn({ envPairs, stdio: ['ignore', 'ignore', 'ignore', 'ipc'] });
}, /^TypeError: "envPairs" must be an array$/);
});
}

{
// Verify that spawn throws if args is not an array or undefined.
const child = new ChildProcess();

[null, 0, 1, NaN, true, false, {}, 'foo'].forEach((args) => {
assert.throws(() => {
child.spawn({ file: 'foo', args });
}, /^TypeError: "args" must be an array$/);
});
}

// test that we can call spawn
const child = new ChildProcess();
child.spawn({
Expand All @@ -16,10 +59,11 @@ child.spawn({
});

assert.strictEqual(child.hasOwnProperty('pid'), true);
assert(Number.isInteger(child.pid));

// try killing with invalid signal
assert.throws(function() {
assert.throws(() => {
child.kill('foo');
}, /Unknown signal: foo/);
}, /^Error: Unknown signal: foo$/);

assert.strictEqual(child.kill(), true);