Fix bug in generating IETF-format ACK Frame

Generation of IETF QUIC ACK frames and calculation of the length of same
differed, due to missing "-1" in the gap-block and ack-block sizes.
This was a problem when the decrement caused the length to cross a VarInt62
encoding size boundary, causing the calculated-length to be different from
the actual, serialized, length.

In this case, the gap block size was 64 in the length calculation method, 63
in the actual serialization. 64 is encoded in two bytes, 63 in one, leading
to the length discrepancy.  Unless the length boundary was crossed, this did
not cause problems in that the serialized value (63) was the correct one.

This was detected in the Chromium clusterfuzz tests and reported as Chromium
issue 859949:
https://bugs.chromium.org/p/chromium/issues/detail?id=859949

Merge internal change: 204947824

R=rch@chromium.org

Change-Id: I6578162b274f8474ca0ef7979a1308afd51cd295
Reviewed-on: https://chromium-review.googlesource.com/1140838Reviewed-by: Ryan Hamilton <rch@chromium.org>
Commit-Queue: Frank Kastenholz <fkastenholz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576089}

net/third_party/quic/core/quic_framer.cc

@@ -3101,11 +3101,13 @@ size_t QuicFramer::GetIetfAckFrameSize(const QuicAckFrame& frame) {
   // Account for the remaining Intervals, if any.
   while (ack_block_count != 0) {
     QuicPacketNumber gap_size = ack_block_smallest - itr->max();
-    size_t size_of_gap_size = QuicDataWriter::GetVarInt62Len(gap_size);
+    // Decrement per the protocol specification
+    size_t size_of_gap_size = QuicDataWriter::GetVarInt62Len(gap_size - 1);
     ack_frame_size += size_of_gap_size;
 
     QuicPacketNumber block_size = itr->max() - itr->min();
-    size_t size_of_block_size = QuicDataWriter::GetVarInt62Len(block_size);
+    // Decrement per the protocol specification
+    size_t size_of_block_size = QuicDataWriter::GetVarInt62Len(block_size - 1);
     ack_frame_size += size_of_block_size;
 
     ack_block_smallest = itr->min();

net/third_party/quic/core/quic_ietf_framer_test.cc

@@ -267,10 +267,14 @@ class QuicIetfFramerTest : public QuicTestWithParam<ParsedQuicVersion> {
     EXPECT_TRUE(QuicFramerPeer::AppendIetfAckFrameAndTypeByte(
         &framer_, transmit_frame, &writer));
 
-    // Better have something in the packet buffer.
-    EXPECT_NE(0u, writer.length());
+    size_t expected_frame_length = QuicFramerPeer::ComputeFrameLength(
+        &framer_, QuicFrame(&transmit_frame), false,
+        static_cast<QuicPacketNumberLength>(123456u));
+
+    // Encoded length should match what ComputeFrameLength returns
+    EXPECT_EQ(expected_frame_length, writer.length());
     // and what is in the buffer should be the expected size.
-    EXPECT_EQ(expected_size, writer.length());
+    EXPECT_EQ(expected_size, writer.length()) << "Frame is " << transmit_frame;
     // Now set up a reader to read in the frame.
     QuicDataReader reader(packet_buffer, writer.length(), NETWORK_BYTE_ORDER);
 
@@ -646,6 +650,15 @@ struct ack_frame ack_frame_variants[] = {
   { 100000000, {{1, 2}, {3, 4}, {5, 6}, {7, 8}, {9, 10}, {11, 12}}},
   { 0, {{1, 65}} },
   { 9223372036854775807, {{1, 11}, {74, 138}} },
+  // This ack is for packets 60 & 125. There are 64 packets in the gap.
+  // The encoded value is gap_size - 1, or 63. Crosses a VarInt62 encoding
+  // boundary...
+  { 1, {{60, 61}, {125, 126}} },
+  { 2, {{ 1, 65}, {129, 130}} },
+  { 3, {{ 1, 65}, {129, 195}} },
+  { 4, {{ 1, 65}, {129, 194}} },
+  { 5, {{ 1, 65}, {129, 193}} },
+  { 6, {{ 1, 65}, {129, 192}} },
 };
 // clang-format on
 

net/third_party/quic/core/quic_packet_creator_test.cc

@@ -1183,6 +1183,17 @@ TEST_P(QuicPacketCreatorTest, FlushWithExternalBuffer) {
   creator_.Flush();
 }
 
+// Test for error found in
+// https://bugs.chromium.org/p/chromium/issues/detail?id=859949 where a gap
+// length that crosses an IETF VarInt length boundary would cause a
+// failure. While this test is not applicable to versions other than version 99,
+// it should still work. Hence, it is not made version-specific.
+TEST_P(QuicPacketCreatorTest, IetfAckGapErrorRegression) {
+  QuicAckFrame ack_frame = InitAckFrame({{60, 61}, {125, 126}});
+  frames_.push_back(QuicFrame(&ack_frame));
+  SerializeAllFrames(frames_);
+}
+
 }  // namespace
 }  // namespace test
 }  // namespace quic

net/third_party/quic/test_tools/quic_framer_peer.cc

@@ -303,5 +303,15 @@ void QuicFramerPeer::SetLastPacketIsIetfQuic(QuicFramer* framer,
   framer->last_packet_is_ietf_quic_ = last_packet_is_ietf_quic;
 }
 
+// static
+size_t QuicFramerPeer::ComputeFrameLength(
+    QuicFramer* framer,
+    const QuicFrame& frame,
+    bool last_frame_in_packet,
+    QuicPacketNumberLength packet_number_length) {
+  return framer->ComputeFrameLength(frame, last_frame_in_packet,
+                                    packet_number_length);
+}
+
 }  // namespace test
 }  // namespace quic

net/third_party/quic/test_tools/quic_framer_peer.h

@@ -143,6 +143,10 @@ class QuicFramerPeer {
   static bool ProcessNewConnectionIdFrame(QuicFramer* framer,
                                           QuicDataReader* reader,
                                           QuicNewConnectionIdFrame* frame);
+  static size_t ComputeFrameLength(QuicFramer* framer,
+                                   const QuicFrame& frame,
+                                   bool last_frame_in_packet,
+                                   QuicPacketNumberLength packet_number_length);
 
  private:
   DISALLOW_COPY_AND_ASSIGN(QuicFramerPeer);