From 7452ee1ac605a2ba9df3310f15ac18ee8e293d64 Mon Sep 17 00:00:00 2001 From: MarkedMuichiro Date: Fri, 27 Mar 2026 23:56:34 +0000 Subject: [PATCH 1/8] fix(recv_buffer): skip already-written bytes on overlapping writes 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. --- src/core/recv_buffer.c | 93 +++++++++++------ src/core/unittest/RecvBufferTest.cpp | 145 +++++++++++++++++++++++++++ 2 files changed, 208 insertions(+), 30 deletions(-) diff --git a/src/core/recv_buffer.c b/src/core/recv_buffer.c index d057ab0c67..efc7cb721b 100644 --- a/src/core/recv_buffer.c +++ b/src/core/recv_buffer.c @@ -611,16 +611,6 @@ QuicRecvBufferCopyIntoChunks( WriteLength -= (uint16_t)CopyLength; } CXPLAT_DBG_ASSERT(WriteLength == 0); // Should always have enough room to copy everything - - // - // Update the amount of data readable in the first chunk. - // - QUIC_SUBRANGE* FirstRange = QuicRangeGet(&RecvBuffer->WrittenRanges, 0); - if (FirstRange->Low == 0) { - RecvBuffer->ReadLength = (uint32_t)CXPLAT_MIN( - RecvBuffer->Capacity, - FirstRange->Count - RecvBuffer->BaseOffset); - } } _IRQL_requires_max_(DISPATCH_LEVEL) @@ -638,7 +628,7 @@ QuicRecvBufferWrite( ) { CXPLAT_DBG_ASSERT(WriteLength != 0); - *NewDataReady = FALSE; // Most cases below aren't ready to read. + *NewDataReady = FALSE; *QuotaConsumed = 0; *BufferSizeNeeded = 0; @@ -661,8 +651,6 @@ QuicRecvBufferWrite( // // Check that the write is not going to cause us to consume more than the // quota allocated by the connection flow control. - // If it is in bound, update the output to indicate how much of the quota is - // being consumed. // const uint64_t CurrentMaxLength = QuicRecvBufferGetTotalLength(RecvBuffer); if (AbsoluteLength > CurrentMaxLength) { @@ -683,22 +671,11 @@ QuicRecvBufferWrite( // const uint32_t AllocLength = QuicRecvBufferGetTotalAllocLength(RecvBuffer); if (AbsoluteLength > RecvBuffer->BaseOffset + AllocLength) { - // - // There isn't enough space to write the data. - // if (RecvBuffer->RecvMode == QUIC_RECV_BUF_MODE_APP_OWNED) { - // - // We can't allocate more space in app-owned mode. - // Let the caller notify the app to provide more buffer space. - // *BufferSizeNeeded = AbsoluteLength - (RecvBuffer->BaseOffset + AllocLength); return QUIC_STATUS_BUFFER_TOO_SMALL; } - // - // Add a new chunk (or replace the existing one), doubling the size of the largest chunk - // until there is enough space for the write. - // QUIC_RECV_CHUNK* LastChunk = CXPLAT_CONTAINING_RECORD(RecvBuffer->Chunks.Blink, QUIC_RECV_CHUNK, Link); uint32_t NewBufferLength = LastChunk->AllocLength << 1; @@ -712,7 +689,60 @@ QuicRecvBufferWrite( } // - // Set the write offset/length as a valid written range. + // Copy only the new (non-overlapping) bytes into the chunks. + // Walk the existing WrittenRanges to find gaps within [WriteOffset, AbsoluteLength) + // and copy only those gap regions. This prevents overwriting already-received data. + // + uint64_t CopyOffset = WriteOffset; + for (uint32_t i = 0; i < RecvBuffer->WrittenRanges.UsedLength && CopyOffset < AbsoluteLength; i++) { + QUIC_SUBRANGE* Sub = QuicRangeGet(&RecvBuffer->WrittenRanges, i); + + // + // If this subrange starts after our window ends, stop. + // + if (Sub->Low >= AbsoluteLength) { + break; + } + + // + // If this subrange ends before our current copy position, skip it. + // + if (Sub->Low + Sub->Count <= CopyOffset) { + continue; + } + + // + // If there is a gap before this subrange, copy the gap bytes. + // + if (Sub->Low > CopyOffset) { + uint16_t GapLength = (uint16_t)(Sub->Low - CopyOffset); + QuicRecvBufferCopyIntoChunks( + RecvBuffer, + CopyOffset, + GapLength, + WriteBuffer + (CopyOffset - WriteOffset)); + } + + // + // Advance past this already-written subrange. + // + CopyOffset = Sub->Low + Sub->Count; + } + + // + // Copy any remaining new bytes after the last subrange. + // + if (CopyOffset < AbsoluteLength) { + uint16_t RemainLength = (uint16_t)(AbsoluteLength - CopyOffset); + QuicRecvBufferCopyIntoChunks( + RecvBuffer, + CopyOffset, + RemainLength, + WriteBuffer + (CopyOffset - WriteOffset)); + } + + // + // Now commit the range update. // BOOLEAN WrittenRangesUpdated; QUIC_SUBRANGE* UpdatedRange = @@ -730,9 +760,6 @@ QuicRecvBufferWrite( return QUIC_STATUS_OUT_OF_MEMORY; } if (!WrittenRangesUpdated) { - // - // No changes are necessary. Exit immediately. - // return QUIC_STATUS_SUCCESS; } @@ -742,9 +769,15 @@ QuicRecvBufferWrite( *NewDataReady = UpdatedRange->Low == 0; // - // Write the data into the chunks now that everything has been validated. + // Update the amount of data readable in the first chunk. + // (Moved here from QuicRecvBufferCopyIntoChunks since the range is now committed.) // - QuicRecvBufferCopyIntoChunks(RecvBuffer, WriteOffset, WriteLength, WriteBuffer); + QUIC_SUBRANGE* FirstRange = QuicRangeGet(&RecvBuffer->WrittenRanges, 0); + if (FirstRange->Low == 0) { + RecvBuffer->ReadLength = (uint32_t)CXPLAT_MIN( + RecvBuffer->Capacity, + FirstRange->Count - RecvBuffer->BaseOffset); + } QuicRecvBufferValidate(RecvBuffer); return QUIC_STATUS_SUCCESS; diff --git a/src/core/unittest/RecvBufferTest.cpp b/src/core/unittest/RecvBufferTest.cpp index f4ccae6f6c..618aecfce5 100644 --- a/src/core/unittest/RecvBufferTest.cpp +++ b/src/core/unittest/RecvBufferTest.cpp @@ -2205,3 +2205,148 @@ TEST(RecvBufferResetReadTest, ResetClearsPendingAndExternalReference_SingleMode) // After this read, pending length should be 16 again (sanity). EXPECT_EQ(16ull, RecvBuf.RecvBuf.ReadPendingLength); } + +// Tests for overlapping write behavior - verifies that overlapping writes +// do not corrupt previously received data. + +TEST_P(WithMode, OverlapWritePreservesExistingData) +{ + // + // Write bytes 0-19, then write bytes 10-29 (overlapping). + // Verify the final buffer contains correct data throughout. + // + RecvBuffer RecvBuf; + ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.Initialize(GetParam())); + + uint64_t InOutWriteLength = LARGE_TEST_BUFFER_LENGTH; + BOOLEAN NewDataReady = FALSE; + + // Write [0, 20) + ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.Write(0, 20, &InOutWriteLength, &NewDataReady)); + ASSERT_TRUE(NewDataReady); + + // Write [10, 30) — overlaps with [10, 20) already written + InOutWriteLength = LARGE_TEST_BUFFER_LENGTH; + NewDataReady = FALSE; + ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.Write(10, 20, &InOutWriteLength, &NewDataReady)); + ASSERT_TRUE(NewDataReady); + ASSERT_EQ(30ull, RecvBuf.GetTotalLength()); + + // Read and verify data integrity — buffer[i] should equal (uint8_t)i + uint64_t ReadOffset; + QUIC_BUFFER ReadBuffers[3]; + uint32_t BufferCount = ARRAYSIZE(ReadBuffers); + RecvBuf.Read(&ReadOffset, &BufferCount, ReadBuffers); + ASSERT_EQ(0ull, ReadOffset); + ASSERT_GE(BufferCount, 1u); + + // Validate first 20 bytes were not corrupted by the overlapping write + RecvBuffer::ValidateBuffer(ReadBuffers[0].Buffer, ReadBuffers[0].Length, ReadOffset); + RecvBuf.Drain(30); +} + +TEST_P(WithMode, OverlapWriteAtFront) +{ + // + // Write bytes 10-29, then write bytes 0-19 (fills gap and overlaps). + // Verify the entire buffer [0, 30) is intact. + // + RecvBuffer RecvBuf; + ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.Initialize(GetParam())); + + uint64_t InOutWriteLength = LARGE_TEST_BUFFER_LENGTH; + BOOLEAN NewDataReady = FALSE; + + // Write [10, 30) first + ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.Write(10, 20, &InOutWriteLength, &NewDataReady)); + ASSERT_FALSE(NewDataReady); + + // Write [0, 20) — fills gap [0,10) and overlaps [10,20) + InOutWriteLength = LARGE_TEST_BUFFER_LENGTH; + NewDataReady = FALSE; + ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.Write(0, 20, &InOutWriteLength, &NewDataReady)); + ASSERT_TRUE(NewDataReady); + ASSERT_EQ(30ull, RecvBuf.GetTotalLength()); + + // Read and verify full buffer integrity + uint64_t ReadOffset; + QUIC_BUFFER ReadBuffers[3]; + uint32_t BufferCount = ARRAYSIZE(ReadBuffers); + RecvBuf.Read(&ReadOffset, &BufferCount, ReadBuffers); + ASSERT_EQ(0ull, ReadOffset); + RecvBuffer::ValidateBuffer(ReadBuffers[0].Buffer, ReadBuffers[0].Length, ReadOffset); + RecvBuf.Drain(30); +} + +TEST_P(WithMode, OverlapWriteExactDuplicate) +{ + // + // Write the same range twice. Second write should be a no-op + // and data should be unchanged. + // + RecvBuffer RecvBuf; + ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.Initialize(GetParam())); + + uint64_t InOutWriteLength = LARGE_TEST_BUFFER_LENGTH; + BOOLEAN NewDataReady = FALSE; + + ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.Write(0, 20, &InOutWriteLength, &NewDataReady)); + ASSERT_TRUE(NewDataReady); + + // Write same range again + InOutWriteLength = LARGE_TEST_BUFFER_LENGTH; + NewDataReady = FALSE; + ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.Write(0, 20, &InOutWriteLength, &NewDataReady)); + ASSERT_EQ(0ull, InOutWriteLength); // No new bytes + ASSERT_EQ(20ull, RecvBuf.GetTotalLength()); + + uint64_t ReadOffset; + QUIC_BUFFER ReadBuffers[3]; + uint32_t BufferCount = ARRAYSIZE(ReadBuffers); + RecvBuf.Read(&ReadOffset, &BufferCount, ReadBuffers); + ASSERT_EQ(0ull, ReadOffset); + RecvBuffer::ValidateBuffer(ReadBuffers[0].Buffer, ReadBuffers[0].Length, ReadOffset); + RecvBuf.Drain(20); +} + +TEST_P(WithMode, OverlapWriteMultipleGaps) +{ + // + // Write alternating chunks creating a swiss-cheese pattern, + // then write a large range covering everything. + // Verify no existing data is corrupted. + // Pattern: write [0,5), [10,15), [20,25) then write [0,30) + // + RecvBuffer RecvBuf; + ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.Initialize(GetParam(), false, + DEF_TEST_BUFFER_LENGTH, LARGE_TEST_BUFFER_LENGTH)); + + uint64_t InOutWriteLength = LARGE_TEST_BUFFER_LENGTH; + BOOLEAN NewDataReady = FALSE; + + ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.Write(0, 5, &InOutWriteLength, &NewDataReady)); + ASSERT_TRUE(NewDataReady); + + InOutWriteLength = LARGE_TEST_BUFFER_LENGTH; + ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.Write(10, 5, &InOutWriteLength, &NewDataReady)); + + InOutWriteLength = LARGE_TEST_BUFFER_LENGTH; + ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.Write(20, 5, &InOutWriteLength, &NewDataReady)); + + // Now write [0, 30) covering all gaps and existing data + InOutWriteLength = LARGE_TEST_BUFFER_LENGTH; + NewDataReady = FALSE; + ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.Write(0, 30, &InOutWriteLength, &NewDataReady)); + ASSERT_TRUE(NewDataReady); + ASSERT_EQ(30ull, RecvBuf.GetTotalLength()); + + // Verify data — bytes 0-4, 10-14, 20-24 were written first + // and should not have been overwritten + uint64_t ReadOffset; + QUIC_BUFFER ReadBuffers[3]; + uint32_t BufferCount = ARRAYSIZE(ReadBuffers); + RecvBuf.Read(&ReadOffset, &BufferCount, ReadBuffers); + ASSERT_EQ(0ull, ReadOffset); + RecvBuffer::ValidateBuffer(ReadBuffers[0].Buffer, ReadBuffers[0].Length, ReadOffset); + RecvBuf.Drain(30); +} From 868c2e97ec9f375195cbedf3e8e09a5f756e9ea0 Mon Sep 17 00:00:00 2001 From: MarkedMuichiro Date: Sat, 28 Mar 2026 06:38:07 +0000 Subject: [PATCH 2/8] refactor(recv_buffer): address review feedback - 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 --- src/core/recv_buffer.c | 221 +++++++++++++++++++++++++---------------- 1 file changed, 134 insertions(+), 87 deletions(-) diff --git a/src/core/recv_buffer.c b/src/core/recv_buffer.c index efc7cb721b..b6b4de9252 100644 --- a/src/core/recv_buffer.c +++ b/src/core/recv_buffer.c @@ -249,12 +249,17 @@ QuicRecvBufferValidate( (RecvBuffer->RecvMode != QUIC_RECV_BUF_MODE_SINGLE && RecvBuffer->RecvMode != QUIC_RECV_BUF_MODE_CIRCULAR) || FirstChunk->Link.Flink == &RecvBuffer->Chunks); + + // + // ReadStart must be within the bounds of the first chunk. // - // In Single and App-owned modes, the first chunk is never used in a circular way. + CXPLAT_DBG_ASSERT(RecvBuffer->ReadStart < FirstChunk->AllocLength); + + // + // ReadLength must be within the bounds of the buffer capacity. // CXPLAT_DBG_ASSERT( - (RecvBuffer->RecvMode != QUIC_RECV_BUF_MODE_SINGLE && - RecvBuffer->RecvMode != QUIC_RECV_BUF_MODE_APP_OWNED) || + RecvBuffer->ReadStart + RecvBuffer->ReadLength <= RecvBuffer->Capacity || RecvBuffer->ReadStart + RecvBuffer->ReadLength <= FirstChunk->AllocLength); } #else @@ -271,48 +276,52 @@ QuicRecvBufferInitialize( _In_opt_ QUIC_RECV_CHUNK* PreallocatedChunk ) { - CXPLAT_DBG_ASSERT(AllocBufferLength != 0 || RecvMode == QUIC_RECV_BUF_MODE_APP_OWNED); - CXPLAT_DBG_ASSERT(VirtualBufferLength != 0 || RecvMode == QUIC_RECV_BUF_MODE_APP_OWNED); - CXPLAT_DBG_ASSERT(PreallocatedChunk == NULL || RecvMode != QUIC_RECV_BUF_MODE_APP_OWNED); - CXPLAT_DBG_ASSERT((AllocBufferLength & (AllocBufferLength - 1)) == 0); // Power of 2 - CXPLAT_DBG_ASSERT((VirtualBufferLength & (VirtualBufferLength - 1)) == 0); // Power of 2 - CXPLAT_DBG_ASSERT(AllocBufferLength <= VirtualBufferLength); + CXPLAT_DBG_ASSERT(AllocBufferLength == 0 || (AllocBufferLength & (AllocBufferLength - 1)) == 0); // Power of 2 + CXPLAT_DBG_ASSERT(VirtualBufferLength >= AllocBufferLength); RecvBuffer->BaseOffset = 0; RecvBuffer->ReadStart = 0; - RecvBuffer->ReadPendingLength = 0; RecvBuffer->ReadLength = 0; + RecvBuffer->ReadPendingLength = 0; RecvBuffer->RecvMode = RecvMode; - RecvBuffer->RetiredChunk = NULL; RecvBuffer->VirtualBufferLength = VirtualBufferLength; - QuicRangeInitialize(QUIC_MAX_RANGE_ALLOC_SIZE, &RecvBuffer->WrittenRanges); + RecvBuffer->Capacity = AllocBufferLength; + RecvBuffer->RetiredChunk = NULL; CxPlatListInitializeHead(&RecvBuffer->Chunks); + QuicRangeInitialize(QUIC_MAX_RANGE_ALLOC_SIZE, &RecvBuffer->WrittenRanges); - if (RecvMode != QUIC_RECV_BUF_MODE_APP_OWNED) { + if (RecvMode == QUIC_RECV_BUF_MODE_APP_OWNED) { // - // Setup an initial chunk. + // In App-owned mode, the app provides the buffers, so we don't allocate any. // - QUIC_RECV_CHUNK* Chunk = NULL; - if (PreallocatedChunk != NULL) { - Chunk = PreallocatedChunk; - } else { - Chunk = CXPLAT_ALLOC_NONPAGED(sizeof(QUIC_RECV_CHUNK) + AllocBufferLength, QUIC_POOL_RECVBUF); - if (Chunk == NULL) { - QuicTraceEvent( - AllocFailure, - "Allocation of '%s' failed. (%llu bytes)", - "recv_buffer", - sizeof(QUIC_RECV_CHUNK) + AllocBufferLength); - return QUIC_STATUS_OUT_OF_MEMORY; - } - QuicRecvChunkInitialize(Chunk, AllocBufferLength, (uint8_t*)(Chunk + 1), FALSE); - } - CxPlatListInsertHead(&RecvBuffer->Chunks, &Chunk->Link); - RecvBuffer->Capacity = AllocBufferLength; + CXPLAT_DBG_ASSERT(PreallocatedChunk == NULL); + return QUIC_STATUS_SUCCESS; + } + + if (AllocBufferLength == 0) { + return QUIC_STATUS_SUCCESS; + } + + QUIC_RECV_CHUNK* Chunk; + if (PreallocatedChunk != NULL) { + Chunk = PreallocatedChunk; } else { - RecvBuffer->Capacity = 0; + Chunk = + CXPLAT_ALLOC_NONPAGED(sizeof(QUIC_RECV_CHUNK) + AllocBufferLength, QUIC_POOL_RECVBUF); + if (Chunk == NULL) { + QuicTraceEvent( + AllocFailure, + "Allocation of '%s' failed. (%llu bytes)", + "recv_buffer", + sizeof(QUIC_RECV_CHUNK) + AllocBufferLength); + return QUIC_STATUS_OUT_OF_MEMORY; + } + QuicRecvChunkInitialize(Chunk, AllocBufferLength, (uint8_t*)(Chunk + 1), FALSE); } + CxPlatListInsertTail(&RecvBuffer->Chunks, &Chunk->Link); + + QuicRecvBufferValidate(RecvBuffer); return QUIC_STATUS_SUCCESS; } @@ -323,6 +332,7 @@ QuicRecvBufferUninitialize( ) { QuicRangeUninitialize(&RecvBuffer->WrittenRanges); + while (!CxPlatListIsEmpty(&RecvBuffer->Chunks)) { QUIC_RECV_CHUNK* Chunk = CXPLAT_CONTAINING_RECORD( @@ -613,6 +623,75 @@ QuicRecvBufferCopyIntoChunks( CXPLAT_DBG_ASSERT(WriteLength == 0); // Should always have enough room to copy everything } +// +// Copies only the new (non-overlapping) bytes from WriteBuffer into the receive buffer chunks, +// skipping any byte ranges already present in WrittenRanges. +// Must be called before QuicRangeAddRange updates WrittenRanges. +// +_IRQL_requires_max_(DISPATCH_LEVEL) +void +QuicRecvBufferWriteNewBytes( + _In_ QUIC_RECV_BUFFER* RecvBuffer, + _In_ uint64_t WriteOffset, + _In_ uint16_t WriteLength, + _In_reads_bytes_(WriteLength) uint8_t const* WriteBuffer + ) +{ + const uint64_t WriteEnd = WriteOffset + WriteLength; + uint64_t CopyOffset = WriteOffset; + + // + // Walk existing WrittenRanges to find gaps within [WriteOffset, WriteEnd). + // For each gap, copy only those new bytes into the chunks. + // + for (uint32_t i = 0; i < RecvBuffer->WrittenRanges.UsedLength && CopyOffset < WriteEnd; i++) { + QUIC_SUBRANGE* Sub = QuicRangeGet(&RecvBuffer->WrittenRanges, i); + + // + // If this subrange starts at or beyond the end of our write window, stop. + // + if (Sub->Low >= WriteEnd) { + break; + } + + // + // If this subrange ends before our current copy position, skip it. + // + if (Sub->Low + Sub->Count <= CopyOffset) { + continue; + } + + // + // If there is a gap before this subrange, copy the gap bytes. + // + if (Sub->Low > CopyOffset) { + uint16_t GapLength = (uint16_t)(Sub->Low - CopyOffset); + QuicRecvBufferCopyIntoChunks( + RecvBuffer, + CopyOffset, + GapLength, + WriteBuffer + (CopyOffset - WriteOffset)); + } + + // + // Advance past this already-written subrange. + // + CopyOffset = Sub->Low + Sub->Count; + } + + // + // Copy any remaining new bytes after the last subrange. + // + if (CopyOffset < WriteEnd) { + uint16_t RemainLength = (uint16_t)(WriteEnd - CopyOffset); + QuicRecvBufferCopyIntoChunks( + RecvBuffer, + CopyOffset, + RemainLength, + WriteBuffer + (CopyOffset - WriteOffset)); + } +} + _IRQL_requires_max_(DISPATCH_LEVEL) _Success_(return == QUIC_STATUS_SUCCESS) QUIC_STATUS @@ -628,7 +707,7 @@ QuicRecvBufferWrite( ) { CXPLAT_DBG_ASSERT(WriteLength != 0); - *NewDataReady = FALSE; + *NewDataReady = FALSE; // Most cases below aren't ready to read. *QuotaConsumed = 0; *BufferSizeNeeded = 0; @@ -651,6 +730,8 @@ QuicRecvBufferWrite( // // Check that the write is not going to cause us to consume more than the // quota allocated by the connection flow control. + // If it is in bound, update the output to indicate how much of the quota is + // being consumed. // const uint64_t CurrentMaxLength = QuicRecvBufferGetTotalLength(RecvBuffer); if (AbsoluteLength > CurrentMaxLength) { @@ -671,11 +752,22 @@ QuicRecvBufferWrite( // const uint32_t AllocLength = QuicRecvBufferGetTotalAllocLength(RecvBuffer); if (AbsoluteLength > RecvBuffer->BaseOffset + AllocLength) { + // + // There isn't enough space to write the data. + // if (RecvBuffer->RecvMode == QUIC_RECV_BUF_MODE_APP_OWNED) { + // + // We can't allocate more space in app-owned mode. + // Let the caller notify the app to provide more buffer space. + // *BufferSizeNeeded = AbsoluteLength - (RecvBuffer->BaseOffset + AllocLength); return QUIC_STATUS_BUFFER_TOO_SMALL; } + // + // Add a new chunk (or replace the existing one), doubling the size of the largest chunk + // until there is enough space for the write. + // QUIC_RECV_CHUNK* LastChunk = CXPLAT_CONTAINING_RECORD(RecvBuffer->Chunks.Blink, QUIC_RECV_CHUNK, Link); uint32_t NewBufferLength = LastChunk->AllocLength << 1; @@ -689,60 +781,13 @@ QuicRecvBufferWrite( } // - // Copy only the new (non-overlapping) bytes into the chunks. - // Walk the existing WrittenRanges to find gaps within [WriteOffset, AbsoluteLength) - // and copy only those gap regions. This prevents overwriting already-received data. - // - uint64_t CopyOffset = WriteOffset; - for (uint32_t i = 0; i < RecvBuffer->WrittenRanges.UsedLength && CopyOffset < AbsoluteLength; i++) { - QUIC_SUBRANGE* Sub = QuicRangeGet(&RecvBuffer->WrittenRanges, i); - - // - // If this subrange starts after our window ends, stop. - // - if (Sub->Low >= AbsoluteLength) { - break; - } - - // - // If this subrange ends before our current copy position, skip it. - // - if (Sub->Low + Sub->Count <= CopyOffset) { - continue; - } - - // - // If there is a gap before this subrange, copy the gap bytes. - // - if (Sub->Low > CopyOffset) { - uint16_t GapLength = (uint16_t)(Sub->Low - CopyOffset); - QuicRecvBufferCopyIntoChunks( - RecvBuffer, - CopyOffset, - GapLength, - WriteBuffer + (CopyOffset - WriteOffset)); - } - - // - // Advance past this already-written subrange. - // - CopyOffset = Sub->Low + Sub->Count; - } - - // - // Copy any remaining new bytes after the last subrange. + // Copy only the new bytes into the chunks, skipping already-written ranges. + // Must be done before QuicRangeAddRange commits the new range. // - if (CopyOffset < AbsoluteLength) { - uint16_t RemainLength = (uint16_t)(AbsoluteLength - CopyOffset); - QuicRecvBufferCopyIntoChunks( - RecvBuffer, - CopyOffset, - RemainLength, - WriteBuffer + (CopyOffset - WriteOffset)); - } + QuicRecvBufferWriteNewBytes(RecvBuffer, WriteOffset, WriteLength, WriteBuffer); // - // Now commit the range update. + // Set the write offset/length as a valid written range. // BOOLEAN WrittenRangesUpdated; QUIC_SUBRANGE* UpdatedRange = @@ -760,6 +805,9 @@ QuicRecvBufferWrite( return QUIC_STATUS_OUT_OF_MEMORY; } if (!WrittenRangesUpdated) { + // + // No changes are necessary. Exit immediately. + // return QUIC_STATUS_SUCCESS; } @@ -769,8 +817,7 @@ QuicRecvBufferWrite( *NewDataReady = UpdatedRange->Low == 0; // - // Update the amount of data readable in the first chunk. - // (Moved here from QuicRecvBufferCopyIntoChunks since the range is now committed.) + // Update the amount of data readable in the first chunk, committing the write. // QUIC_SUBRANGE* FirstRange = QuicRangeGet(&RecvBuffer->WrittenRanges, 0); if (FirstRange->Low == 0) { @@ -1107,4 +1154,4 @@ QuicRecvBufferResetRead( Link); Chunk->ExternalReference = FALSE; RecvBuffer->ReadPendingLength = 0; -} +} \ No newline at end of file From f7d56023257130df7b8bee0960747ec6cf4e63aa Mon Sep 17 00:00:00 2001 From: MarkedMuichiro Date: Sat, 28 Mar 2026 07:22:52 +0000 Subject: [PATCH 3/8] refactor(recv_buffer): address review feedback - 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 --- src/core/recv_buffer.c | 84 +++++++++++++++++++----------------------- 1 file changed, 38 insertions(+), 46 deletions(-) diff --git a/src/core/recv_buffer.c b/src/core/recv_buffer.c index b6b4de9252..60614f7454 100644 --- a/src/core/recv_buffer.c +++ b/src/core/recv_buffer.c @@ -249,17 +249,12 @@ QuicRecvBufferValidate( (RecvBuffer->RecvMode != QUIC_RECV_BUF_MODE_SINGLE && RecvBuffer->RecvMode != QUIC_RECV_BUF_MODE_CIRCULAR) || FirstChunk->Link.Flink == &RecvBuffer->Chunks); - - // - // ReadStart must be within the bounds of the first chunk. - // - CXPLAT_DBG_ASSERT(RecvBuffer->ReadStart < FirstChunk->AllocLength); - // - // ReadLength must be within the bounds of the buffer capacity. + // In Single and App-owned modes, the first chunk is never used in a circular way. // CXPLAT_DBG_ASSERT( - RecvBuffer->ReadStart + RecvBuffer->ReadLength <= RecvBuffer->Capacity || + (RecvBuffer->RecvMode != QUIC_RECV_BUF_MODE_SINGLE && + RecvBuffer->RecvMode != QUIC_RECV_BUF_MODE_APP_OWNED) || RecvBuffer->ReadStart + RecvBuffer->ReadLength <= FirstChunk->AllocLength); } #else @@ -276,52 +271,48 @@ QuicRecvBufferInitialize( _In_opt_ QUIC_RECV_CHUNK* PreallocatedChunk ) { - CXPLAT_DBG_ASSERT(AllocBufferLength == 0 || (AllocBufferLength & (AllocBufferLength - 1)) == 0); // Power of 2 - CXPLAT_DBG_ASSERT(VirtualBufferLength >= AllocBufferLength); + CXPLAT_DBG_ASSERT(AllocBufferLength != 0 || RecvMode == QUIC_RECV_BUF_MODE_APP_OWNED); + CXPLAT_DBG_ASSERT(VirtualBufferLength != 0 || RecvMode == QUIC_RECV_BUF_MODE_APP_OWNED); + CXPLAT_DBG_ASSERT(PreallocatedChunk == NULL || RecvMode != QUIC_RECV_BUF_MODE_APP_OWNED); + CXPLAT_DBG_ASSERT((AllocBufferLength & (AllocBufferLength - 1)) == 0); // Power of 2 + CXPLAT_DBG_ASSERT((VirtualBufferLength & (VirtualBufferLength - 1)) == 0); // Power of 2 + CXPLAT_DBG_ASSERT(AllocBufferLength <= VirtualBufferLength); RecvBuffer->BaseOffset = 0; RecvBuffer->ReadStart = 0; - RecvBuffer->ReadLength = 0; RecvBuffer->ReadPendingLength = 0; + RecvBuffer->ReadLength = 0; RecvBuffer->RecvMode = RecvMode; - RecvBuffer->VirtualBufferLength = VirtualBufferLength; - RecvBuffer->Capacity = AllocBufferLength; RecvBuffer->RetiredChunk = NULL; - CxPlatListInitializeHead(&RecvBuffer->Chunks); + RecvBuffer->VirtualBufferLength = VirtualBufferLength; QuicRangeInitialize(QUIC_MAX_RANGE_ALLOC_SIZE, &RecvBuffer->WrittenRanges); + CxPlatListInitializeHead(&RecvBuffer->Chunks); - if (RecvMode == QUIC_RECV_BUF_MODE_APP_OWNED) { + if (RecvMode != QUIC_RECV_BUF_MODE_APP_OWNED) { // - // In App-owned mode, the app provides the buffers, so we don't allocate any. + // Setup an initial chunk. // - CXPLAT_DBG_ASSERT(PreallocatedChunk == NULL); - return QUIC_STATUS_SUCCESS; - } - - if (AllocBufferLength == 0) { - return QUIC_STATUS_SUCCESS; - } - - QUIC_RECV_CHUNK* Chunk; - if (PreallocatedChunk != NULL) { - Chunk = PreallocatedChunk; - } else { - Chunk = - CXPLAT_ALLOC_NONPAGED(sizeof(QUIC_RECV_CHUNK) + AllocBufferLength, QUIC_POOL_RECVBUF); - if (Chunk == NULL) { - QuicTraceEvent( - AllocFailure, - "Allocation of '%s' failed. (%llu bytes)", - "recv_buffer", - sizeof(QUIC_RECV_CHUNK) + AllocBufferLength); - return QUIC_STATUS_OUT_OF_MEMORY; + QUIC_RECV_CHUNK* Chunk = NULL; + if (PreallocatedChunk != NULL) { + Chunk = PreallocatedChunk; + } else { + Chunk = CXPLAT_ALLOC_NONPAGED(sizeof(QUIC_RECV_CHUNK) + AllocBufferLength, QUIC_POOL_RECVBUF); + if (Chunk == NULL) { + QuicTraceEvent( + AllocFailure, + "Allocation of '%s' failed. (%llu bytes)", + "recv_buffer", + sizeof(QUIC_RECV_CHUNK) + AllocBufferLength); + return QUIC_STATUS_OUT_OF_MEMORY; + } + QuicRecvChunkInitialize(Chunk, AllocBufferLength, (uint8_t*)(Chunk + 1), FALSE); } - QuicRecvChunkInitialize(Chunk, AllocBufferLength, (uint8_t*)(Chunk + 1), FALSE); + CxPlatListInsertHead(&RecvBuffer->Chunks, &Chunk->Link); + RecvBuffer->Capacity = AllocBufferLength; + } else { + RecvBuffer->Capacity = 0; } - CxPlatListInsertTail(&RecvBuffer->Chunks, &Chunk->Link); - - QuicRecvBufferValidate(RecvBuffer); return QUIC_STATUS_SUCCESS; } @@ -332,7 +323,6 @@ QuicRecvBufferUninitialize( ) { QuicRangeUninitialize(&RecvBuffer->WrittenRanges); - while (!CxPlatListIsEmpty(&RecvBuffer->Chunks)) { QUIC_RECV_CHUNK* Chunk = CXPLAT_CONTAINING_RECORD( @@ -624,8 +614,8 @@ QuicRecvBufferCopyIntoChunks( } // -// Copies only the new (non-overlapping) bytes from WriteBuffer into the receive buffer chunks, -// skipping any byte ranges already present in WrittenRanges. +// Copies only the new (non-overlapping) bytes from WriteBuffer into the receive +// buffer chunks, skipping any byte ranges already present in WrittenRanges. // Must be called before QuicRangeAddRange updates WrittenRanges. // _IRQL_requires_max_(DISPATCH_LEVEL) @@ -644,7 +634,9 @@ QuicRecvBufferWriteNewBytes( // Walk existing WrittenRanges to find gaps within [WriteOffset, WriteEnd). // For each gap, copy only those new bytes into the chunks. // - for (uint32_t i = 0; i < RecvBuffer->WrittenRanges.UsedLength && CopyOffset < WriteEnd; i++) { + for (uint32_t i = 0; + i < RecvBuffer->WrittenRanges.UsedLength && CopyOffset < WriteEnd; + i++) { QUIC_SUBRANGE* Sub = QuicRangeGet(&RecvBuffer->WrittenRanges, i); // @@ -1154,4 +1146,4 @@ QuicRecvBufferResetRead( Link); Chunk->ExternalReference = FALSE; RecvBuffer->ReadPendingLength = 0; -} \ No newline at end of file +} From 14ff1d5d9c73c73bc7f3bfe3f2e25f0b45c924bc Mon Sep 17 00:00:00 2001 From: MarkedMuichiro Date: Mon, 30 Mar 2026 20:45:10 +0000 Subject: [PATCH 4/8] test(recv_buffer): use sentinel values to validate overlap write correctnessAdd 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. --- src/core/unittest/RecvBufferTest.cpp | 193 +++++++++++++++++++++------ 1 file changed, 154 insertions(+), 39 deletions(-) diff --git a/src/core/unittest/RecvBufferTest.cpp b/src/core/unittest/RecvBufferTest.cpp index 618aecfce5..270f5842f8 100644 --- a/src/core/unittest/RecvBufferTest.cpp +++ b/src/core/unittest/RecvBufferTest.cpp @@ -159,6 +159,37 @@ struct RecvBuffer { Dump(); return Status; } + // + // WriteCustom allows the caller to provide an explicit data buffer, + // enabling tests to detect overwrite corruption by using sentinel values + // (e.g. 0xff) in the overlap region that differ from the original data. + // + QUIC_STATUS WriteCustom( + _In_ uint64_t WriteOffset, + _In_ uint16_t WriteLength, + _In_reads_bytes_(WriteLength) const uint8_t* Data, + _Inout_ uint64_t* WriteQuota, + _Out_ BOOLEAN* NewDataReady + ) { + printf("WriteCustom: Offset=%llu, Length=%u\n", (unsigned long long)WriteOffset, WriteLength); + uint64_t SizeNeeded = 0; + uint64_t QuotaConsumed = 0; + auto Status = + QuicRecvBufferWrite( + &RecvBuf, + WriteOffset, + WriteLength, + Data, + *WriteQuota, + &QuotaConsumed, + NewDataReady, + &SizeNeeded); + if (QUIC_SUCCEEDED(Status)) { + *WriteQuota = QuotaConsumed; + } + Dump(); + return Status; + } void Read( _Out_ uint64_t* BufferOffset, _Inout_ uint32_t* BufferCount, @@ -249,6 +280,16 @@ struct RecvBuffer { ASSERT_EQ((uint8_t)(BufferOffset + i), Buffer[i]); } } + // Validates the buffer matches an explicitly provided expected array. + static void ValidateBufferCustom( + _In_reads_(BufferLength) const uint8_t* Buffer, + _In_ uint32_t BufferLength, + _In_reads_(BufferLength) const uint8_t* Expected + ) { + for (uint32_t i = 0; i < BufferLength; ++i) { + ASSERT_EQ(Expected[i], Buffer[i]); + } + } void Dump() { printf("RecvBuffer: %p [mode=%u,vlen=%u,base=%llu,pending=%llu]\n", &RecvBuf, RecvBuf.RecvMode, RecvBuf.VirtualBufferLength, (unsigned long long)RecvBuf.BaseOffset, (unsigned long long)RecvBuf.ReadPendingLength); @@ -2208,98 +2249,150 @@ TEST(RecvBufferResetReadTest, ResetClearsPendingAndExternalReference_SingleMode) // Tests for overlapping write behavior - verifies that overlapping writes // do not corrupt previously received data. +// +// Each test uses WriteCustom to write sentinel values (0xff) in the overlap +// region of the second write. If the fix is working, the original bytes +// survive. If broken, the 0xff values would overwrite them and +// ValidateBufferCustom would catch the corruption. TEST_P(WithMode, OverlapWritePreservesExistingData) { // - // Write bytes 0-19, then write bytes 10-29 (overlapping). - // Verify the final buffer contains correct data throughout. + // 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. // RecvBuffer RecvBuf; ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.Initialize(GetParam())); + // Write [0, 20) — pattern: byte[i] = (uint8_t)i uint64_t InOutWriteLength = LARGE_TEST_BUFFER_LENGTH; BOOLEAN NewDataReady = FALSE; - - // Write [0, 20) ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.Write(0, 20, &InOutWriteLength, &NewDataReady)); ASSERT_TRUE(NewDataReady); - // Write [10, 30) — overlaps with [10, 20) already written + // Write [10, 30) with 0xff in [10,20) and valid pattern in [20,30). + // If the fix is broken, bytes [10,20) would be overwritten with 0xff. + uint8_t SecondWrite[20]; + for (uint16_t i = 0; i < 10; ++i) { + SecondWrite[i] = 0xff; // sentinel — should NOT land in the buffer + } + for (uint16_t i = 10; i < 20; ++i) { + SecondWrite[i] = (uint8_t)(20 + i); // valid new data for [20,30) + } InOutWriteLength = LARGE_TEST_BUFFER_LENGTH; NewDataReady = FALSE; - ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.Write(10, 20, &InOutWriteLength, &NewDataReady)); + ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.WriteCustom(10, 20, SecondWrite, &InOutWriteLength, &NewDataReady)); ASSERT_TRUE(NewDataReady); ASSERT_EQ(30ull, RecvBuf.GetTotalLength()); - // Read and verify data integrity — buffer[i] should equal (uint8_t)i + // Build expected buffer: + // [0,20) from first write: byte[i] = (uint8_t)i + // [20,30) from second write: SecondWrite[i-10] = (uint8_t)(20 + (i-10)) = (uint8_t)(i+10) + uint8_t Expected[30]; + for (uint32_t i = 0; i < 20; ++i) { + Expected[i] = (uint8_t)i; + } + for (uint32_t i = 20; i < 30; ++i) { + Expected[i] = (uint8_t)(i + 10); + } + uint64_t ReadOffset; QUIC_BUFFER ReadBuffers[3]; uint32_t BufferCount = ARRAYSIZE(ReadBuffers); - RecvBuf.Read(&ReadOffset, &BufferCount, ReadBuffers); + QuicRecvBufferRead(&RecvBuf.RecvBuf, &ReadOffset, &BufferCount, ReadBuffers); ASSERT_EQ(0ull, ReadOffset); ASSERT_GE(BufferCount, 1u); - - // Validate first 20 bytes were not corrupted by the overlapping write - RecvBuffer::ValidateBuffer(ReadBuffers[0].Buffer, ReadBuffers[0].Length, ReadOffset); + uint32_t TotalRead = 0; + for (uint32_t b = 0; b < BufferCount; ++b) { + RecvBuffer::ValidateBufferCustom(ReadBuffers[b].Buffer, ReadBuffers[b].Length, Expected + TotalRead); + TotalRead += ReadBuffers[b].Length; + } + ASSERT_EQ(30u, TotalRead); RecvBuf.Drain(30); } TEST_P(WithMode, OverlapWriteAtFront) { // - // Write bytes 10-29, then write bytes 0-19 (fills gap and overlaps). - // Verify the entire buffer [0, 30) is intact. + // Write [10, 30) with pattern A first. + // Then write [0, 20) with valid new data in [0,10) and 0xff in [10,20). + // Verify [10, 30) still contains pattern A, and [0,10) has the new data. // RecvBuffer RecvBuf; ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.Initialize(GetParam())); + // Write [10, 30) — pattern: byte[i] = (uint8_t)(10 + i) for i in [0,20) uint64_t InOutWriteLength = LARGE_TEST_BUFFER_LENGTH; BOOLEAN NewDataReady = FALSE; - - // Write [10, 30) first ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.Write(10, 20, &InOutWriteLength, &NewDataReady)); ASSERT_FALSE(NewDataReady); - // Write [0, 20) — fills gap [0,10) and overlaps [10,20) + // Write [0, 20) with valid data in [0,10) and 0xff sentinel in [10,20). + // If fix is broken, bytes [10,20) would be overwritten with 0xff. + uint8_t SecondWrite[20]; + for (uint16_t i = 0; i < 10; ++i) { + SecondWrite[i] = (uint8_t)i; // new valid data for [0,10) + } + for (uint16_t i = 10; i < 20; ++i) { + SecondWrite[i] = 0xff; // sentinel — should NOT land in the buffer + } InOutWriteLength = LARGE_TEST_BUFFER_LENGTH; NewDataReady = FALSE; - ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.Write(0, 20, &InOutWriteLength, &NewDataReady)); + ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.WriteCustom(0, 20, SecondWrite, &InOutWriteLength, &NewDataReady)); ASSERT_TRUE(NewDataReady); ASSERT_EQ(30ull, RecvBuf.GetTotalLength()); - // Read and verify full buffer integrity + // Build expected: [0,10) from second write, [10,30) from first write + uint8_t Expected[30]; + for (uint32_t i = 0; i < 10; ++i) { + Expected[i] = (uint8_t)i; + } + for (uint32_t i = 10; i < 30; ++i) { + Expected[i] = (uint8_t)i; // Write(10, 20) fills byte[j] = (uint8_t)(10+j) for j in [0,20) + } + uint64_t ReadOffset; QUIC_BUFFER ReadBuffers[3]; uint32_t BufferCount = ARRAYSIZE(ReadBuffers); - RecvBuf.Read(&ReadOffset, &BufferCount, ReadBuffers); + QuicRecvBufferRead(&RecvBuf.RecvBuf, &ReadOffset, &BufferCount, ReadBuffers); ASSERT_EQ(0ull, ReadOffset); - RecvBuffer::ValidateBuffer(ReadBuffers[0].Buffer, ReadBuffers[0].Length, ReadOffset); + uint32_t TotalRead = 0; + for (uint32_t b = 0; b < BufferCount; ++b) { + RecvBuffer::ValidateBufferCustom(ReadBuffers[b].Buffer, ReadBuffers[b].Length, Expected + TotalRead); + TotalRead += ReadBuffers[b].Length; + } + ASSERT_EQ(30u, TotalRead); RecvBuf.Drain(30); } TEST_P(WithMode, OverlapWriteExactDuplicate) { // - // Write the same range twice. Second write should be a no-op - // and data should be unchanged. + // Write [0, 20) with pattern A. + // Then write the same range with all 0xff sentinel values. + // Verify the buffer still contains pattern A — the duplicate write + // should be a no-op. // RecvBuffer RecvBuf; ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.Initialize(GetParam())); uint64_t InOutWriteLength = LARGE_TEST_BUFFER_LENGTH; BOOLEAN NewDataReady = FALSE; - ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.Write(0, 20, &InOutWriteLength, &NewDataReady)); ASSERT_TRUE(NewDataReady); - // Write same range again + // Write same range again with 0xff sentinel — should be fully ignored + uint8_t Sentinel[20]; + memset(Sentinel, 0xff, sizeof(Sentinel)); InOutWriteLength = LARGE_TEST_BUFFER_LENGTH; NewDataReady = FALSE; - ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.Write(0, 20, &InOutWriteLength, &NewDataReady)); - ASSERT_EQ(0ull, InOutWriteLength); // No new bytes + ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.WriteCustom(0, 20, Sentinel, &InOutWriteLength, &NewDataReady)); + ASSERT_EQ(0ull, InOutWriteLength); // No new bytes written ASSERT_EQ(20ull, RecvBuf.GetTotalLength()); + // Buffer should still contain original pattern A uint64_t ReadOffset; QUIC_BUFFER ReadBuffers[3]; uint32_t BufferCount = ARRAYSIZE(ReadBuffers); @@ -2312,10 +2405,10 @@ TEST_P(WithMode, OverlapWriteExactDuplicate) TEST_P(WithMode, OverlapWriteMultipleGaps) { // - // Write alternating chunks creating a swiss-cheese pattern, - // then write a large range covering everything. - // Verify no existing data is corrupted. - // Pattern: write [0,5), [10,15), [20,25) then write [0,30) + // Write [0,5), [10,15), [20,25) with pattern A. + // Then write [0,30) with 0xff in the already-written regions + // and valid pattern in the gaps [5,10), [15,20), [25,30). + // Verify the originally written bytes survive and the gaps are filled. // RecvBuffer RecvBuf; ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.Initialize(GetParam(), false, @@ -2326,27 +2419,49 @@ TEST_P(WithMode, OverlapWriteMultipleGaps) ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.Write(0, 5, &InOutWriteLength, &NewDataReady)); ASSERT_TRUE(NewDataReady); - InOutWriteLength = LARGE_TEST_BUFFER_LENGTH; ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.Write(10, 5, &InOutWriteLength, &NewDataReady)); - InOutWriteLength = LARGE_TEST_BUFFER_LENGTH; ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.Write(20, 5, &InOutWriteLength, &NewDataReady)); - // Now write [0, 30) covering all gaps and existing data + // Write [0,30) with 0xff in already-written regions [0,5), [10,15), [20,25) + // and valid gap-fill data in [5,10), [15,20), [25,30). + // If fix is broken, the 0xff values overwrite the originally written bytes. + uint8_t BigWrite[30]; + for (uint16_t i = 0; i < 30; ++i) { + if ((i < 5) || (i >= 10 && i < 15) || (i >= 20 && i < 25)) { + BigWrite[i] = 0xff; // sentinel over already-written regions + } else { + BigWrite[i] = (uint8_t)i; // valid fill for gaps + } + } InOutWriteLength = LARGE_TEST_BUFFER_LENGTH; NewDataReady = FALSE; - ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.Write(0, 30, &InOutWriteLength, &NewDataReady)); + ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.WriteCustom(0, 30, BigWrite, &InOutWriteLength, &NewDataReady)); ASSERT_TRUE(NewDataReady); ASSERT_EQ(30ull, RecvBuf.GetTotalLength()); - // Verify data — bytes 0-4, 10-14, 20-24 were written first - // and should not have been overwritten + // Build expected: originally written regions keep pattern A, + // gaps get their fill values from BigWrite + uint8_t Expected[30]; + for (uint32_t i = 0; i < 30; ++i) { + if ((i < 5) || (i >= 10 && i < 15) || (i >= 20 && i < 25)) { + Expected[i] = (uint8_t)i; // original Write() pattern + } else { + Expected[i] = (uint8_t)i; // gap fill — same value by coincidence + } + } + uint64_t ReadOffset; QUIC_BUFFER ReadBuffers[3]; uint32_t BufferCount = ARRAYSIZE(ReadBuffers); - RecvBuf.Read(&ReadOffset, &BufferCount, ReadBuffers); + QuicRecvBufferRead(&RecvBuf.RecvBuf, &ReadOffset, &BufferCount, ReadBuffers); ASSERT_EQ(0ull, ReadOffset); - RecvBuffer::ValidateBuffer(ReadBuffers[0].Buffer, ReadBuffers[0].Length, ReadOffset); + uint32_t TotalRead = 0; + for (uint32_t b = 0; b < BufferCount; ++b) { + RecvBuffer::ValidateBufferCustom(ReadBuffers[b].Buffer, ReadBuffers[b].Length, Expected + TotalRead); + TotalRead += ReadBuffers[b].Length; + } + ASSERT_EQ(30u, TotalRead); RecvBuf.Drain(30); -} +} \ No newline at end of file From d185fb8eee6065a909fbef76d0e7aae5d5298f45 Mon Sep 17 00:00:00 2001 From: MarkedMuichiro Date: Mon, 30 Mar 2026 12:40:26 -0700 Subject: [PATCH 5/8] Update src/core/recv_buffer.c Update comment. Co-authored-by: Guillaume Hetier --- src/core/recv_buffer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/recv_buffer.c b/src/core/recv_buffer.c index 60614f7454..659d20978b 100644 --- a/src/core/recv_buffer.c +++ b/src/core/recv_buffer.c @@ -809,7 +809,7 @@ QuicRecvBufferWrite( *NewDataReady = UpdatedRange->Low == 0; // - // Update the amount of data readable in the first chunk, committing the write. + // Update the amount of data readable in the first chunk. // QUIC_SUBRANGE* FirstRange = QuicRangeGet(&RecvBuffer->WrittenRanges, 0); if (FirstRange->Low == 0) { From 30c2493df9889c3527f76519e641f315921854e2 Mon Sep 17 00:00:00 2001 From: MarkedMuichiro Date: Tue, 31 Mar 2026 00:23:51 +0000 Subject: [PATCH 6/8] test(recv_buffer): simplify validation using memcmp, clean up comments --- src/core/unittest/RecvBufferTest.cpp | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/core/unittest/RecvBufferTest.cpp b/src/core/unittest/RecvBufferTest.cpp index 270f5842f8..16b70e196a 100644 --- a/src/core/unittest/RecvBufferTest.cpp +++ b/src/core/unittest/RecvBufferTest.cpp @@ -2251,9 +2251,8 @@ TEST(RecvBufferResetReadTest, ResetClearsPendingAndExternalReference_SingleMode) // do not corrupt previously received data. // // Each test uses WriteCustom to write sentinel values (0xff) in the overlap -// region of the second write. If the fix is working, the original bytes -// survive. If broken, the 0xff values would overwrite them and -// ValidateBufferCustom would catch the corruption. +// region of the second write, so that any unintended overwrite of already- +// received bytes would be detectable when validating the read output. TEST_P(WithMode, OverlapWritePreservesExistingData) { @@ -2273,10 +2272,10 @@ TEST_P(WithMode, OverlapWritePreservesExistingData) ASSERT_TRUE(NewDataReady); // Write [10, 30) with 0xff in [10,20) and valid pattern in [20,30). - // If the fix is broken, bytes [10,20) would be overwritten with 0xff. + // The 0xff sentinel values cover the already-written region. uint8_t SecondWrite[20]; for (uint16_t i = 0; i < 10; ++i) { - SecondWrite[i] = 0xff; // sentinel — should NOT land in the buffer + SecondWrite[i] = 0xff; // sentinel over already-written region [10,20) } for (uint16_t i = 10; i < 20; ++i) { SecondWrite[i] = (uint8_t)(20 + i); // valid new data for [20,30) @@ -2306,7 +2305,7 @@ TEST_P(WithMode, OverlapWritePreservesExistingData) ASSERT_GE(BufferCount, 1u); uint32_t TotalRead = 0; for (uint32_t b = 0; b < BufferCount; ++b) { - RecvBuffer::ValidateBufferCustom(ReadBuffers[b].Buffer, ReadBuffers[b].Length, Expected + TotalRead); + ASSERT_EQ(0, memcmp(ReadBuffers[b].Buffer, Expected + TotalRead, ReadBuffers[b].Length)); TotalRead += ReadBuffers[b].Length; } ASSERT_EQ(30u, TotalRead); @@ -2330,13 +2329,13 @@ TEST_P(WithMode, OverlapWriteAtFront) ASSERT_FALSE(NewDataReady); // Write [0, 20) with valid data in [0,10) and 0xff sentinel in [10,20). - // If fix is broken, bytes [10,20) would be overwritten with 0xff. + // The 0xff sentinel values cover the already-written region. uint8_t SecondWrite[20]; for (uint16_t i = 0; i < 10; ++i) { SecondWrite[i] = (uint8_t)i; // new valid data for [0,10) } for (uint16_t i = 10; i < 20; ++i) { - SecondWrite[i] = 0xff; // sentinel — should NOT land in the buffer + SecondWrite[i] = 0xff; // sentinel over already-written region [10,20) } InOutWriteLength = LARGE_TEST_BUFFER_LENGTH; NewDataReady = FALSE; @@ -2360,7 +2359,7 @@ TEST_P(WithMode, OverlapWriteAtFront) ASSERT_EQ(0ull, ReadOffset); uint32_t TotalRead = 0; for (uint32_t b = 0; b < BufferCount; ++b) { - RecvBuffer::ValidateBufferCustom(ReadBuffers[b].Buffer, ReadBuffers[b].Length, Expected + TotalRead); + ASSERT_EQ(0, memcmp(ReadBuffers[b].Buffer, Expected + TotalRead, ReadBuffers[b].Length)); TotalRead += ReadBuffers[b].Length; } ASSERT_EQ(30u, TotalRead); @@ -2426,7 +2425,6 @@ TEST_P(WithMode, OverlapWriteMultipleGaps) // Write [0,30) with 0xff in already-written regions [0,5), [10,15), [20,25) // and valid gap-fill data in [5,10), [15,20), [25,30). - // If fix is broken, the 0xff values overwrite the originally written bytes. uint8_t BigWrite[30]; for (uint16_t i = 0; i < 30; ++i) { if ((i < 5) || (i >= 10 && i < 15) || (i >= 20 && i < 25)) { @@ -2459,7 +2457,7 @@ TEST_P(WithMode, OverlapWriteMultipleGaps) ASSERT_EQ(0ull, ReadOffset); uint32_t TotalRead = 0; for (uint32_t b = 0; b < BufferCount; ++b) { - RecvBuffer::ValidateBufferCustom(ReadBuffers[b].Buffer, ReadBuffers[b].Length, Expected + TotalRead); + ASSERT_EQ(0, memcmp(ReadBuffers[b].Buffer, Expected + TotalRead, ReadBuffers[b].Length)); TotalRead += ReadBuffers[b].Length; } ASSERT_EQ(30u, TotalRead); From d104bd3e8ee64bd5746d5ba1e8f2c9b4be1c1f7e Mon Sep 17 00:00:00 2001 From: MarkedMuichiro Date: Tue, 31 Mar 2026 09:56:40 -0700 Subject: [PATCH 7/8] Update src/core/unittest/RecvBufferTest.cpp Remove dead code. Co-authored-by: Guillaume Hetier --- src/core/unittest/RecvBufferTest.cpp | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/src/core/unittest/RecvBufferTest.cpp b/src/core/unittest/RecvBufferTest.cpp index 16b70e196a..165a7cd47f 100644 --- a/src/core/unittest/RecvBufferTest.cpp +++ b/src/core/unittest/RecvBufferTest.cpp @@ -280,16 +280,6 @@ struct RecvBuffer { ASSERT_EQ((uint8_t)(BufferOffset + i), Buffer[i]); } } - // Validates the buffer matches an explicitly provided expected array. - static void ValidateBufferCustom( - _In_reads_(BufferLength) const uint8_t* Buffer, - _In_ uint32_t BufferLength, - _In_reads_(BufferLength) const uint8_t* Expected - ) { - for (uint32_t i = 0; i < BufferLength; ++i) { - ASSERT_EQ(Expected[i], Buffer[i]); - } - } void Dump() { printf("RecvBuffer: %p [mode=%u,vlen=%u,base=%llu,pending=%llu]\n", &RecvBuf, RecvBuf.RecvMode, RecvBuf.VirtualBufferLength, (unsigned long long)RecvBuf.BaseOffset, (unsigned long long)RecvBuf.ReadPendingLength); From cbf137dcf9bfdd67f40141411f8867eca4f2d81e Mon Sep 17 00:00:00 2001 From: MarkedMuichiro Date: Mon, 6 Apr 2026 23:52:41 +0000 Subject: [PATCH 8/8] test(recv_buffer): add three-range overlap and contiguous non-overlap tests --- src/core/unittest/RecvBufferTest.cpp | 105 +++++++++++++++++++++++++++ 1 file changed, 105 insertions(+) diff --git a/src/core/unittest/RecvBufferTest.cpp b/src/core/unittest/RecvBufferTest.cpp index 165a7cd47f..90ecfc8e9c 100644 --- a/src/core/unittest/RecvBufferTest.cpp +++ b/src/core/unittest/RecvBufferTest.cpp @@ -2440,6 +2440,111 @@ TEST_P(WithMode, OverlapWriteMultipleGaps) } } + uint64_t ReadOffset; + QUIC_BUFFER ReadBuffers[3]; + uint32_t BufferCount = ARRAYSIZE(ReadBuffers); + QuicRecvBufferRead(&RecvBuf.RecvBuf, &ReadOffset, &BufferCount, ReadBuffers); + ASSERT_EQ(0ull, ReadOffset); + uint32_t TotalRead = 0; + for (uint32_t b = 0; b < BufferCount; ++b) { + ASSERT_EQ(0, memcmp(ReadBuffers[b].Buffer, Expected + TotalRead, ReadBuffers[b].Length)); + TotalRead += ReadBuffers[b].Length; + } + ASSERT_EQ(30u, TotalRead); + RecvBuf.Drain(30); +} +TEST_P(WithMode, OverlapWriteThreeRangesTwoOverlaps) +{ + // + // Write [0, 15), then [10, 25) overlapping [10, 15), then [20, 30) overlapping [20, 25). + // Sentinel values (0xff) cover both overlap regions to verify existing data is preserved. + // + RecvBuffer RecvBuf; + ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.Initialize(GetParam())); + + uint64_t InOutWriteLength = LARGE_TEST_BUFFER_LENGTH; + BOOLEAN NewDataReady = FALSE; + + // Write [0, 15) with pattern: byte[i] = (uint8_t)i + ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.Write(0, 15, &InOutWriteLength, &NewDataReady)); + ASSERT_TRUE(NewDataReady); + + // Write [10, 25): 0xff sentinel in overlap [10, 15), valid data in [15, 25) + uint8_t SecondWrite[15]; + for (uint16_t i = 0; i < 5; ++i) SecondWrite[i] = 0xff; + for (uint16_t i = 5; i < 15; ++i) SecondWrite[i] = (uint8_t)(10 + i); + InOutWriteLength = LARGE_TEST_BUFFER_LENGTH; + NewDataReady = FALSE; + ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.WriteCustom(10, 15, SecondWrite, &InOutWriteLength, &NewDataReady)); + ASSERT_TRUE(NewDataReady); + + // Write [20, 30): 0xff sentinel in overlap [20, 25), valid data in [25, 30) + uint8_t ThirdWrite[10]; + for (uint16_t i = 0; i < 5; ++i) ThirdWrite[i] = 0xff; + for (uint16_t i = 5; i < 10; ++i) ThirdWrite[i] = (uint8_t)(20 + i); + InOutWriteLength = LARGE_TEST_BUFFER_LENGTH; + NewDataReady = FALSE; + ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.WriteCustom(20, 10, ThirdWrite, &InOutWriteLength, &NewDataReady)); + ASSERT_TRUE(NewDataReady); + ASSERT_EQ(30ull, RecvBuf.GetTotalLength()); + + // Expected: [0,15) from first write, [15,25) from SecondWrite[5..14], [25,30) from ThirdWrite[5..9] + uint8_t Expected[30]; + for (uint32_t i = 0; i < 15; ++i) Expected[i] = (uint8_t)i; + for (uint32_t i = 15; i < 25; ++i) Expected[i] = (uint8_t)(10 + (i - 10)); + for (uint32_t i = 25; i < 30; ++i) Expected[i] = (uint8_t)(20 + (i - 20)); + + uint64_t ReadOffset; + QUIC_BUFFER ReadBuffers[3]; + uint32_t BufferCount = ARRAYSIZE(ReadBuffers); + QuicRecvBufferRead(&RecvBuf.RecvBuf, &ReadOffset, &BufferCount, ReadBuffers); + ASSERT_EQ(0ull, ReadOffset); + uint32_t TotalRead = 0; + for (uint32_t b = 0; b < BufferCount; ++b) { + ASSERT_EQ(0, memcmp(ReadBuffers[b].Buffer, Expected + TotalRead, ReadBuffers[b].Length)); + TotalRead += ReadBuffers[b].Length; + } + ASSERT_EQ(30u, TotalRead); + RecvBuf.Drain(30); +} + +TEST_P(WithMode, OverlapWriteThreeContiguousNonOverlapping) +{ + // + // Write three non-overlapping contiguous ranges: [0,10), [10,20), [20,30). + // Ranges touch but do not overlap. All data should land correctly without corruption. + // + RecvBuffer RecvBuf; + ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.Initialize(GetParam())); + + uint64_t InOutWriteLength = LARGE_TEST_BUFFER_LENGTH; + BOOLEAN NewDataReady = FALSE; + + // Write [0, 10) with pattern: byte[i] = (uint8_t)i + ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.Write(0, 10, &InOutWriteLength, &NewDataReady)); + ASSERT_TRUE(NewDataReady); + + // Write [10, 20) — contiguous, no overlap + uint8_t SecondWrite[10]; + for (uint16_t i = 0; i < 10; ++i) SecondWrite[i] = (uint8_t)(10 + i); + InOutWriteLength = LARGE_TEST_BUFFER_LENGTH; + NewDataReady = FALSE; + ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.WriteCustom(10, 10, SecondWrite, &InOutWriteLength, &NewDataReady)); + ASSERT_TRUE(NewDataReady); + + // Write [20, 30) — contiguous, no overlap + uint8_t ThirdWrite[10]; + for (uint16_t i = 0; i < 10; ++i) ThirdWrite[i] = (uint8_t)(20 + i); + InOutWriteLength = LARGE_TEST_BUFFER_LENGTH; + NewDataReady = FALSE; + ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.WriteCustom(20, 10, ThirdWrite, &InOutWriteLength, &NewDataReady)); + ASSERT_TRUE(NewDataReady); + ASSERT_EQ(30ull, RecvBuf.GetTotalLength()); + + // Expected: byte[i] = (uint8_t)i across all 30 bytes + uint8_t Expected[30]; + for (uint32_t i = 0; i < 30; ++i) Expected[i] = (uint8_t)i; + uint64_t ReadOffset; QUIC_BUFFER ReadBuffers[3]; uint32_t BufferCount = ARRAYSIZE(ReadBuffers);