Fix a bug in RtcEventLogSource

webrtc/modules/rtp_rtcp/source/rtp_utility.cc

Index: webrtc/modules/rtp_rtcp/source/rtp_utility.cc
diff --git a/webrtc/modules/rtp_rtcp/source/rtp_utility.cc b/webrtc/modules/rtp_rtcp/source/rtp_utility.cc
index d003c0bdb0a511f2190b51e6b1b46e17297c518f..4cd61eccc2c80308a06df1c7cbb18f9c6c484eea 100644
--- a/webrtc/modules/rtp_rtcp/source/rtp_utility.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtp_utility.cc
@@ -296,9 +296,14 @@ bool RtpHeaderParser::Parse(RTPHeader* header,
     }
     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;
 }