Skip to content

Fix uint32 overflow in receive buffer growth #6226

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.

Sign up for GitHub

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

Open
Gaurav Singh (gaurav2699) wants to merge 18 commits into
base: main
Choose a base branch
from
+34 −4
Open

Fix uint32 overflow in receive buffer growth #6226

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
8fa9fae
fixed .net test issue for opensuse 16 and sles 16
gaurav2699 Mar 23, 2026
1071065
merge
gaurav2699 May 11, 2026
298714b
Merge branch 'main' of https://github.com/microsoft/msquic
gaurav2699 May 14, 2026
b053b33
Merge branch 'main' of https://github.com/microsoft/msquic
gaurav2699 May 21, 2026
5b5b933
Merge branch 'main' of https://github.com/microsoft/msquic
gaurav2699 May 28, 2026
0194d49
Merge branch 'main' of https://github.com/microsoft/msquic
gaurav2699 Jun 1, 2026
6efd483
Merge branch 'main' of https://github.com/microsoft/msquic
gaurav2699 Jun 23, 2026
38d7f3a
erge branch 'main' of https://github.com/microsoft/msquic
gaurav2699 Jul 2, 2026
9ae9fa3
Merge branch 'main' of https://github.com/microsoft/msquic
gaurav2699 Jul 10, 2026
f527d14
Merge branch 'main' of https://github.com/microsoft/msquic
gaurav2699 Jul 16, 2026
beabe69
Merge branch 'main' of https://github.com/microsoft/msquic
gaurav2699 Jul 17, 2026
10c75c4
Merge branch 'main' of https://github.com/microsoft/msquic
gaurav2699 Jul 20, 2026
9537b63
Merge branch 'main' of https://github.com/microsoft/msquic
gaurav2699 Jul 28, 2026
0287dc4
Merge branch 'main' of https://github.com/microsoft/msquic
gaurav2699 Aug 3, 2026
524eb22
Merge branch 'main' of https://github.com/microsoft/msquic
gaurav2699 Aug 13, 2026
ad97fa2
Modified recvbuffer and virtual buffer autotuning logic
gaurav2699 Aug 13, 2026
3acb4fc
Recv Buffer max Vritual length named constant
gaurav2699 Aug 13, 2026
070a7c9
removed constant
gaurav2699 Aug 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/core/recv_buffer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -762,11 +762,12 @@ QuicRecvBufferWrite(
//
QUIC_RECV_CHUNK* LastChunk =
CXPLAT_CONTAINING_RECORD(RecvBuffer->Chunks.Blink, QUIC_RECV_CHUNK, Link);
uint32_t NewBufferLength = LastChunk->AllocLength << 1;
uint64_t NewBufferLength = (uint64_t)LastChunk->AllocLength << 1;
while (AbsoluteLength > RecvBuffer->BaseOffset + NewBufferLength) {
NewBufferLength <<= 1;
}
if (!QuicRecvBufferResize(RecvBuffer, NewBufferLength)) {
if (NewBufferLength > UINT32_MAX ||

@guhetier Guillaume Hetier (guhetier) Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be impossible as long as the invariant of the receive buffer are respected.
This path is already not reachable when app-owned buffer are used, so the virtual length is a power of two that fit in a uint32_t, and we would not reach this line if the write was requiring an allocation larger than the virtual length.

!QuicRecvBufferResize(RecvBuffer, (uint32_t)NewBufferLength)) {
*BufferSizeNeeded = AbsoluteLength - (RecvBuffer->BaseOffset + AllocLength);
return QUIC_STATUS_OUT_OF_MEMORY;
}
Expand Down
9 changes: 7 additions & 2 deletions src/core/stream_recv.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -804,8 +804,13 @@ QuicStreamOnBytesDelivered(
//
// Limit stream FC window growth by the connection FC window size.
//
const uint32_t MaxVirtualBufferLength =
Stream->RecvBuffer.RecvMode == QUIC_RECV_BUF_MODE_APP_OWNED ?

@guhetier Guillaume Hetier (guhetier) Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure we need this.
Initially, QUIC_RECV_BUF_MODE_APP_OWNED would tie the virtual buffer length to the amount of memory provided, but that changed.
It may take some code inspection and some adjustments, but I am think that now, the virtual buffer length should be managed the same for all buffering modes.

UINT32_MAX :
0x80000000U;
if (Stream->RecvBuffer.VirtualBufferLength != 0 &&
Stream->RecvBuffer.VirtualBufferLength < Stream->Connection->Settings.ConnFlowControlWindow) {
Stream->RecvBuffer.VirtualBufferLength < Stream->Connection->Settings.ConnFlowControlWindow &&
Stream->RecvBuffer.VirtualBufferLength < MaxVirtualBufferLength) {
uint64_t TimeThreshold =
((Stream->RecvWindowBytesDelivered * Stream->Connection->Paths[0].SmoothedRtt) / RecvBufferDrainThreshold);
if (CxPlatTimeDiff64(Stream->RecvWindowLastUpdate, TimeNow) <= TimeThreshold) {
Expand All @@ -830,7 +835,7 @@ QuicStreamOnBytesDelivered(
//

uint64_t NewLength = (uint64_t)Stream->RecvBuffer.VirtualBufferLength * 2;
NewLength = CXPLAT_MIN(NewLength, UINT32_MAX);
NewLength = CXPLAT_MIN(NewLength, MaxVirtualBufferLength);

@guhetier Guillaume Hetier (guhetier) Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simply don't do the doubling (or undo it) when it would overflow rather than using a constant. This line is actually redundant with the check in the if test above.


QuicRecvBufferIncreaseVirtualBufferLength(
&Stream->RecvBuffer,
Expand Down
24 changes: 24 additions & 0 deletions src/core/unittest/RecvBufferTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -547,6 +547,30 @@ TEST_P(WithMode, WriteTooMuch2)
ASSERT_FALSE(RecvBuf.HasUnreadData());
}

TEST(RecvBufferGrowthTest, WriteGrowthOverflow)

@guhetier Guillaume Hetier (guhetier) Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider having this test running with all buffer modes.

{
RecvBuffer RecvBuf;
ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.Initialize(QUIC_RECV_BUF_MODE_SINGLE));

RecvBuf.IncreaseVirtualBufferLength(UINT32_MAX);

@guhetier Guillaume Hetier (guhetier) Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting the virtual buffer size to something that isn't a power of 2 is not meant to happen unless in app-owned buffer mode (and even that is probably what should be fixed).


uint8_t WriteBuffer = 0;
uint64_t QuotaConsumed = 0;
uint64_t BufferSizeNeeded = 0;
BOOLEAN NewDataReady = FALSE;
ASSERT_EQ(
QUIC_STATUS_OUT_OF_MEMORY,
QuicRecvBufferWrite(
&RecvBuf.RecvBuf,
0x80000000U,
sizeof(WriteBuffer),
&WriteBuffer,
UINT32_MAX,
&QuotaConsumed,
&NewDataReady,
&BufferSizeNeeded));
}

TEST_P(WithMode, WriteWhilePendingRead)
{
RecvBuffer RecvBuf;
Expand Down
Loading