Ignore data appearing after close_notify #1950
Merged
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
Benchmark resultsInstruction countsSignificant differencesThere are no significant instruction count differences Other differencesClick to expand
Wall-timeSignificant differencesThere are no significant wall-time differences Other differencesClick to expand
Additional informationCheckout details:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1950 +/- ##
=======================================
Coverage 95.48% 95.48%
=======================================
Files 86 86
Lines 18654 18664 +10
=======================================
+ Hits 17811 17821 +10
Misses 843 843 ☔ View full report in Codecov by Sentry. |
|
Hm, the duplication of this code for |
Was just working on that. Probably need to add a test. Any suggestions on which test to crib off of/would you be able to take care of that? |
|
hmm, is error the correct thing to do? |
Sure! |
Do you have references to the appropriate sections? (See #1951 which handles that cleanly for application data.) |
|
I guess this is a question of what is meant by "data". It could be application data (my interpretation in #1951), or more generally data that appears on the wire. Probably the latter makes the most sense, in retrospect? |
|
my interpretation is that data = any bytes after close_notify has been received, regardless of if its a valid ApplicationData frame, or random bytes, as you are not supposed to try and read and of it... i think. |
|
Yeah, I think dropping any data that was read after a CloseNotify probably makes sense. |
|
we could take a peek at what other implementations (openssl?) deal with this, maybe theres prior art. |
|
im not sure im looking at the right place, but https://github.com/openssl/openssl/blob/master/ssl/record/rec_layer_s3.c#L970 seems to only deal with application data after close notify, but that seems to send fatal alert if that happens... |
|
@ctz do you want to pronounce on whether the case of junk after CloseNotify is |
|
I think we should ignore the data after the close notify, and return an EOF as if it didn't exist. I'll update the test to that effect in the morning. @VladimirBramstedt are you in a position to see how your application behaves with this branch as it currently stands? |
djc
commented
May 15, 2024
djc
commented
May 16, 2024
djc
changed the title
|
LGTM. I think we'll probably want a release for this? Maybe also for 0.21/0.22? |
|
tested this out, seems to help, no more excessive wakeups. 👍 |
|
Great -- thank you for checking that! |
This was referenced May 16, 2024
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.
Intended to fix rustls/tokio-rustls#72.
cc @quininer @VladimirBramstedt