commit 94d1500be0482e407c32ce92624b846ab49b8399
author dschinazi <dschinazi@google.com> Fri Apr 16 05:30:49 2021 -0700
committer Copybara-Service <copybara-worker@google.com> Fri Apr 16 05:31:20 2021 -0700
tree 292d3d3cc2f892b4b6c2b9c5b9554aa23a08e496
parent 81e779db54d3d1543e5923112b0dd42253c222c4

Make sure that a low QuicPacketCreator soft limit does not cause issues

This issue was causing a DCHECK failure in Envoy QUIC integration tests. In this test, Envoy would receive a QUIC packet and queue up some 1-RTT stream data which would trigger coalescing and set the soft max packet length. Then the code would try to send an initial ack. The issue there is that when switching the encryption level to initial, the header size would increase - and then when trying to add a frame we would trigger a DCHECK in BytesFree because the soft max packet length is lower than the header size. After performing code analysis, it is clear that any caller of BytesFree() properly handles the case where BytesFree() returns 0, so this DCHECK can be safely removed as it provides no value. This CL adds a regression test that would trigger the DCHECK if it were still present.

PiperOrigin-RevId: 368828653
Change-Id: I300326dd4e068569f4cec2690002cd4352d2afc2

quic/core/quic_packet_creator.cc

diff --git a/quic/core/quic_packet_creator.cc b/quic/core/quic_packet_creator.cc
index 9fb2f5d..48da702 100644
--- a/quic/core/quic_packet_creator.cc
+++ b/quic/core/quic_packet_creator.cc
@@ -724,7 +724,6 @@
 }
 
 size_t QuicPacketCreator::BytesFree() const {
-  QUICHE_DCHECK_GE(max_plaintext_size_, PacketSize());
   return max_plaintext_size_ -
          std::min(max_plaintext_size_, PacketSize() + ExpansionOnNewFrame());
 }

quic/core/quic_packet_creator_test.cc

diff --git a/quic/core/quic_packet_creator_test.cc b/quic/core/quic_packet_creator_test.cc
index 3ccf3d8..1cee22f 100644
--- a/quic/core/quic_packet_creator_test.cc
+++ b/quic/core/quic_packet_creator_test.cc
@@ -2262,6 +2262,32 @@
   EXPECT_TRUE(creator_.HasPendingFrames());
 }
 
+TEST_P(QuicPacketCreatorTest,
+       ChangingEncryptionLevelRemovesSoftMaxPacketLength) {
+  if (!client_framer_.version().CanSendCoalescedPackets()) {
+    return;
+  }
+  // First set encryption level to forward secure which has the shortest header.
+  creator_.set_encryption_level(ENCRYPTION_FORWARD_SECURE);
+  const QuicByteCount previous_max_packet_length = creator_.max_packet_length();
+  const size_t min_acceptable_packet_size =
+      GetPacketHeaderOverhead(client_framer_.transport_version()) +
+      QuicPacketCreator::MinPlaintextPacketSize(client_framer_.version()) +
+      GetEncryptionOverhead();
+  // Then set the soft max packet length to the lowest allowed value.
+  creator_.SetSoftMaxPacketLength(min_acceptable_packet_size);
+  // Make sure that the low value was accepted.
+  EXPECT_EQ(creator_.max_packet_length(), min_acceptable_packet_size);
+  // Now set the encryption level to handshake which increases the header size.
+  creator_.set_encryption_level(ENCRYPTION_HANDSHAKE);
+  // Make sure that adding a frame removes the the soft max packet length.
+  QuicAckFrame ack_frame(InitAckFrame(1));
+  frames_.push_back(QuicFrame(&ack_frame));
+  SerializedPacket serialized = SerializeAllFrames(frames_);
+  EXPECT_EQ(serialized.encryption_level, ENCRYPTION_HANDSHAKE);
+  EXPECT_EQ(creator_.max_packet_length(), previous_max_packet_length);
+}
+
 class MockDelegate : public QuicPacketCreator::DelegateInterface {
  public:
   MockDelegate() {}