commit 70fcc7419b84a63b3d257d25c4d4fb4dc9e2c244
author wub <wub@google.com> Wed Oct 20 11:59:44 2021 -0700
committer Copybara-Service <copybara-worker@google.com> Wed Oct 20 12:00:44 2021 -0700
tree 1ac07a9f033e93e793c52e54426eac6f48f0727a
parent e6f28c3a1f5c5e3aaf4e0239c8566e53f07b5263

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

quic/core/quic_chaos_protector.cc

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() %

quic/core/quic_chaos_protector_test.cc

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