From 5a306748e9e9cbcad8943b6f148ba472243337fd Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Mon, 24 Sep 2018 11:51:14 +0200
Subject: [PATCH] process: allow reading from stdout/stderr sockets

Allow reading from stdio streams that are conventionally
associated with process output, since this is only convention.

This involves disabling the oddness around closing stdio
streams. Its purpose is to prevent the file descriptors
0 through 2 from being closed, since doing so can lead
to information leaks when new file descriptors are being
opened; instead, not doing anything seems like a more
reasonable choice.

Fixes: https://github.com/nodejs/node/issues/21203

PR-URL: https://github.com/nodejs/node/pull/23053
Reviewed-By: James M Snell <jasnell@gmail.com>
---
 doc/api/errors.md                             | 43 +++++++++++++------
 doc/api/process.md                            |  6 +--
 lib/internal/errors.js                        |  2 -
 lib/internal/process/stdio.js                 | 18 +++-----
 ...out-cannot-be-closed-child-process-pipe.js |  4 +-
 test/pseudo-tty/test-stdout-read.in           |  1 +
 test/pseudo-tty/test-stdout-read.js           |  3 ++
 test/pseudo-tty/test-stdout-read.out          |  1 +
 8 files changed, 45 insertions(+), 33 deletions(-)
 create mode 100644 test/pseudo-tty/test-stdout-read.in
 create mode 100644 test/pseudo-tty/test-stdout-read.js
 create mode 100644 test/pseudo-tty/test-stdout-read.out

diff --git a/doc/api/errors.md b/doc/api/errors.md
index dcb2c77a7bf353..218aaff50ad09c 100644
--- a/doc/api/errors.md
+++ b/doc/api/errors.md
@@ -1555,18 +1555,6 @@ An attempt was made to operate on an already closed socket.
 
 A call was made and the UDP subsystem was not running.
 
-<a id="ERR_STDERR_CLOSE"></a>
-### ERR_STDERR_CLOSE
-
-An attempt was made to close the `process.stderr` stream. By design, Node.js
-does not allow `stdout` or `stderr` streams to be closed by user code.
-
-<a id="ERR_STDOUT_CLOSE"></a>
-### ERR_STDOUT_CLOSE
-
-An attempt was made to close the `process.stdout` stream. By design, Node.js
-does not allow `stdout` or `stderr` streams to be closed by user code.
-
 <a id="ERR_STREAM_CANNOT_PIPE"></a>
 ### ERR_STREAM_CANNOT_PIPE
 
@@ -1952,6 +1940,37 @@ removed: v10.0.0
 
 The `repl` module was unable to parse data from the REPL history file.
 
+
+<a id="ERR_STDERR_CLOSE"></a>
+### ERR_STDERR_CLOSE
+<!-- YAML
+removed: REPLACEME
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/23053
+    description: Rather than emitting an error, `process.stderr.end()` now
+                 only closes the stream side but not the underlying resource,
+                 making this error obsolete.
+-->
+
+An attempt was made to close the `process.stderr` stream. By design, Node.js
+does not allow `stdout` or `stderr` streams to be closed by user code.
+
+<a id="ERR_STDOUT_CLOSE"></a>
+### ERR_STDOUT_CLOSE
+<!-- YAML
+removed: REPLACEME
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/23053
+    description: Rather than emitting an error, `process.stderr.end()` now
+                 only closes the stream side but not the underlying resource,
+                 making this error obsolete.
+-->
+
+An attempt was made to close the `process.stdout` stream. By design, Node.js
+does not allow `stdout` or `stderr` streams to be closed by user code.
+
 <a id="ERR_STREAM_READ_NOT_IMPLEMENTED"></a>
 ### ERR_STREAM_READ_NOT_IMPLEMENTED
 <!-- YAML
diff --git a/doc/api/process.md b/doc/api/process.md
index 07d9b43b55b8c5..0eaeef8ba3e4f6 100644
--- a/doc/api/process.md
+++ b/doc/api/process.md
@@ -1907,9 +1907,7 @@ important ways:
 
 1. They are used internally by [`console.log()`][] and [`console.error()`][],
    respectively.
-2. They cannot be closed ([`end()`][] will throw).
-3. They will never emit the [`'finish'`][] event.
-4. Writes may be synchronous depending on what the stream is connected to
+2. Writes may be synchronous depending on what the stream is connected to
    and whether the system is Windows or POSIX:
    - Files: *synchronous* on Windows and POSIX
    - TTYs (Terminals): *asynchronous* on Windows, *synchronous* on POSIX
@@ -2134,7 +2132,6 @@ cases:
   code will be `128` + `6`, or `134`.
 
 [`'exit'`]: #process_event_exit
-[`'finish'`]: stream.html#stream_event_finish
 [`'message'`]: child_process.html#child_process_event_message
 [`'rejectionHandled'`]: #process_event_rejectionhandled
 [`'uncaughtException'`]: #process_event_uncaughtexception
@@ -2148,7 +2145,6 @@ cases:
 [`console.error()`]: console.html#console_console_error_data_args
 [`console.log()`]: console.html#console_console_log_data_args
 [`domain`]: domain.html
-[`end()`]: stream.html#stream_writable_end_chunk_encoding_callback
 [`net.Server`]: net.html#net_class_net_server
 [`net.Socket`]: net.html#net_class_net_socket
 [`NODE_OPTIONS`]: cli.html#cli_node_options_options
diff --git a/lib/internal/errors.js b/lib/internal/errors.js
index 6153f7e2994605..656c7e2c6c5a5b 100644
--- a/lib/internal/errors.js
+++ b/lib/internal/errors.js
@@ -817,8 +817,6 @@ E('ERR_SOCKET_BUFFER_SIZE',
 E('ERR_SOCKET_CANNOT_SEND', 'Unable to send data', Error);
 E('ERR_SOCKET_CLOSED', 'Socket is closed', Error);
 E('ERR_SOCKET_DGRAM_NOT_RUNNING', 'Not running', Error);
-E('ERR_STDERR_CLOSE', 'process.stderr cannot be closed', Error);
-E('ERR_STDOUT_CLOSE', 'process.stdout cannot be closed', Error);
 E('ERR_STREAM_CANNOT_PIPE', 'Cannot pipe, not readable', Error);
 E('ERR_STREAM_DESTROYED', 'Cannot call %s after a stream was destroyed', Error);
 E('ERR_STREAM_NULL_VALUES', 'May not write null values to stream', TypeError);
diff --git a/lib/internal/process/stdio.js b/lib/internal/process/stdio.js
index e4207a1aa768bd..ca09f4ff821a00 100644
--- a/lib/internal/process/stdio.js
+++ b/lib/internal/process/stdio.js
@@ -1,8 +1,6 @@
 'use strict';
 
 const {
-  ERR_STDERR_CLOSE,
-  ERR_STDOUT_CLOSE,
   ERR_UNKNOWN_STDIN_TYPE,
   ERR_UNKNOWN_STREAM_TYPE
 } = require('internal/errors').codes;
@@ -10,6 +8,8 @@ const {
 exports.setupProcessStdio = setupProcessStdio;
 exports.getMainThreadStdio = getMainThreadStdio;
 
+function dummyDestroy(err, cb) { cb(err); }
+
 function getMainThreadStdio() {
   var stdin;
   var stdout;
@@ -19,11 +19,8 @@ function getMainThreadStdio() {
     if (stdout) return stdout;
     stdout = createWritableStdioStream(1);
     stdout.destroySoon = stdout.destroy;
-    stdout._destroy = function(er, cb) {
-      // Avoid errors if we already emitted
-      er = er || new ERR_STDOUT_CLOSE();
-      cb(er);
-    };
+    // Override _destroy so that the fd is never actually closed.
+    stdout._destroy = dummyDestroy;
     if (stdout.isTTY) {
       process.on('SIGWINCH', () => stdout._refreshSize());
     }
@@ -34,11 +31,8 @@ function getMainThreadStdio() {
     if (stderr) return stderr;
     stderr = createWritableStdioStream(2);
     stderr.destroySoon = stderr.destroy;
-    stderr._destroy = function(er, cb) {
-      // Avoid errors if we already emitted
-      er = er || new ERR_STDERR_CLOSE();
-      cb(er);
-    };
+    // Override _destroy so that the fd is never actually closed.
+    stdout._destroy = dummyDestroy;
     if (stderr.isTTY) {
       process.on('SIGWINCH', () => stderr._refreshSize());
     }
diff --git a/test/parallel/test-stdout-cannot-be-closed-child-process-pipe.js b/test/parallel/test-stdout-cannot-be-closed-child-process-pipe.js
index d19b522e290ba9..7cd4b90c008a2f 100644
--- a/test/parallel/test-stdout-cannot-be-closed-child-process-pipe.js
+++ b/test/parallel/test-stdout-cannot-be-closed-child-process-pipe.js
@@ -24,9 +24,9 @@ function parent() {
   });
 
   child.on('close', function(code, signal) {
-    assert(code);
+    assert.strictEqual(code, 0);
+    assert.strictEqual(err, '');
     assert.strictEqual(out, 'foo');
-    assert(/process\.stdout cannot be closed/.test(err));
     console.log('ok');
   });
 }
diff --git a/test/pseudo-tty/test-stdout-read.in b/test/pseudo-tty/test-stdout-read.in
new file mode 100644
index 00000000000000..10ddd6d257e013
--- /dev/null
+++ b/test/pseudo-tty/test-stdout-read.in
@@ -0,0 +1 @@
+Hello!
diff --git a/test/pseudo-tty/test-stdout-read.js b/test/pseudo-tty/test-stdout-read.js
new file mode 100644
index 00000000000000..90f017ed77b7be
--- /dev/null
+++ b/test/pseudo-tty/test-stdout-read.js
@@ -0,0 +1,3 @@
+'use strict';
+const common = require('../common');
+process.stderr.on('data', common.mustCall(console.log));
diff --git a/test/pseudo-tty/test-stdout-read.out b/test/pseudo-tty/test-stdout-read.out
new file mode 100644
index 00000000000000..3b7fda223d0e6c
--- /dev/null
+++ b/test/pseudo-tty/test-stdout-read.out
@@ -0,0 +1 @@
+<Buffer 48 65 6c 6c 6f 21 0a>