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

PR-URL: https://github.com/nodejs/node/pull/27373
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>

4 files changed, 64 insertions, 3 deletions

diff --git a/test/parallel/test-child-process-pipe-dataflow.js b/test/parallel/test-child-process-pipe-dataflow.js
index f5068b5d36..88a31f4ff8 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-server-close.js b/test/parallel/test-child-process-server-close.js
index ec95fed67b..d70926f2e8 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 0000000000..64373e9e16
--- /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');
+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', '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:
+// - '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');
+    }));
+  }));
+}));
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 0000000000..263270f8e8
--- /dev/null
+++ b/test/parallel/test-child-process-stdio-reuse-readable-stdio.js
@@ -0,0 +1,27 @@
+'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', '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');
+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');
+  }));
+  p1.stdout.resume();
+}));