From c117aeb6807892ccf484686e1bb02f592d848162 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Fri, 22 Mar 2024 12:50:29 +0100 Subject: [PATCH 1/7] Don't do full drain on chunks containing future buffered data --- src/core/recv_buffer.c | 7 +++- src/core/unittest/RecvBufferTest.cpp | 56 ++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 1 deletion(-) diff --git a/src/core/recv_buffer.c b/src/core/recv_buffer.c index b27a01fcef..0b89768aec 100644 --- a/src/core/recv_buffer.c +++ b/src/core/recv_buffer.c @@ -907,7 +907,12 @@ QuicRecvBufferDrain( } do { - if ((uint64_t)RecvBuffer->ReadLength > DrainLength) { + if ((uint64_t)RecvBuffer->ReadLength > DrainLength || + // + // If there are more than 2 written ranges, it means that there may be + // more data later in the chunk that couldn't be read because there is a gap. + // + RecvBuffer->WrittenRanges.UsedLength > 1) { QuicRecvBufferPartialDrain(RecvBuffer, DrainLength); return FALSE; } diff --git a/src/core/unittest/RecvBufferTest.cpp b/src/core/unittest/RecvBufferTest.cpp index 4f3a1eb0e0..17a0d2267d 100644 --- a/src/core/unittest/RecvBufferTest.cpp +++ b/src/core/unittest/RecvBufferTest.cpp @@ -625,6 +625,62 @@ TEST_P(WithMode, RecvCrypto) // Note - Values based on a previous failing MsQuic } } +TEST_P(WithMode, DrainFrontChunkWithPendingGap) +{ + RecvBuffer RecvBuf; + ASSERT_EQ(QUIC_STATUS_SUCCESS, RecvBuf.Initialize(GetParam(), false)); + uint64_t InOutWriteLength = DEF_TEST_BUFFER_LENGTH; + BOOLEAN NewDataReady = FALSE; + + // place data at some future offset + ASSERT_EQ( + QUIC_STATUS_SUCCESS, + RecvBuf.Write( + 2, + 1, + &InOutWriteLength, + &NewDataReady)); + ASSERT_FALSE(RecvBuf.HasUnreadData()); + + // place data to the front and drain + InOutWriteLength = DEF_TEST_BUFFER_LENGTH; + ASSERT_EQ( + QUIC_STATUS_SUCCESS, + RecvBuf.Write( + 0, + 1, + &InOutWriteLength, + &NewDataReady)); + ASSERT_TRUE(NewDataReady); + + uint64_t ReadOffset; + QUIC_BUFFER ReadBuffers[3]; + uint32_t BufferCount = ARRAYSIZE(ReadBuffers); + RecvBuf.Read(&ReadOffset, &BufferCount, ReadBuffers); + ASSERT_EQ(1ul, BufferCount); + ASSERT_EQ(1ul, ReadBuffers[0].Length); + RecvBuf.Drain(1); + + // insert missing chunk and drain the rest + InOutWriteLength = DEF_TEST_BUFFER_LENGTH; + ASSERT_EQ( + QUIC_STATUS_SUCCESS, + RecvBuf.Write( + 1, + 1, + &InOutWriteLength, + &NewDataReady)); + ASSERT_TRUE(NewDataReady); + + BufferCount = ARRAYSIZE(ReadBuffers); + RecvBuf.Read(&ReadOffset, &BufferCount, ReadBuffers); + uint64_t TotalRead = 0; + for (uint32_t i = 0; i < BufferCount; ++i) { + TotalRead += ReadBuffers[i].Length; + } + ASSERT_EQ(2ul, TotalRead); +} + INSTANTIATE_TEST_SUITE_P( RecvBufferTest, WithMode, From 9498d50418b83cf325b2f224e0077ffb68e8d34c Mon Sep 17 00:00:00 2001 From: Radek Zikmund <32671551+rzikm@users.noreply.github.com> Date: Mon, 25 Mar 2024 16:01:37 +0100 Subject: [PATCH 2/7] Fix comment --- 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 0b89768aec..3a63461a67 100644 --- a/src/core/recv_buffer.c +++ b/src/core/recv_buffer.c @@ -909,7 +909,7 @@ QuicRecvBufferDrain( do { if ((uint64_t)RecvBuffer->ReadLength > DrainLength || // - // If there are more than 2 written ranges, it means that there may be + // If there are 2 or more written ranges, it means that there may be // more data later in the chunk that couldn't be read because there is a gap. // RecvBuffer->WrittenRanges.UsedLength > 1) { From 0e2341ff7a8563f50b9fc2c91a04000e5f2b2441 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Mon, 8 Apr 2024 16:14:28 +0200 Subject: [PATCH 3/7] Fix --- src/core/recv_buffer.c | 14 +++++++++----- src/core/unittest/RecvBufferTest.cpp | 14 +++++++++++++- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/src/core/recv_buffer.c b/src/core/recv_buffer.c index 3a63461a67..a6ad44c7e9 100644 --- a/src/core/recv_buffer.c +++ b/src/core/recv_buffer.c @@ -830,7 +830,8 @@ QuicRecvBufferPartialDrain( } // -// Handles draining the entire first chunk (and possibly more). Return the new +// Handles draining the entire first chunk (and possibly more). This function +// expects the chunk to not contain more (unread) data. Return the new // drain length. // _IRQL_requires_max_(DISPATCH_LEVEL) @@ -907,20 +908,23 @@ QuicRecvBufferDrain( } do { - if ((uint64_t)RecvBuffer->ReadLength > DrainLength || + BOOLEAN PartialDrain = (uint64_t)RecvBuffer->ReadLength > DrainLength; + if (PartialDrain || // // If there are 2 or more written ranges, it means that there may be // more data later in the chunk that couldn't be read because there is a gap. + // Reuse the partial drain logic to preserve data after the gap. // - RecvBuffer->WrittenRanges.UsedLength > 1) { + QuicRangeSize(&RecvBuffer->WrittenRanges) > 1) { QuicRecvBufferPartialDrain(RecvBuffer, DrainLength); - return FALSE; + return PartialDrain; } DrainLength = QuicRecvBufferFullDrain(RecvBuffer, DrainLength); } while (DrainLength != 0); - return TRUE; + // return TRUE; + return QuicRecvBufferHasUnreadData(RecvBuffer); } _IRQL_requires_max_(DISPATCH_LEVEL) diff --git a/src/core/unittest/RecvBufferTest.cpp b/src/core/unittest/RecvBufferTest.cpp index 17a0d2267d..9367423fad 100644 --- a/src/core/unittest/RecvBufferTest.cpp +++ b/src/core/unittest/RecvBufferTest.cpp @@ -659,7 +659,7 @@ TEST_P(WithMode, DrainFrontChunkWithPendingGap) RecvBuf.Read(&ReadOffset, &BufferCount, ReadBuffers); ASSERT_EQ(1ul, BufferCount); ASSERT_EQ(1ul, ReadBuffers[0].Length); - RecvBuf.Drain(1); + ASSERT_FALSE(RecvBuf.Drain(1)); // insert missing chunk and drain the rest InOutWriteLength = DEF_TEST_BUFFER_LENGTH; @@ -679,6 +679,18 @@ TEST_P(WithMode, DrainFrontChunkWithPendingGap) TotalRead += ReadBuffers[i].Length; } ASSERT_EQ(2ul, TotalRead); + ASSERT_TRUE(RecvBuf.Drain(1)); // more data left in buffer + + if (GetParam() != QUIC_RECV_BUF_MODE_MULTIPLE) { + BufferCount = ARRAYSIZE(ReadBuffers); + RecvBuf.Read(&ReadOffset, &BufferCount, ReadBuffers); + TotalRead = 0; + for (uint32_t i = 0; i < BufferCount; ++i) { + TotalRead += ReadBuffers[i].Length; + } + ASSERT_EQ(1ul, TotalRead); + } + ASSERT_FALSE(RecvBuf.Drain(1)); } INSTANTIATE_TEST_SUITE_P( From c05854e674650e6abdf1a80385451609a363cee2 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Mon, 8 Apr 2024 16:18:48 +0200 Subject: [PATCH 4/7] Remove commented out code --- src/core/recv_buffer.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/core/recv_buffer.c b/src/core/recv_buffer.c index a6ad44c7e9..f088eb3e71 100644 --- a/src/core/recv_buffer.c +++ b/src/core/recv_buffer.c @@ -923,7 +923,6 @@ QuicRecvBufferDrain( DrainLength = QuicRecvBufferFullDrain(RecvBuffer, DrainLength); } while (DrainLength != 0); - // return TRUE; return QuicRecvBufferHasUnreadData(RecvBuffer); } From a1f7bb3293e55145ee1be93b2e170a680680727e Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Tue, 9 Apr 2024 09:54:49 +0200 Subject: [PATCH 5/7] Revert change which breaks tests --- 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 f088eb3e71..ed1f10c03d 100644 --- a/src/core/recv_buffer.c +++ b/src/core/recv_buffer.c @@ -923,7 +923,7 @@ QuicRecvBufferDrain( DrainLength = QuicRecvBufferFullDrain(RecvBuffer, DrainLength); } while (DrainLength != 0); - return QuicRecvBufferHasUnreadData(RecvBuffer); + return TRUE; } _IRQL_requires_max_(DISPATCH_LEVEL) From 7b02ecd0f81832d6775c2ff3a53902bb49715179 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Tue, 9 Apr 2024 12:28:47 +0200 Subject: [PATCH 6/7] Fix test failures --- 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 ed1f10c03d..8f9e844f1d 100644 --- a/src/core/recv_buffer.c +++ b/src/core/recv_buffer.c @@ -917,7 +917,7 @@ QuicRecvBufferDrain( // QuicRangeSize(&RecvBuffer->WrittenRanges) > 1) { QuicRecvBufferPartialDrain(RecvBuffer, DrainLength); - return PartialDrain; + return !PartialDrain; } DrainLength = QuicRecvBufferFullDrain(RecvBuffer, DrainLength); From bb006f471f5d645a0f1296a2296cf578dcff3d68 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Tue, 9 Apr 2024 12:45:10 +0200 Subject: [PATCH 7/7] Fix test --- src/core/unittest/RecvBufferTest.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/core/unittest/RecvBufferTest.cpp b/src/core/unittest/RecvBufferTest.cpp index 9367423fad..3b4a615a60 100644 --- a/src/core/unittest/RecvBufferTest.cpp +++ b/src/core/unittest/RecvBufferTest.cpp @@ -659,7 +659,7 @@ TEST_P(WithMode, DrainFrontChunkWithPendingGap) RecvBuf.Read(&ReadOffset, &BufferCount, ReadBuffers); ASSERT_EQ(1ul, BufferCount); ASSERT_EQ(1ul, ReadBuffers[0].Length); - ASSERT_FALSE(RecvBuf.Drain(1)); + ASSERT_TRUE(RecvBuf.Drain(1)); // insert missing chunk and drain the rest InOutWriteLength = DEF_TEST_BUFFER_LENGTH; @@ -679,7 +679,7 @@ TEST_P(WithMode, DrainFrontChunkWithPendingGap) TotalRead += ReadBuffers[i].Length; } ASSERT_EQ(2ul, TotalRead); - ASSERT_TRUE(RecvBuf.Drain(1)); // more data left in buffer + ASSERT_FALSE(RecvBuf.Drain(1)); // more data left in buffer if (GetParam() != QUIC_RECV_BUF_MODE_MULTIPLE) { BufferCount = ARRAYSIZE(ReadBuffers); @@ -690,7 +690,7 @@ TEST_P(WithMode, DrainFrontChunkWithPendingGap) } ASSERT_EQ(1ul, TotalRead); } - ASSERT_FALSE(RecvBuf.Drain(1)); + ASSERT_TRUE(RecvBuf.Drain(1)); } INSTANTIATE_TEST_SUITE_P(