tls: fix bugs of double TLS#48796
Closed
tls: fix bugs of double TLS #48796
Uh oh!
There was an error while loading. Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Fixs two issues in `TLSWrap`, one of them is reported in nodejs#30896. 1. `TLSWrap` has exactly one `StreamListener`, however, that `StreamListener` can be replaced. We have not been rigorous enough here: if an active write has not been finished before the transition, the finish callback of it will be wrongly fired the successor `StreamListener`. 2. A `TLSWrap` does not allow more than one active write, as checked in the assertion about current_write in `TLSWrap::DoWrite()`. However, when users make use of an existing `tls.TLSSocket` to establish double TLS, by either tls.connect({socket: tlssock}) or tlsServer.emit('connection', tlssock) we have both of the user provided `tls.TLSSocket`, tlssock and a brand new created `TLSWrap` writing to the `TLSWrap` bound to tlssock, which easily violates the constranint because two writers have no idea of each other. The design of the fix is: when a `TLSWrap` is created on top of a user provided socket, do not send any data to the socket until all existing writes of the socket are done and ensure registered callbacks of those writes can be fired.
Collaborator
nodejs-github-bot commented Jul 16, 2023
Review requested:
|
nodejs-github-bot added c++ Format code.
github-actionsbot removed the request-ci Collaborator
nodejs-github-bot commented Jul 16, 2023
mscdex reviewed Jul 16, 2023
Uh oh!
There was an error while loading. Please reload this page.
25 tasks
Remove unnecessary saving of `this` for arrow function.
mscdex reviewed Jul 17, 2023
Uh oh!
There was an error while loading. Please reload this page.
26 tasks
ContributorAuthor
ywave620 commented Jul 18, 2023
leftover processes found in the github hooked CI But it seems like these failures have nothing to do with this PR. Could somebody rerun the CI? |
25 tasks
ContributorAuthor
ywave620 commented Jul 20, 2023
cc @nodejs/crypto |
github-actionsbot removed the request-ci Collaborator
nodejs-github-bot commented Jul 20, 2023
github-actionsbot removed the request-ci Collaborator
nodejs-github-bot commented Jul 21, 2023
This was referenced Jul 22, 2023
Collaborator
nodejs-github-bot commented Jul 23, 2023
This was referenced Jul 24, 2023
Collaborator
nodejs-github-bot commented Jul 25, 2023
This was referenced Jul 26, 2023
ShogunPanda approved these changes Jul 27, 2023
19 tasks
jasnell reviewed Jul 28, 2023
| StreamBase* stream, | ||
| SecureContext* sc); | ||
| SecureContext* sc, | ||
| bool stream_has_active_write); |
Member
There was a problem hiding this comment.
Non-blocking nit: I'm generally not a fan of bool arguments to methods, largely because just seeing the true or false a the callsite tends not to be descriptive enough to let folks know what the impact is. I certainly won't block this change because of it, but I'd prefer a enum arg here instead.
jasnell approved these changes Jul 28, 2023
Member
mhdawson commented Jul 28, 2023
Landed in 662fe0be9d8e |
Member
mhdawson commented Jul 28, 2023
23 tasks
ContributorAuthor
ywave620 commented Jul 29, 2023
21 tasks
ContributorAuthor
ywave620 commented Jul 30, 2023
Member
mhdawson commented Aug 2, 2023
@ywave620 many thanks |
Member
mhdawson commented Aug 2, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixs two issues in
TLSWrap, one of them is reported in #30896 (a 3 years log bug ...).TLSWraphas exactly oneStreamListener, however, thatStreamListenercan be replaced. We have not been rigorous enough here: if an active write has not been finished before the transition, the finish callback of it will be wrongly fired the successorStreamListener.A
TLSWrapdoes not allow more than one active write, as checked in the assertion about current_write inTLSWrap::DoWrite().However, when users make use of an existing
tls.TLSSocketto establish double TLS, byeither
or
we have both of the user provided
tls.TLSSocket, tlssock and a brand new createdTLSWrapwriting to theTLSWrapbound to tlssock, which easily violates the constranint because two writers have no idea of each other.The design of the fix is:
when a
TLSWrapis created on top of a user provided socket, do not send any data to the socket until all existing writes of the socket are done and ensure registered callbacks of those writes can be fired.