[8.x] tls: fix legacy SecurePair clienthello race window #26452
Closed
Conversation
This file contains 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
This seems to have been broken ever since its introduction 5 years ago in commit 75ea11f ("tls: introduce asynchronous `newSession`") and no one complained but that's not going to stop me from fixing it anyway because otherwise I can't write a regression test for issue nodejs#26428. Refs: nodejs#26428
There is a time window between the first and the last step of processing the clienthello event and the SecurePair may have been destroyed during that interval. Fixes: nodejs#26428
nodejs-github-bot
added
c++
bnoordhuis
commented
Mar 5, 2019
| sessionCb(); | ||
| server.close(); | ||
| }, 100); | ||
| } |
There was a problem hiding this comment.
Apropos the somewhat uncommon style in this file, it's a mashup of two (fairly old and crufty) tests: test-tls-async-cb-after-socket-end.js and test-tls-securepair-server.js. I didn't want to deviate too much from the style used in those files.
sam-github
approved these changes
Mar 5, 2019
mscdex
reviewed
Mar 5, 2019
| setTimeout(function() { | ||
| sessionCb(); | ||
| server.close(); | ||
| }, 100); |
There was a problem hiding this comment.
Does this need common.platformTimeout() ?
There was a problem hiding this comment.
It's not used in test-tls-async-cb-after-socket-end.js either so I'm inclined to say it's not.
jasnell
approved these changes
Mar 5, 2019
BridgeAR
added
author ready
BridgeAR
changed the title
|
CI failures on AIX and Windows appear unrelated. |
BethGriggs
approved these changes
Mar 20, 2019
BethGriggs
pushed a commit
that referenced
this pull request
Mar 21, 2019
This seems to have been broken ever since its introduction 5 years ago in commit 75ea11f ("tls: introduce asynchronous `newSession`") and no one complained but that's not going to stop me from fixing it anyway because otherwise I can't write a regression test for issue #26428. Refs: #26428 PR-URL: #26452 Fixes: #26428 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Beth Griggs <[email protected]>
BethGriggs
pushed a commit
that referenced
this pull request
Mar 21, 2019
There is a time window between the first and the last step of processing the clienthello event and the SecurePair may have been destroyed during that interval. Fixes: #26428 PR-URL: #26452 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Beth Griggs <[email protected]>
|
Landed on v8.x-staging |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
There is a time window between the first and the last step of processing
the clienthello event and the SecurePair may have been destroyed during
that interval.
Fixes: #26428
The first commit fixes a bug that's been around for five years.
CI: https://ci.nodejs.org/job/node-test-pull-request/21215/