Fix bug where rtcp::TransportFeedback may generate incorrect messages.

webrtc/modules/rtp_rtcp/source/rtcp_receiver_unittest.cc

 /*
  *  Copyright (c) 2012 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.
  */
 
 
 /*
  * This file includes unit tests for the RTCPReceiver.
  */
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 // Note: This file has no directory. Lint warning must be ignored.
 #include "webrtc/common_types.h"
 #include "webrtc/modules/remote_bitrate_estimator/include/mock/mock_remote_bitrate_observer.h"
 #include "webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.h"
+#include "webrtc/modules/rtp_rtcp/source/byte_io.h"
 #include "webrtc/modules/rtp_rtcp/source/rtcp_packet.h"
 #include "webrtc/modules/rtp_rtcp/source/rtcp_receiver.h"
 #include "webrtc/modules/rtp_rtcp/source/rtcp_sender.h"
 #include "webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h"
 #include "webrtc/modules/rtp_rtcp/source/rtp_utility.h"
+#include "webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h"
 
 namespace webrtc {
 
 namespace {  // Anonymous namespace; hide utility functions and classes.
 
 // This test transport verifies that no functions get called.
 class TestTransport : public Transport,
                       public NullRtpData {
  public:
   explicit TestTransport() : rtcp_receiver_(nullptr) {}
@@ skipping 76 matching lines @@
     rtcp_packet_info_.interArrivalJitter =
         rtcpPacketInformation.interArrivalJitter;
     rtcp_packet_info_.sliPictureId = rtcpPacketInformation.sliPictureId;
     rtcp_packet_info_.rpsiPictureId = rtcpPacketInformation.rpsiPictureId;
     rtcp_packet_info_.receiverEstimatedMaxBitrate =
         rtcpPacketInformation.receiverEstimatedMaxBitrate;
     rtcp_packet_info_.ntp_secs = rtcpPacketInformation.ntp_secs;
     rtcp_packet_info_.ntp_frac = rtcpPacketInformation.ntp_frac;
     rtcp_packet_info_.rtp_timestamp = rtcpPacketInformation.rtp_timestamp;
     rtcp_packet_info_.xr_dlrr_item = rtcpPacketInformation.xr_dlrr_item;
-    if (rtcpPacketInformation.VoIPMetric) {
+    if (rtcpPacketInformation.VoIPMetric)
       rtcp_packet_info_.AddVoIPMetric(rtcpPacketInformation.VoIPMetric);
-    }
+    rtcp_packet_info_.transport_feedback_.reset(
+        rtcpPacketInformation.transport_feedback_.release());
     return 0;
   }
 
   OverUseDetectorOptions over_use_detector_options_;
   SimulatedClock system_clock_;
   ModuleRtpRtcpImpl* rtp_rtcp_impl_;
   RTCPReceiver* rtcp_receiver_;
   TestTransport* test_transport_;
   RTCPHelp::RTCPPacketInformation rtcp_packet_info_;
   MockRemoteBitrateObserver remote_bitrate_observer_;
@@ skipping 885 matching lines @@
 
   rtcp::ReceiverReport rr2;
   rr2.From(kSenderSsrc);
   rr2.WithReportBlock(rb2);
   rtc::scoped_ptr<rtcp::RawPacket> p2(rr2.Build());
   EXPECT_EQ(0, InjectRtcpPacket(p2->Buffer(), p2->Length()));
   EXPECT_TRUE(callback.Matches(kSourceSsrc, kSequenceNumber, kFractionLoss,
                                kCumulativeLoss, kJitter));
 }
 
+TEST_F(RtcpReceiverTest, ReceivesTransportFeedback) {
+  const uint32_t kSenderSsrc = 0x10203;
+  const uint32_t kSourceSsrc = 0x123456;
+
+  std::set<uint32_t> ssrcs;
+  ssrcs.insert(kSourceSsrc);
+  rtcp_receiver_->SetSsrcs(kSourceSsrc, ssrcs);
+
+  // Send a compound packet with a TransportFeedback followed by something else.

[stefan-webrtc, 2015/09/30 10:45:27]

Where's the "followed by something else" part? I only see a transport feedback
being sent here?

[sprang_webrtc, 2015/09/30 11:09:27]

Copy/paste mistake. Removed comment.

+  rtcp::TransportFeedback packet;
+  packet.WithMediaSourceSsrc(kSourceSsrc);
+  packet.WithPacketSenderSsrc(kSenderSsrc);
+  packet.WithBase(1, 1000);
+  packet.WithReceivedPacket(1, 1000);
+
+  rtc::scoped_ptr<rtcp::RawPacket> built_packet = packet.Build();
+  ASSERT_TRUE(built_packet.get() != nullptr);
+
+  EXPECT_EQ(0,
+            InjectRtcpPacket(built_packet->Buffer(), built_packet->Length()));
+
+  EXPECT_NE(0u, rtcp_packet_info_.rtcpPacketTypeFlags & kRtcpTransportFeedback);
+  EXPECT_TRUE(rtcp_packet_info_.transport_feedback_.get() != nullptr);
+}
+
+TEST_F(RtcpReceiverTest, HandlesInvalidTransportFeedback) {
+  const uint32_t kSenderSsrc = 0x10203;
+  const uint32_t kSourceSsrc = 0x123456;
+
+  std::set<uint32_t> ssrcs;
+  ssrcs.insert(kSourceSsrc);
+  rtcp_receiver_->SetSsrcs(kSourceSsrc, ssrcs);
+
+  // Send a compound packet with a TransportFeedback followed by something else.
+  rtcp::TransportFeedback packet;
+  packet.WithMediaSourceSsrc(kSourceSsrc);
+  packet.WithPacketSenderSsrc(kSenderSsrc);
+  packet.WithBase(1, 1000);
+  packet.WithReceivedPacket(1, 1000);
+
+  static uint32_t kBitrateBps = 50000;
+  rtcp::Remb remb;
+  remb.From(kSourceSsrc);
+  remb.WithBitrateBps(kBitrateBps);
+  packet.Append(&remb);
+
+  rtc::scoped_ptr<rtcp::RawPacket> built_packet = packet.Build();
+  ASSERT_TRUE(built_packet.get() != nullptr);
+
+  // Modify the TransportFeedback packet so that it is invalid.
+  const size_t kStatusCountOffset = 14;
+  ByteWriter<uint16_t>::WriteBigEndian(
+      &built_packet->MutableBuffer()[kStatusCountOffset], 42);

[stefan-webrtc, 2015/09/30 10:45:27]

Sounds like we may want to fuzz this code. Maybe ask pbos if he could help out?

[sprang_webrtc, 2015/09/30 11:09:27]

Acknowledged.

+
+  EXPECT_EQ(0,
+            InjectRtcpPacket(built_packet->Buffer(), built_packet->Length()));
+
+  // Transport feedback should be ignored, but next packet should work.
+  EXPECT_EQ(0u, rtcp_packet_info_.rtcpPacketTypeFlags & kRtcpTransportFeedback);
+  EXPECT_NE(0u, rtcp_packet_info_.rtcpPacketTypeFlags & kRtcpRemb);
+  EXPECT_EQ(kBitrateBps, rtcp_packet_info_.receiverEstimatedMaxBitrate);
+}
+
 }  // Anonymous namespace
 
 }  // namespace webrtc