NetEq: Fixing a bug that caused rtc::checked_cast to trigger

NetEq: Fixing a bug that caused rtc::checked_cast to trigger

This is a bug that was introduced in
https://codereview.webrtc.org/1230503003, where the variable "int
temp_bufsize" was changed to a size_t. If the packet buffer was
flushed while inserting a packet, temp_bufsize became negative, which
was tested later in an if-statement. Now, with size_t instead, it
would just become very large, and the if-statement would never see a
negative value. The effect was that the packet size in samples could
be updated with a very large positive number, causing an overflow
which triggered rtc::checked_cast in
StatisticsCalculator::GetNetworkStatistics, line 220.

Also adding a test to reproduce the crash. Without the fix, the test
results in the above mentioned checked_cast to trigger. With the fix,
everything works fine.

BUG=chromium:525260
Committed: https://crrev.com/116c84e1b0b7d584a0f04f11de9b2bdb71b25040
Cr-Commit-Position: refs/heads/master@{#9802}

[hlundin-webrtc, 2015-08-27 08:21:55]

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

[hlundin-webrtc, 2015-08-27 08:21:55]

henrik.lundin@webrtc.org changed required reviewers:
+ minyue@webrtc.org

[hlundin-webrtc, 2015-08-27 08:21:56]

Minyue, Ivo (cc: pkasting)

Please, take a look at this CL.

Thanks!
/HL

[hlundin-webrtc, 2015-08-27 08:22:28]

The CQ bit was checked by henrik.lundin@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-27 08:22:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1307893004/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1307893004/1

[hlundin-webrtc, 2015-08-27 08:28:45]

Adding test description

[ivoc, 2015-08-27 08:42:59]

LGTM, nice find, looks like a tricky bug to track down.

[Peter Kasting, 2015-08-27 08:45:33]

pkasting@chromium.org changed reviewers:
+ pkasting@chromium.org

[Peter Kasting, 2015-08-27 08:45:33]

Thanks again for this!  It looks like this was the only non-test callsite of
InsertPacketList(), and the pattern here doesn't look familiar to me as
something I saw other code doing -- not that that's definitive.

The fix seems good to this non-reviewer.

https://codereview.webrtc.org/1307893004/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc (right):

https://codereview.webrtc.org/1307893004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:919: const int
kPayloadLengthSamples = 80;
Nit: Use size_t here...

https://codereview.webrtc.org/1307893004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:939:
rtp_header.header.timestamp += kPayloadLengthSamples;
Nit: ...and cast to uint32_t here

https://codereview.webrtc.org/1307893004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:940:
rtp_header.header.sequenceNumber += 1;
Nit: ++rtp_header.header.sequenceNumber;

[minyue-webrtc, 2015-08-27 09:09:30]

lgtm

Nice catch.

https://codereview.chromium.org/1307893004/diff/20001/webrtc/modules/audio_co...
File webrtc/modules/audio_coding/neteq/neteq_impl.cc (left):

https://codereview.chromium.org/1307893004/diff/20001/webrtc/modules/audio_co...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:661: if ((temp_bufsize > 0) &&
Since we introduced size_t - size_t here, it should not have been difficult to
detect when changing the datatype. But the preceding CL was huge and might not
be easy.

Hope there are no other errors.

[Peter Kasting, 2015-08-27 09:14:54]

https://codereview.chromium.org/1307893004/diff/20001/webrtc/modules/audio_co...
File webrtc/modules/audio_coding/neteq/neteq_impl.cc (left):

https://codereview.chromium.org/1307893004/diff/20001/webrtc/modules/audio_co...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:661: if ((temp_bufsize > 0) &&
On 2015/08/27 09:09:30, minyue-webrtc wrote:
> Since we introduced size_t - size_t here, it should not have been difficult to
> detect when changing the datatype. But the preceding CL was huge and might not
> be easy.

I wouldn't have caught this even had I just done this portion of the change. 
InsertPacketList() sounds as if it will either increase the buffer size or do
nothing, and the "> 0" check here looks as if it's distinguishing from the "0"
case rather than from the negative number case.

(I did look as a result of this CL to make sure InsertPacketList() documents
that it can decrease the buffer size.  I thought about suggesting it be called
InsertPacketsAndMaybeFlush() or something like that, but it seems awkward.)

[minyue-webrtc, 2015-08-27 09:36:50]

On 2015/08/27 09:14:54, Peter Kasting wrote:
>
https://codereview.chromium.org/1307893004/diff/20001/webrtc/modules/audio_co...
> File webrtc/modules/audio_coding/neteq/neteq_impl.cc (left):
> 
>
https://codereview.chromium.org/1307893004/diff/20001/webrtc/modules/audio_co...
> webrtc/modules/audio_coding/neteq/neteq_impl.cc:661: if ((temp_bufsize > 0) &&
> On 2015/08/27 09:09:30, minyue-webrtc wrote:
> > Since we introduced size_t - size_t here, it should not have been difficult
to
> > detect when changing the datatype. But the preceding CL was huge and might
not
> > be easy.
> 
> I wouldn't have caught this even had I just done this portion of the change. 
> InsertPacketList() sounds as if it will either increase the buffer size or do
> nothing, and the "> 0" check here looks as if it's distinguishing from the "0"
> case rather than from the negative number case.
> 
> (I did look as a result of this CL to make sure InsertPacketList() documents
> that it can decrease the buffer size.  I thought about suggesting it be called
> InsertPacketsAndMaybeFlush() or something like that, but it seems awkward.)

Yes, Peter, when I reviewed your CL, I appreciated that you did it with very
good carefulness. There were a lot of background to look at, plus the need to
understand many not-so-comprehensible codes which were written long ago. It was
a nice work, no doubt.

Just technically, can any compiler switch (-Wconversion) give a warning on
"unsigned = unsigned - unsigned"

[Peter Kasting, 2015-08-27 09:44:01]

On 2015/08/27 09:36:50, minyue-webrtc wrote:
> On 2015/08/27 09:14:54, Peter Kasting wrote:
> >
>
https://codereview.chromium.org/1307893004/diff/20001/webrtc/modules/audio_co...
> > File webrtc/modules/audio_coding/neteq/neteq_impl.cc (left):
> > 
> >
>
https://codereview.chromium.org/1307893004/diff/20001/webrtc/modules/audio_co...
> > webrtc/modules/audio_coding/neteq/neteq_impl.cc:661: if ((temp_bufsize > 0)
&&
> > On 2015/08/27 09:09:30, minyue-webrtc wrote:
> > > Since we introduced size_t - size_t here, it should not have been
difficult
> to
> > > detect when changing the datatype. But the preceding CL was huge and might
> not
> > > be easy.
> > 
> > I wouldn't have caught this even had I just done this portion of the change.

> > InsertPacketList() sounds as if it will either increase the buffer size or
do
> > nothing, and the "> 0" check here looks as if it's distinguishing from the
"0"
> > case rather than from the negative number case.
> > 
> > (I did look as a result of this CL to make sure InsertPacketList() documents
> > that it can decrease the buffer size.  I thought about suggesting it be
called
> > InsertPacketsAndMaybeFlush() or something like that, but it seems awkward.)
> 
> Yes, Peter, when I reviewed your CL, I appreciated that you did it with very
> good carefulness. There were a lot of background to look at, plus the need to
> understand many not-so-comprehensible codes which were written long ago. It
was
> a nice work, no doubt.
> 
> Just technically, can any compiler switch (-Wconversion) give a warning on
> "unsigned = unsigned - unsigned"

I'm not very familiar with the gcc/clang warnings, but I don't see why "unsigned
= unsigned - unsigned" would be a generalized unsafe pattern to warn about. 
It's no more or less safe than any other version of "unsigned = anything -
anything".

[hlundin-webrtc, 2015-08-27 09:47:57]

On 2015/08/27 09:44:01, Peter Kasting wrote:
> On 2015/08/27 09:36:50, minyue-webrtc wrote:
> > On 2015/08/27 09:14:54, Peter Kasting wrote:
> > >
> >
>
https://codereview.chromium.org/1307893004/diff/20001/webrtc/modules/audio_co...
> > > File webrtc/modules/audio_coding/neteq/neteq_impl.cc (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/1307893004/diff/20001/webrtc/modules/audio_co...
> > > webrtc/modules/audio_coding/neteq/neteq_impl.cc:661: if ((temp_bufsize >
0)
> &&
> > > On 2015/08/27 09:09:30, minyue-webrtc wrote:
> > > > Since we introduced size_t - size_t here, it should not have been
> difficult
> > to
> > > > detect when changing the datatype. But the preceding CL was huge and
might
> > not
> > > > be easy.
> > > 
> > > I wouldn't have caught this even had I just done this portion of the
change.
> 
> > > InsertPacketList() sounds as if it will either increase the buffer size or
> do
> > > nothing, and the "> 0" check here looks as if it's distinguishing from the
> "0"
> > > case rather than from the negative number case.
> > > 
> > > (I did look as a result of this CL to make sure InsertPacketList()
documents
> > > that it can decrease the buffer size.  I thought about suggesting it be
> called
> > > InsertPacketsAndMaybeFlush() or something like that, but it seems
awkward.)
> > 
> > Yes, Peter, when I reviewed your CL, I appreciated that you did it with very
> > good carefulness. There were a lot of background to look at, plus the need
to
> > understand many not-so-comprehensible codes which were written long ago. It
> was
> > a nice work, no doubt.
> > 
> > Just technically, can any compiler switch (-Wconversion) give a warning on
> > "unsigned = unsigned - unsigned"
> 
> I'm not very familiar with the gcc/clang warnings, but I don't see why
"unsigned
> = unsigned - unsigned" would be a generalized unsafe pattern to warn about. 
> It's no more or less safe than any other version of "unsigned = anything -
> anything".

And, sometimes we deliberately make use of the wrap-around, e.g., when comparing
uint32 RTP timestamps.

[hlundin-webrtc, 2015-08-27 09:50:42]

Review comments

[hlundin-webrtc, 2015-08-27 09:53:25]

Review comments

[hlundin-webrtc, 2015-08-27 09:53:43]

Patchset #3 (id:40001) has been deleted

[hlundin-webrtc, 2015-08-27 09:54:03]

https://codereview.webrtc.org/1307893004/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc (right):

https://codereview.webrtc.org/1307893004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:919: const int
kPayloadLengthSamples = 80;
On 2015/08/27 08:45:33, Peter Kasting wrote:
> Nit: Use size_t here...

Done.

https://codereview.webrtc.org/1307893004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:939:
rtp_header.header.timestamp += kPayloadLengthSamples;
On 2015/08/27 08:45:33, Peter Kasting wrote:
> Nit: ...and cast to uint32_t here

Done.

https://codereview.webrtc.org/1307893004/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:940:
rtp_header.header.sequenceNumber += 1;
On 2015/08/27 08:45:33, Peter Kasting wrote:
> Nit: ++rtp_header.header.sequenceNumber;

Done.

[hlundin-webrtc, 2015-08-27 09:54:06]

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

[hlundin-webrtc, 2015-08-27 09:54:07]

The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org,
minyue@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1307893004/#ps60001 (title:
"Review comments")

[commit-bot: I haz the power, 2015-08-27 09:54:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1307893004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1307893004/60001

[minyue-webrtc, 2015-08-27 10:30:29]

On 2015/08/27 09:44:01, Peter Kasting wrote:
> On 2015/08/27 09:36:50, minyue-webrtc wrote:
> > On 2015/08/27 09:14:54, Peter Kasting wrote:
> > >
> >
>
https://codereview.chromium.org/1307893004/diff/20001/webrtc/modules/audio_co...
> > > File webrtc/modules/audio_coding/neteq/neteq_impl.cc (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/1307893004/diff/20001/webrtc/modules/audio_co...
> > > webrtc/modules/audio_coding/neteq/neteq_impl.cc:661: if ((temp_bufsize >
0)
> &&
> > > On 2015/08/27 09:09:30, minyue-webrtc wrote:
> > > > Since we introduced size_t - size_t here, it should not have been
> difficult
> > to
> > > > detect when changing the datatype. But the preceding CL was huge and
might
> > not
> > > > be easy.
> > > 
> > > I wouldn't have caught this even had I just done this portion of the
change.
> 
> > > InsertPacketList() sounds as if it will either increase the buffer size or
> do
> > > nothing, and the "> 0" check here looks as if it's distinguishing from the
> "0"
> > > case rather than from the negative number case.
> > > 
> > > (I did look as a result of this CL to make sure InsertPacketList()
documents
> > > that it can decrease the buffer size.  I thought about suggesting it be
> called
> > > InsertPacketsAndMaybeFlush() or something like that, but it seems
awkward.)
> > 
> > Yes, Peter, when I reviewed your CL, I appreciated that you did it with very
> > good carefulness. There were a lot of background to look at, plus the need
to
> > understand many not-so-comprehensible codes which were written long ago. It
> was
> > a nice work, no doubt.
> > 
> > Just technically, can any compiler switch (-Wconversion) give a warning on
> > "unsigned = unsigned - unsigned"
> 
> I'm not very familiar with the gcc/clang warnings, but I don't see why
"unsigned
> = unsigned - unsigned" would be a generalized unsafe pattern to warn about. 
> It's no more or less safe than any other version of "unsigned = anything -
> anything".

Yes, they are equally unsafe. What I know is that gcc warns "unsigned = signed -
signed", not because of unsafety but an implicit conversion. unsigned = unsigned
- unsigned is considered well defined and free of conversion, and hence has no
warning.

[commit-bot: I haz the power, 2015-08-27 11:54:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-27 11:54:34]

Try jobs failed on following builders:
  android on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[hlundin-webrtc, 2015-08-27 20:13:55]

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

[commit-bot: I haz the power, 2015-08-27 20:14:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1307893004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1307893004/60001

[commit-bot: I haz the power, 2015-08-27 20:14:52]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-08-27 20:15:02]

Patchset 3 (id:??) landed as
https://crrev.com/116c84e1b0b7d584a0f04f11de9b2bdb71b25040
Cr-Commit-Position: refs/heads/master@{#9802}