Check frame coded size in H264 parsers to avoid integer overflows

Check frame coded size in H264 parsers to avoid integer overflows

The bug crbug.com/541669 was originally found by fuzzing and reported
for media/formats/mp2t/es_parser_h264.cc, but I've noticed that
content/common/gpu/media/h264_decoder.cc potentially has the same
issue.

BUG=541669
Committed: https://crrev.com/eed4015f99aedcc6a989ffc43869c324bc5d3c1b
Cr-Commit-Position: refs/heads/master@{#388808}

[servolk, 2016-04-15 18:38:32]

Description was changed from

==========
Check frame coded size in H264 parsers to avoid integer overflows

The bug crbug.com/541669 was originally found by fuzzing and reported
for media/formats/mp2t/es_parser_h264.cc, but I've noticed that
content/common/gpu/media/h264_decoder.cc potentially has the same
issue.

BUG=541669
==========

to

==========
Check frame coded size in H264 parsers to avoid integer overflows

The bug crbug.com/541669 was originally found by fuzzing and reported
for media/formats/mp2t/es_parser_h264.cc, but I've noticed that
content/common/gpu/media/h264_decoder.cc potentially has the same
issue.

BUG=541669
==========

[servolk, 2016-04-15 18:38:32]

servolk@chromium.org changed reviewers:
+ posciak@chromium.org, watk@chromium.org

[watk, 2016-04-15 20:01:59]

Nice, lgtm

[servolk, 2016-04-19 19:03:37]

On 2016/04/15 20:01:59, watk wrote:
> Nice, lgtm

Ping.
posciak@ could you take a look at content/common/gpu/media/h264_decoder.cc
please?

[Pawel Osciak, 2016-04-20 04:22:09]

https://codereview.chromium.org/1896533002/diff/1/content/common/gpu/media/h2...
File content/common/gpu/media/h264_decoder.cc (right):

https://codereview.chromium.org/1896533002/diff/1/content/common/gpu/media/h2...
content/common/gpu/media/h264_decoder.cc:1089: if (width_mb >
std::numeric_limits<int>::max() / 16 ||
Could we perhaps use:
base::IsValueInRangeForNumericType<int>(width_mb * 16) ?

[servolk, 2016-04-20 17:32:53]

https://codereview.chromium.org/1896533002/diff/1/content/common/gpu/media/h2...
File content/common/gpu/media/h264_decoder.cc (right):

https://codereview.chromium.org/1896533002/diff/1/content/common/gpu/media/h2...
content/common/gpu/media/h264_decoder.cc:1089: if (width_mb >
std::numeric_limits<int>::max() / 16 ||
On 2016/04/20 04:22:09, Pawel Osciak wrote:
> Could we perhaps use:
> base::IsValueInRangeForNumericType<int>(width_mb * 16) ?

Well, we are actually trying to avoid integer overflow when multiplying width_mb
by 16. So correct me if I'm missing something, but I think your suggested code
would do multiplication first (with potential integer overflow, since boths
multiplicands are ints), then would pass the multiplication result into
IsValueInRangeForNumericType, which would always be in range, since the overflow
has already happened. We could probably add an explicit widening cast on
width_mb before attempting multiplication, but that would result in uglier
(IMHO) code. I believe base::IsValueInRangeForNumericType is mostly useful in
situations where you are about to make a narrowing cast and want to ensure that
it's safe. This is not that situation.
Besides, I believe that dividing by 16 is optimized into a simple bit shift by
any decent modern compiler, so the performance shouldn't be an issue here.

[Pawel Osciak, 2016-04-21 01:03:53]

https://codereview.chromium.org/1896533002/diff/1/content/common/gpu/media/h2...
File content/common/gpu/media/h264_decoder.cc (right):

https://codereview.chromium.org/1896533002/diff/1/content/common/gpu/media/h2...
content/common/gpu/media/h264_decoder.cc:1089: if (width_mb >
std::numeric_limits<int>::max() / 16 ||
On 2016/04/20 17:32:53, servolk wrote:
> On 2016/04/20 04:22:09, Pawel Osciak wrote:
> > Could we perhaps use:
> > base::IsValueInRangeForNumericType<int>(width_mb * 16) ?
> 
> Well, we are actually trying to avoid integer overflow when multiplying
width_mb
> by 16. So correct me if I'm missing something, but I think your suggested code
> would do multiplication first (with potential integer overflow, since boths
> multiplicands are ints), then would pass the multiplication result into
> IsValueInRangeForNumericType, which would always be in range, since the
overflow
> has already happened. 

Yes, unless the result was of a larger type:
int a = INT_MAX; uint64_t b = a * 16; will not result in an overflow. I thought
that was how this template worked. In any case, saying something like:
base::IsValueInRangeForNumericType<int>(width_mb * 16u) should be enough.

[servolk, 2016-04-21 01:24:06]

https://codereview.chromium.org/1896533002/diff/1/content/common/gpu/media/h2...
File content/common/gpu/media/h264_decoder.cc (right):

https://codereview.chromium.org/1896533002/diff/1/content/common/gpu/media/h2...
content/common/gpu/media/h264_decoder.cc:1089: if (width_mb >
std::numeric_limits<int>::max() / 16 ||
On 2016/04/21 01:03:53, Pawel Osciak wrote:
> On 2016/04/20 17:32:53, servolk wrote:
> > On 2016/04/20 04:22:09, Pawel Osciak wrote:
> > > Could we perhaps use:
> > > base::IsValueInRangeForNumericType<int>(width_mb * 16) ?
> > 
> > Well, we are actually trying to avoid integer overflow when multiplying
> width_mb
> > by 16. So correct me if I'm missing something, but I think your suggested
code
> > would do multiplication first (with potential integer overflow, since boths
> > multiplicands are ints), then would pass the multiplication result into
> > IsValueInRangeForNumericType, which would always be in range, since the
> overflow
> > has already happened. 
> 
> Yes, unless the result was of a larger type:
> int a = INT_MAX; uint64_t b = a * 16; will not result in an overflow. I
thought
> that was how this template worked. In any case, saying something like:
> base::IsValueInRangeForNumericType<int>(width_mb * 16u) should be enough.

First of all, you probably meant 16ull, as 16u is still the same bitness as int,
just unsigned.
I understand that we could work around this by explicitly widening one of the
arguments (or both). But to be honest, I don't understand why it would be any
better than the current code. Why would we resort to doing 128-bit
multiplication (on 64-bit platforms), when we could achieve the same goal with
64-bit arithmetic with the current code? How is it any better than dividing by
16, which is going to be a simple bitshift?

[Pawel Osciak, 2016-04-21 01:33:11]

On 2016/04/21 01:24:06, servolk wrote:
>
https://codereview.chromium.org/1896533002/diff/1/content/common/gpu/media/h2...
> File content/common/gpu/media/h264_decoder.cc (right):
> 
>
https://codereview.chromium.org/1896533002/diff/1/content/common/gpu/media/h2...
> content/common/gpu/media/h264_decoder.cc:1089: if (width_mb >
> std::numeric_limits<int>::max() / 16 ||
> On 2016/04/21 01:03:53, Pawel Osciak wrote:
> > On 2016/04/20 17:32:53, servolk wrote:
> > > On 2016/04/20 04:22:09, Pawel Osciak wrote:
> > > > Could we perhaps use:
> > > > base::IsValueInRangeForNumericType<int>(width_mb * 16) ?
> > > 
> > > Well, we are actually trying to avoid integer overflow when multiplying
> > width_mb
> > > by 16. So correct me if I'm missing something, but I think your suggested
> code
> > > would do multiplication first (with potential integer overflow, since
boths
> > > multiplicands are ints), then would pass the multiplication result into
> > > IsValueInRangeForNumericType, which would always be in range, since the
> > overflow
> > > has already happened. 
> > 
> > Yes, unless the result was of a larger type:
> > int a = INT_MAX; uint64_t b = a * 16; will not result in an overflow. I
> thought
> > that was how this template worked. In any case, saying something like:
> > base::IsValueInRangeForNumericType<int>(width_mb * 16u) should be enough.
> 
> First of all, you probably meant 16ull, as 16u is still the same bitness as
int,
> just unsigned.

Sorry, I actually did mean 16u, because that makes it one bit larger, as there
is no sign bit :)

int a = INT_MAX;
base::IsValueInRangeForNumericType<int>(a * 16) == true
base::IsValueInRangeForNumericType<int>(a * 16u) == false

[Pawel Osciak, 2016-04-21 01:41:25]

On 2016/04/21 01:33:11, Pawel Osciak wrote:
> On 2016/04/21 01:24:06, servolk wrote:
> >
>
https://codereview.chromium.org/1896533002/diff/1/content/common/gpu/media/h2...
> > File content/common/gpu/media/h264_decoder.cc (right):
> > 
> >
>
https://codereview.chromium.org/1896533002/diff/1/content/common/gpu/media/h2...
> > content/common/gpu/media/h264_decoder.cc:1089: if (width_mb >
> > std::numeric_limits<int>::max() / 16 ||
> > On 2016/04/21 01:03:53, Pawel Osciak wrote:
> > > On 2016/04/20 17:32:53, servolk wrote:
> > > > On 2016/04/20 04:22:09, Pawel Osciak wrote:
> > > > > Could we perhaps use:
> > > > > base::IsValueInRangeForNumericType<int>(width_mb * 16) ?
> > > > 
> > > > Well, we are actually trying to avoid integer overflow when multiplying
> > > width_mb
> > > > by 16. So correct me if I'm missing something, but I think your
suggested
> > code
> > > > would do multiplication first (with potential integer overflow, since
> boths
> > > > multiplicands are ints), then would pass the multiplication result into
> > > > IsValueInRangeForNumericType, which would always be in range, since the
> > > overflow
> > > > has already happened. 
> > > 
> > > Yes, unless the result was of a larger type:
> > > int a = INT_MAX; uint64_t b = a * 16; will not result in an overflow. I
> > thought
> > > that was how this template worked. In any case, saying something like:
> > > base::IsValueInRangeForNumericType<int>(width_mb * 16u) should be enough.
> > 
> > First of all, you probably meant 16ull, as 16u is still the same bitness as
> int,
> > just unsigned.
> 
> Sorry, I actually did mean 16u, because that makes it one bit larger, as there
> is no sign bit :)
> 
> int a = INT_MAX;
> base::IsValueInRangeForNumericType<int>(a * 16) == true
> base::IsValueInRangeForNumericType<int>(a * 16u) == false

That said, I don't disagree with your arguments, I'm not particularly opposed to
the current version. Please choose whichever variant you'd prefer, lgtm for the
current version as well. Thanks for the fix!

[servolk, 2016-04-21 01:56:11]

On 2016/04/21 01:41:25, Pawel Osciak wrote:
> On 2016/04/21 01:33:11, Pawel Osciak wrote:
> > On 2016/04/21 01:24:06, servolk wrote:
> > >
> >
>
https://codereview.chromium.org/1896533002/diff/1/content/common/gpu/media/h2...
> > > File content/common/gpu/media/h264_decoder.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1896533002/diff/1/content/common/gpu/media/h2...
> > > content/common/gpu/media/h264_decoder.cc:1089: if (width_mb >
> > > std::numeric_limits<int>::max() / 16 ||
> > > On 2016/04/21 01:03:53, Pawel Osciak wrote:
> > > > On 2016/04/20 17:32:53, servolk wrote:
> > > > > On 2016/04/20 04:22:09, Pawel Osciak wrote:
> > > > > > Could we perhaps use:
> > > > > > base::IsValueInRangeForNumericType<int>(width_mb * 16) ?
> > > > > 
> > > > > Well, we are actually trying to avoid integer overflow when
multiplying
> > > > width_mb
> > > > > by 16. So correct me if I'm missing something, but I think your
> suggested
> > > code
> > > > > would do multiplication first (with potential integer overflow, since
> > boths
> > > > > multiplicands are ints), then would pass the multiplication result
into
> > > > > IsValueInRangeForNumericType, which would always be in range, since
the
> > > > overflow
> > > > > has already happened. 
> > > > 
> > > > Yes, unless the result was of a larger type:
> > > > int a = INT_MAX; uint64_t b = a * 16; will not result in an overflow. I
> > > thought
> > > > that was how this template worked. In any case, saying something like:
> > > > base::IsValueInRangeForNumericType<int>(width_mb * 16u) should be
enough.
> > > 
> > > First of all, you probably meant 16ull, as 16u is still the same bitness
as
> > int,
> > > just unsigned.
> > 
> > Sorry, I actually did mean 16u, because that makes it one bit larger, as
there
> > is no sign bit :)
> > 
> > int a = INT_MAX;
> > base::IsValueInRangeForNumericType<int>(a * 16) == true
> > base::IsValueInRangeForNumericType<int>(a * 16u) == false
> 
> That said, I don't disagree with your arguments, I'm not particularly opposed
to
> the current version. Please choose whichever variant you'd prefer, lgtm for
the
> current version as well. Thanks for the fix!

Unfortunately 16u still doesn't work. Yes, it happens to give the right result
for INT_MAX. But here is a counter example (let's consider 32-bit for
simplicity). Imagine we got width_mb = 0x77FF FFFF. Then width_mb * 16u ==
0x0007 7FFF FFF0, but in 32-bit unsigned arithmetic it becomes just 0x7FFF FFF0
due to overflow, which is less than INT_MAX == 0x7FFF FFFF! So
base::IsValueInRangeForNumericType<int>(width_mb * 16u) would return true for
width_mb==0x77FF FFFF, which is wrong here.
I believe the current code is much simpler and does the job correctly. So I'll
land it as it is. Thanks for the review!

[servolk, 2016-04-21 01:56:16]

The CQ bit was checked by servolk@chromium.org

[commit-bot: I haz the power, 2016-04-21 01:56:47]

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

[commit-bot: I haz the power, 2016-04-21 02:08:22]

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

[commit-bot: I haz the power, 2016-04-21 02:08:23]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[servolk, 2016-04-21 02:29:31]

The CQ bit was checked by servolk@chromium.org

[commit-bot: I haz the power, 2016-04-21 02:29:57]

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

[commit-bot: I haz the power, 2016-04-21 02:41:00]

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

[commit-bot: I haz the power, 2016-04-21 02:41:01]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[servolk, 2016-04-21 16:37:45]

The CQ bit was checked by servolk@chromium.org

[commit-bot: I haz the power, 2016-04-21 16:37:58]

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

[commit-bot: I haz the power, 2016-04-21 17:46:48]

Description was changed from

==========
Check frame coded size in H264 parsers to avoid integer overflows

The bug crbug.com/541669 was originally found by fuzzing and reported
for media/formats/mp2t/es_parser_h264.cc, but I've noticed that
content/common/gpu/media/h264_decoder.cc potentially has the same
issue.

BUG=541669
==========

to

==========
Check frame coded size in H264 parsers to avoid integer overflows

The bug crbug.com/541669 was originally found by fuzzing and reported
for media/formats/mp2t/es_parser_h264.cc, but I've noticed that
content/common/gpu/media/h264_decoder.cc potentially has the same
issue.

BUG=541669
==========

[commit-bot: I haz the power, 2016-04-21 17:46:49]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-04-22 19:35:56]

Description was changed from

==========
Check frame coded size in H264 parsers to avoid integer overflows

The bug crbug.com/541669 was originally found by fuzzing and reported
for media/formats/mp2t/es_parser_h264.cc, but I've noticed that
content/common/gpu/media/h264_decoder.cc potentially has the same
issue.

BUG=541669
==========

to

==========
Check frame coded size in H264 parsers to avoid integer overflows

The bug crbug.com/541669 was originally found by fuzzing and reported
for media/formats/mp2t/es_parser_h264.cc, but I've noticed that
content/common/gpu/media/h264_decoder.cc potentially has the same
issue.

BUG=541669

Committed: https://crrev.com/eed4015f99aedcc6a989ffc43869c324bc5d3c1b
Cr-Commit-Position: refs/heads/master@{#388808}
==========

[commit-bot: I haz the power, 2016-04-22 19:35:57]

Patchset 1 (id:??) landed as
https://crrev.com/eed4015f99aedcc6a989ffc43869c324bc5d3c1b
Cr-Commit-Position: refs/heads/master@{#388808}