Add a ParseHeader method to RtcpPacket, for parsing common RTCP header.

webrtc/modules/rtp_rtcp/source/rtcp_utility.cc

Index: webrtc/modules/rtp_rtcp/source/rtcp_utility.cc
diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_utility.cc b/webrtc/modules/rtp_rtcp/source/rtcp_utility.cc
index 4e37cf3716d916c7ecb1bfd6185b1790dfa15f5f..6f369802a1ed6dd5c63a5727600aeb051d0bd9c1 100644
--- a/webrtc/modules/rtp_rtcp/source/rtcp_utility.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtcp_utility.cc
@@ -14,6 +14,10 @@
 #include <math.h>   // ceil
 #include <string.h> // memcpy
 
+#include "webrtc/base/checks.h"
+#include "webrtc/base/logging.h"
+#include "webrtc/modules/rtp_rtcp/source/byte_io.h"
+
 namespace webrtc {
 
 namespace RTCPUtility {
@@ -155,43 +159,40 @@ RTCPUtility::RTCPParserV2::IterateTopLevel()
 {
     for (;;)
     {
-        RTCPCommonHeader header;
-
-        const bool success = RTCPParseCommonHeader(_ptrRTCPData,
-                                                    _ptrRTCPDataEnd,
-                                                    header);
+      RtcpCommonHeader header;
+      size_t bytes_left = _ptrRTCPDataEnd - _ptrRTCPData;
+      if (bytes_left == 0)

[åsapersson, 2015/09/08 10:20:42]

checked within RtcpParseCommonHeader?

[sprang_webrtc, 2015/09/09 09:18:54]

I think it would be better for RtcpParseCommonHeader to look for actual errors.
In the context here, having 0 bytes left in the buffer is OK. On the other hand,
having 3 bytes left is probably not OK but would yield the same return value.

Rather than adding a special case to RtcpParseCommonHeader for 0 bytes left (so
it doesn't spam the logs with warnings), I think it's better to handle that case
here.

Does that make sense?

+        return;
 
-        if (!success)
-        {
+      if (!RtcpParseCommonHeader(_ptrRTCPData, 0, bytes_left, &header)) {
             return;
         }
-        _ptrRTCPBlockEnd = _ptrRTCPData + header.LengthInOctets;
+        _ptrRTCPBlockEnd = _ptrRTCPData + header.BlockSize();
         if (_ptrRTCPBlockEnd > _ptrRTCPDataEnd)
         {
             // Bad block!
             return;
         }
 
-        switch (header.PT)
-        {
+        switch (header.packet_type) {
         case PT_SR:
         {
             // number of Report blocks
-            _numberOfBlocks = header.IC;
+            _numberOfBlocks = header.count_or_format;
             ParseSR();
             return;
         }
         case PT_RR:
         {
             // number of Report blocks
-            _numberOfBlocks = header.IC;
+            _numberOfBlocks = header.count_or_format;
             ParseRR();
             return;
         }
         case PT_SDES:
         {
             // number of SDES blocks
-            _numberOfBlocks = header.IC;
+            _numberOfBlocks = header.count_or_format;
             const bool ok = ParseSDES();
             if (!ok)
             {
@@ -202,7 +203,7 @@ RTCPUtility::RTCPParserV2::IterateTopLevel()
         }
         case PT_BYE:
         {
-            _numberOfBlocks = header.IC;
+          _numberOfBlocks = header.count_or_format;
             const bool ok = ParseBYE();
             if (!ok)
             {
@@ -214,7 +215,7 @@ RTCPUtility::RTCPParserV2::IterateTopLevel()
         case PT_IJ:
         {
             // number of Report blocks
-            _numberOfBlocks = header.IC;
+            _numberOfBlocks = header.count_or_format;
             ParseIJ();
             return;
         }
@@ -415,10 +416,9 @@ RTCPUtility::RTCPParserV2::Validate()
         return; // NOT VALID
     }
 
-    RTCPCommonHeader header;
-    const bool success = RTCPParseCommonHeader(_ptrRTCPDataBegin,
-                                               _ptrRTCPDataEnd,
-                                               header);
+    RtcpCommonHeader header;
+    const bool success = RtcpParseCommonHeader(

[åsapersson, 2015/09/08 10:20:42]

could _ptrRTCPDataBegin be larger than _ptrRTCPDataEnd?

[sprang_webrtc, 2015/09/09 09:18:54]

It shouldn't, but I can add a check anyway.

+        _ptrRTCPDataBegin, 0, _ptrRTCPDataEnd - _ptrRTCPDataBegin, &header);
 
     if (!success)
     {
@@ -437,8 +437,7 @@ RTCPUtility::RTCPParserV2::Validate()
 
     if (!_RTCPReducedSizeEnable)
     {
-        if ((header.PT != PT_SR) && (header.PT != PT_RR))
-        {
+      if ((header.packet_type != PT_SR) && (header.packet_type != PT_RR)) {
             return; // NOT VALID
         }
     }
@@ -458,48 +457,70 @@ RTCPUtility::RTCPParserV2::EndCurrentBlock()
     _ptrRTCPData = _ptrRTCPBlockEnd;
 }
 
-bool
-RTCPUtility::RTCPParseCommonHeader( const uint8_t* ptrDataBegin,
-                                    const uint8_t* ptrDataEnd,
-                                    RTCPCommonHeader& parsedHeader)
-{
-    if (!ptrDataBegin || !ptrDataEnd)
-    {
-        return false;
-    }
-
-    //  0                   1                   2                   3
-    //  0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
-    // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-    // |V=2|P|    IC   |      PT       |             length            |
-    // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-    //
-    // Common header for all RTCP packets, 4 octets.
+//  0                   1                   2                   3
+//  0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
+// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+// |V=2|P|    IC   |      PT       |             length            |
+// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+//
+// Common header for all RTCP packets, 4 octets.
+
+bool RTCPUtility::RtcpParseCommonHeader(const uint8_t* packet,
+                                        size_t index,
+                                        size_t max_length,

[åsapersson, 2015/09/08 10:20:42]

maybe remove index

[sprang_webrtc, 2015/09/09 09:18:54]

Ok. I use an offset in other upcoming CL's but I can change them to add offset
to the parameter instead.

+                                        RtcpCommonHeader* parsed_header) {
+  DCHECK(parsed_header != nullptr);
+  size_t length = max_length - index;

[åsapersson, 2015/09/08 10:20:42]

what if index > max_length

[sprang_webrtc, 2015/09/09 09:18:54]

Removed index.

+  const size_t kHeaderLength = 4;
+  if (length < kHeaderLength) {
+    LOG(LS_WARNING) << "Too little data (" << length << " byte"
+                    << (length != 1 ? "s" : "")
+                    << ") remaining in buffer to parse RTCP header (4 bytes).";
+    return false;
+  }
 
-    if ((ptrDataEnd - ptrDataBegin) < 4)
-    {
-        return false;
-    }
+  const uint8_t kRtcpVersion = 2;
+  uint8_t version = packet[index] >> 6;
+  if (version != kRtcpVersion) {
+    LOG(LS_WARNING) << "Invalid RTCP header: Version must be " << kRtcpVersion
+                    << " but was " << version;
+    return false;
+  }
 
-    parsedHeader.V              = ptrDataBegin[0] >> 6;
-    parsedHeader.P              = ((ptrDataBegin[0] & 0x20) == 0) ? false : true;
-    parsedHeader.IC             = ptrDataBegin[0] & 0x1f;
-    parsedHeader.PT             = ptrDataBegin[1];
+  bool has_padding = (packet[index] & 0x20) != 0;
+  uint8_t format = packet[index] & 0x1F;
+  uint8_t packet_type = packet[index + 1];
+  size_t packet_size_words =
+      ByteReader<uint16_t>::ReadBigEndian(&packet[index + 2]) + 1;
 
-    parsedHeader.LengthInOctets = (ptrDataBegin[2] << 8) + ptrDataBegin[3] + 1;
-    parsedHeader.LengthInOctets *= 4;
+  if (length < packet_size_words * 4) {
+    LOG(LS_WARNING) << "Buffer too small (" << length
+                    << " bytes) to fit a FeedbackPacket of "

[åsapersson, 2015/09/08 10:20:42]

remove FeedbackPacket in comment

[sprang_webrtc, 2015/09/09 09:18:54]

Done.

+                    << packet_size_words << " 32bit words.";
+    return false;
+  }
 
-    if(parsedHeader.LengthInOctets == 0)
-    {
-        return false;
-    }
-    // Check if RTP version field == 2
-    if (parsedHeader.V != 2)
-    {
-        return false;
+  size_t payload_size = packet_size_words * 4;
+  uint8_t padding_bytes = 0;
+  if (has_padding) {
+    padding_bytes = packet[index + payload_size - 1];
+    if (kHeaderLength + padding_bytes > payload_size) {
+      LOG(LS_WARNING) << "Invalid RTCP header: Too many padding bytes ("
+                      << padding_bytes << ") for a packet size of "
+                      << payload_size << "bytes.";
+      return false;
     }
+    payload_size -= padding_bytes;
+  }
+  payload_size -= kHeaderLength;
 
-    return true;
+  parsed_header->version = kRtcpVersion;
+  parsed_header->count_or_format = format;
+  parsed_header->packet_type = packet_type;
+  parsed_header->payload_size_bytes = payload_size;
+  parsed_header->padding_bytes = padding_bytes;
+
+  return true;
 }
 
 bool
@@ -1137,10 +1158,9 @@ bool RTCPUtility::RTCPParserV2::ParseXrUnsupportedBlockType(
   return false;
 }
 
-bool
-RTCPUtility::RTCPParserV2::ParseFBCommon(const RTCPCommonHeader& header)
-{
-    assert((header.PT == PT_RTPFB) || (header.PT == PT_PSFB)); // Parser logic check
+bool RTCPUtility::RTCPParserV2::ParseFBCommon(const RtcpCommonHeader& header) {
+  assert((header.packet_type == PT_RTPFB) ||
+         (header.packet_type == PT_PSFB));  // Parser logic check
 
     const ptrdiff_t length = _ptrRTCPBlockEnd - _ptrRTCPData;
 
@@ -1162,12 +1182,10 @@ RTCPUtility::RTCPParserV2::ParseFBCommon(const RTCPCommonHeader& header)
     mediaSSRC += *_ptrRTCPData++ << 8;
     mediaSSRC += *_ptrRTCPData++;
 
-    if (header.PT == PT_RTPFB)
-    {
+    if (header.packet_type == PT_RTPFB) {
         // Transport layer feedback
 
-        switch (header.IC)
-        {
+        switch (header.count_or_format) {
         case 1:
         {
             // NACK
@@ -1222,12 +1240,9 @@ RTCPUtility::RTCPParserV2::ParseFBCommon(const RTCPCommonHeader& header)
         }
         EndCurrentBlock();
         return false;
-    }
-    else if (header.PT == PT_PSFB)
-    {
+    } else if (header.packet_type == PT_PSFB) {
         // Payload specific feedback
-        switch (header.IC)
-        {
+        switch (header.count_or_format) {
         case 1:
             // PLI
           _packetType = RTCPPacketTypes::kPsfbPli;
@@ -1579,9 +1594,7 @@ RTCPUtility::RTCPParserV2::ParseFIRItem()
     return true;
 }
 
-bool
-RTCPUtility::RTCPParserV2::ParseAPP( const RTCPCommonHeader& header)
-{
+bool RTCPUtility::RTCPParserV2::ParseAPP(const RtcpCommonHeader& header) {
     ptrdiff_t length = _ptrRTCPBlockEnd - _ptrRTCPData;
 
     if (length < 12) // 4 * 3, RFC 3550 6.7 APP: Application-Defined RTCP Packet
@@ -1606,7 +1619,7 @@ RTCPUtility::RTCPParserV2::ParseAPP( const RTCPCommonHeader& header)
 
     _packetType = RTCPPacketTypes::kApp;
 
-    _packet.APP.SubType = header.IC;
+    _packet.APP.SubType = header.count_or_format;
     _packet.APP.Name = name;
 
     _state = ParseState::State_AppItem;
@@ -1651,24 +1664,22 @@ RTCPUtility::RTCPPacketIterator::RTCPPacketIterator(uint8_t* rtcpData,
 RTCPUtility::RTCPPacketIterator::~RTCPPacketIterator() {
 }
 
-const RTCPUtility::RTCPCommonHeader*
-RTCPUtility::RTCPPacketIterator::Begin()
-{
+const RTCPUtility::RtcpCommonHeader* RTCPUtility::RTCPPacketIterator::Begin() {
     _ptrBlock = _ptrBegin;
 
     return Iterate();
 }
 
-const RTCPUtility::RTCPCommonHeader*
-RTCPUtility::RTCPPacketIterator::Iterate()
-{
-    const bool success = RTCPParseCommonHeader(_ptrBlock, _ptrEnd, _header);
+const RTCPUtility::RtcpCommonHeader*
+RTCPUtility::RTCPPacketIterator::Iterate() {
+  const bool success =
+      RtcpParseCommonHeader(_ptrBlock, 0, _ptrEnd - _ptrBlock, &_header);
     if (!success)
     {
         _ptrBlock = NULL;
         return NULL;
     }
-    _ptrBlock += _header.LengthInOctets;
+    _ptrBlock += _header.BlockSize();
 
     if (_ptrBlock > _ptrEnd)
     {
@@ -1679,9 +1690,8 @@ RTCPUtility::RTCPPacketIterator::Iterate()
     return &_header;
 }
 
-const RTCPUtility::RTCPCommonHeader*
-RTCPUtility::RTCPPacketIterator::Current()
-{
+const RTCPUtility::RtcpCommonHeader*
+RTCPUtility::RTCPPacketIterator::Current() {
     if (!_ptrBlock)
     {
         return NULL;