Skip to content

Commit

Permalink
tls: improve handling of shutdown
Browse files Browse the repository at this point in the history
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.

PR-URL: #36111
Fixes: #35946
Refs: libuv/libuv#3036
Refs: #35904
Co-authored-by: Momtchil Momtchev <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
  • Loading branch information
2 people authored and danielleadams committed Feb 1, 2022
1 parent 2c28f86 commit 3b5b893
Show file tree
Hide file tree
Showing 3 changed files with 13 additions and 23 deletions.
5 changes: 0 additions & 5 deletions lib/_http_client.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
20 changes: 6 additions & 14 deletions lib/internal/stream_base_commons.js
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,12 @@ function onStreamRead(arrayBuffer) {
return;
}

// 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.
// Ref: https://github.com/nodejs/node/pull/34375
Expand All @@ -220,20 +226,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`
Expand Down
11 changes: 7 additions & 4 deletions src/crypto/crypto_tls.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -1049,14 +1049,17 @@ 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)
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;
}

Expand Down

0 comments on commit 3b5b893

Please sign in to comment.