Fix QuicStreamSequencerBuffer::PrefetchNextRegion() behavior after Clear().
If some data are prefetched then QuicStreamSequencerBuffer::Clear() is called,
then QuicStreamSequencerBuffer::FirstByteMissing() will take the value of
|total_bytes_read_|, which can be less than |total_bytes_prefetched_|. In this
case, QuicStreamSequencerBuffer::PrefetchNextRegion() used to return true, but
really ought to return false. This CL fixes that.
This CL addresses the root cause of the fuzzer-found ASAN crash at
https://crbug.com/969391. It is complementary to cr/253592180, which addresses
the stream level behavior. Either CLs would be enough to make that particular
crash go away, but they are both necessary as they fix different bugs.
gfe-relnote: Change in code only mean for QUIC v99, not flag protected.
We believe this change does not need flag protection, because it only affects |total_bytes_prefetched_|, which is only read in QuicStreamSequencerBuffer::PrefetchNextRegion(), which is only called (other than tests) in QuicStreamSequencer::PrefetchNextRegion(), which is only called in three places: QuicSpdyStream::OnDataAvailable() but only when using v99, QuicReceiveControlStream::OnDataAvailable() which is not currently wired up but will be v99-only anyway, and //depot/google3/vr/c9/playability/yperf/message_stream.cc in a galaxy far-far away. Therefore this change should not affect production GFE.
PiperOrigin-RevId: 253995330
Change-Id: I40b92da16dcf6ec1bdeda9de9ddeeb49ff3542f4
diff --git a/quic/core/quic_stream_sequencer_buffer.cc b/quic/core/quic_stream_sequencer_buffer.cc
index 7f0f2cb..9d239b1 100644
--- a/quic/core/quic_stream_sequencer_buffer.cc
+++ b/quic/core/quic_stream_sequencer_buffer.cc
@@ -53,6 +53,7 @@
num_bytes_buffered_ = 0;
bytes_received_.Clear();
bytes_received_.Add(0, total_bytes_read_);
+ total_bytes_prefetched_ = total_bytes_read_;
}
bool QuicStreamSequencerBuffer::RetireBlock(size_t idx) {
@@ -336,6 +337,8 @@
bool QuicStreamSequencerBuffer::PrefetchNextRegion(iovec* iov) {
DCHECK(iov != nullptr);
+ DCHECK_LE(total_bytes_read_, total_bytes_prefetched_);
+ DCHECK_LE(total_bytes_prefetched_, FirstMissingByte());
if (total_bytes_prefetched_ == FirstMissingByte()) {
return false;
diff --git a/quic/core/quic_stream_sequencer_buffer_test.cc b/quic/core/quic_stream_sequencer_buffer_test.cc
index 335aa2d..67d9b28 100644
--- a/quic/core/quic_stream_sequencer_buffer_test.cc
+++ b/quic/core/quic_stream_sequencer_buffer_test.cc
@@ -1078,6 +1078,31 @@
EXPECT_LE(bytes_to_buffer_, total_bytes_written_);
}
+// Regression test for https://crbug.com/969391.
+TEST_F(QuicStreamSequencerBufferTest, PrefetchAfterClear) {
+ // Write a few bytes.
+ const std::string kData("foo");
+ size_t written = 0;
+ EXPECT_EQ(QUIC_NO_ERROR, buffer_->OnStreamData(/* offset = */ 0, kData,
+ &written, &error_details_));
+ EXPECT_EQ(kData.size(), written);
+
+ // Prefetch all buffered data.
+ iovec iov;
+ EXPECT_TRUE(buffer_->PrefetchNextRegion(&iov));
+ EXPECT_EQ(kData, QuicStringPiece(reinterpret_cast<const char*>(iov.iov_base),
+ iov.iov_len));
+
+ // No more data to prefetch.
+ EXPECT_FALSE(buffer_->PrefetchNextRegion(&iov));
+
+ // Clear all buffered data.
+ buffer_->Clear();
+
+ // Still no data to prefetch.
+ EXPECT_FALSE(buffer_->PrefetchNextRegion(&iov));
+}
+
} // anonymous namespace
} // namespace test
