From fdac9bd5a362c0e4cc712bb665c91f89491e05fa Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Tue, 3 Jul 2018 18:35:59 +0200 Subject: [PATCH] tty: make _read throw ERR_TTY_WRITABLE_NOT_READABLE This change avoid an 'read ENOTCONN' error introduced by libuv 1.20.0 when trying to read from a TTY WriteStream. Instead, we are throwing a more actionable ERR_TTY_WRITABLE_NOT_READABLE. Fixes: https://github.com/nodejs/node/issues/21203 --- doc/api/errors.md | 6 ++++++ lib/internal/errors.js | 3 +++ lib/tty.js | 13 ++++++++++++- .../test-tty-write-stream-resume-crash.js | 17 +++++++++++++++++ .../test-tty-write-stream-resume-crash.out | 0 5 files changed, 38 insertions(+), 1 deletion(-) create mode 100644 test/pseudo-tty/test-tty-write-stream-resume-crash.js create mode 100644 test/pseudo-tty/test-tty-write-stream-resume-crash.out diff --git a/doc/api/errors.md b/doc/api/errors.md index 91ec811b2af194..8eee9c9eaf9652 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -1684,6 +1684,12 @@ A `Transform` stream finished with data still in the write buffer. The initialization of a TTY failed due to a system error. + +### ERR_TTY_WRITABLE_NOT_READABLE + +This `Error` is thrown when a read is attempted on a TTY `WriteStream`, +such as `process.stdout.on('data')`. + ### ERR_UNCAUGHT_EXCEPTION_CAPTURE_ALREADY_SET diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 862ec2dbe62b67..2d06f8f9deb96d 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -825,6 +825,9 @@ E('ERR_TRANSFORM_ALREADY_TRANSFORMING', E('ERR_TRANSFORM_WITH_LENGTH_0', 'Calling transform done when writableState.length != 0', Error); E('ERR_TTY_INIT_FAILED', 'TTY initialization failed', SystemError); +E('ERR_TTY_WRITABLE_NOT_READABLE', + 'The Writable side of a TTY is not Readable', + Error); E('ERR_UNCAUGHT_EXCEPTION_CAPTURE_ALREADY_SET', '`process.setupUncaughtExceptionCapture()` was called while a capture ' + 'callback was already active', diff --git a/lib/tty.js b/lib/tty.js index b36adf2534c326..5e0e528c7e4593 100644 --- a/lib/tty.js +++ b/lib/tty.js @@ -25,7 +25,11 @@ const { inherits, _extend } = require('util'); const net = require('net'); const { TTY, isTTY } = process.binding('tty_wrap'); const errors = require('internal/errors'); -const { ERR_INVALID_FD, ERR_TTY_INIT_FAILED } = errors.codes; +const { + ERR_INVALID_FD, + ERR_TTY_INIT_FAILED, + ERR_TTY_WRITABLE_NOT_READABLE +} = errors.codes; const { getColorDepth } = require('internal/tty'); // Lazy loaded for startup performance. @@ -122,6 +126,13 @@ WriteStream.prototype._refreshSize = function() { } }; +// A WriteStream is not readable from, so _read become a no-op. +// this method could still be called because net.Socket() +// is a duplex +WriteStream.prototype._read = function() { + this.destroy(new ERR_TTY_WRITABLE_NOT_READABLE()); +}; + // Backwards-compat WriteStream.prototype.cursorTo = function(x, y) { if (readline === undefined) readline = require('readline'); diff --git a/test/pseudo-tty/test-tty-write-stream-resume-crash.js b/test/pseudo-tty/test-tty-write-stream-resume-crash.js new file mode 100644 index 00000000000000..12ec1df45ad598 --- /dev/null +++ b/test/pseudo-tty/test-tty-write-stream-resume-crash.js @@ -0,0 +1,17 @@ +'use strict'; + +const common = require('../common'); +const { WriteStream } = require('tty'); +const fd = common.getTTYfd(); + +// Calling resume on a tty.WriteStream should be a no-op +// Ref: https://github.com/nodejs/node/issues/21203 + +const stream = new WriteStream(fd); +stream.resume(); + +stream.on('error', common.expectsError({ + code: 'ERR_TTY_WRITABLE_NOT_READABLE', + type: Error, + message: 'The Writable side of a TTY is not Readable' +})); diff --git a/test/pseudo-tty/test-tty-write-stream-resume-crash.out b/test/pseudo-tty/test-tty-write-stream-resume-crash.out new file mode 100644 index 00000000000000..e69de29bb2d1d6