relnote: Protect against a QUIC crash bug but logging a DFATAL and

closing the connection instead when a packet cannot be serialized.

This could help fixing Chromium BUG=479880.

Merge internal change: 98506577

R=rch@chromium.org

Review URL: https://codereview.chromium.org/1246793002

Cr-Commit-Position: refs/heads/master@{#339532}

5 files changed, 42 insertions, 4 deletions

diff --git a/net/quic/quic_packet_creator.cc b/net/quic/quic_packet_creator.cc
index a8afaa7..7474b0b 100644
--- a/net/quic/quic_packet_creator.cc
+++ b/net/quic/quic_packet_creator.cc
@@ -463,8 +463,11 @@ SerializedPacket QuicPacketCreator::SerializePacket(
     packet.reset(framer_->BuildDataPacket(header, queued_frames_,
                                           large_buffer.get(), packet_size_));
   }
-  LOG_IF(DFATAL, packet == nullptr) << "Failed to serialize "
-                                    << queued_frames_.size() << " frames.";
+  if (packet == nullptr) {
+    LOG(DFATAL) << "Failed to serialize " << queued_frames_.size()
+                << " frames.";
+    return NoPacket();
+  }
 
   OnBuiltFecProtectedPayload(header, packet->FecProtectedData());
 
diff --git a/net/quic/quic_packet_generator.cc b/net/quic/quic_packet_generator.cc
index 49cdc7f..cc3a793 100644
--- a/net/quic/quic_packet_generator.cc
+++ b/net/quic/quic_packet_generator.cc
@@ -457,7 +457,12 @@ void QuicPacketGenerator::SerializeAndSendPacket() {
   char buffer[kMaxPacketSize];
   SerializedPacket serialized_packet =
       packet_creator_.SerializePacket(buffer, kMaxPacketSize);
-  DCHECK(serialized_packet.packet);
+  if (serialized_packet.packet == nullptr) {
+    LOG(DFATAL) << "Failed to SerializePacket. fec_policy:" << fec_send_policy_
+                << " should_fec_protect_:" << should_fec_protect_;
+    delegate_->CloseConnection(QUIC_FAILED_TO_SERIALIZE_PACKET, false);
+    return;
+  }
 
   // There may be AckNotifiers interested in this packet.
   serialized_packet.notifiers.swap(ack_notifiers_);
diff --git a/net/quic/quic_packet_generator_test.cc b/net/quic/quic_packet_generator_test.cc
index 273ad94..737fa61 100644
--- a/net/quic/quic_packet_generator_test.cc
+++ b/net/quic/quic_packet_generator_test.cc
@@ -1625,5 +1625,32 @@ TEST_P(QuicPacketGeneratorTest, GenerateMtuDiscoveryPacket_SurroundedByData) {
   CheckPacketHasSingleStreamFrame(4);
 }
 
+TEST_P(QuicPacketGeneratorTest, DontCrashOnInvalidStopWaiting) {
+  // Test added to ensure the generator does not crash when an invalid frame is
+  // added.  Because this is an indication of internal programming errors,
+  // DFATALs are expected.
+  // A 1 byte sequence number length can't encode a gap of 1000.
+  QuicPacketCreatorPeer::SetSequenceNumber(creator_, 1000);
+
+  delegate_.SetCanNotWrite();
+  generator_.SetShouldSendAck(true);
+  delegate_.SetCanWriteAnything();
+  generator_.StartBatchOperations();
+
+  // Set up frames to write into the creator when control frames are written.
+  EXPECT_CALL(delegate_, PopulateAckFrame(_));
+  EXPECT_CALL(delegate_, PopulateStopWaitingFrame(_));
+  // Generator should have queued control frames, and creator should be empty.
+  EXPECT_TRUE(generator_.HasQueuedFrames());
+  EXPECT_FALSE(creator_->HasPendingFrames());
+
+  // This will not serialize any packets, because of the invalid frame.
+  EXPECT_CALL(delegate_,
+              CloseConnection(QUIC_FAILED_TO_SERIALIZE_PACKET, false));
+  EXPECT_DFATAL(generator_.FinishBatchOperations(),
+                "sequence_number_length 1 is too small "
+                "for least_unacked_delta: 1001");
+}
+
 }  // namespace test
 }  // namespace net
diff --git a/net/quic/quic_protocol.h b/net/quic/quic_protocol.h
index da60c28..3d24b81 100644
--- a/net/quic/quic_protocol.h
+++ b/net/quic/quic_protocol.h
@@ -550,6 +550,8 @@ enum QuicErrorCode {
   QUIC_PUBLIC_RESETS_POST_HANDSHAKE = 73,
   // Disabled QUIC because of too many timeouts with streams open.
   QUIC_TIMEOUTS_WITH_OPEN_STREAMS = 74,
+  // Closed because we failed to serialize a packet.
+  QUIC_FAILED_TO_SERIALIZE_PACKET = 75,
 
   // Crypto errors.
 
@@ -609,7 +611,7 @@ enum QuicErrorCode {
   QUIC_VERSION_NEGOTIATION_MISMATCH = 55,
 
   // No error. Used as bound while iterating.
-  QUIC_LAST_ERROR = 75,
+  QUIC_LAST_ERROR = 76,
 };
 
 struct NET_EXPORT_PRIVATE QuicPacketPublicHeader {
diff --git a/net/quic/quic_utils.cc b/net/quic/quic_utils.cc
index 89a5acc..32f8c09 100644
--- a/net/quic/quic_utils.cc
+++ b/net/quic/quic_utils.cc
@@ -230,6 +230,7 @@ const char* QuicUtils::ErrorToString(QuicErrorCode error) {
     RETURN_STRING_LITERAL(QUIC_BAD_PACKET_LOSS_RATE);
     RETURN_STRING_LITERAL(QUIC_PUBLIC_RESETS_POST_HANDSHAKE);
     RETURN_STRING_LITERAL(QUIC_TIMEOUTS_WITH_OPEN_STREAMS);
+    RETURN_STRING_LITERAL(QUIC_FAILED_TO_SERIALIZE_PACKET);
     RETURN_STRING_LITERAL(QUIC_LAST_ERROR);
     // Intentionally have no default case, so we'll break the build
     // if we add errors and don't put them here.