From 067b9810373b3a67bf2f6d0b4bcb207c20637b3e Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 24 Sep 2018 11:47:13 +0200 Subject: [PATCH 1/4] tools: allow input for TTY tests Since faking TTY input is not otherwise fake-able, we need support in the test runner for it. PR-URL: https://github.com/nodejs/node/pull/23053 Reviewed-By: James M Snell --- test/pseudo-tty/testcfg.py | 12 +++++++++--- tools/test.py | 13 ++++++++++++- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/test/pseudo-tty/testcfg.py b/test/pseudo-tty/testcfg.py index 40396db247e279..c6c93c13b98340 100644 --- a/test/pseudo-tty/testcfg.py +++ b/test/pseudo-tty/testcfg.py @@ -35,10 +35,11 @@ class TTYTestCase(test.TestCase): - def __init__(self, path, file, expected, arch, mode, context, config): + def __init__(self, path, file, expected, input, arch, mode, context, config): super(TTYTestCase, self).__init__(context, path, arch, mode) self.file = file self.expected = expected + self.input = input self.config = config self.arch = arch self.mode = mode @@ -104,12 +105,16 @@ def GetSource(self): + open(self.expected).read()) def RunCommand(self, command, env): + input = None + if self.input is not None and exists(self.input): + input = open(self.input).read() full_command = self.context.processor(command) output = test.Execute(full_command, self.context, self.context.GetTimeout(self.mode), env, - True) + faketty=True, + input=input) self.Cleanup() return test.TestOutput(self, full_command, @@ -140,11 +145,12 @@ def ListTests(self, current_path, path, arch, mode): if self.Contains(path, test): file_prefix = join(self.root, reduce(join, test[1:], "")) file_path = file_prefix + ".js" + input_path = file_prefix + ".in" output_path = file_prefix + ".out" if not exists(output_path): raise Exception("Could not find %s" % output_path) result.append(TTYTestCase(test, file_path, output_path, - arch, mode, self.context, self)) + input_path, arch, mode, self.context, self)) return result def GetBuildRequirements(self): diff --git a/tools/test.py b/tools/test.py index dafcc3d2ad4122..a51b475b1f23cd 100755 --- a/tools/test.py +++ b/tools/test.py @@ -717,12 +717,23 @@ def CheckedUnlink(name): PrintError("os.unlink() " + str(e)) break -def Execute(args, context, timeout=None, env={}, faketty=False, disable_core_files=False): +def Execute(args, context, timeout=None, env={}, faketty=False, disable_core_files=False, input=None): if faketty: import pty (out_master, fd_out) = pty.openpty() fd_in = fd_err = fd_out pty_out = out_master + + if input is not None: + # Before writing input data, disable echo so the input doesn't show + # up as part of the output. + import termios + attr = termios.tcgetattr(fd_in) + attr[3] = attr[3] & ~termios.ECHO + termios.tcsetattr(fd_in, termios.TCSADRAIN, attr) + + os.write(pty_out, input) + os.write(pty_out, '\x04') # End-of-file marker (Ctrl+D) else: (fd_out, outname) = tempfile.mkstemp() (fd_err, errname) = tempfile.mkstemp() From 93d10bc5cd81bce389a0ae3d676116cfa930bfde Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 24 Sep 2018 11:51:14 +0200 Subject: [PATCH 2/4] 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 --- doc/api/errors.md | 20 +++++++++++++++++++ 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, 36 insertions(+), 19 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 53ef8395ac7065..59222b1aa82c10 100755 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -1147,12 +1147,32 @@ A call was made and the UDP subsystem was not running. ### 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. ### 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. diff --git a/doc/api/process.md b/doc/api/process.md index 8279f596a51fc1..e7020249c3a267 100644 --- 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 @@ -1937,6 +1934,7 @@ cases: [`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 +[`domain`]: domain.html [`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 index e878766f680605..c260b547b8a7e8 100644 --- 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 index 7faef381f895bf..46641b5871646f 100644 --- 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 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 @@ + From 02bea9911e95d9922b2fed100d84f09135efb50c Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 24 Sep 2018 12:08:46 +0200 Subject: [PATCH 3/4] test: add stdin writable regression test Make sure that `process.stdin.write()`, and in particular ending the stream, works. PR-URL: https://github.com/nodejs/node/pull/23053 Reviewed-By: James M Snell --- test/pseudo-tty/test-stdin-write.js | 3 +++ test/pseudo-tty/test-stdin-write.out | 1 + 2 files changed, 4 insertions(+) create mode 100644 test/pseudo-tty/test-stdin-write.js create mode 100644 test/pseudo-tty/test-stdin-write.out diff --git a/test/pseudo-tty/test-stdin-write.js b/test/pseudo-tty/test-stdin-write.js new file mode 100644 index 00000000000000..39091f20bbb745 --- /dev/null +++ b/test/pseudo-tty/test-stdin-write.js @@ -0,0 +1,3 @@ +'use strict'; +require('../common'); +process.stdin.end('foobar\n'); diff --git a/test/pseudo-tty/test-stdin-write.out b/test/pseudo-tty/test-stdin-write.out new file mode 100644 index 00000000000000..323fae03f4606e --- /dev/null +++ b/test/pseudo-tty/test-stdin-write.out @@ -0,0 +1 @@ +foobar From 8d6637f6c0f32fe8d6b7b1d8bb964ac21a30f281 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Mon, 17 Sep 2018 13:21:47 +0200 Subject: [PATCH 4/4] test: add process.stdin.end() TTY regression test PR-URL: https://github.com/nodejs/node/pull/23051 Fixes: https://github.com/nodejs/node/issues/22814 Reviewed-By: Matteo Collina Reviewed-By: Colin Ihrig Reviewed-By: Trivikram Kamat Reviewed-By: Luigi Pinca Reviewed-By: George Adams Reviewed-By: James M Snell --- test/pseudo-tty/test-tty-stdin-call-end.js | 8 ++++++++ test/pseudo-tty/test-tty-stdin-call-end.out | 0 2 files changed, 8 insertions(+) create mode 100644 test/pseudo-tty/test-tty-stdin-call-end.js create mode 100644 test/pseudo-tty/test-tty-stdin-call-end.out diff --git a/test/pseudo-tty/test-tty-stdin-call-end.js b/test/pseudo-tty/test-tty-stdin-call-end.js new file mode 100644 index 00000000000000..e3c3fd9af469ba --- /dev/null +++ b/test/pseudo-tty/test-tty-stdin-call-end.js @@ -0,0 +1,8 @@ +'use strict'; + +require('../common'); + +// This tests verifies that process.stdin.end() does not +// crash the process with ENOTCONN + +process.stdin.end(); diff --git a/test/pseudo-tty/test-tty-stdin-call-end.out b/test/pseudo-tty/test-tty-stdin-call-end.out new file mode 100644 index 00000000000000..e69de29bb2d1d6