Fix a bug in RtcEventLogSource

webrtc/modules/rtp_rtcp/source/rtp_utility.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.
  */
 
@@ skipping 278 matching lines @@
     static constexpr uint16_t kRtpOneByteHeaderExtensionId = 0xBEDE;
     if (definedByProfile == kRtpOneByteHeaderExtensionId) {
       const uint8_t* ptrRTPDataExtensionEnd = ptr + XLen;
       ParseOneByteExtensionHeader(header,
                                   ptrExtensionMap,
                                   ptrRTPDataExtensionEnd,
                                   ptr);
     }
     header->headerLength += XLen;
   }
-  if (header->headerLength + header->paddingLength >
-      static_cast<size_t>(length))
+  // If the packet consists of only the header, then ignore the padding length.
+  // This is likely only happening when parsing a header-only rtpdump file,
+  // where the payloads have been stripped out.
+  if (header->headerLength != static_cast<size_t>(length) &&

[danilchap, 2017/05/31 17:30:21]

this change looks dangerous to me:
It will not be hard to send packet over network with length = headerLength and
paddingLength > 0 then.
by returning true RtpHeaderParser promise header is valid, in particular
provides guarantee that length >= header_length + padding_length.

RtpReceiver later calculate the payload size as
size_t payload_size = length - header_length - padding_length; which will be
positive number and passed to depacketizer causing read out of boundary error.


Is it an option for dump reader to create full packet buffer, populate only
header part of it (and, for padding case, put 1 into last byte), but pass all of
the buffer to the parser so that parser wouldn't need to make an exception for
it?

[danilchap, 2017/05/31 18:57:14]

Realized there is a simpler solution:
if (headerLength == length) {
  header->paddingLength = 0;
  return true;
}

That looks both safe and support parsing header only.

[hlundin-webrtc, 2017/05/31 21:02:56]

But that solution does in fact parse out values that were not in the packet.
This seems to be unjust.

I would argue that the *header* parser need not bother to check that the total
packet length is correct, only that it does not parse header fields outside of
the supplied bitstream length. Would you consider changing so that callers of
Parse became responsible of checking the payload length? That would mean here:
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/rtp_rtcp/sour....

[danilchap, 2017/06/01 09:45:15]

HeaderParser need to fill RTPHeader structure and confirm result is valid.
paddingLength is one of the fields in the RTPHeader.
when headerLength == length it is not right to take padding size from the last
byte.
One (current way) - declare those packets invalid by returning false.
Another (my proposal) - declare those packets valid ignoring P bit.

Moving check to RtpHeaderParserImpl imho will be confusing since currently
RtpHeaderParserImpl has no validation logic, it is only a wrapper.

Long term receiving code shifting to use RtpPacketReceived::Parse instead to
handle incoming packets.

+      header->headerLength + header->paddingLength >
+          static_cast<size_t>(length)) {
     return false;
+  }
   return true;
 }
 
 void RtpHeaderParser::ParseOneByteExtensionHeader(
     RTPHeader* header,
     const RtpHeaderExtensionMap* ptrExtensionMap,
     const uint8_t* ptrRTPDataExtensionEnd,
     const uint8_t* ptr) const {
   if (!ptrExtensionMap) {
     return;
@@ skipping 172 matching lines @@
           return;
         }
       }
     }
     ptr += (len + 1);
   }
 }
 
 }  // namespace RtpUtility
 }  // namespace webrtc