diff --git a/lib/internal/child_process.js b/lib/internal/child_process.js index 05e1cd63b02f70..e411a4661dc599 100644 --- a/lib/internal/child_process.js +++ b/lib/internal/child_process.js @@ -157,8 +157,6 @@ const handleConversion = { function ChildProcess() { EventEmitter.call(this); - var self = this; - this._closesNeeded = 1; this._closesGot = 0; this.connected = false; @@ -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'; + 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, @@ -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); @@ -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'; @@ -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); @@ -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) @@ -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); }); } } @@ -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); @@ -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 @@ -860,7 +863,7 @@ function _validateStdio(stdio, sync) { return acc; }, []); - return {stdio: stdio, ipc: ipc, ipcFd: ipcFd}; + return { stdio, ipc, ipcFd }; } diff --git a/test/parallel/test-child-process-constructor.js b/test/parallel/test-child-process-constructor.js index d33c7ae3acf626..7e86ad198fceeb 100644 --- a/test/parallel/test-child-process-constructor.js +++ b/test/parallel/test-child-process-constructor.js @@ -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({ @@ -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);