Fix: Prevent RELIABLE_RESET offset increase by using correct sentinel#6160
Fix: Prevent RELIABLE_RESET offset increase by using correct sentinel#6160basavaraj-sm05 wants to merge 4 commits into
Conversation
guhetier
left a comment
There was a problem hiding this comment.
Thanks for the PR.
Some nitpick on the comment, but looks good to me otherwise.
Adding a test for this corner case would be ideal.
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (40.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #6160 +/- ##
==========================================
- Coverage 85.85% 85.00% -0.86%
==========================================
Files 60 60
Lines 18974 18978 +4
==========================================
- Hits 16291 16133 -158
- Misses 2683 2845 +162 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…remum queue is full
|
@microsoft-github-policy-service agree company="Digiscrypt Technologies" |
|
|
||
| // | ||
| // Set ReliableOffset to 0 — this is the corner case. The old code used | ||
| // RecvMaxLength == 0 as "first frame not yet received", which means |
There was a problem hiding this comment.
Let's avoid comments referring to the past state of the code, that won't be relevant for future readers.
As far as they are concerned, this test validates the corner case where the reliable offset is 0.
There was a problem hiding this comment.
Fair enough! Removed the references to the old behavior and kept it focused on the corner case itself.
| StreamReliableResetZeroOffset Context; | ||
|
|
||
| // | ||
| // We need to send at least 1 byte so that QueuedSendOffset > 0 and |
There was a problem hiding this comment.
nit: for the sake of it, I'd send more than 1 byte, so there is data after the reliable reset offset.
There was a problem hiding this comment.
Good nit! Bumped it to 16 bytes so there's actually some data sitting after the reliable reset offset, which makes the test feel more realistic.
…nges, send more bytes, drop unnecessary sleep
Description
Fixes a bug where a peer could bypass the strictly-decreasing offset rule for
RELIABLE_RESETframes.Right now,
QuicStreamProcessReliableResetFramechecksStream->RecvMaxLength == 0to see if this is the firstRELIABLE_RESETwe've received. But0is actually a validReliableOffsetvalue!If a peer sends a
RELIABLE_RESETwith offset0, we updateRecvMaxLengthto0. If they then send another one with offset5000, the== 0check still passes, and we end up increasing the offset to5000— which violates the spec.So instead of checking against
0, this PR just uses theRemoteCloseResetReliableflag to check if we've seen a reliable reset frame yet.Testing
Existing reliable reset tests pass fine since standard behavior isn't affected.
We might want to add a quick test case that sends a
RELIABLE_RESET(offset=0)followed by aRELIABLE_RESET(offset=5000)to verify it gets rejected properly now.Documentation
None needed, just an internal state fix.