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}
diff --git a/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc b/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc
index 66c2f03..4de8fb3 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 @@
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 @@
}
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 @@
// 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 @@
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 @@
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 @@
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 @@
// 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 @@
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 @@
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 683b951..be4b453 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 @@
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(¶ms, 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
diff --git a/webrtc/test/fuzzers/BUILD.gn b/webrtc/test/fuzzers/BUILD.gn
index 07eea27..7b530b6 100644
--- a/webrtc/test/fuzzers/BUILD.gn
+++ b/webrtc/test/fuzzers/BUILD.gn
@@ -52,6 +52,22 @@
}
}
+test("producer_fec_fuzzer") {
+ sources = [
+ "producer_fec_fuzzer.cc",
+ ]
+ deps = [
+ ":webrtc_fuzzer_main",
+ "../../modules/rtp_rtcp/",
+ ]
+
+ if (is_clang) {
+ # Suppress warnings from Chrome's Clang plugins.
+ # See http://code.google.com/p/webrtc/issues/detail?id=163 for details.
+ configs -= [ "//build/config/clang:find_bad_constructs" ]
+ }
+}
+
source_set("audio_decoder_fuzzer") {
sources = [
"audio_decoder_fuzzer.cc",
diff --git a/webrtc/test/fuzzers/producer_fec_fuzzer.cc b/webrtc/test/fuzzers/producer_fec_fuzzer.cc
new file mode 100644
index 0000000..7322fed
--- /dev/null
+++ b/webrtc/test/fuzzers/producer_fec_fuzzer.cc
@@ -0,0 +1,60 @@
+/*
+ * Copyright (c) 2015 The WebRTC project authors. All Rights Reserved.
+ *
+ * Use of this source code is governed by a BSD-style license
+ * that can be found in the LICENSE file in the root of the source
+ * tree. An additional intellectual property rights grant can be found
+ * in the file PATENTS. All contributing project authors may
+ * be found in the AUTHORS file in the root of the source tree.
+ */
+#include "webrtc/base/checks.h"
+#include "webrtc/base/scoped_ptr.h"
+#include "webrtc/modules/rtp_rtcp/source/byte_io.h"
+#include "webrtc/modules/rtp_rtcp/source/producer_fec.h"
+
+namespace webrtc {
+
+void FuzzOneInput(const uint8_t* data, size_t size) {
+ ForwardErrorCorrection fec;
+ ProducerFec producer(&fec);
+ size_t i = 0;
+ if (size < 4)
+ return;
+ FecProtectionParams params = {data[i++] % 128, data[i++] % 1,
+ static_cast<int>(data[i++] % 10),
+ kFecMaskBursty};
+ producer.SetFecParameters(¶ms, 0);
+ uint16_t seq_num = data[i++];
+
+ while (i + 3 < size) {
+ size_t rtp_header_length = data[i++] % 10 + 12;
+ size_t payload_size = data[i++] % 10;
+ if (i + payload_size + rtp_header_length + 2 > size)
+ break;
+ rtc::scoped_ptr<uint8_t[]> packet(
+ new uint8_t[payload_size + rtp_header_length]);
+ memcpy(packet.get(), &data[i], payload_size + rtp_header_length);
+ ByteWriter<uint16_t>::WriteBigEndian(&packet[2], seq_num++);
+ i += payload_size + rtp_header_length;
+ // Make sure sequence numbers are increasing.
+ const int kRedPayloadType = 98;
+ rtc::scoped_ptr<RedPacket> red_packet(producer.BuildRedPacket(
+ packet.get(), payload_size, rtp_header_length, kRedPayloadType));
+ bool protect = static_cast<bool>(data[i++] % 2);
+ if (protect) {
+ producer.AddRtpPacketAndGenerateFec(packet.get(), payload_size,
+ rtp_header_length);
+ }
+ uint16_t num_fec_packets = producer.NumAvailableFecPackets();
+ std::vector<RedPacket*> fec_packets;
+ if (num_fec_packets > 0) {
+ fec_packets =
+ producer.GetFecPackets(kRedPayloadType, 99, 100, rtp_header_length);
+ RTC_CHECK_EQ(num_fec_packets, fec_packets.size());
+ }
+ for (RedPacket* fec_packet : fec_packets) {
+ delete fec_packet;
+ }
+ }
+}
+} // namespace webrtc
