cc: Prevent integer overflow with software TextureMailbox.

cc: Prevent integer overflow with software TextureMailbox.

Perform a CHECK() when creating a software TextureMailbox since these
come from outside of cc's control.

Do unsigned integer multiplication to produce an unsigned result,
preventing overflow when possible.

R=piman@chromium.org
BUG=348332
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260969

[piman, 2014-03-31 22:33:54]

lgtm

https://codereview.chromium.org/220093002/diff/1/cc/resources/texture_mailbox.cc
File cc/resources/texture_mailbox.cc (right):

https://codereview.chromium.org/220093002/diff/1/cc/resources/texture_mailbox...
cc/resources/texture_mailbox.cc:33:
CHECK(CheckedSharedMemorySizeInBytes().IsValid());
Can cc generate one for mask layers (not tiled)?

[danakj, 2014-03-31 22:34:49]

https://codereview.chromium.org/220093002/diff/1/cc/resources/texture_mailbox.cc
File cc/resources/texture_mailbox.cc (right):

https://codereview.chromium.org/220093002/diff/1/cc/resources/texture_mailbox...
cc/resources/texture_mailbox.cc:33:
CHECK(CheckedSharedMemorySizeInBytes().IsValid());
On 2014/03/31 22:33:54, piman wrote:
> Can cc generate one for mask layers (not tiled)?

We don't make a TextureMailbox in that case. It's a TiledLayer/PictureLayer with
a single tile. Perhaps overflow in other places but not in this code path. I'm
not forgetting something tho am I?

[danakj, 2014-03-31 22:34:57]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-03-31 22:35:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/220093002/1

[piman, 2014-03-31 22:40:45]

On Mon, Mar 31, 2014 at 3:34 PM, <danakj@chromium.org> wrote:

>
> https://codereview.chromium.org/220093002/diff/1/cc/
> resources/texture_mailbox.cc
> File cc/resources/texture_mailbox.cc (right):
>
> https://codereview.chromium.org/220093002/diff/1/cc/
> resources/texture_mailbox.cc#newcode33
> cc/resources/texture_mailbox.cc:33:
> CHECK(CheckedSharedMemorySizeInBytes().IsValid());
> On 2014/03/31 22:33:54, piman wrote:
>
>> Can cc generate one for mask layers (not tiled)?
>>
>
> We don't make a TextureMailbox in that case. It's a
> TiledLayer/PictureLayer with a single tile. Perhaps overflow in other
> places but not in this code path. I'm not forgetting something tho am I?
>

Oh, right. We make a SharedBitmap not a TextureMailbox. Separately we may
want to check those code paths.


>
> https://codereview.chromium.org/220093002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2014-03-31 22:41:37]

On 2014/03/31 22:40:45, piman wrote:
> On Mon, Mar 31, 2014 at 3:34 PM, <mailto:danakj@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.org/220093002/diff/1/cc/
> > resources/texture_mailbox.cc
> > File cc/resources/texture_mailbox.cc (right):
> >
> > https://codereview.chromium.org/220093002/diff/1/cc/
> > resources/texture_mailbox.cc#newcode33
> > cc/resources/texture_mailbox.cc:33:
> > CHECK(CheckedSharedMemorySizeInBytes().IsValid());
> > On 2014/03/31 22:33:54, piman wrote:
> >
> >> Can cc generate one for mask layers (not tiled)?
> >>
> >
> > We don't make a TextureMailbox in that case. It's a
> > TiledLayer/PictureLayer with a single tile. Perhaps overflow in other
> > places but not in this code path. I'm not forgetting something tho am I?
> >
> 
> Oh, right. We make a SharedBitmap not a TextureMailbox. Separately we may
> want to check those code paths.

Right, true.

[jbauman, 2014-03-31 22:44:50]

On 2014/03/31 22:41:37, danakj wrote:
> On 2014/03/31 22:40:45, piman wrote:
> > On Mon, Mar 31, 2014 at 3:34 PM, <mailto:danakj@chromium.org> wrote:
> > 
> > >
> > > https://codereview.chromium.org/220093002/diff/1/cc/
> > > resources/texture_mailbox.cc
> > > File cc/resources/texture_mailbox.cc (right):
> > >
> > > https://codereview.chromium.org/220093002/diff/1/cc/
> > > resources/texture_mailbox.cc#newcode33
> > > cc/resources/texture_mailbox.cc:33:
> > > CHECK(CheckedSharedMemorySizeInBytes().IsValid());
> > > On 2014/03/31 22:33:54, piman wrote:
> > >
> > >> Can cc generate one for mask layers (not tiled)?
> > >>
> > >
> > > We don't make a TextureMailbox in that case. It's a
> > > TiledLayer/PictureLayer with a single tile. Perhaps overflow in other
> > > places but not in this code path. I'm not forgetting something tho am I?
> > >
> > 
> > Oh, right. We make a SharedBitmap not a TextureMailbox. Separately we may
> > want to check those code paths.
> 
> Right, true.

HostSharedBitmapManager::GetSharedBitmapFromId already checks for overflow using
cc::SharedBitmap::GetSizeInBytes, so that's not an issue.

[danakj, 2014-03-31 22:46:30]

On Mon, Mar 31, 2014 at 6:44 PM, <jbauman@chromium.org> wrote:

> On 2014/03/31 22:41:37, danakj wrote:
>
>> On 2014/03/31 22:40:45, piman wrote:
>> > On Mon, Mar 31, 2014 at 3:34 PM, <mailto:danakj@chromium.org> wrote:
>> >
>> > >
>> > > https://codereview.chromium.org/220093002/diff/1/cc/
>> > > resources/texture_mailbox.cc
>> > > File cc/resources/texture_mailbox.cc (right):
>> > >
>> > > https://codereview.chromium.org/220093002/diff/1/cc/
>> > > resources/texture_mailbox.cc#newcode33
>> > > cc/resources/texture_mailbox.cc:33:
>> > > CHECK(CheckedSharedMemorySizeInBytes().IsValid());
>> > > On 2014/03/31 22:33:54, piman wrote:
>> > >
>> > >> Can cc generate one for mask layers (not tiled)?
>> > >>
>> > >
>> > > We don't make a TextureMailbox in that case. It's a
>> > > TiledLayer/PictureLayer with a single tile. Perhaps overflow in other
>> > > places but not in this code path. I'm not forgetting something tho am
>> I?
>> > >
>> >
>> > Oh, right. We make a SharedBitmap not a TextureMailbox. Separately we
>> may
>> > want to check those code paths.
>>
>
>  Right, true.
>>
>
> HostSharedBitmapManager::GetSharedBitmapFromId already checks for
> overflow using
> cc::SharedBitmap::GetSizeInBytes, so that's not an issue.
>

Oh, though that method is multiplying ints instead of size_t's so it'll
overflow when it doesn't need to.


>
> https://codereview.chromium.org/220093002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[piman, 2014-03-31 22:47:05]

On Mon, Mar 31, 2014 at 3:44 PM, <jbauman@chromium.org> wrote:

> On 2014/03/31 22:41:37, danakj wrote:
>
>> On 2014/03/31 22:40:45, piman wrote:
>> > On Mon, Mar 31, 2014 at 3:34 PM, <mailto:danakj@chromium.org> wrote:
>> >
>> > >
>> > > https://codereview.chromium.org/220093002/diff/1/cc/
>> > > resources/texture_mailbox.cc
>> > > File cc/resources/texture_mailbox.cc (right):
>> > >
>> > > https://codereview.chromium.org/220093002/diff/1/cc/
>> > > resources/texture_mailbox.cc#newcode33
>> > > cc/resources/texture_mailbox.cc:33:
>> > > CHECK(CheckedSharedMemorySizeInBytes().IsValid());
>> > > On 2014/03/31 22:33:54, piman wrote:
>> > >
>> > >> Can cc generate one for mask layers (not tiled)?
>> > >>
>> > >
>> > > We don't make a TextureMailbox in that case. It's a
>> > > TiledLayer/PictureLayer with a single tile. Perhaps overflow in other
>> > > places but not in this code path. I'm not forgetting something tho am
>> I?
>> > >
>> >
>> > Oh, right. We make a SharedBitmap not a TextureMailbox. Separately we
>> may
>> > want to check those code paths.
>>
>
>  Right, true.
>>
>
> HostSharedBitmapManager::GetSharedBitmapFromId already checks for
> overflow using
> cc::SharedBitmap::GetSizeInBytes, so that's not an issue.
>

Looking at ResourceProvider::CreateBitmap, we'd
fail shared_bitmap_manager_->AllocateSharedBitmap(size), and fall back to
'pixels = new uint8_t[4 * size.GetArea()];' which would be bad I think.


> https://codereview.chromium.org/220093002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2014-03-31 22:55:17]

On Mon, Mar 31, 2014 at 6:46 PM, Antoine Labour <piman@chromium.org> wrote:

>
>
>
> On Mon, Mar 31, 2014 at 3:44 PM, <jbauman@chromium.org> wrote:
>
>> On 2014/03/31 22:41:37, danakj wrote:
>>
>>> On 2014/03/31 22:40:45, piman wrote:
>>> > On Mon, Mar 31, 2014 at 3:34 PM, <mailto:danakj@chromium.org> wrote:
>>> >
>>> > >
>>> > > https://codereview.chromium.org/220093002/diff/1/cc/
>>> > > resources/texture_mailbox.cc
>>> > > File cc/resources/texture_mailbox.cc (right):
>>> > >
>>> > > https://codereview.chromium.org/220093002/diff/1/cc/
>>> > > resources/texture_mailbox.cc#newcode33
>>> > > cc/resources/texture_mailbox.cc:33:
>>> > > CHECK(CheckedSharedMemorySizeInBytes().IsValid());
>>> > > On 2014/03/31 22:33:54, piman wrote:
>>> > >
>>> > >> Can cc generate one for mask layers (not tiled)?
>>> > >>
>>> > >
>>> > > We don't make a TextureMailbox in that case. It's a
>>> > > TiledLayer/PictureLayer with a single tile. Perhaps overflow in other
>>> > > places but not in this code path. I'm not forgetting something tho
>>> am I?
>>> > >
>>> >
>>> > Oh, right. We make a SharedBitmap not a TextureMailbox. Separately we
>>> may
>>> > want to check those code paths.
>>>
>>
>>  Right, true.
>>>
>>
>> HostSharedBitmapManager::GetSharedBitmapFromId already checks for
>> overflow using
>> cc::SharedBitmap::GetSizeInBytes, so that's not an issue.
>>
>
> Looking at ResourceProvider::CreateBitmap, we'd
> fail shared_bitmap_manager_->AllocateSharedBitmap(size), and fall back to
> 'pixels = new uint8_t[4 * size.GetArea()];' which would be bad I think.
>

We should not fallback if the Allocate fails.. there's no point in having
an unsharable resource when we need one. But we should also not make shared
memory allocation in the browser compositor.


>
>
>> https://codereview.chromium.org/220093002/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2014-04-01 00:13:39]

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

[commit-bot: I haz the power, 2014-04-01 00:13:40]

Retried try job too often on win_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[danakj, 2014-04-01 14:59:36]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-04-01 14:59:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/220093002/1

[commit-bot: I haz the power, 2014-04-01 16:04:12]

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

[commit-bot: I haz the power, 2014-04-01 16:04:12]

Retried try job too often on win_rel for step(s) net_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[danakj, 2014-04-01 16:48:23]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-04-01 16:48:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/220093002/1

[commit-bot: I haz the power, 2014-04-01 22:33:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/220093002/1

[commit-bot: I haz the power, 2014-04-01 22:59:21]

Change committed as 260969