Skip to content

Commit

Permalink
process: allow reading from stdout/stderr sockets
Browse files Browse the repository at this point in the history
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

PR-URL: #23053
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
addaleax authored and targos committed Oct 7, 2018
1 parent d7031df commit 5a30674
Show file tree
Hide file tree
Showing 8 changed files with 45 additions and 33 deletions.
43 changes: 31 additions & 12 deletions doc/api/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down
6 changes: 1 addition & 5 deletions doc/api/process.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
2 changes: 0 additions & 2 deletions lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
18 changes: 6 additions & 12 deletions lib/internal/process/stdio.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
'use strict';

const {
ERR_STDERR_CLOSE,
ERR_STDOUT_CLOSE,
ERR_UNKNOWN_STDIN_TYPE,
ERR_UNKNOWN_STREAM_TYPE
} = require('internal/errors').codes;

exports.setupProcessStdio = setupProcessStdio;
exports.getMainThreadStdio = getMainThreadStdio;

function dummyDestroy(err, cb) { cb(err); }

function getMainThreadStdio() {
var stdin;
var stdout;
Expand All @@ -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());
}
Expand All @@ -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());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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');
});
}
1 change: 1 addition & 0 deletions test/pseudo-tty/test-stdout-read.in
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Hello!
3 changes: 3 additions & 0 deletions test/pseudo-tty/test-stdout-read.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
'use strict';
const common = require('../common');
process.stderr.on('data', common.mustCall(console.log));
1 change: 1 addition & 0 deletions test/pseudo-tty/test-stdout-read.out
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<Buffer 48 65 6c 6c 6f 21 0a>

0 comments on commit 5a30674

Please sign in to comment.