From 4c1093b86f4d0a1c8ade68a4b6a411b2674deac8 Mon Sep 17 00:00:00 2001 From: Stefan Holmer Date: Fri, 11 Dec 2015 18:25:45 +0100 Subject: [PATCH] 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} --- .../source/forward_error_correction.cc | 42 +++++++------ .../rtp_rtcp/source/producer_fec_unittest.cc | 49 +++++++++++++++ webrtc/test/fuzzers/BUILD.gn | 16 +++++ webrtc/test/fuzzers/producer_fec_fuzzer.cc | 60 +++++++++++++++++++ 4 files changed, 145 insertions(+), 22 deletions(-) create mode 100644 webrtc/test/fuzzers/producer_fec_fuzzer.cc 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 #include #include #include #include +#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 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(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 +#include #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 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::WriteBigEndian(&packet[2], p.seq_num); + producer_->AddRtpPacketAndGenerateFec(packet, p.payload_size, + p.header_size); + uint16_t num_fec_packets = producer_->NumAvailableFecPackets(); + std::vector 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 07eea2795e..7b530b6a57 100644 --- a/webrtc/test/fuzzers/BUILD.gn +++ b/webrtc/test/fuzzers/BUILD.gn @@ -52,6 +52,22 @@ test("vp8_qp_parser_fuzzer") { } } +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 0000000000..7322fed4bf --- /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(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 packet( + new uint8_t[payload_size + rtp_header_length]); + memcpy(packet.get(), &data[i], payload_size + rtp_header_length); + ByteWriter::WriteBigEndian(&packet[2], seq_num++); + i += payload_size + rtp_header_length; + // Make sure sequence numbers are increasing. + const int kRedPayloadType = 98; + rtc::scoped_ptr red_packet(producer.BuildRedPacket( + packet.get(), payload_size, rtp_header_length, kRedPayloadType)); + bool protect = static_cast(data[i++] % 2); + if (protect) { + producer.AddRtpPacketAndGenerateFec(packet.get(), payload_size, + rtp_header_length); + } + uint16_t num_fec_packets = producer.NumAvailableFecPackets(); + std::vector 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