Use CheckedNumeric when converting SkIRect to gfx::Rect.

Use CheckedNumeric when converting SkIRect to gfx::Rect.

BUG=634134
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

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

[jbroman, 2016-08-10 21:41:34]

Description was changed from

==========
Use CheckedNumeric when converting rect types in FilterOperation::MapRect.

BUG=634134
==========

to

==========
Use CheckedNumeric when converting rect types in FilterOperation::MapRect.

BUG=634134
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[jbroman, 2016-08-10 21:41:37]

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

[commit-bot: I haz the power, 2016-08-10 21:42:24]

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

[jbroman, 2016-08-10 22:14:12]

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

[commit-bot: I haz the power, 2016-08-10 22:14:27]

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

[jbroman, 2016-08-10 22:16:59]

jbroman@chromium.org changed reviewers:
+ danakj@chromium.org

[jbroman, 2016-08-10 22:17:00]

I don't really like this fix, but I also don't really have any better ideas
about what to do here, so PTAL.

Note that while the unit test does fail before this patch, the compiler is free
to use undefined behaviour to make it pass before this patch if it pleases.

I don't know that we have much hope of actually stamping out this sort of
integer-overflow issue at the gfx/skia boundary, and I suspect there are many
others.

[commit-bot: I haz the power, 2016-08-11 01:40:27]

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

[commit-bot: I haz the power, 2016-08-11 01:40:28]

Dry run: This issue passed the CQ dry run.

[danakj, 2016-08-11 17:51:20]

I think the bug is that we created a FilterOperation with such a large rect. We
should clamp things wayy earlier like when we read them from css or something
IMHO.

Can we CHECK that the bounds width/height right/left are all representable on
whatever original rect went into this FilterOperation?

[jbroman, 2016-08-11 18:04:02]

On 2016/08/11 at 17:51:20, danakj wrote:
> I think the bug is that we created a FilterOperation with such a large rect.
We should clamp things wayy earlier like when we read them from css or something
IMHO.

The filter operation doesn't have a rect; it just has a large offset. A
reference filter could have moved pixels outside the range of gfx::Rect in any
number of ways (a reflection was the one clusterfuzz found).

> Can we CHECK that the bounds width/height right/left are all representable on
whatever original rect went into this FilterOperation?

Again, not clear to me what you mean by the rect of a filter operation.

[danakj, 2016-08-11 18:13:38]

https://codereview.chromium.org/2231243002/diff/20001/ui/gfx/skia_util.cc
File ui/gfx/skia_util.cc (right):

https://codereview.chromium.org/2231243002/diff/20001/ui/gfx/skia_util.cc#new...
ui/gfx/skia_util.cc:63: Rect SkIRectToRectChecked(const SkIRect& rect) {
OK I think I'd rather not have Checked version that we call sometimes and slowly
add that to more callsites as the fuzzer finds ways to break them.

I'm troubled that we have an SkIRect though, for whom calling width() would also
cause an overflow. Fixing that when we convert it to a gfx::Rect seems like a
systemic problem we're ignoring. Like, we should be exploding (or clamping?)
when we make that SkIRect rather than the gfx::Rect.

[danakj, 2016-08-11 20:36:15]

I'm super unhappy at whackamole with security problems, we need to solve this at
some high level so we don't have rects that we're not actually allowed to use.

Proposal: gfx::Rect just always clamps the width so that width() and right()
always are valid. Same for height/bottom.

[enne (OOO), 2016-08-12 22:41:50]

On 2016/08/11 at 20:36:15, danakj wrote:
> Proposal: gfx::Rect just always clamps the width so that width() and right()
always are valid. Same for height/bottom.

+1

[jbroman, 2016-08-22 14:50:35]

I'm a little confused. This particular bug involves an overflow in
SkIRectToRect. Are you saying that Skia should never have rects for which
height/width produce undefined behaviour, or that gfx should just always clamp
in SkIRectToRect?

[danakj, 2016-08-22 17:47:21]

On 2016/08/22 14:50:35, jbroman wrote:
> I'm a little confused. This particular bug involves an overflow in
> SkIRectToRect. Are you saying that Skia should never have rects for which
> height/width produce undefined behaviour, or that gfx should just always clamp
> in SkIRectToRect?

I'm saying we should turn them into gfx::Rects before we use them (which you're
doing with SkIRectToRect) and the gfx::Rect class should clamp things so
right/bottom/width/height are valid.

[jbroman, 2016-08-22 19:23:33]

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

[commit-bot: I haz the power, 2016-08-22 19:24:26]

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

[jbroman, 2016-08-22 19:38:13]

Description was changed from

==========
Use CheckedNumeric when converting rect types in FilterOperation::MapRect.

BUG=634134
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Use CheckedNumeric when converting SkIRect to gfx::Rect.

BUG=634134
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[jbroman, 2016-08-22 19:38:49]

Made SkIRectToRect do that sort of clamping.

[danakj, 2016-08-22 19:48:04]

On 2016/08/22 19:38:49, jbroman wrote:
> Made SkIRectToRect do that sort of clamping.

Can it be in gfx::Rect instead? There's other places we make gfx::Rects with bad
inputs like from IntRects or just from applying a transform.

[jbroman, 2016-08-22 19:52:18]

On 2016/08/22 at 19:48:04, danakj wrote:
> On 2016/08/22 19:38:49, jbroman wrote:
> > Made SkIRectToRect do that sort of clamping.
> 
> Can it be in gfx::Rect instead? There's other places we make gfx::Rects with
bad inputs like from IntRects or just from applying a transform.

It can't, because the overflow happens inside SkIRect::height, which is before
the gfx::Rect constructor is called. I suppose gfx::Rect could provide a static
constructor-like method that took LTRB, but it would be essentially this method
but with SkIRect's members as separate arguments.

IntRect stores width/height internally, so if it has an overflow it would have
occurred previously.

[danakj, 2016-08-22 20:01:49]

https://codereview.chromium.org/2231243002/diff/40001/ui/gfx/skia_util.cc
File ui/gfx/skia_util.cc (right):

https://codereview.chromium.org/2231243002/diff/40001/ui/gfx/skia_util.cc#new...
ui/gfx/skia_util.cc:52: (base::CheckedNumeric<int>(rect.right()) - rect.left())
I see, ok so checking right-left here makes sense then.

https://codereview.chromium.org/2231243002/diff/40001/ui/gfx/skia_util.cc#new...
ui/gfx/skia_util.cc:53: .ValueOrDefault(std::numeric_limits<int>::max()),
But if x >= 0, the right() will still overflow with a width of max() so we need
some clamping in gfx::Rect too?

[commit-bot: I haz the power, 2016-08-22 21:16:50]

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

[commit-bot: I haz the power, 2016-08-22 21:16:51]

Dry run: This issue passed the CQ dry run.

[jbroman, 2016-08-23 03:25:01]

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

[commit-bot: I haz the power, 2016-08-23 03:25:28]

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

[jbroman, 2016-08-23 03:26:58]

https://codereview.chromium.org/2231243002/diff/40001/ui/gfx/skia_util.cc
File ui/gfx/skia_util.cc (right):

https://codereview.chromium.org/2231243002/diff/40001/ui/gfx/skia_util.cc#new...
ui/gfx/skia_util.cc:53: .ValueOrDefault(std::numeric_limits<int>::max()),
On 2016/08/22 at 20:01:49, danakj wrote:
> But if x >= 0, the right() will still overflow with a width of max() so we
need some clamping in gfx::Rect too?

That cannot happen here, since SkIRect::fRight must have been in the range of
int32_t, and we do not support any platforms where sizeof(int) <
sizeof(int32_t).

The only exception I can think of is the case of a rectangle with negative
width/height in Skia, which could underflow. I'll handle that case and add unit
tests to clarify.

[commit-bot: I haz the power, 2016-08-23 05:43:25]

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

[commit-bot: I haz the power, 2016-08-23 05:43:26]

Dry run: This issue passed the CQ dry run.

[danakj, 2016-08-23 17:11:09]

LGTM

[jbroman, 2016-08-23 17:16:34]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2016-08-23 17:17:14]

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-08-23 17:22:20]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-23 17:24:12]

Description was changed from

==========
Use CheckedNumeric when converting SkIRect to gfx::Rect.

BUG=634134
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Use CheckedNumeric when converting SkIRect to gfx::Rect.

BUG=634134
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

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

[commit-bot: I haz the power, 2016-08-23 17:24:14]

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