Skip to content

Commit

Permalink
child_process: use non-infinite maxBuffer defaults
Browse files Browse the repository at this point in the history
Set the default maxBuffer size to 204,800 bytes for execSync,
execFileSync, and spawnSync.

APIs that return the child output as a string should have non-infinite
defaults for maxBuffer sizes to avoid out-of-memory error conditions. A
non-infinite default used to be the documented behaviour for all
relevant APIs, but the implemented behaviour for execSync, execFileSync
and spawnSync was to have no maxBuffer limits.

PR-URL: #23027
Refs: #22894
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
  • Loading branch information
koh110 authored and sam-github committed Apr 9, 2019
1 parent 1656cd2 commit eb8a51a
Show file tree
Hide file tree
Showing 9 changed files with 138 additions and 16 deletions.
6 changes: 3 additions & 3 deletions doc/api/child_process.md
Original file line number Diff line number Diff line change
Expand Up @@ -721,7 +721,7 @@ changes:
process will be killed. **Default:** `'SIGTERM'`.
* `maxBuffer` {number} Largest amount of data in bytes allowed on stdout or
stderr. If exceeded, the child process is terminated. See caveat at
[`maxBuffer` and Unicode][]. **Default:** `Infinity`.
[`maxBuffer` and Unicode][]. **Default:** `200 * 1024`.
* `encoding` {string} The encoding used for all stdio inputs and outputs.
**Default:** `'buffer'`.
* `windowsHide` {boolean} Hide the subprocess console window that would
Expand Down Expand Up @@ -788,7 +788,7 @@ changes:
* `maxBuffer` {number} Largest amount of data in bytes allowed on stdout or
stderr. If exceeded, the child process is terminated and any output is
truncated. See caveat at [`maxBuffer` and Unicode][].
**Default:** `Infinity`.
**Default:** `200 * 1024`.
* `encoding` {string} The encoding used for all stdio inputs and outputs.
**Default:** `'buffer'`.
* `windowsHide` {boolean} Hide the subprocess console window that would
Expand Down Expand Up @@ -852,7 +852,7 @@ changes:
* `maxBuffer` {number} Largest amount of data in bytes allowed on stdout or
stderr. If exceeded, the child process is terminated and any output is
truncated. See caveat at [`maxBuffer` and Unicode][].
**Default:** `Infinity`.
**Default:** `200 * 1024`.
* `encoding` {string} The encoding used for all stdio inputs and outputs.
**Default:** `'buffer'`.
* `shell` {boolean|string} If `true`, runs `command` inside of a shell. Uses
Expand Down
10 changes: 8 additions & 2 deletions lib/child_process.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ const {
ChildProcess
} = child_process;

const MAX_BUFFER = 200 * 1024;

exports.ChildProcess = ChildProcess;

function stdioStringToArray(option) {
Expand Down Expand Up @@ -206,7 +208,7 @@ exports.execFile = function execFile(file /* , args, options, callback */) {
options = {
encoding: 'utf8',
timeout: 0,
maxBuffer: 200 * 1024,
maxBuffer: MAX_BUFFER,
killSignal: 'SIGTERM',
cwd: null,
env: null,
Expand Down Expand Up @@ -560,7 +562,11 @@ var spawn = exports.spawn = function spawn(file, args, options) {
function spawnSync(file, args, options) {
const opts = normalizeSpawnArguments(file, args, options);

options = opts.options;
const defaults = {
maxBuffer: MAX_BUFFER,
...opts.options
};
options = opts.options = Object.assign(defaults, opts.options);

debug('spawnSync', opts.args, options);

Expand Down
3 changes: 2 additions & 1 deletion test/parallel/test-async-wrap-pop-id-during-load.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ const { spawnSync } = require('child_process');

const ret = spawnSync(
process.execPath,
['--stack_size=150', __filename, 'async']
['--stack_size=150', __filename, 'async'],
{ maxBuffer: Infinity }
);
assert.strictEqual(ret.status, 0,
`EXIT CODE: ${ret.status}, STDERR:\n${ret.stderr}`);
Expand Down
24 changes: 24 additions & 0 deletions test/parallel/test-child-process-exec-maxbuf.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,30 @@ function runChecks(err, stdio, streamName, expected) {
);
}

// default value
{
const cmd = `"${process.execPath}" -e "console.log('a'.repeat(200 * 1024))"`;

cp.exec(
cmd,
common.mustCall((err, stdout, stderr) => {
runChecks(err, { stdout, stderr }, 'stdout', 'a'.repeat(200 * 1024));
})
);
}

// default value
{
const cmd =
`"${process.execPath}" -e "console.log('a'.repeat(200 * 1024 - 1))"`;

cp.exec(cmd, common.mustCall((err, stdout, stderr) => {
assert.ifError(err);
assert.strictEqual(stdout.trim(), 'a'.repeat(200 * 1024 - 1));
assert.strictEqual(stderr, '');
}));
}

const unicode = '中文测试'; // length = 4, byte length = 12

{
Expand Down
50 changes: 50 additions & 0 deletions test/parallel/test-child-process-execfilesync-maxBuffer.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
'use strict';
require('../common');

// This test checks that the maxBuffer option for child_process.spawnFileSync()
// works as expected.

const assert = require('assert');
const { execFileSync } = require('child_process');
const msgOut = 'this is stdout';
const msgOutBuf = Buffer.from(`${msgOut}\n`);

const args = [
'-e',
`console.log("${msgOut}");`
];

// Verify that an error is returned if maxBuffer is surpassed.
{
assert.throws(() => {
execFileSync(process.execPath, args, { maxBuffer: 1 });
}, (e) => {
assert.ok(e, 'maxBuffer should error');
assert.strictEqual(e.errno, 'ENOBUFS');
// We can have buffers larger than maxBuffer because underneath we alloc 64k
// that matches our read sizes.
assert.deepStrictEqual(e.stdout, msgOutBuf);
return true;
});
}

// Verify that a maxBuffer size of Infinity works.
{
const ret = execFileSync(process.execPath, args, { maxBuffer: Infinity });

assert.deepStrictEqual(ret, msgOutBuf);
}

// Default maxBuffer size is 200 * 1024.
{
assert.throws(() => {
execFileSync(
process.execPath,
['-e', "console.log('a'.repeat(200 * 1024))"]
);
}, (e) => {
assert.ok(e, 'maxBuffer should error');
assert.strictEqual(e.errno, 'ENOBUFS');
return true;
});
}
20 changes: 13 additions & 7 deletions test/parallel/test-child-process-execfilesync-maxbuf.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,19 @@ const args = [
assert.deepStrictEqual(ret, msgOutBuf);
}

// maxBuffer size is Infinity at default.
// maxBuffer size is 200 * 1024 at default.
{
const ret = execFileSync(
process.execPath,
['-e', "console.log('a'.repeat(200 * 1024))"],
{ encoding: 'utf-8' }
assert.throws(
() => {
execFileSync(
process.execPath,
['-e', "console.log('a'.repeat(200 * 1024))"],
{ encoding: 'utf-8' }
);
}, (e) => {
assert.ok(e, 'maxBuffer should error');
assert.strictEqual(e.errno, 'ENOBUFS');
return true;
}
);

assert.ifError(ret.error);
}
22 changes: 22 additions & 0 deletions test/parallel/test-child-process-execsync-maxbuf.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ const args = [
}, (e) => {
assert.ok(e, 'maxBuffer should error');
assert.strictEqual(e.errno, 'ENOBUFS');
// We can have buffers larger than maxBuffer because underneath we alloc 64k
// that matches our read sizes.
assert.deepStrictEqual(e.stdout, msgOutBuf);
return true;
});
Expand All @@ -35,3 +37,23 @@ const args = [

assert.deepStrictEqual(ret, msgOutBuf);
}

// Default maxBuffer size is 200 * 1024.
{
assert.throws(() => {
execSync(`"${process.execPath}" -e "console.log('a'.repeat(200 * 1024))"`);
}, (e) => {
assert.ok(e, 'maxBuffer should error');
assert.strictEqual(e.errno, 'ENOBUFS');
return true;
});
}

// Default maxBuffer size is 200 * 1024.
{
const ret = execSync(
`"${process.execPath}" -e "console.log('a'.repeat(200 * 1024 - 1))"`
);

assert.deepStrictEqual(ret.toString().trim(), 'a'.repeat(200 * 1024 - 1));
}
17 changes: 15 additions & 2 deletions test/parallel/test-child-process-spawnsync-maxbuf.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,23 @@ const args = [
assert.deepStrictEqual(ret.stdout, msgOutBuf);
}

// maxBuffer size is Infinity at default.
// Default maxBuffer size is 200 * 1024.
{
const args = ['-e', "console.log('a'.repeat(200 * 1024))"];
const ret = spawnSync(process.execPath, args, { encoding: 'utf-8' });
const ret = spawnSync(process.execPath, args);

assert.ok(ret.error, 'maxBuffer should error');
assert.strictEqual(ret.error.errno, 'ENOBUFS');
}

// Default maxBuffer size is 200 * 1024.
{
const args = ['-e', "console.log('a'.repeat(200 * 1024 - 1))"];
const ret = spawnSync(process.execPath, args);

assert.ifError(ret.error);
assert.deepStrictEqual(
ret.stdout.toString().trim(),
'a'.repeat(200 * 1024 - 1)
);
}
2 changes: 1 addition & 1 deletion test/parallel/test-tick-processor-arguments.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ assert(logfile);
const { stdout } = spawnSync(
process.execPath,
[ '--prof-process', '--preprocess', logfile ],
{ cwd: tmpdir.path, encoding: 'utf8' });
{ cwd: tmpdir.path, encoding: 'utf8', maxBuffer: Infinity });

// Make sure that the result is valid JSON.
JSON.parse(stdout);

0 comments on commit eb8a51a

Please sign in to comment.