From 9cd75c7c0d1432e061a94d2dce36a2006abfb346 Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Tue, 23 Apr 2019 21:08:14 +0200
Subject: [PATCH 1/4] child_process: only stop readable side of stream passed
 to proc

Closing the underlying resource completely has the unwanted side effect
that the stream can no longer be used at all, including passing it
to other child processes.

What we want to avoid is accidentally reading from the stream;
accordingly, it should be sufficient to stop its readable side
manually, and otherwise leave the underlying resource intact.

Fixes: https://github.com/nodejs/node/issues/27097
Refs: https://github.com/nodejs/node/pull/21209
---
 lib/internal/child_process.js                 | 10 +++----
 .../test-child-process-server-close.js        |  6 ++--
 ...ld-process-stdio-merge-stdouts-into-cat.js | 30 +++++++++++++++++++
 3 files changed, 38 insertions(+), 8 deletions(-)
 create mode 100644 test/parallel/test-child-process-stdio-merge-stdouts-into-cat.js

diff --git a/lib/internal/child_process.js b/lib/internal/child_process.js
index 8e6a303a2e27a5..591ff8440537c0 100644
--- a/lib/internal/child_process.js
+++ b/lib/internal/child_process.js
@@ -384,12 +384,12 @@ ChildProcess.prototype.spawn = function(options) {
       continue;
     }
 
-    // The stream is already cloned and piped, thus close it.
+    // The stream is already cloned and piped, thus stop its readable side,
+    // otherwise we might attempt to read from the stream when at the same time
+    // the child process does.
     if (stream.type === 'wrap') {
-      stream.handle.close();
-      if (stream._stdio && stream._stdio instanceof EventEmitter) {
-        stream._stdio.emit('close');
-      }
+      stream.handle.readStop();
+      stream._stdio.pause();
       continue;
     }
 
diff --git a/test/parallel/test-child-process-server-close.js b/test/parallel/test-child-process-server-close.js
index ec95fed67b4fa7..d70926f2e8278e 100644
--- a/test/parallel/test-child-process-server-close.js
+++ b/test/parallel/test-child-process-server-close.js
@@ -8,11 +8,11 @@ const tmpdir = require('../common/tmpdir');
 tmpdir.refresh();
 
 const server = net.createServer((conn) => {
-  conn.on('close', common.mustCall());
-
   spawn(process.execPath, ['-v'], {
     stdio: ['ignore', conn, 'ignore']
-  }).on('close', common.mustCall());
+  }).on('close', common.mustCall(() => {
+    conn.end();
+  }));
 }).listen(common.PIPE, () => {
   const client = net.connect(common.PIPE, common.mustCall());
   client.on('data', () => {
diff --git a/test/parallel/test-child-process-stdio-merge-stdouts-into-cat.js b/test/parallel/test-child-process-stdio-merge-stdouts-into-cat.js
new file mode 100644
index 00000000000000..0158cbc2271aec
--- /dev/null
+++ b/test/parallel/test-child-process-stdio-merge-stdouts-into-cat.js
@@ -0,0 +1,30 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+
+// Regression test for https://github.com/nodejs/node/issues/27097.
+// Check that (cat [p1] ; cat [p2]) | cat [p3] works.
+
+const { spawn } = require('child_process');
+const p3 = spawn('cat', { stdio: ['pipe', 'pipe', process.stderr] });
+const p1 = spawn('cat', { stdio: ['pipe', p3.stdin, process.stderr] });
+const p2 = spawn('cat', { stdio: ['pipe', p3.stdin, process.stderr] });
+p3.stdout.setEncoding('utf8');
+
+// Write three different chunks:
+// - 'hello' from this process to p1 to p3 back to us
+// - 'world' from this process to p2 to p3 back to us
+// - 'foobar' from this process  to p3 back to us
+// Do so sequentially in order to avoid race conditions.
+p1.stdin.end('hello\n');
+p3.stdout.once('data', common.mustCall((chunk) => {
+  assert.strictEqual(chunk, 'hello\n');
+  p2.stdin.end('world\n');
+  p3.stdout.once('data', common.mustCall((chunk) => {
+    assert.strictEqual(chunk, 'world\n');
+    p3.stdin.end('foobar\n');
+    p3.stdout.once('data', common.mustCall((chunk) => {
+      assert.strictEqual(chunk, 'foobar\n');
+    }));
+  }));
+}));

From 5ac726d341bde9552fd87f54359903118cf0ed3d Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Tue, 23 Apr 2019 22:23:23 +0200
Subject: [PATCH 2/4] fixup! child_process: only stop readable side of stream
 passed to proc

---
 lib/internal/child_process.js                 |  3 ++
 ...ld-process-stdio-merge-stdouts-into-cat.js |  2 +-
 ...hild-process-stdio-reuse-readable-stdio.js | 28 +++++++++++++++++++
 3 files changed, 32 insertions(+), 1 deletion(-)
 create mode 100644 test/parallel/test-child-process-stdio-reuse-readable-stdio.js

diff --git a/lib/internal/child_process.js b/lib/internal/child_process.js
index 591ff8440537c0..659dbf004900da 100644
--- a/lib/internal/child_process.js
+++ b/lib/internal/child_process.js
@@ -388,8 +388,11 @@ ChildProcess.prototype.spawn = function(options) {
     // otherwise we might attempt to read from the stream when at the same time
     // the child process does.
     if (stream.type === 'wrap') {
+      stream.handle.reading = false;
       stream.handle.readStop();
       stream._stdio.pause();
+      stream._stdio.readableFlowing = false;
+      stream._stdio._readableState.reading = false;
       continue;
     }
 
diff --git a/test/parallel/test-child-process-stdio-merge-stdouts-into-cat.js b/test/parallel/test-child-process-stdio-merge-stdouts-into-cat.js
index 0158cbc2271aec..0324873dc57c99 100644
--- a/test/parallel/test-child-process-stdio-merge-stdouts-into-cat.js
+++ b/test/parallel/test-child-process-stdio-merge-stdouts-into-cat.js
@@ -1,11 +1,11 @@
 'use strict';
 const common = require('../common');
 const assert = require('assert');
+const { spawn } = require('child_process');
 
 // Regression test for https://github.com/nodejs/node/issues/27097.
 // Check that (cat [p1] ; cat [p2]) | cat [p3] works.
 
-const { spawn } = require('child_process');
 const p3 = spawn('cat', { stdio: ['pipe', 'pipe', process.stderr] });
 const p1 = spawn('cat', { stdio: ['pipe', p3.stdin, process.stderr] });
 const p2 = spawn('cat', { stdio: ['pipe', p3.stdin, process.stderr] });
diff --git a/test/parallel/test-child-process-stdio-reuse-readable-stdio.js b/test/parallel/test-child-process-stdio-reuse-readable-stdio.js
new file mode 100644
index 00000000000000..4e7be1e8cc62ad
--- /dev/null
+++ b/test/parallel/test-child-process-stdio-reuse-readable-stdio.js
@@ -0,0 +1,28 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+const { spawn } = require('child_process');
+
+// Check that, once a child process has ended, it’s safe to read from a pipe
+// that the child had used as input.
+// We simulate that using cat | (head -n1; ...)
+
+const p1 = spawn('cat', { stdio: ['pipe', 'pipe', process.stderr] });
+const p2 = spawn('head', [ '-n1'], {
+  stdio: [p1.stdout, 'pipe', process.stderr]
+});
+
+// First, write the line that gets passed through p2, making 'head' exit.
+p1.stdin.write('hello\n');
+p2.stdout.setEncoding('utf8');
+p2.stdout.on('data', common.mustCall((chunk) => {
+  assert.strictEqual(chunk, 'hello\n');
+}));
+p2.on('exit', common.mustCall(() => {
+  // We can now use cat’s output, because 'head' is no longer reading from it.
+  p1.stdin.end('world\n');
+  p1.stdout.setEncoding('utf8');
+  p1.stdout.on('data', common.mustCall((chunk) => {
+    assert.strictEqual(chunk, 'world\n');
+  }));
+}));

From 2467b1fe252695fa89eaa999347bf7ed4e206418 Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Tue, 23 Apr 2019 22:27:15 +0200
Subject: [PATCH 3/4] fixup! fixup! child_process: only stop readable side of
 stream passed to proc

---
 .../test-child-process-stdio-merge-stdouts-into-cat.js      | 6 +++---
 .../test-child-process-stdio-reuse-readable-stdio.js        | 6 ++----
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/test/parallel/test-child-process-stdio-merge-stdouts-into-cat.js b/test/parallel/test-child-process-stdio-merge-stdouts-into-cat.js
index 0324873dc57c99..64373e9e160937 100644
--- a/test/parallel/test-child-process-stdio-merge-stdouts-into-cat.js
+++ b/test/parallel/test-child-process-stdio-merge-stdouts-into-cat.js
@@ -6,9 +6,9 @@ const { spawn } = require('child_process');
 // Regression test for https://github.com/nodejs/node/issues/27097.
 // Check that (cat [p1] ; cat [p2]) | cat [p3] works.
 
-const p3 = spawn('cat', { stdio: ['pipe', 'pipe', process.stderr] });
-const p1 = spawn('cat', { stdio: ['pipe', p3.stdin, process.stderr] });
-const p2 = spawn('cat', { stdio: ['pipe', p3.stdin, process.stderr] });
+const p3 = spawn('cat', { stdio: ['pipe', 'pipe', 'inherit'] });
+const p1 = spawn('cat', { stdio: ['pipe', p3.stdin, 'inherit'] });
+const p2 = spawn('cat', { stdio: ['pipe', p3.stdin, 'inherit'] });
 p3.stdout.setEncoding('utf8');
 
 // Write three different chunks:
diff --git a/test/parallel/test-child-process-stdio-reuse-readable-stdio.js b/test/parallel/test-child-process-stdio-reuse-readable-stdio.js
index 4e7be1e8cc62ad..304a5c51d5e533 100644
--- a/test/parallel/test-child-process-stdio-reuse-readable-stdio.js
+++ b/test/parallel/test-child-process-stdio-reuse-readable-stdio.js
@@ -7,10 +7,8 @@ const { spawn } = require('child_process');
 // that the child had used as input.
 // We simulate that using cat | (head -n1; ...)
 
-const p1 = spawn('cat', { stdio: ['pipe', 'pipe', process.stderr] });
-const p2 = spawn('head', [ '-n1'], {
-  stdio: [p1.stdout, 'pipe', process.stderr]
-});
+const p1 = spawn('cat', { stdio: ['pipe', 'pipe', 'inherit'] });
+const p2 = spawn('head', [ '-n1'], { stdio: [p1.stdout, 'pipe', 'inherit'] });
 
 // First, write the line that gets passed through p2, making 'head' exit.
 p1.stdin.write('hello\n');

From a67dd6f8d8b307641436d0bee88b4c1bc6302362 Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Tue, 23 Apr 2019 23:40:34 +0200
Subject: [PATCH 4/4] fixup! child_process: only stop readable side of stream
 passed to proc

---
 lib/internal/child_process.js                          | 10 +++++++++-
 test/parallel/test-child-process-pipe-dataflow.js      |  4 ++++
 .../test-child-process-stdio-reuse-readable-stdio.js   |  3 ++-
 3 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/lib/internal/child_process.js b/lib/internal/child_process.js
index 659dbf004900da..03c0ed9159f64a 100644
--- a/lib/internal/child_process.js
+++ b/lib/internal/child_process.js
@@ -63,6 +63,7 @@ let freeParser;
 let HTTPParser;
 
 const MAX_HANDLE_RETRANSMISSIONS = 3;
+const kIsUsedAsStdio = Symbol('kIsUsedAsStdio');
 
 // This object contain function to convert TCP objects to native handle objects
 // and back again.
@@ -278,8 +279,14 @@ function flushStdio(subprocess) {
 
   for (var i = 0; i < stdio.length; i++) {
     const stream = stdio[i];
-    if (!stream || !stream.readable || stream._readableState.readableListening)
+    // TODO(addaleax): This doesn't necessarily account for all the ways in
+    // which data can be read from a stream, e.g. being consumed on the
+    // native layer directly as a StreamBase.
+    if (!stream || !stream.readable ||
+        stream._readableState.readableListening ||
+        stream[kIsUsedAsStdio]) {
       continue;
+    }
     stream.resume();
   }
 }
@@ -393,6 +400,7 @@ ChildProcess.prototype.spawn = function(options) {
       stream._stdio.pause();
       stream._stdio.readableFlowing = false;
       stream._stdio._readableState.reading = false;
+      stream._stdio[kIsUsedAsStdio] = true;
       continue;
     }
 
diff --git a/test/parallel/test-child-process-pipe-dataflow.js b/test/parallel/test-child-process-pipe-dataflow.js
index f5068b5d366468..88a31f4ff8429b 100644
--- a/test/parallel/test-child-process-pipe-dataflow.js
+++ b/test/parallel/test-child-process-pipe-dataflow.js
@@ -33,6 +33,10 @@ const MB = KB * KB;
   grep = spawn('grep', ['x'], { stdio: [cat.stdout, 'pipe', 'pipe'] });
   wc = spawn('wc', ['-c'], { stdio: [grep.stdout, 'pipe', 'pipe'] });
 
+  // Extra checks: We never try to start reading data ourselves.
+  cat.stdout._handle.readStart = common.mustNotCall();
+  grep.stdout._handle.readStart = common.mustNotCall();
+
   [cat, grep, wc].forEach((child, index) => {
     child.stderr.on('data', (d) => {
       // Don't want to assert here, as we might miss error code info.
diff --git a/test/parallel/test-child-process-stdio-reuse-readable-stdio.js b/test/parallel/test-child-process-stdio-reuse-readable-stdio.js
index 304a5c51d5e533..263270f8e8891f 100644
--- a/test/parallel/test-child-process-stdio-reuse-readable-stdio.js
+++ b/test/parallel/test-child-process-stdio-reuse-readable-stdio.js
@@ -8,7 +8,7 @@ const { spawn } = require('child_process');
 // We simulate that using cat | (head -n1; ...)
 
 const p1 = spawn('cat', { stdio: ['pipe', 'pipe', 'inherit'] });
-const p2 = spawn('head', [ '-n1'], { stdio: [p1.stdout, 'pipe', 'inherit'] });
+const p2 = spawn('head', ['-n1'], { stdio: [p1.stdout, 'pipe', 'inherit'] });
 
 // First, write the line that gets passed through p2, making 'head' exit.
 p1.stdin.write('hello\n');
@@ -23,4 +23,5 @@ p2.on('exit', common.mustCall(() => {
   p1.stdout.on('data', common.mustCall((chunk) => {
     assert.strictEqual(chunk, 'world\n');
   }));
+  p1.stdout.resume();
 }));