Add FEC producer fuzzing and a unittest for one of the issues found.

BUG=webrtc:4800
R=pbos@webrtc.org

Review URL: https://codereview.webrtc.org/1522463002 .

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

2 files changed, 69 insertions, 22 deletions

diff --git a/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc b/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc
index 66c2f03aca..4de8fb3a46 100644
--- a/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc
+++ b/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc
@@ -10,13 +10,13 @@
 
 #include "webrtc/modules/rtp_rtcp/source/forward_error_correction.h"
 
-#include <assert.h>
 #include <stdlib.h>
 #include <string.h>
 
 #include <algorithm>
 #include <iterator>
 
+#include "webrtc/base/checks.h"
 #include "webrtc/base/logging.h"
 #include "webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h"
 #include "webrtc/modules/rtp_rtcp/source/byte_io.h"
@@ -112,7 +112,6 @@ int32_t ForwardErrorCorrection::GenerateFEC(const PacketList& media_packet_list,
                                             FecMaskType fec_mask_type,
                                             PacketList* fec_packet_list) {
   const uint16_t num_media_packets = media_packet_list.size();
-
   // Sanity check arguments.
   assert(num_media_packets > 0);
   assert(num_important_packets >= 0 &&
@@ -126,7 +125,7 @@ int32_t ForwardErrorCorrection::GenerateFEC(const PacketList& media_packet_list,
   }
 
   bool l_bit = (num_media_packets > 8 * kMaskSizeLBitClear);
-  int num_maskBytes = l_bit ? kMaskSizeLBitSet : kMaskSizeLBitClear;
+  int num_mask_bytes = l_bit ? kMaskSizeLBitSet : kMaskSizeLBitClear;
 
   // Do some error checking on the media packets.
   for (Packet* media_packet : media_packet_list) {
@@ -166,21 +165,20 @@ int32_t ForwardErrorCorrection::GenerateFEC(const PacketList& media_packet_list,
   // Always allocate space for a large mask.
   rtc::scoped_ptr<uint8_t[]> packet_mask(
       new uint8_t[num_fec_packets * kMaskSizeLBitSet]);
-  memset(packet_mask.get(), 0, num_fec_packets * num_maskBytes);
+  memset(packet_mask.get(), 0, num_fec_packets * num_mask_bytes);
   internal::GeneratePacketMasks(num_media_packets, num_fec_packets,
                                 num_important_packets, use_unequal_protection,
                                 mask_table, packet_mask.get());
 
   int num_mask_bits = InsertZerosInBitMasks(
-      media_packet_list, packet_mask.get(), num_maskBytes, num_fec_packets);
-
-  l_bit = (num_mask_bits > 8 * kMaskSizeLBitClear);
+      media_packet_list, packet_mask.get(), num_mask_bytes, num_fec_packets);
 
   if (num_mask_bits < 0) {
     return -1;
   }
+  l_bit = (num_mask_bits > 8 * kMaskSizeLBitClear);
   if (l_bit) {
-    num_maskBytes = kMaskSizeLBitSet;
+    num_mask_bytes = kMaskSizeLBitSet;
   }
 
   GenerateFecBitStrings(media_packet_list, packet_mask.get(), num_fec_packets,
@@ -212,7 +210,7 @@ void ForwardErrorCorrection::GenerateFecBitStrings(
     return;
   }
   uint8_t media_payload_length[2];
-  const int num_maskBytes = l_bit ? kMaskSizeLBitSet : kMaskSizeLBitClear;
+  const int num_mask_bytes = l_bit ? kMaskSizeLBitSet : kMaskSizeLBitClear;
   const uint16_t ulp_header_size =
       l_bit ? kUlpHeaderSizeLBitSet : kUlpHeaderSizeLBitClear;
   const uint16_t fec_rtp_offset =
@@ -220,12 +218,13 @@ void ForwardErrorCorrection::GenerateFecBitStrings(
 
   for (int i = 0; i < num_fec_packets; ++i) {
     PacketList::const_iterator media_list_it = media_packet_list.begin();
-    uint32_t pkt_mask_idx = i * num_maskBytes;
+    uint32_t pkt_mask_idx = i * num_mask_bytes;
     uint32_t media_pkt_idx = 0;
     uint16_t fec_packet_length = 0;
     uint16_t prev_seq_num = ParseSequenceNumber((*media_list_it)->data);
     while (media_list_it != media_packet_list.end()) {
-      // Each FEC packet has a multiple byte mask.
+      // Each FEC packet has a multiple byte mask. Determine if this media
+      // packet should be included in FEC packet i.
       if (packet_mask[pkt_mask_idx] & (1 << (7 - media_pkt_idx))) {
         Packet* media_packet = *media_list_it;
 
@@ -279,15 +278,11 @@ void ForwardErrorCorrection::GenerateFecBitStrings(
         media_pkt_idx += static_cast<uint16_t>(seq_num - prev_seq_num);
         prev_seq_num = seq_num;
       }
-      if (media_pkt_idx == 8) {
-        // Switch to the next mask byte.
-        media_pkt_idx = 0;
-        pkt_mask_idx++;
-      }
+      pkt_mask_idx += media_pkt_idx / 8;
+      media_pkt_idx %= 8;
     }
-    assert(generated_fec_packets_[i].length);
-    // Note: This shouldn't happen: means packet mask is wrong or poorly
-    // designed
+    RTC_DCHECK_GT(generated_fec_packets_[i].length, 0u)
+        << "Packet mask is wrong or poorly designed.";
   }
 }
 
@@ -310,6 +305,9 @@ int ForwardErrorCorrection::InsertZerosInBitMasks(
     // required.
     return media_packets.size();
   }
+  // We can only protect 8 * kMaskSizeLBitSet packets.
+  if (total_missing_seq_nums + media_packets.size() > 8 * kMaskSizeLBitSet)
+    return -1;
   // Allocate the new mask.
   int new_mask_bytes = kMaskSizeLBitClear;
   if (media_packets.size() + total_missing_seq_nums > 8 * kMaskSizeLBitClear) {
@@ -421,7 +419,7 @@ void ForwardErrorCorrection::GenerateFecUlpHeaders(
   PacketList::const_iterator media_list_it = media_packet_list.begin();
   Packet* media_packet = *media_list_it;
   assert(media_packet != NULL);
-  int num_maskBytes = l_bit ? kMaskSizeLBitSet : kMaskSizeLBitClear;
+  int num_mask_bytes = l_bit ? kMaskSizeLBitSet : kMaskSizeLBitClear;
   const uint16_t ulp_header_size =
       l_bit ? kUlpHeaderSizeLBitSet : kUlpHeaderSizeLBitClear;
 
@@ -446,8 +444,8 @@ void ForwardErrorCorrection::GenerateFecUlpHeaders(
         generated_fec_packets_[i].length - kFecHeaderSize - ulp_header_size);
 
     // Copy the packet mask.
-    memcpy(&generated_fec_packets_[i].data[12], &packet_mask[i * num_maskBytes],
-           num_maskBytes);
+    memcpy(&generated_fec_packets_[i].data[12],
+           &packet_mask[i * num_mask_bytes], num_mask_bytes);
   }
 }
 
diff --git a/webrtc/modules/rtp_rtcp/source/producer_fec_unittest.cc b/webrtc/modules/rtp_rtcp/source/producer_fec_unittest.cc
index 683b951f1e..be4b453454 100644
--- a/webrtc/modules/rtp_rtcp/source/producer_fec_unittest.cc
+++ b/webrtc/modules/rtp_rtcp/source/producer_fec_unittest.cc
@@ -9,8 +9,10 @@
  */
 
 #include <list>
+#include <vector>
 
 #include "testing/gtest/include/gtest/gtest.h"
+#include "webrtc/modules/rtp_rtcp/source/byte_io.h"
 #include "webrtc/modules/rtp_rtcp/source/fec_test_helper.h"
 #include "webrtc/modules/rtp_rtcp/source/forward_error_correction.h"
 #include "webrtc/modules/rtp_rtcp/source/producer_fec.h"
@@ -54,6 +56,53 @@ class ProducerFecTest : public ::testing::Test {
   FrameGenerator* generator_;
 };
 
+// Verifies bug found via fuzzing, where a gap in the packet sequence caused us
+// to move past the end of the current FEC packet mask byte without moving to
+// the next byte. That likely caused us to repeatedly read from the same byte,
+// and if that byte didn't protect packets we would generate empty FEC.
+TEST_F(ProducerFecTest, NoEmptyFecWithSeqNumGaps) {
+  struct Packet {
+    size_t header_size;
+    size_t payload_size;
+    uint16_t seq_num;
+    bool marker_bit;
+  };
+  std::vector<Packet> protected_packets;
+  protected_packets.push_back({15, 3, 41, 0});
+  protected_packets.push_back({14, 1, 43, 0});
+  protected_packets.push_back({19, 0, 48, 0});
+  protected_packets.push_back({19, 0, 50, 0});
+  protected_packets.push_back({14, 3, 51, 0});
+  protected_packets.push_back({13, 8, 52, 0});
+  protected_packets.push_back({19, 2, 53, 0});
+  protected_packets.push_back({12, 3, 54, 0});
+  protected_packets.push_back({21, 0, 55, 0});
+  protected_packets.push_back({13, 3, 57, 1});
+  FecProtectionParams params = {117, 0, 3, kFecMaskBursty};
+  producer_->SetFecParameters(&params, 0);
+  uint8_t packet[28] = {0};
+  for (Packet p : protected_packets) {
+    if (p.marker_bit) {
+      packet[1] |= 0x80;
+    } else {
+      packet[1] &= ~0x80;
+    }
+    ByteWriter<uint16_t>::WriteBigEndian(&packet[2], p.seq_num);
+    producer_->AddRtpPacketAndGenerateFec(packet, p.payload_size,
+                                          p.header_size);
+    uint16_t num_fec_packets = producer_->NumAvailableFecPackets();
+    std::vector<RedPacket*> fec_packets;
+    if (num_fec_packets > 0) {
+      fec_packets =
+          producer_->GetFecPackets(kRedPayloadType, 99, 100, p.header_size);
+      EXPECT_EQ(num_fec_packets, fec_packets.size());
+    }
+    for (RedPacket* fec_packet : fec_packets) {
+      delete fec_packet;
+    }
+  }
+}
+
 TEST_F(ProducerFecTest, OneFrameFec) {
   // The number of media packets (|kNumPackets|), number of frames (one for
   // this test), and the protection factor (|params->fec_rate|) are set to make