process: allow reading from stdout/stderr sockets

addaleax · BethGriggs · commit ea5628e77ae1 · 2019-03-19T22:55:13.000Z
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: #21203 Backport-PR-URL: #25351 PR-URL: #23053 Reviewed-By: James M Snell <jasnell@gmail.com>
diff --git a/doc/api/errors.md b/doc/api/errors.md
@@ -1172,12 +1172,32 @@ A call was made and the UDP subsystem was not running.
 <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.
 
diff --git a/doc/api/process.md b/doc/api/process.md
@@ -1703,9 +1703,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
@@ -1925,7 +1923,6 @@ cases:
 
 
 [`'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
@@ -1936,7 +1933,6 @@ cases:
 [`EventEmitter`]: events.html#events_class_eventemitter
 [`console.error()`]: console.html#console_console_error_data_args
 [`console.log()`]: console.html#console_console_log_data_args
-[`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
 [`process.argv`]: #process_process_argv
diff --git a/lib/internal/errors.js b/lib/internal/errors.js
@@ -427,8 +427,6 @@ E('ERR_SOCKET_BUFFER_SIZE',
   (reason) => `Could not get or set buffer size: ${reason}`);
 E('ERR_SOCKET_CANNOT_SEND', 'Unable to send data');
 E('ERR_SOCKET_DGRAM_NOT_RUNNING', 'Not running');
-E('ERR_STDERR_CLOSE', 'process.stderr cannot be closed');
-E('ERR_STDOUT_CLOSE', 'process.stdout cannot be closed');
 E('ERR_UNKNOWN_BUILTIN_MODULE', (id) => `No such built-in module: ${id}`);
 E('ERR_UNKNOWN_FILE_EXTENSION', 'Unknown file extension: %s');
 E('ERR_UNKNOWN_MODULE_FORMAT', 'Unknown module format: %s');
diff --git a/lib/internal/process/stdio.js b/lib/internal/process/stdio.js
@@ -1,9 +1,11 @@
 'use strict';
 
-const errors = require('internal/errors');
+const errors = require('internal/errors').codes;
 
 exports.setup = setupStdio;
 
+function dummyDestroy(err, cb) { cb(err); }
+
 function setupStdio() {
   var stdin;
   var stdout;
@@ -13,11 +15,8 @@ function setupStdio() {
     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 errors.Error('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());
     }
@@ -28,11 +27,8 @@ function setupStdio() {
     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 errors.Error('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
@@ -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
@@ -0,0 +1 @@
+Hello!
diff --git a/test/pseudo-tty/test-stdout-read.js 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
@@ -0,0 +1 @@
+<Buffer 48 65 6c 6c 6f 21 0a>

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+'use strict';`
	`2`	`+const common = require('../common');`
	`3`	`+process.stderr.on('data', common.mustCall(console.log));`