child_process: measure buffer length in bytes

This change fixes a known issue where `maxBuffer` limits by characters
rather than bytes. Benchmark added to confirm no performance regression
occurs with this change.

PR-URL: #6764
Fixes: #1901
Reviewed-By: Brian White <mscdex@mscdex.net>

Author: Trott

benchmark/child_process/child-process-exec-stdout.js

@@ -0,0 +1,30 @@
+'use strict';
+const common = require('../common.js');
+const bench = common.createBenchmark(main, {
+  len: [64, 256, 1024, 4096, 32768],
+  dur: [5]
+});
+
+const exec = require('child_process').exec;
+function main(conf) {
+  bench.start();
+
+  const dur = +conf.dur;
+  const len = +conf.len;
+
+  const msg = `"${'.'.repeat(len)}"`;
+  msg.match(/./);
+  const options = {'stdio': ['ignore', 'pipe', 'ignore']};
+  // NOTE: Command below assumes bash shell.
+  const child = exec(`while\n  echo ${msg}\ndo :; done\n`, options);
+
+  var bytes = 0;
+  child.stdout.on('data', function(msg) {
+    bytes += msg.length;
+  });
+
+  setTimeout(function() {
+    child.kill();
+    bench.end(bytes);
+  }, dur * 1000);
+}

benchmark/child_process/child-process-read.js

@@ -1,20 +1,20 @@
 'use strict';
-var common = require('../common.js');
-var bench = common.createBenchmark(main, {
+const common = require('../common.js');
+const bench = common.createBenchmark(main, {
   len: [64, 256, 1024, 4096, 32768],
   dur: [5]
 });
 
-var spawn = require('child_process').spawn;
+const spawn = require('child_process').spawn;
 function main(conf) {
   bench.start();
 
-  var dur = +conf.dur;
-  var len = +conf.len;
+  const dur = +conf.dur;
+  const len = +conf.len;
 
-  var msg = '"' + Array(len).join('.') + '"';
-  var options = { 'stdio': ['ignore', 'ipc', 'ignore'] };
-  var child = spawn('yes', [msg], options);
+  const msg = '"' + Array(len).join('.') + '"';
+  const options = {'stdio': ['ignore', 'ipc', 'ignore']};
+  const child = spawn('yes', [msg], options);
 
   var bytes = 0;
   child.on('message', function(msg) {
@@ -23,7 +23,7 @@ function main(conf) {
 
   setTimeout(function() {
     child.kill();
-    var gbits = (bytes * 8) / (1024 * 1024 * 1024);
+    const gbits = (bytes * 8) / (1024 * 1024 * 1024);
     bench.end(gbits);
   }, dur * 1000);
 }

lib/child_process.js

@@ -146,15 +146,13 @@ exports.execFile = function(file /*, args, options, callback*/) {
   });
 
   var encoding;
-  var _stdout;
-  var _stderr;
+  var stdoutState;
+  var stderrState;
+  var _stdout = [];
+  var _stderr = [];
   if (options.encoding !== 'buffer' && Buffer.isEncoding(options.encoding)) {
     encoding = options.encoding;
-    _stdout = '';
-    _stderr = '';
   } else {
-    _stdout = [];
-    _stderr = [];
     encoding = null;
   }
   var stdoutLen = 0;
@@ -176,16 +174,23 @@ exports.execFile = function(file /*, args, options, callback*/) {
 
     if (!callback) return;
 
-    // merge chunks
-    var stdout;
-    var stderr;
-    if (!encoding) {
-      stdout = Buffer.concat(_stdout);
-      stderr = Buffer.concat(_stderr);
-    } else {
-      stdout = _stdout;
-      stderr = _stderr;
-    }
+    var stdout = Buffer.concat(_stdout, stdoutLen);
+    var stderr = Buffer.concat(_stderr, stderrLen);
+
+    var stdoutEncoding = encoding;
+    var stderrEncoding = encoding;
+
+    if (stdoutState && stdoutState.decoder)
+      stdoutEncoding = stdoutState.decoder.encoding;
+
+    if (stderrState && stderrState.decoder)
+      stderrEncoding = stderrState.decoder.encoding;
+
+    if (stdoutEncoding)
+      stdout = stdout.toString(stdoutEncoding);
+
+    if (stderrEncoding)
+      stderr = stderr.toString(stderrEncoding);
 
     if (ex) {
       // Will be handled later
@@ -245,39 +250,45 @@ exports.execFile = function(file /*, args, options, callback*/) {
   }
 
   if (child.stdout) {
-    if (encoding)
-      child.stdout.setEncoding(encoding);
+    stdoutState = child.stdout._readableState;
 
     child.stdout.addListener('data', function(chunk) {
-      stdoutLen += chunk.length;
+      // If `child.stdout.setEncoding()` happened in userland, convert string to
+      // Buffer.
+      if (stdoutState.decoder) {
+        chunk = Buffer.from(chunk, stdoutState.decoder.encoding);
+      }
+
+      stdoutLen += chunk.byteLength;
 
       if (stdoutLen > options.maxBuffer) {
         ex = new Error('stdout maxBuffer exceeded');
+        stdoutLen -= chunk.byteLength;
         kill();
       } else {
-        if (!encoding)
-          _stdout.push(chunk);
-        else
-          _stdout += chunk;
+        _stdout.push(chunk);
       }
     });
   }
 
   if (child.stderr) {
-    if (encoding)
-      child.stderr.setEncoding(encoding);
+    stderrState = child.stderr._readableState;
 
     child.stderr.addListener('data', function(chunk) {
-      stderrLen += chunk.length;
+      // If `child.stderr.setEncoding()` happened in userland, convert string to
+      // Buffer.
+      if (stderrState.decoder) {
+        chunk = Buffer.from(chunk, stderrState.decoder.encoding);
+      }
+
+      stderrLen += chunk.byteLength;
 
       if (stderrLen > options.maxBuffer) {
         ex = new Error('stderr maxBuffer exceeded');
+        stderrLen -= chunk.byteLength;
         kill();
       } else {
-        if (!encoding)
-          _stderr.push(chunk);
-        else
-          _stderr += chunk;
+        _stderr.push(chunk);
       }
     });
   }

test/parallel/test-child-process-maxBuffer-stderr.js

@@ -0,0 +1,15 @@
+'use strict';
+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.error(unicode);
+} else {
+  const cmd = `${process.execPath} ${__filename} child`;
+
+  cp.exec(cmd, {maxBuffer: 10}, common.mustCall((err, stdout, stderr) => {
+    assert.strictEqual(err.message, 'stderr maxBuffer exceeded');
+  }));
+}

test/known_issues/test-child-process-max-buffer.js renamed to test/parallel/test-child-process-maxBuffer-stdout.js

@@ -1,5 +1,4 @@
 'use strict';
-// Refs: https://github.com/nodejs/node/issues/1901
 const common = require('../common');
 const assert = require('assert');
 const cp = require('child_process');
@@ -10,7 +9,7 @@ if (process.argv[2] === 'child') {
 } else {
   const cmd = `${process.execPath} ${__filename} child`;
 
-  cp.exec(cmd, { maxBuffer: 10 }, common.mustCall((err, stdout, stderr) => {
+  cp.exec(cmd, {maxBuffer: 10}, common.mustCall((err, stdout, stderr) => {
     assert.strictEqual(err.message, 'stdout maxBuffer exceeded');
   }));
 }