Prevent bad casting in ImageBitmap when calling ArrayBuffer::createOrNull

Prevent bad casting in ImageBitmap when calling ArrayBuffer::createOrNull

Currently when ImageBitmap's constructor is invoked, we check whether
dstSize will overflow size_t or not. The problem comes when we call
ArrayBuffer::createOrNull some times in the code.

Both parameters of ArrayBuffer::createOrNull are unsigned. In ImageBitmap
when we call this method, the first parameter is usually width * height.
This could overflow unsigned even if it has been checked safe with size_t,
the reason is that unsigned is a 32-bit value on 64-bit systems, while
size_t is a 64-bit value.

This CL makes a change such that we check whether the dstSize will overflow
unsigned or not. In this case, we can guarantee that createOrNull will not have
any crash.

BUG=664139
Committed: https://crrev.com/d59a4441697f6253e7dc3f7ae5caad6e5fd2c778
Cr-Commit-Position: refs/heads/master@{#431936}

[xidachen, 2016-11-11 14:18:32]

xidachen@chromium.org changed reviewers:
+ junov@chromium.org

[xidachen, 2016-11-11 14:18:33]

PTAL

[Justin Novosad, 2016-11-11 14:40:29]

There should be a test in this CL.

https://codereview.chromium.org/2500493002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/2500493002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:130: numElement *= height;
It'a a bit wasteful that this product is always re-computed after the check. 
You should just use CheckedNumeric in-line IMHO

https://codereview.chromium.org/2500493002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:146: size_t width =
static_cast<size_t>(input->width());
Here width is size_t, but the check was done using unsigned. So you might be
getting false rejections from that test.

[Justin Novosad, 2016-11-11 14:42:25]

>
https://codereview.chromium.org/2500493002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/frame/ImageBitmap.cpp:146: size_t width =
> static_cast<size_t>(input->width());
> Here width is size_t, but the check was done using unsigned. So you might be
> getting false rejections from that test.

After reading the CL description, I retract this comment, what you should do is
use unsigned upfront.  Would be clearer that way

[xidachen, 2016-11-11 15:34:49]

Description was changed from

==========
Prevent bad casting in ImageBitmap when calling ArrayBuffer::createOrNull

Currently when ImageBitmap's constructor is invoked, we check whether
dstSize will overflow size_t or not. The problem comes when we call
ArrayBuffer::createOrNull some times in the code.

Both parameters of ArrayBuffer::createOrNull are unsigned. In ImageBitmap
when we call this method, the first parameter is usually width * height.
This could overflow unsigned even if it has been checked safe with size_t,
the reason is that unsigned is a 32-bit value on 64-bit systems, while
size_t is a 64-bit value.

There are two options here:
1. Check overflow against unsigned at the place where createOrNull is called.
2. Change the initial check against size_t to be against unsigned.

This CL took the first approach because I think option 2 is an overkill.
ArrayBuffer::createOrNull is called in 4 places and it doesn't fit all
cases of ImageBitmap.

BUG=664139
==========

to

==========
Prevent bad casting in ImageBitmap when calling ArrayBuffer::createOrNull

Currently when ImageBitmap's constructor is invoked, we check whether
dstSize will overflow size_t or not. The problem comes when we call
ArrayBuffer::createOrNull some times in the code.

Both parameters of ArrayBuffer::createOrNull are unsigned. In ImageBitmap
when we call this method, the first parameter is usually width * height.
This could overflow unsigned even if it has been checked safe with size_t,
the reason is that unsigned is a 32-bit value on 64-bit systems, while
size_t is a 64-bit value.

This CL makes a change such that we check whether the dstSize will overflow 
unsigned or not. In this case, we can guarantee that createOrNull will not have
any crash.

BUG=664139
==========

[xidachen, 2016-11-11 15:42:58]

I have changed the CL description, please take a look at the new patch.

The newly added test case crashes locally on tip-of-tree, but runs correctly
with this fix.

[Justin Novosad, 2016-11-14 18:14:01]

lgtm

[xidachen, 2016-11-14 18:19:17]

The CQ bit was checked by xidachen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-14 18:19:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xidachen, 2016-11-14 19:45:13]

The CQ bit was unchecked by xidachen@chromium.org

[xidachen, 2016-11-14 19:45:33]

The CQ bit was checked by xidachen@chromium.org

[commit-bot: I haz the power, 2016-11-14 19:47:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xidachen, 2016-11-14 20:14:05]

The CQ bit was unchecked by xidachen@chromium.org

[xidachen, 2016-11-14 20:14:26]

The CQ bit was checked by xidachen@chromium.org

[commit-bot: I haz the power, 2016-11-14 20:33:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-14 22:53:22]

Description was changed from

==========
Prevent bad casting in ImageBitmap when calling ArrayBuffer::createOrNull

Currently when ImageBitmap's constructor is invoked, we check whether
dstSize will overflow size_t or not. The problem comes when we call
ArrayBuffer::createOrNull some times in the code.

Both parameters of ArrayBuffer::createOrNull are unsigned. In ImageBitmap
when we call this method, the first parameter is usually width * height.
This could overflow unsigned even if it has been checked safe with size_t,
the reason is that unsigned is a 32-bit value on 64-bit systems, while
size_t is a 64-bit value.

This CL makes a change such that we check whether the dstSize will overflow 
unsigned or not. In this case, we can guarantee that createOrNull will not have
any crash.

BUG=664139
==========

to

==========
Prevent bad casting in ImageBitmap when calling ArrayBuffer::createOrNull

Currently when ImageBitmap's constructor is invoked, we check whether
dstSize will overflow size_t or not. The problem comes when we call
ArrayBuffer::createOrNull some times in the code.

Both parameters of ArrayBuffer::createOrNull are unsigned. In ImageBitmap
when we call this method, the first parameter is usually width * height.
This could overflow unsigned even if it has been checked safe with size_t,
the reason is that unsigned is a 32-bit value on 64-bit systems, while
size_t is a 64-bit value.

This CL makes a change such that we check whether the dstSize will overflow 
unsigned or not. In this case, we can guarantee that createOrNull will not have
any crash.

BUG=664139
==========

[commit-bot: I haz the power, 2016-11-14 22:53:23]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-14 22:59:11]

Description was changed from

==========
Prevent bad casting in ImageBitmap when calling ArrayBuffer::createOrNull

Currently when ImageBitmap's constructor is invoked, we check whether
dstSize will overflow size_t or not. The problem comes when we call
ArrayBuffer::createOrNull some times in the code.

Both parameters of ArrayBuffer::createOrNull are unsigned. In ImageBitmap
when we call this method, the first parameter is usually width * height.
This could overflow unsigned even if it has been checked safe with size_t,
the reason is that unsigned is a 32-bit value on 64-bit systems, while
size_t is a 64-bit value.

This CL makes a change such that we check whether the dstSize will overflow 
unsigned or not. In this case, we can guarantee that createOrNull will not have
any crash.

BUG=664139
==========

to

==========
Prevent bad casting in ImageBitmap when calling ArrayBuffer::createOrNull

Currently when ImageBitmap's constructor is invoked, we check whether
dstSize will overflow size_t or not. The problem comes when we call
ArrayBuffer::createOrNull some times in the code.

Both parameters of ArrayBuffer::createOrNull are unsigned. In ImageBitmap
when we call this method, the first parameter is usually width * height.
This could overflow unsigned even if it has been checked safe with size_t,
the reason is that unsigned is a 32-bit value on 64-bit systems, while
size_t is a 64-bit value.

This CL makes a change such that we check whether the dstSize will overflow
unsigned or not. In this case, we can guarantee that createOrNull will not have
any crash.

BUG=664139

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

[commit-bot: I haz the power, 2016-11-14 22:59:12]

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