Add extra checks to avoid integer overflow.

Add extra checks to avoid integer overflow.

BUG=425980
TEST=no crash with ASAN

Committed: https://crrev.com/b2006ac87cec58363090e7d5e10d5d9e3bbda9f9
Cr-Commit-Position: refs/heads/master@{#301249}

[jrummell, 2014-10-24 17:32:14]

jrummell@chromium.org changed reviewers:
+ dalecurtis@chromium.org, xhwang@chromium.org

[jrummell, 2014-10-24 17:32:14]

PTAL.

[DaleCurtis, 2014-10-24 17:37:01]

Ditto everywhere you have a seemingly magic value.

https://codereview.chromium.org/659743004/diff/1/media/base/container_names.cc
File media/base/container_names.cc (right):

https://codereview.chromium.org/659743004/diff/1/media/base/container_names.c...
media/base/container_names.cc:126: RCHECK(size > 0 && size < 8192);
Why not < buffer_size ?

https://codereview.chromium.org/659743004/diff/1/media/base/container_names.c...
media/base/container_names.cc:193: RCHECK(frame_size >= 7 && frame_size <=
4096);
ditto?

[jrummell, 2014-10-24 18:09:49]

Comments only.

https://codereview.chromium.org/659743004/diff/1/media/base/container_names.cc
File media/base/container_names.cc (right):

https://codereview.chromium.org/659743004/diff/1/media/base/container_names.c...
media/base/container_names.cc:126: RCHECK(size > 0 && size < 8192);
On 2014/10/24 17:37:01, DaleCurtis wrote:
> Why not < buffer_size ?

RCHECK() returns false if the condition is not met. Having the offset go off the
end of the buffer may do something different (in this case it returns true), so
I didn't want to affect that.

And they're not really magic numbers. 2**13 = 8192, and is the largest value
that the line above should return. It doesn't add much, but makes it clear that
what gets added to offset is not close to maxint.

[jrummell, 2014-10-24 20:48:35]

Updated to only do the check for MOV files.

[DaleCurtis, 2014-10-24 20:51:35]

lgtm

https://codereview.chromium.org/659743004/diff/20001/media/base/container_nam...
File media/base/container_names.cc (right):

https://codereview.chromium.org/659743004/diff/20001/media/base/container_nam...
media/base/container_names.cc:988: if (atomsize == 0 || atomsize >
static_cast<unsigned>(buffer_size))
Hmm we don't typically use unsigned, maybe size_t instead?

[xhwang, 2014-10-24 21:04:28]

lgtm % dale's comment about size_t

https://codereview.chromium.org/659743004/diff/1/media/base/container_names.cc
File media/base/container_names.cc (right):

https://codereview.chromium.org/659743004/diff/1/media/base/container_names.c...
media/base/container_names.cc:126: RCHECK(size > 0 && size < 8192);
On 2014/10/24 18:09:49, jrummell wrote:
> On 2014/10/24 17:37:01, DaleCurtis wrote:
> > Why not < buffer_size ?
> 
> RCHECK() returns false if the condition is not met. Having the offset go off
the
> end of the buffer may do something different (in this case it returns true),
so
> I didn't want to affect that.
> 
> And they're not really magic numbers. 2**13 = 8192, and is the largest value
> that the line above should return. It doesn't add much, but makes it clear
that
> what gets added to offset is not close to maxint.

Does the spec say the size can't be 8192?

[xhwang, 2014-10-24 21:04:59]

https://codereview.chromium.org/659743004/diff/1/media/base/container_names.cc
File media/base/container_names.cc (right):

https://codereview.chromium.org/659743004/diff/1/media/base/container_names.c...
media/base/container_names.cc:126: RCHECK(size > 0 && size < 8192);
On 2014/10/24 21:04:28, xhwang wrote:
> On 2014/10/24 18:09:49, jrummell wrote:
> > On 2014/10/24 17:37:01, DaleCurtis wrote:
> > > Why not < buffer_size ?
> > 
> > RCHECK() returns false if the condition is not met. Having the offset go off
> the
> > end of the buffer may do something different (in this case it returns true),
> so
> > I didn't want to affect that.
> > 
> > And they're not really magic numbers. 2**13 = 8192, and is the largest value
> > that the line above should return. It doesn't add much, but makes it clear
> that
> > what gets added to offset is not close to maxint.
> 
> Does the spec say the size can't be 8192?

This is an old comment. Please ignore.

[jrummell, 2014-10-24 23:17:27]

Patchset #3 (id:40001) has been deleted

[jrummell, 2014-10-24 23:18:29]

Thanks for the reviews.

https://codereview.chromium.org/659743004/diff/20001/media/base/container_nam...
File media/base/container_names.cc (right):

https://codereview.chromium.org/659743004/diff/20001/media/base/container_nam...
media/base/container_names.cc:988: if (atomsize == 0 || atomsize >
static_cast<unsigned>(buffer_size))
On 2014/10/24 20:51:35, DaleCurtis wrote:
> Hmm we don't typically use unsigned, maybe size_t instead?

Done.

[jrummell, 2014-10-24 23:18:34]

The CQ bit was checked by jrummell@chromium.org

[commit-bot: I haz the power, 2014-10-24 23:20:44]

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

[commit-bot: I haz the power, 2014-10-25 00:36:35]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2014-10-25 00:37:38]

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