Avoid integer overflow in LayoutUnit's unary minus operator

Avoid integer overflow in LayoutUnit's unary minus operator

BUG=629932
Committed: https://crrev.com/ef6bea224f0266018c3b4fc63262769020b9a4c2
Cr-Commit-Position: refs/heads/master@{#406734}

[Xianzhu, 2016-07-20 20:59:42]

wangxianzhu@chromium.org changed reviewers:
+ pdr@chromium.org

[Xianzhu, 2016-07-20 20:59:53]

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

[commit-bot: I haz the power, 2016-07-20 21:00:20]

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

[pdr., 2016-07-20 21:03:55]

https://codereview.chromium.org/2160983007/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/LayoutUnitTest.cpp (right):

https://codereview.chromium.org/2160983007/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/LayoutUnitTest.cpp:259: TEST(LayoutUnitTest,
UnaryMinusSaturated)
What about -(LayoutUnit::min + 1)?

[Xianzhu, 2016-07-20 21:09:10]

https://codereview.chromium.org/2160983007/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/LayoutUnitTest.cpp (right):

https://codereview.chromium.org/2160983007/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/LayoutUnitTest.cpp:259: TEST(LayoutUnitTest,
UnaryMinusSaturated)
On 2016/07/20 21:03:55, pdr. wrote:
> What about -(LayoutUnit::min + 1)?

This won't be saturated, just a normal negative.

[Xianzhu, 2016-07-20 21:18:36]

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

[commit-bot: I haz the power, 2016-07-20 21:18:56]

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

[Xianzhu, 2016-07-20 21:19:20]

https://codereview.chromium.org/2160983007/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/LayoutUnitTest.cpp (right):

https://codereview.chromium.org/2160983007/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/LayoutUnitTest.cpp:259: TEST(LayoutUnitTest,
UnaryMinusSaturated)
On 2016/07/20 21:09:10, Xianzhu wrote:
> On 2016/07/20 21:03:55, pdr. wrote:
> > What about -(LayoutUnit::min + 1)?
> 
> This won't be saturated, just a normal negative.

Converted this test case to a full UnaryMinus case.

[pdr., 2016-07-20 22:00:50]

On 2016/07/20 at 21:19:20, wangxianzhu wrote:
>
https://codereview.chromium.org/2160983007/diff/1/third_party/WebKit/Source/p...
> File third_party/WebKit/Source/platform/LayoutUnitTest.cpp (right):
> 
>
https://codereview.chromium.org/2160983007/diff/1/third_party/WebKit/Source/p...
> third_party/WebKit/Source/platform/LayoutUnitTest.cpp:259:
TEST(LayoutUnitTest, UnaryMinusSaturated)
> On 2016/07/20 21:09:10, Xianzhu wrote:
> > On 2016/07/20 21:03:55, pdr. wrote:
> > > What about -(LayoutUnit::min + 1)?
> > 
> > This won't be saturated, just a normal negative.
> 
> Converted this test case to a full UnaryMinus case.

Thanks, just wanted to check in case min and max were not symmetric.

LGTM

[commit-bot: I haz the power, 2016-07-21 00:43:25]

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

[commit-bot: I haz the power, 2016-07-21 00:43:26]

Dry run: This issue passed the CQ dry run.

[Xianzhu, 2016-07-21 00:52:13]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2016-07-21 00:52:37]

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

[Xianzhu, 2016-07-21 00:59:06]

Description was changed from

==========
Avoid integer overflow in LayoutUnit's unary minus operator

BUG=629947
==========

to

==========
Avoid integer overflow in LayoutUnit's unary minus operator

BUG=629932
==========

[commit-bot: I haz the power, 2016-07-21 00:59:24]

Description was changed from

==========
Avoid integer overflow in LayoutUnit's unary minus operator

BUG=629932
==========

to

==========
Avoid integer overflow in LayoutUnit's unary minus operator

BUG=629932
==========

[commit-bot: I haz the power, 2016-07-21 00:59:26]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-21 01:03:06]

Description was changed from

==========
Avoid integer overflow in LayoutUnit's unary minus operator

BUG=629932
==========

to

==========
Avoid integer overflow in LayoutUnit's unary minus operator

BUG=629932

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

[commit-bot: I haz the power, 2016-07-21 01:03:07]

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

[esprehn, 2016-07-21 01:57:31]

esprehn@chromium.org changed reviewers:
+ eae@chromium.org, esprehn@chromium.org

[esprehn, 2016-07-21 01:57:31]

Why does this matter for real content? It's not clear to me that we should be
taking a branch for behavior that no real page ever hits and that isn't a
security bug.

[pdr., 2016-07-21 02:06:28]

On 2016/07/21 at 01:57:31, esprehn wrote:
> Why does this matter for real content? It's not clear to me that we should be
taking a branch for behavior that no real page ever hits and that isn't a
security bug.

In reviewing this, my thinking was that security bugs frequently come from
mistakes in over/underflow so it's better to be safe with consistent behavior.
I'm not dead-set on that though, especially if it hurts performance (and
saturated arithmetic can actually cause security bugs too).

Do you think it would be okay to wait and see how this performs on the perf
bots?

[Xianzhu, 2016-07-21 06:11:50]

On 2016/07/21 02:06:28, pdr. wrote:
> On 2016/07/21 at 01:57:31, esprehn wrote:
> > Why does this matter for real content? It's not clear to me that we should
be
> taking a branch for behavior that no real page ever hits and that isn't a
> security bug.
> 
> In reviewing this, my thinking was that security bugs frequently come from
> mistakes in over/underflow so it's better to be safe with consistent behavior.
> I'm not dead-set on that though, especially if it hurts performance (and
> saturated arithmetic can actually cause security bugs too).
> 
> Do you think it would be okay to wait and see how this performs on the perf
> bots?

I agree with pdr@ about consistent behavior (saturated arithmetic for
LayoutUnits). This CL makes the negative operator behaves consistently with
other operators.

However, I wonder how saturated or overflowed arithmetic of LayoutUnits affects
security. Is there any material about this? If saturated arithmetic is just to
handle cases that no real page ever hits and there is no security concern, how
about removing all the saturated arithmetic for LayoutUnits?

[esprehn, 2016-07-21 06:17:09]

On 2016/07/21 at 06:11:50, wangxianzhu wrote:
> On 2016/07/21 02:06:28, pdr. wrote:
> > On 2016/07/21 at 01:57:31, esprehn wrote:
> > > Why does this matter for real content? It's not clear to me that we should
be
> > taking a branch for behavior that no real page ever hits and that isn't a
> > security bug.
> > 
> > In reviewing this, my thinking was that security bugs frequently come from
> > mistakes in over/underflow so it's better to be safe with consistent
behavior.
> > I'm not dead-set on that though, especially if it hurts performance (and
> > saturated arithmetic can actually cause security bugs too).
> > 
> > Do you think it would be okay to wait and see how this performs on the perf
> > bots?
> 
> I agree with pdr@ about consistent behavior (saturated arithmetic for
LayoutUnits). This CL makes the negative operator behaves consistently with
other operators.
> 
> However, I wonder how saturated or overflowed arithmetic of LayoutUnits
affects security. Is there any material about this? If saturated arithmetic is
just to handle cases that no real page ever hits and there is no security
concern, how about removing all the saturated arithmetic for LayoutUnits?

Layout does hit it as the result of some algorithms which use very large values.
Thinking about this, why can't we use saturatedSubtraction in unary minus like
plus uses saturatedAddition? That has special assembly for ARM.