Skip to content

child_process: internal cleanup and input validation improvements #12348

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

Merged
cjihrig merged 3 commits into from
Apr 17, 2017
Merged

child_process: internal cleanup and input validation improvements #12348

Changes from 1 commit
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
Next Next commit
child_process: minor cleanup of internals 
This commit removes self = this style assignments and replaces
some array iteration functions with a simpler for loop.

PR-URL: #12348
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
  • Loading branch information
@cjihrig
cjihrig committed Apr 17, 2017
commit a9111f973801e88b4d27aa80af8fa17a901a1415
81 changes: 34 additions & 47 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
Copy Markdown
Member

@santigimeno santigimeno Apr 16, 2017

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
Copy Markdown
Member

@gibfahn gibfahn Apr 16, 2017

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
Copy Markdown
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);

if (this.spawnfile)
err.path = this.spawnfile;

err.spawnargs = self.spawnargs.slice(1);
self.emit('error', err);
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,10 @@ function getHandleWrapType(stream) {


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

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

Expand All @@ -291,7 +279,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 +307,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 +333,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 +767,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 +847,7 @@ function _validateStdio(stdio, sync) {
return acc;
}, []);

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


Expand Down