-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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.
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
tls: fix bugs of double TLS #48796
tls: fix bugs of double TLS #48796
Conversation
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.
Review requested:
|
Format code.
Remove unnecessary saving of `this` for arrow function.
test/parallel/test-socket-writes-before-paased-to-tls-socket.js
Outdated
Show resolved
Hide resolved
Fix typo
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? |
cc @nodejs/crypto |
@@ -136,7 +136,8 @@ class TLSWrap : public AsyncWrap, | |||
v8::Local<v8::Object> obj, | |||
Kind kind, | |||
StreamBase* stream, | |||
SecureContext* sc); | |||
SecureContext* sc, | |||
bool stream_has_active_write); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Landed in 662fe0be9d8e |
@ywave620 many thanks |
Fixs two issues in
TLSWrap
, one of them is reported in #30896 (a 3 years log bug ...).TLSWrap
has exactly oneStreamListener
, however, thatStreamListener
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 successorStreamListener
.A
TLSWrap
does 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.TLSSocket
to establish double TLS, byeither
or
we have both of the user provided
tls.TLSSocket
, tlssock and a brand new createdTLSWrap
writing to theTLSWrap
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.