From f199b1c0681aec18c64eea90505c4b235204da32 Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Fri, 13 Nov 2020 15:40:04 -0500 Subject: [PATCH 1/2] TLS: improve handling of shutdown RFC 5246 section-7.2.1 requires that the implementation must immediately stop reading from the stream, as it is no longer TLS-encrypted. The underlying stream is permitted to still pump events (and errors) to other users, but those are now unencrypted, so we should not process them here. But therefore, we do not want to stop the underlying stream, as there could be another user of it, but we do need to remove ourselves as a listener. Per TLS v1.2, we should have also destroy the TLS state entirely here (including the writing side), but this was revised in TLS v1.3 to permit the stream to continue to flush output. There appears to be some inconsistencies in the way nodejs handles ownership of the underlying stream, with `TLS.close()` on the write side also calling shutdown on the underlying stream (thus assuming other users of the underlying stream are not permitted), while receiving EOF on the read side leaves the underlying channel open. These inconsistencies are left for a later person to resolve, if the extra functionality is needed (as described in #35904). The current goal here is to the fix the occasional CI exceptions depending on the timing of these kernel messages through the TCP stack. Refs: https://github.com/libuv/libuv/pull/3036 Refs: https://github.com/nodejs/node/issues/35904 Closes: https://github.com/nodejs/node/pull/35946 Co-authored-by: Momtchil Momtchev --- lib/_http_client.js | 5 ----- lib/internal/stream_base_commons.js | 17 +++-------------- src/crypto/crypto_tls.cc | 12 ++++++++---- 3 files changed, 11 insertions(+), 23 deletions(-) diff --git a/lib/_http_client.js b/lib/_http_client.js index 273460693b4561..59c4cac89e9d9d 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -390,11 +390,6 @@ function socketCloseListener() { const req = socket._httpMessage; debug('HTTP socket close'); - // Pull through final chunk, if anything is buffered. - // the ondata function will handle it properly, and this - // is a no-op if no final chunk remains. - socket.read(); - // NOTE: It's important to get parser here, because it could be freed by // the `socketOnData`. const parser = socket.parser; diff --git a/lib/internal/stream_base_commons.js b/lib/internal/stream_base_commons.js index 5254fc1553dd77..b3a4cf62f6b954 100644 --- a/lib/internal/stream_base_commons.js +++ b/lib/internal/stream_base_commons.js @@ -205,6 +205,9 @@ function onStreamRead(arrayBuffer) { return; } + // note: handle should never deliver any more read events after this point. + // (equivalently, it should have called readStop on itself alread). + if (nread !== UV_EOF) { // CallJSOnreadMethod expects the return value to be a buffer. // Ref: https://github.com/nodejs/node/pull/34375 @@ -220,20 +223,6 @@ function onStreamRead(arrayBuffer) { if (stream[kMaybeDestroy]) stream.on('end', stream[kMaybeDestroy]); - // TODO(ronag): Without this `readStop`, `onStreamRead` - // will be called once more (i.e. after Readable.ended) - // on Windows causing a ECONNRESET, failing the - // test-https-truncate test. - if (handle.readStop) { - const err = handle.readStop(); - if (err) { - // CallJSOnreadMethod expects the return value to be a buffer. - // Ref: https://github.com/nodejs/node/pull/34375 - stream.destroy(errnoException(err, 'read')); - return; - } - } - // Push a null to signal the end of data. // Do it before `maybeDestroy` for correct order of events: // `end` -> `close` diff --git a/src/crypto/crypto_tls.cc b/src/crypto/crypto_tls.cc index 8efbdeed1d9fc6..62f149533f32f7 100644 --- a/src/crypto/crypto_tls.cc +++ b/src/crypto/crypto_tls.cc @@ -886,7 +886,7 @@ bool TLSWrap::IsClosing() { int TLSWrap::ReadStart() { Debug(this, "ReadStart()"); - if (underlying_stream() != nullptr) + if (underlying_stream() != nullptr && !eof_) return underlying_stream()->ReadStart(); return 0; } @@ -1049,14 +1049,18 @@ uv_buf_t TLSWrap::OnStreamAlloc(size_t suggested_size) { void TLSWrap::OnStreamRead(ssize_t nread, const uv_buf_t& buf) { Debug(this, "Read %zd bytes from underlying stream", nread); + + // Ignore everything after close_notify (rfc5246#section-7.2.1) + // TODO: unregister our interest in read events + if (eof_) + return; + if (nread < 0) { // Error should be emitted only after all data was read ClearOut(); - // Ignore EOF if received close_notify if (nread == UV_EOF) { - if (eof_) - return; + // underlying stream already should have also called ReadStop on itself eof_ = true; } From a18156c730619bb1229090096f58491fbd5e2f59 Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Thu, 8 Apr 2021 13:16:01 -0400 Subject: [PATCH 2/2] Apply suggestions from code review --- lib/internal/stream_base_commons.js | 7 +++++-- src/crypto/crypto_tls.cc | 1 - 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/internal/stream_base_commons.js b/lib/internal/stream_base_commons.js index b3a4cf62f6b954..f126d66d04b297 100644 --- a/lib/internal/stream_base_commons.js +++ b/lib/internal/stream_base_commons.js @@ -205,8 +205,11 @@ function onStreamRead(arrayBuffer) { return; } - // note: handle should never deliver any more read events after this point. - // (equivalently, it should have called readStop on itself alread). + // After seeing EOF, most streams will be closed permanently, + // and will not deliver any more read events after this point. + // (equivalently, it should have called readStop on itself already). + // Some streams may be reset and explicitly started again with a call + // to readStart, such as TTY. if (nread !== UV_EOF) { // CallJSOnreadMethod expects the return value to be a buffer. diff --git a/src/crypto/crypto_tls.cc b/src/crypto/crypto_tls.cc index 62f149533f32f7..cfe760adb3af1d 100644 --- a/src/crypto/crypto_tls.cc +++ b/src/crypto/crypto_tls.cc @@ -1051,7 +1051,6 @@ void TLSWrap::OnStreamRead(ssize_t nread, const uv_buf_t& buf) { Debug(this, "Read %zd bytes from underlying stream", nread); // Ignore everything after close_notify (rfc5246#section-7.2.1) - // TODO: unregister our interest in read events if (eof_) return;