From 6a08535dd10b754a89d5788a6b72dd64e960225c Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Thu, 23 Jun 2016 10:51:48 -0700 Subject: [PATCH] child_process: preserve argument type A previous fix for a `maxBuffer` bug resulted in a change to the argument type for the `data` event on `child.stdin` and `child.stdout` when using `child_process.exec()`. This fixes the `maxBuffer` bug in a way that does not have that side effect. PR-URL: https://github.com/nodejs/node/pull/7391 Reviewed-By: Ben Noordhuis Reviewed-By: Jackson Tian Fixes: https://github.com/nodejs/node/issues/7342 Refs: https://github.com/nodejs/node/issues/1901 --- lib/child_process.js | 24 +++++++------- .../test-child-process-max-buffer.js | 16 ---------- .../test-child-process-exec-maxBuffer.js | 31 +++++++++++++++++++ ...process-exec-stdout-stderr-data-string.js} | 21 ++++++++----- test/parallel/test-exec-max-buffer.js | 11 ------- 5 files changed, 57 insertions(+), 46 deletions(-) delete mode 100644 test/known_issues/test-child-process-max-buffer.js create mode 100644 test/parallel/test-child-process-exec-maxBuffer.js rename test/parallel/{test-child-process-exec-stdout-data-string.js => test-child-process-exec-stdout-stderr-data-string.js} (51%) delete mode 100644 test/parallel/test-exec-max-buffer.js diff --git a/lib/child_process.js b/lib/child_process.js index e43093ace1..02d7c23447 100644 --- a/lib/child_process.js +++ b/lib/child_process.js @@ -190,12 +190,12 @@ exports.execFile = function(file /*, args, options, callback*/) { // merge chunks var stdout; var stderr; - if (!encoding) { - stdout = Buffer.concat(_stdout); - stderr = Buffer.concat(_stderr); - } else { + if (encoding) { stdout = _stdout; stderr = _stderr; + } else { + stdout = Buffer.concat(_stdout); + stderr = Buffer.concat(_stderr); } if (ex) { @@ -260,16 +260,16 @@ exports.execFile = function(file /*, args, options, callback*/) { child.stdout.setEncoding(encoding); child.stdout.addListener('data', function(chunk) { - stdoutLen += chunk.length; + stdoutLen += encoding ? Buffer.byteLength(chunk, encoding) : chunk.length; if (stdoutLen > options.maxBuffer) { ex = new Error('stdout maxBuffer exceeded'); kill(); } else { - if (!encoding) - _stdout.push(chunk); - else + if (encoding) _stdout += chunk; + else + _stdout.push(chunk); } }); } @@ -279,16 +279,16 @@ exports.execFile = function(file /*, args, options, callback*/) { child.stderr.setEncoding(encoding); child.stderr.addListener('data', function(chunk) { - stderrLen += chunk.length; + stderrLen += encoding ? Buffer.byteLength(chunk, encoding) : chunk.length; if (stderrLen > options.maxBuffer) { ex = new Error('stderr maxBuffer exceeded'); kill(); } else { - if (!encoding) - _stderr.push(chunk); - else + if (encoding) _stderr += chunk; + else + _stderr.push(chunk); } }); } diff --git a/test/known_issues/test-child-process-max-buffer.js b/test/known_issues/test-child-process-max-buffer.js deleted file mode 100644 index 14a344c706..0000000000 --- a/test/known_issues/test-child-process-max-buffer.js +++ /dev/null @@ -1,16 +0,0 @@ -'use strict'; -// Refs: https://github.com/nodejs/node/issues/1901 -const common = require('../common'); -const assert = require('assert'); -const cp = require('child_process'); -const unicode = '中文测试'; // Length = 4, Byte length = 13 - -if (process.argv[2] === 'child') { - console.log(unicode); -} else { - const cmd = `${process.execPath} ${__filename} child`; - - cp.exec(cmd, { maxBuffer: 10 }, common.mustCall((err, stdout, stderr) => { - assert.strictEqual(err.message, 'stdout maxBuffer exceeded'); - })); -} diff --git a/test/parallel/test-child-process-exec-maxBuffer.js b/test/parallel/test-child-process-exec-maxBuffer.js new file mode 100644 index 0000000000..714e029d72 --- /dev/null +++ b/test/parallel/test-child-process-exec-maxBuffer.js @@ -0,0 +1,31 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const cp = require('child_process'); + +function checkFactory(streamName) { + return common.mustCall((err) => { + const message = `${streamName} maxBuffer exceeded`; + assert.strictEqual(err.message, message); + }); +} + +{ + const cmd = 'echo "hello world"'; + + cp.exec(cmd, { maxBuffer: 5 }, checkFactory('stdout')); +} + +const unicode = '中文测试'; // length = 4, byte length = 12 + +{ + const cmd = `"${process.execPath}" -e "console.log('${unicode}');"`; + + cp.exec(cmd, {maxBuffer: 10}, checkFactory('stdout')); +} + +{ + const cmd = `"${process.execPath}" -e "console.('${unicode}');"`; + + cp.exec(cmd, {maxBuffer: 10}, checkFactory('stderr')); +} diff --git a/test/parallel/test-child-process-exec-stdout-data-string.js b/test/parallel/test-child-process-exec-stdout-stderr-data-string.js similarity index 51% rename from test/parallel/test-child-process-exec-stdout-data-string.js rename to test/parallel/test-child-process-exec-stdout-stderr-data-string.js index b267ff5e98..8ab834c98f 100644 --- a/test/parallel/test-child-process-exec-stdout-data-string.js +++ b/test/parallel/test-child-process-exec-stdout-stderr-data-string.js @@ -4,13 +4,20 @@ const common = require('../common'); const assert = require('assert'); const exec = require('child_process').exec; -const expectedCalls = 2; - -const cb = common.mustCall((data) => { - assert.strictEqual(typeof data, 'string'); -}, expectedCalls); +var stdoutCalls = 0; +var stderrCalls = 0; const command = common.isWindows ? 'dir' : 'ls'; -exec(command).stdout.on('data', cb); +exec(command).stdout.on('data', (data) => { + stdoutCalls += 1; +}); + +exec('fhqwhgads').stderr.on('data', (data) => { + assert.strictEqual(typeof data, 'string'); + stderrCalls += 1; +}); -exec('fhqwhgads').stderr.on('data', cb); +process.on('exit', () => { + assert(stdoutCalls > 0); + assert(stderrCalls > 0); +}); diff --git a/test/parallel/test-exec-max-buffer.js b/test/parallel/test-exec-max-buffer.js deleted file mode 100644 index 15c6e70259..0000000000 --- a/test/parallel/test-exec-max-buffer.js +++ /dev/null @@ -1,11 +0,0 @@ -'use strict'; -require('../common'); -var exec = require('child_process').exec; -var assert = require('assert'); - -var cmd = 'echo "hello world"'; - -exec(cmd, { maxBuffer: 5 }, function(err, stdout, stderr) { - assert.ok(err); - assert.ok(/maxBuffer/.test(err.message)); -});