fix(recv_buffer): skip already-written bytes on overlapping writes by MarkedMuichiro · Pull Request #5900 · microsoft/msquic

MarkedMuichiro · 2026-03-27T23:59:10Z

Problem

QuicRecvBufferCopyIntoChunks previously overwrote already-received data
when a new write partially overlapped with existing WrittenRanges.
QuicRangeAddRange correctly returns WrittenRangesUpdated=TRUE for partial
overlaps, causing CopyIntoChunks to write the full incoming buffer
including bytes at already-received offsets.

Fix

Walk existing WrittenRanges before calling QuicRangeAddRange to identify
gap regions within the incoming write window. Call
QuicRecvBufferCopyIntoChunks once per gap, copying only new bytes.
ReadLength update moved out of QuicRecvBufferCopyIntoChunks into
QuicRecvBufferWrite after the range is committed.

This fixes the issue for both CRYPTO and STREAM frame processing.

Testing

Added four parameterized unit tests across all four buffer modes:

OverlapWritePreservesExistingData: partial overlap at end
OverlapWriteAtFront: fill gap and overlap
OverlapWriteExactDuplicate: pure duplicate write
OverlapWriteMultipleGaps: swiss-cheese pattern covered by large write

@guhetier

src/core/recv_buffer.c

guhetier

The product code update looks good, thanks for the contribution!
The tests need some changes to actually validate the feature.

src/core/unittest/RecvBufferTest.cpp

MarkedMuichiro · 2026-03-30T20:51:24Z

The product code update looks good, thanks for the contribution! The tests need some changes to actually validate the feature.

Thanks for the detailed feedback! I'm glad to help. Here's the approach I took in addressing your comments:

For the test issue: added a WriteCustom helper that accepts an explicit buffer, and a ValidateBufferCustom that validates against a provided expected array. The overlap tests now write 0xff sentinel values in the already-written regions of the second write. If the fix were broken and those bytes got overwritten, the sentinels would appear in the buffer and ValidateBufferCustom would catch the corruption immediately.

Also removed the redundant ValidateBuffer call you flagged — it was already handled by Read.

src/core/unittest/RecvBufferTest.cpp

guhetier

Looks good to me one the dead code is removed.

src/core/unittest/RecvBufferTest.cpp

MarkedMuichiro · 2026-03-31T16:57:51Z

Looks good to me one the dead code is removed.

Done. Thank you.

guhetier · 2026-03-31T17:28:26Z

Thanks for the contribution! I'll merge this once the CI succeeds.

MarkedMuichiro · 2026-04-03T21:36:27Z

@guhetier Any idea why some test cases are failing?

guhetier · 2026-04-03T22:47:54Z

There is a regression (or at least change of behavior) in CI runners. We just checked in a workaround for the "WinPrerelease" ones. I am investigating the linux one.

If you rebase, more should succeed, I don't think this is related to your PR in any way but we try to limit churn until we solve it.

sm-msft · 2026-04-06T22:53:13Z

src/core/unittest/RecvBufferTest.cpp

+    // Write [0, 20) with pattern A (offset-based).
+    // Then write [10, 30) with 0xff in the overlap region [10, 20).
+    // Verify bytes [0, 20) still contain pattern A, and [20, 30) contain
+    // the new data from the second write.


Could you please include tests for the following scenarios:

3 ranges that have 2 overlaps between them

3 non-overlapping contiguous ranges

4 ranges - 3 with 2 range overlaps and one range that is non-contiguous with the others

Added OverlapWriteThreeRangesTwoOverlaps and OverlapWriteThreeContiguousNonOverlapping per your request. Thanks for the suggestion!

QuicRecvBufferCopyIntoChunks previously overwrote already-received data when a new write overlapped with existing WrittenRanges. Fix by walking existing WrittenRanges before calling QuicRangeAddRange to identify and copy only the new (gap) bytes. ReadLength update moved out of QuicRecvBufferCopyIntoChunks into QuicRecvBufferWrite after the range is committed. Fixes overlapping CRYPTO and STREAM frame data corruption. Adds unit tests covering partial overlap, front overlap, exact duplicate, and multiple gap scenarios across all buffer modes.

- Restore existing comment on quota check block - Factor gap-walking copy logic into QuicRecvBufferWriteNewBytes helper - Update ReadLength comment to refer only to present state of code

- Factor gap-walking copy logic into QuicRecvBufferWriteNewBytes helper - Move ReadLength update out of QuicRecvBufferCopyIntoChunks into QuicRecvBufferWrite - Update comment to refer only to present state of code

…ectnessAdd WriteCustom helper to allow tests to provide explicit buffer data. Add ValidateBufferCustom to validate against an expected array. Rewrite overlap tests to write 0xff sentinel values in the overlap region of the second write, verifying the originally written bytes survive and are not overwritten.

Update comment. Co-authored-by: Guillaume Hetier <hetier.guillaume@gmail.com>

Remove dead code. Co-authored-by: Guillaume Hetier <hetier.guillaume@gmail.com>

MarkedMuichiro · 2026-04-06T23:21:59Z

There is a regression (or at least change of behavior) in CI runners. We just checked in a workaround for the "WinPrerelease" ones. I am investigating the linux one.

If you rebase, more should succeed, I don't think this is related to your PR in any way but we try to limit churn until we solve it.

Thanks for the update and for investigating the CI issue. Rebased onto main.

… tests

sm-msft · 2026-04-07T16:51:27Z

src/core/recv_buffer.c

+        // If this subrange ends before our current copy position, skip it.
+        //
+        if (Sub->Low + Sub->Count <= CopyOffset) {
+            continue;


We should trace this as a datapath event since there is no other tracking of this overlap or an indication of this occurring. A new type of DataPath event traced here seems appropriate, but I will leave it to @guhetier to comment on specifics.

MarkedMuichiro requested a review from a team as a code owner March 27, 2026 23:59

guhetier reviewed Mar 28, 2026

View reviewed changes

src/core/recv_buffer.c Outdated Show resolved Hide resolved

src/core/recv_buffer.c Outdated Show resolved Hide resolved

src/core/recv_buffer.c Show resolved Hide resolved

MarkedMuichiro requested a review from guhetier March 30, 2026 05:46

guhetier reviewed Mar 30, 2026

View reviewed changes

src/core/recv_buffer.c Outdated Show resolved Hide resolved

guhetier reviewed Mar 30, 2026

View reviewed changes

src/core/unittest/RecvBufferTest.cpp Outdated Show resolved Hide resolved

src/core/unittest/RecvBufferTest.cpp Outdated Show resolved Hide resolved

guhetier reviewed Mar 30, 2026

View reviewed changes

src/core/unittest/RecvBufferTest.cpp Outdated Show resolved Hide resolved

guhetier reviewed Mar 30, 2026

View reviewed changes

src/core/unittest/RecvBufferTest.cpp Outdated Show resolved Hide resolved

MarkedMuichiro requested a review from guhetier March 31, 2026 00:27

guhetier reviewed Mar 31, 2026

View reviewed changes

src/core/unittest/RecvBufferTest.cpp Outdated Show resolved Hide resolved

guhetier previously approved these changes Mar 31, 2026

View reviewed changes

MarkedMuichiro requested a review from guhetier April 1, 2026 16:52

sm-msft reviewed Apr 6, 2026

View reviewed changes

MarkedMuichiro and others added 7 commits April 6, 2026 23:20

refactor(recv_buffer): address review feedback

868c2e9

- Restore existing comment on quota check block - Factor gap-walking copy logic into QuicRecvBufferWriteNewBytes helper - Update ReadLength comment to refer only to present state of code

refactor(recv_buffer): address review feedback

f7d5602

- Factor gap-walking copy logic into QuicRecvBufferWriteNewBytes helper - Move ReadLength update out of QuicRecvBufferCopyIntoChunks into QuicRecvBufferWrite - Update comment to refer only to present state of code

Update src/core/recv_buffer.c

d185fb8

Update comment. Co-authored-by: Guillaume Hetier <hetier.guillaume@gmail.com>

test(recv_buffer): simplify validation using memcmp, clean up comments

30c2493

Update src/core/unittest/RecvBufferTest.cpp

d104bd3

Remove dead code. Co-authored-by: Guillaume Hetier <hetier.guillaume@gmail.com>

MarkedMuichiro force-pushed the fix/recv-buffer-overlap-copy branch from 99d16a6 to d104bd3 Compare April 6, 2026 23:20

test(recv_buffer): add three-range overlap and contiguous non-overlap…

cbf137d

… tests

MarkedMuichiro dismissed guhetier’s stale review via cbf137d April 6, 2026 23:52

MarkedMuichiro requested a review from sm-msft April 6, 2026 23:54

sm-msft reviewed Apr 7, 2026

View reviewed changes

Conversation

MarkedMuichiro commented Mar 27, 2026

Problem

Fix

Testing

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

guhetier left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MarkedMuichiro commented Mar 30, 2026

Uh oh!

Uh oh!

Uh oh!

guhetier left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MarkedMuichiro commented Mar 31, 2026

Uh oh!

guhetier commented Mar 31, 2026

Uh oh!

MarkedMuichiro commented Apr 3, 2026

Uh oh!

guhetier commented Apr 3, 2026

Uh oh!

sm-msft Apr 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarkedMuichiro Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

MarkedMuichiro commented Apr 6, 2026

Uh oh!

sm-msft Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sm-msft Apr 6, 2026 •

edited

Loading