Fix a bug in RtcEventLogSource

Fix a bug in RtcEventLogSource

A recent change (https://codereview.webrtc.org/2855143002/) introduced
a bug in RtcEventLogSource::NextPacket(). The rtp_packet_index_ must
be incremented when a valid packet is found and delivered. Otherwise,
the same packet will be delivered over and over again.

The recent change also altered the way that audio packets are sifted out. Now, the RTP header is always parsed before discarding any non-audio packets. This means that RtpHeaderParser::Parse is always called, also with video packets, which sometimes contain padding. When header-only dumps (such as RtcEventLogs) are created, the payload is stripped, and the payload length is equal to
the RTP header length. However, if the original packet was padded, the
RTP header will carry information about this padding length, and the
parser will check that the pyaload length is at least the header +
padding. This is not the case for header-only dumps, and the parser will return an error. In this CL, we ignore that error when a header-only packet has padding length larger than 0.

BUG=webrtc:7538
Review-Url: https://codereview.webrtc.org/2912323003
Cr-Commit-Position: refs/heads/master@{#18385}
Committed: https://chromium.googlesource.com/external/webrtc/+/7a2862a933addface23586ef01ac555543a228c7

[hlundin-webrtc, 2017-05-31 15:22:49]

Description was changed from

==========
Fix a bug in RtcEventLogSource

A recent change (https://codereview.webrtc.org/2855143002/) introduced
a bug in RtcEventLogSource::NextPacket(). The rtp_packet_index_ must
be incremented when a valid packet is found and delivered. Otherwise,
the same packet will be delivered over and over again.

This CL also fixes a problem in RtpHeaderParser::Parse related to
padded RTP packets. When header-only dumps (such as RtcEventLogs) are
parsed, the payload is stripped, and the payload length is equal to
the RTP header length. However, if the original packet was padded, the
RTP header will carry information about this padding length, and the
parser will check that the pyaload length is at least the header +
padding. This is not the case for header-only dumps.

BUG=webrtc:7538
==========

to

==========
Fix a bug in RtcEventLogSource

A recent change (https://codereview.webrtc.org/2855143002/) introduced
a bug in RtcEventLogSource::NextPacket(). The rtp_packet_index_ must
be incremented when a valid packet is found and delivered. Otherwise,
the same packet will be delivered over and over again.

This CL also fixes a problem in RtpHeaderParser::Parse related to
padded RTP packets. When header-only dumps (such as RtcEventLogs) are
parsed, the payload is stripped, and the payload length is equal to
the RTP header length. However, if the original packet was padded, the
RTP header will carry information about this padding length, and the
parser will check that the pyaload length is at least the header +
padding. This is not the case for header-only dumps.

BUG=webrtc:7538
==========

[hlundin-webrtc, 2017-05-31 15:22:49]

henrik.lundin@webrtc.org changed reviewers:
+ danilchap@webrtc.org, ivoc@webrtc.org

[hlundin-webrtc, 2017-05-31 15:23:46]

PTAL at this small bugfix.

danilchap: webrtc/modules/rtp_rtcp/source/rtp_utility.cc
ivoc: webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc

Thanks!

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

https://codereview.webrtc.org/2912323003/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtp_utility.cc (right):

https://codereview.webrtc.org/2912323003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_utility.cc:302: if (header->headerLength !=
static_cast<size_t>(length) &&
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]

https://codereview.webrtc.org/2912323003/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtp_utility.cc (right):

https://codereview.webrtc.org/2912323003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_utility.cc:302: if (header->headerLength !=
static_cast<size_t>(length) &&
On 2017/05/31 17:30:21, danilchap wrote:
> 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?

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:57]

https://codereview.webrtc.org/2912323003/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtp_utility.cc (right):

https://codereview.webrtc.org/2912323003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_utility.cc:302: if (header->headerLength !=
static_cast<size_t>(length) &&
On 2017/05/31 18:57:14, danilchap wrote:
> On 2017/05/31 17:30:21, danilchap wrote:
> > 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?
> 
> Realized there is a simpler solution:
> if (headerLength == length) {
>   header->paddingLength = 0;
>   return true;
> }
> 
> That looks both safe and support parsing header only.

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....

[ivoc, 2017-06-01 07:10:39]

webrtc/modules/audio_coding/neteq/tools/rtc_event_log_source.cc lgtm

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

https://codereview.webrtc.org/2912323003/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtp_utility.cc (right):

https://codereview.webrtc.org/2912323003/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtp_utility.cc:302: if (header->headerLength !=
static_cast<size_t>(length) &&
On 2017/05/31 21:02:56, hlundin-webrtc wrote:
> On 2017/05/31 18:57:14, danilchap wrote:
> > On 2017/05/31 17:30:21, danilchap wrote:
> > > 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?
> > 
> > Realized there is a simpler solution:
> > if (headerLength == length) {
> >   header->paddingLength = 0;
> >   return true;
> > }
> > 
> > That looks both safe and support parsing header only.
> 
> 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....

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.

[hlundin-webrtc, 2017-06-01 10:10:31]

Description was changed from

==========
Fix a bug in RtcEventLogSource

A recent change (https://codereview.webrtc.org/2855143002/) introduced
a bug in RtcEventLogSource::NextPacket(). The rtp_packet_index_ must
be incremented when a valid packet is found and delivered. Otherwise,
the same packet will be delivered over and over again.

This CL also fixes a problem in RtpHeaderParser::Parse related to
padded RTP packets. When header-only dumps (such as RtcEventLogs) are
parsed, the payload is stripped, and the payload length is equal to
the RTP header length. However, if the original packet was padded, the
RTP header will carry information about this padding length, and the
parser will check that the pyaload length is at least the header +
padding. This is not the case for header-only dumps.

BUG=webrtc:7538
==========

to

==========
Fix a bug in RtcEventLogSource

A recent change (https://codereview.webrtc.org/2855143002/) introduced
a bug in RtcEventLogSource::NextPacket(). The rtp_packet_index_ must
be incremented when a valid packet is found and delivered. Otherwise,
the same packet will be delivered over and over again.

The recent change also altered the way that audio packets are sifted out. Now,
the RTP header is always parsed before discarding any non-audio packets. This
means that RtpHeaderParser::Parse is always called, also with video packets,
which sometimes contain padding. When header-only dumps (such as RtcEventLogs)
are created, the payload is stripped, and the payload length is equal to
the RTP header length. However, if the original packet was padded, the
RTP header will carry information about this padding length, and the
parser will check that the pyaload length is at least the header +
padding. This is not the case for header-only dumps, and the parser will return
an error. In this CL, we ignore that error when a header-only packet has padding
length larger than 0.

BUG=webrtc:7538
==========

[hlundin-webrtc, 2017-06-01 10:10:31]

henrik.lundin@webrtc.org changed reviewers:
- danilchap@webrtc.org

[hlundin-webrtc, 2017-06-01 10:12:17]

This seems a bit hairy. Since the problem I'm trying to solve is for test tools
only, I'm now resorting to solving it in the tools as well. That is, I'm
reverting my change to rtp_utility.cc, and make a work-around in
neteq/tools/packet.cc.

Ivo, please, take a new look.
Danil, you are off the hook :)

[ivoc, 2017-06-01 13:02:25]

https://codereview.webrtc.org/2912323003/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/tools/packet.cc (right):

https://codereview.webrtc.org/2912323003/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/tools/packet.cc:138: if (!valid_header &&
!header_only_with_padding) {
It's a bit unfortunate that we can't tell the difference between valid_header
being false for this reason or for other reasons. It could happen that
header_only_with_padding is true but valid_header is false for another
(unrelated) reason, and it will be ignored.
I'm not sure how to improve this, or maybe it's not that important for this
testing code?

[hlundin-webrtc, 2017-06-01 13:31:32]

The CQ bit was checked by henrik.lundin@webrtc.org

[hlundin-webrtc, 2017-06-01 13:31:32]

The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2912323003/#ps40001 (title:
"One more small fix")

[commit-bot: I haz the power, 2017-06-01 13:31:42]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-06-01 14:41:12]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496323892543030,
"parent_rev": "e80f4c91d0a2854a339e419162fdcd1d916f7de0", "commit_rev":
"7a2862a933addface23586ef01ac555543a228c7"}

[commit-bot: I haz the power, 2017-06-01 14:41:15]

Description was changed from

==========
Fix a bug in RtcEventLogSource

A recent change (https://codereview.webrtc.org/2855143002/) introduced
a bug in RtcEventLogSource::NextPacket(). The rtp_packet_index_ must
be incremented when a valid packet is found and delivered. Otherwise,
the same packet will be delivered over and over again.

The recent change also altered the way that audio packets are sifted out. Now,
the RTP header is always parsed before discarding any non-audio packets. This
means that RtpHeaderParser::Parse is always called, also with video packets,
which sometimes contain padding. When header-only dumps (such as RtcEventLogs)
are created, the payload is stripped, and the payload length is equal to
the RTP header length. However, if the original packet was padded, the
RTP header will carry information about this padding length, and the
parser will check that the pyaload length is at least the header +
padding. This is not the case for header-only dumps, and the parser will return
an error. In this CL, we ignore that error when a header-only packet has padding
length larger than 0.

BUG=webrtc:7538
==========

to

==========
Fix a bug in RtcEventLogSource

A recent change (https://codereview.webrtc.org/2855143002/) introduced
a bug in RtcEventLogSource::NextPacket(). The rtp_packet_index_ must
be incremented when a valid packet is found and delivered. Otherwise,
the same packet will be delivered over and over again.

The recent change also altered the way that audio packets are sifted out. Now,
the RTP header is always parsed before discarding any non-audio packets. This
means that RtpHeaderParser::Parse is always called, also with video packets,
which sometimes contain padding. When header-only dumps (such as RtcEventLogs)
are created, the payload is stripped, and the payload length is equal to
the RTP header length. However, if the original packet was padded, the
RTP header will carry information about this padding length, and the
parser will check that the pyaload length is at least the header +
padding. This is not the case for header-only dumps, and the parser will return
an error. In this CL, we ignore that error when a header-only packet has padding
length larger than 0.

BUG=webrtc:7538

Review-Url: https://codereview.webrtc.org/2912323003
Cr-Commit-Position: refs/heads/master@{#18385}
Committed:
https://chromium.googlesource.com/external/webrtc/+/7a2862a933addface23586ef0...
==========

[commit-bot: I haz the power, 2017-06-01 14:41:16]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/webrtc/+/7a2862a933addface23586ef0...