From b1f82e4342f8a630b1ef83cd33781a725428f569 Mon Sep 17 00:00:00 2001 From: Gireesh Punathil Date: Fri, 8 Jun 2018 08:33:37 -0400 Subject: [PATCH] child_process: close pipe ends that are re-piped when t0 and t1 are spawned with t0's outputstream [1, 2] is piped into t1's input, a new pipe is created which uses a copy of the t0's fd. This leaves the original copy in Node parent, unattended. Net result is that when t0 produces data, it gets bifurcated into both the copies Detect the passed handle to be of 'wrap' type and close after the native spawn invocation by which time piping would have been over. Fixes: https://github.com/nodejs/node/issues/9413 Fixes: https://github.com/nodejs/node/issues/18016 PR-URL: https://github.com/nodejs/node/pull/21209 Reviewed-By: Matteo Collina Reviewed-By: James M Snell Reviewed-By: Anna Henningsen --- lib/internal/child_process.js | 6 +++ .../test-child-process-pipe-dataflow.js | 51 +++++++++++++++++++ 2 files changed, 57 insertions(+) create mode 100644 test/parallel/test-child-process-pipe-dataflow.js diff --git a/lib/internal/child_process.js b/lib/internal/child_process.js index 498dbf6a1cad75..cb0d8ec6b65bbe 100644 --- a/lib/internal/child_process.js +++ b/lib/internal/child_process.js @@ -384,6 +384,12 @@ ChildProcess.prototype.spawn = function(options) { continue; } + // stream is already cloned and piped, so close + if (stream.type === 'wrap') { + stream.handle.close(); + continue; + } + if (stream.handle) { // when i === 0 - we're dealing with stdin // (which is the only one writable pipe) diff --git a/test/parallel/test-child-process-pipe-dataflow.js b/test/parallel/test-child-process-pipe-dataflow.js new file mode 100644 index 00000000000000..501fb29032b3c8 --- /dev/null +++ b/test/parallel/test-child-process-pipe-dataflow.js @@ -0,0 +1,51 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const path = require('path'); +const fs = require('fs'); +const spawn = require('child_process').spawn; +const tmpdir = require('../common/tmpdir'); + +let cat, grep, wc; + +const KB = 1024; +const MB = KB * KB; + + +// Make sure process chaining allows desired data flow: +// check cat | grep 'x' | wc -c === 1MB +// This helps to make sure no data is lost between pipes. + +{ + tmpdir.refresh(); + const file = path.resolve(tmpdir.path, 'data.txt'); + const buf = Buffer.alloc(MB).fill('x'); + + // Most OS commands that deal with data, attach special + // meanings to new line - for example, line buffering. + // So cut the buffer into lines at some points, forcing + // data flow to be split in the stream. + for (let i = 0; i < KB; i++) + buf[i * KB] = 10; + fs.writeFileSync(file, buf.toString()); + + cat = spawn('cat', [file]); + grep = spawn('grep', ['x'], { stdio: [cat.stdout, 'pipe', 'pipe'] }); + wc = spawn('wc', ['-c'], { stdio: [grep.stdout, 'pipe', 'pipe'] }); + + wc.stdout.on('data', common.mustCall(function(data) { + assert.strictEqual(data.toString().trim(), MB.toString()); + })); + + cat.on('exit', common.mustCall(function(code) { + assert.strictEqual(code, 0); + })); + + grep.on('exit', common.mustCall(function(code) { + assert.strictEqual(code, 0); + })); + + wc.on('exit', common.mustCall(function(code) { + assert.strictEqual(code, 0); + })); +}