Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

TLS: improve compliance with shutdown standard, remove hacks #36111

Closed
wants to merge 2 commits into from
Closed

TLS: improve compliance with shutdown standard, remove hacks #36111

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f199b1c
TLS: improve handling of shutdown
vtjnash Nov 13, 2020
a18156c
Apply suggestions from code review
vtjnash Apr 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions lib/_http_client.js
View file
Open in desktop
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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this change needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the comment says, this should be a no-op. The underlying socket is not supposed to deliver a read event after a close event, but TLS would try to do so (while racing to destroy it first). Looking back at the commit history, it was added as a hack to work around this issue, but didn't really fix it, as evidenced by flaky CI.

Copy link
Member

@lpinca lpinca Jul 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is buffered data (for example if there is buffered data and the socket is destroyed) shouldn't it be read? We do something similar in ws to ensure that all received data is actually used and delivered to the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no data buffered here, that wouldn't make any sense as the http protocol doesn't allow for more http data to be included after the http message is ended.

Copy link
Member

@lpinca lpinca Jul 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you know that the http message ended? Isn't this only a listener for the 'close' event of the socket? That 'close' event can be emitted in the middle of a HTTP request/response.

Copy link
Member

@lpinca lpinca Aug 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change does not affect ws luckily. I've only mentioned it because the same pattern used here (to read buffered data after 'close') is also used by ws as per #36111 (comment).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you have an example for why you added those lines to ws? I see only the commit where you added it:
websockets/ws@6046a28. Note that the documentation for 'error' also indicates that this call to .read should not trigger any callbacks to occur ("no further events other than 'close' should be emitted"), so by adding that comment and code there, it seems like you may be causing ws to violate the stream.Readable interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also seems like you had previously changed it to explicitly have the opposite behavior in websockets/ws@5d8ab0e, but was changed back later during a general code refactoring? From the comment description, it appears to me to say that code was intending to be be part of the 'error' handler (before that handler called destroy), and should be triggering the 'readable' callback, not the 'read' callback.

But why would the underlying socket be doing an active 'read', thus causing it to encounter an 'error', while data was already pending in the buffer? That seems like it would be disregarding of flow control, so wouldn't actually happen in a correctly implemented stream.

Copy link
Member

@lpinca lpinca Aug 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK calling readable.read() is the only way to pull out buffered data from a readable stream after it is destroyed and in my opinion there must be a way to do that. The fact that doing so causes an additional 'data' event is a side effect that is actually very useful.

Did you have an example for why you added those lines to ws?

See #36111 (comment) and #36111 (comment). The user can call ws.terminate() and destroy the socket. When that happens there might be buffered data in the socket. That data might contain many vaid WebSocket frames that we do not want to drop.

websockets/ws@5d8ab0e is still in place, see https://github.com/websockets/ws/blob/8.0.0/lib/websocket.js#L946. Any data after the close frame must be discarded per specification.

Anyway this is not about ws. ws is not affected by this change. This is about discarding data that was previously not discarded.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't fully followed this discussion but #39639 might be of relevance.


// 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
View file
Open in desktop
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;
}
}
jasnell marked this conversation as resolved.
Show resolved Hide resolved

// 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
View file
Open in desktop
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