Fix an undefined behavior in QuicChaosProtector::AddPingFrames when remaining_padding_bytes_ is zero after split. The newly added test fails without the fix, see http://sponge2/644a2be0-0db6-4f83-a1b9-19a5ef431826. PiperOrigin-RevId: 404606531
diff --git a/quic/core/quic_chaos_protector.cc b/quic/core/quic_chaos_protector.cc index 4eb353b..e21d26c 100644 --- a/quic/core/quic_chaos_protector.cc +++ b/quic/core/quic_chaos_protector.cc
@@ -175,6 +175,9 @@ } void QuicChaosProtector::AddPingFrames() { + if (remaining_padding_bytes_ == 0) { + return; + } constexpr uint64_t kMaxAddedPingFrames = 10; const uint64_t num_ping_frames = random_->InsecureRandUint64() %
diff --git a/quic/core/quic_chaos_protector_test.cc b/quic/core/quic_chaos_protector_test.cc index 1ec985b..675e00f 100644 --- a/quic/core/quic_chaos_protector_test.cc +++ b/quic/core/quic_chaos_protector_test.cc
@@ -30,9 +30,7 @@ public: QuicChaosProtectorTest() : version_(GetParam()), - framer_({version_}, - QuicTime::Zero(), - Perspective::IS_CLIENT, + framer_({version_}, QuicTime::Zero(), Perspective::IS_CLIENT, kQuicDefaultConnectionIdLength), validation_framer_({version_}), random_(/*base=*/3), @@ -42,12 +40,15 @@ crypto_frame_(level_, crypto_offset_, crypto_data_length_), num_padding_bytes_(50), packet_size_(1000), - packet_buffer_(std::make_unique<char[]>(packet_size_)), - chaos_protector_(crypto_frame_, - num_padding_bytes_, - packet_size_, - SetupHeaderAndFramers(), - &random_) {} + packet_buffer_(std::make_unique<char[]>(packet_size_)) { + ReCreateChaosProtector(); + } + + void ReCreateChaosProtector() { + chaos_protector_ = std::make_unique<QuicChaosProtector>( + crypto_frame_, num_padding_bytes_, packet_size_, + SetupHeaderAndFramers(), &random_); + } // From QuicStreamFrameDataProducer. WriteStreamDataResult WriteStreamData(QuicStreamId /*id*/, @@ -100,7 +101,7 @@ void BuildEncryptAndParse() { absl::optional<size_t> length = - chaos_protector_.BuildDataPacket(header_, packet_buffer_.get()); + chaos_protector_->BuildDataPacket(header_, packet_buffer_.get()); ASSERT_TRUE(length.has_value()); ASSERT_GT(length.value(), 0u); size_t encrypted_length = framer_.EncryptInPlace( @@ -115,7 +116,13 @@ void ResetOffset(QuicStreamOffset offset) { crypto_offset_ = offset; crypto_frame_.offset = offset; - chaos_protector_.crypto_buffer_offset_ = offset; + ReCreateChaosProtector(); + } + + void ResetLength(QuicByteCount length) { + crypto_data_length_ = length; + crypto_frame_.data_length = length; + ReCreateChaosProtector(); } ParsedQuicVersion version_; @@ -130,7 +137,7 @@ int num_padding_bytes_; size_t packet_size_; std::unique_ptr<char[]> packet_buffer_; - QuicChaosProtector chaos_protector_; + std::unique_ptr<QuicChaosProtector> chaos_protector_; }; namespace { @@ -202,6 +209,22 @@ ASSERT_EQ(validation_framer_.padding_frames().size(), 1u); } +TEST_P(QuicChaosProtectorTest, ZeroRemainingBytesAfterSplit) { + QuicPacketLength new_length = 63; + num_padding_bytes_ = QuicFramer::GetMinCryptoFrameSize( + crypto_frame_.offset + new_length, new_length); + ResetLength(new_length); + BuildEncryptAndParse(); + + ASSERT_EQ(validation_framer_.crypto_frames().size(), 2u); + EXPECT_EQ(validation_framer_.crypto_frames()[0]->offset, crypto_offset_); + EXPECT_EQ(validation_framer_.crypto_frames()[0]->data_length, 4); + EXPECT_EQ(validation_framer_.crypto_frames()[1]->offset, crypto_offset_ + 4); + EXPECT_EQ(validation_framer_.crypto_frames()[1]->data_length, + crypto_data_length_ - 4); + ASSERT_EQ(validation_framer_.ping_frames().size(), 0u); +} + } // namespace } // namespace test } // namespace quic