Do not accept fin that would decrease the stream's final offset. gfe-relnote: protected by gfe2_reloadable_flag_quic_no_decrease_in_final_offset PiperOrigin-RevId: 276115511 Change-Id: I6c6fa7e6f8a6374aceb7ef0c19756ea3928479c9
diff --git a/quic/core/quic_stream_sequencer.cc b/quic/core/quic_stream_sequencer.cc index 8f84fb9..ad07830 100644 --- a/quic/core/quic_stream_sequencer.cc +++ b/quic/core/quic_stream_sequencer.cc
@@ -9,6 +9,7 @@ #include <string> #include <utility> +#include "net/third_party/quiche/src/quic/core/quic_error_codes.h" #include "net/third_party/quiche/src/quic/core/quic_packets.h" #include "net/third_party/quiche/src/quic/core/quic_stream.h" #include "net/third_party/quiche/src/quic/core/quic_stream_sequencer_buffer.h" @@ -26,6 +27,7 @@ QuicStreamSequencer::QuicStreamSequencer(StreamInterface* quic_stream) : stream_(quic_stream), buffered_frames_(kStreamReceiveWindowLimit), + highest_offset_(0), close_offset_(std::numeric_limits<QuicStreamOffset>::max()), blocked_(false), num_frames_received_(0), @@ -66,6 +68,7 @@ void QuicStreamSequencer::OnFrameData(QuicStreamOffset byte_offset, size_t data_len, const char* data_buffer) { + highest_offset_ = std::max(highest_offset_, byte_offset + data_len); const size_t previous_readable_bytes = buffered_frames_.ReadableBytes(); size_t bytes_written; std::string error_details; @@ -123,10 +126,33 @@ // If there is a scheduled close, the new offset should match it. if (close_offset_ != kMaxOffset && offset != close_offset_) { - stream_->Reset(QUIC_MULTIPLE_TERMINATION_OFFSETS); + if (!GetQuicReloadableFlag(quic_no_decrease_in_final_offset)) { + stream_->Reset(QUIC_MULTIPLE_TERMINATION_OFFSETS); + return false; + } + QUIC_RELOADABLE_FLAG_COUNT_N(quic_no_decrease_in_final_offset, 1, 2); + stream_->CloseConnectionWithDetails( + QUIC_STREAM_SEQUENCER_INVALID_STATE, + QuicStrCat("Stream ", stream_->id(), + " received new final offset: ", offset, + ", which is different from close offset: ", close_offset_)); return false; } + // The final offset should be no less than the highest offset that is + // received. + if (GetQuicReloadableFlag(quic_no_decrease_in_final_offset)) { + QUIC_RELOADABLE_FLAG_COUNT_N(quic_no_decrease_in_final_offset, 2, 2); + if (offset < highest_offset_) { + stream_->CloseConnectionWithDetails( + QUIC_STREAM_SEQUENCER_INVALID_STATE, + QuicStrCat( + "Stream ", stream_->id(), " received fin with offset: ", offset, + ", which reduces current highest offset: ", highest_offset_)); + return false; + } + } + close_offset_ = offset; MaybeCloseStream();
diff --git a/quic/core/quic_stream_sequencer.h b/quic/core/quic_stream_sequencer.h index a735a1e..8f8a6a0 100644 --- a/quic/core/quic_stream_sequencer.h +++ b/quic/core/quic_stream_sequencer.h
@@ -11,6 +11,7 @@ #include "net/third_party/quiche/src/quic/core/quic_packets.h" #include "net/third_party/quiche/src/quic/core/quic_stream_sequencer_buffer.h" +#include "net/third_party/quiche/src/quic/core/quic_types.h" #include "net/third_party/quiche/src/quic/platform/api/quic_export.h" namespace quic { @@ -182,6 +183,9 @@ // Stores received data in offset order. QuicStreamSequencerBuffer buffered_frames_; + // The highest offset that is received so far. + QuicStreamOffset highest_offset_; + // The offset, if any, we got a stream termination for. When this many bytes // have been processed, the sequencer will be closed. QuicStreamOffset close_offset_;
diff --git a/quic/core/quic_stream_sequencer_test.cc b/quic/core/quic_stream_sequencer_test.cc index 8ad845b..15484d8 100644 --- a/quic/core/quic_stream_sequencer_test.cc +++ b/quic/core/quic_stream_sequencer_test.cc
@@ -15,6 +15,7 @@ #include "net/third_party/quiche/src/quic/core/quic_utils.h" #include "net/third_party/quiche/src/quic/platform/api/quic_arraysize.h" #include "net/third_party/quiche/src/quic/platform/api/quic_expect_bug.h" +#include "net/third_party/quiche/src/quic/platform/api/quic_flags.h" #include "net/third_party/quiche/src/quic/platform/api/quic_logging.h" #include "net/third_party/quiche/src/quic/platform/api/quic_string_piece.h" #include "net/third_party/quiche/src/quic/platform/api/quic_test.h" @@ -374,12 +375,15 @@ OnFinFrame(3, ""); EXPECT_EQ(3u, QuicStreamSequencerPeer::GetCloseOffset(sequencer_.get())); - EXPECT_CALL(stream_, Reset(QUIC_MULTIPLE_TERMINATION_OFFSETS)); + if (!GetQuicReloadableFlag(quic_no_decrease_in_final_offset)) { + EXPECT_CALL(stream_, Reset(QUIC_MULTIPLE_TERMINATION_OFFSETS)); + } else { + EXPECT_CALL(stream_, CloseConnectionWithDetails( + QUIC_STREAM_SEQUENCER_INVALID_STATE, + "Stream 1 received new final offset: 1, which is " + "different from close offset: 3")); + } OnFinFrame(1, ""); - EXPECT_EQ(3u, QuicStreamSequencerPeer::GetCloseOffset(sequencer_.get())); - - OnFinFrame(3, ""); - EXPECT_EQ(3u, QuicStreamSequencerPeer::GetCloseOffset(sequencer_.get())); } class QuicSequencerRandomTest : public QuicStreamSequencerTest { @@ -757,13 +761,32 @@ // Regression test for https://crbug.com/992486. TEST_F(QuicStreamSequencerTest, CorruptFinFrames) { SetQuicReloadableFlag(quic_no_stream_data_after_reset, true); - EXPECT_CALL(stream_, Reset(QUIC_MULTIPLE_TERMINATION_OFFSETS)); + if (!GetQuicReloadableFlag(quic_no_decrease_in_final_offset)) { + EXPECT_CALL(stream_, Reset(QUIC_MULTIPLE_TERMINATION_OFFSETS)); + } else { + EXPECT_CALL(stream_, CloseConnectionWithDetails( + QUIC_STREAM_SEQUENCER_INVALID_STATE, + "Stream 1 received new final offset: 1, which is " + "different from close offset: 2")); + } OnFinFrame(2u, ""); OnFinFrame(0u, "a"); EXPECT_FALSE(sequencer_->HasBytesToRead()); } +// Regression test for crbug.com/1015693 +TEST_F(QuicStreamSequencerTest, ReceiveFinLessThanHighestOffset) { + SetQuicReloadableFlag(quic_no_decrease_in_final_offset, true); + EXPECT_CALL(stream_, OnDataAvailable()).Times(1); + EXPECT_CALL(stream_, CloseConnectionWithDetails( + QUIC_STREAM_SEQUENCER_INVALID_STATE, + "Stream 1 received fin with offset: 0, which " + "reduces current highest offset: 3")); + OnFrame(0u, "abc"); + OnFinFrame(0u, ""); +} + } // namespace } // namespace test } // namespace quic