Fix overflow/underflow in gfx geometry once and for all

Fix overflow/underflow in ui/gfx once and for all

ui/gfx/geometry currently has a number of spots where adding or
subtracting two integers can cause overflow or underflow.  Fix this so
that Point/Vector2d/Size/Rect all correctly clamp integer arithmetic to
not fall into undefined overflow or underflow.

The behavior is now that any operation that would have overflowed or
underflowed is now clamped to the max or min integer.  Additionally, for
rects, if their size is large enough to overflow right/bottom, this will
also be clamped.

gfx::Insets is left unchanged as it is only used by ui/views and so is
much more likely to be under user control and not overflow.

This patch also fixes an overflow error in the unit test that happened
in Rect::Inset.

BUG=648214
Committed: https://crrev.com/8c14bf7d37bec879a5caa162f2c72a303da1a6e0
Cr-Commit-Position: refs/heads/master@{#423243}

[enne (OOO), 2016-09-21 00:48:24]

https://codereview.chromium.org/2354783004/diff/1/ui/gfx/geometry/rect.cc
File ui/gfx/geometry/rect.cc (right):

https://codereview.chromium.org/2354783004/diff/1/ui/gfx/geometry/rect.cc#new...
ui/gfx/geometry/rect.cc:67: origin_ += Vector2d(left, top);
Adding and subtracting to vectors and points and such all can overflow as well,
so I probably need to go fix and test those before clusterfuzz figures it out.

https://codereview.chromium.org/2354783004/diff/1/ui/gfx/geometry/rect.h
File ui/gfx/geometry/rect.h (right):

https://codereview.chromium.org/2354783004/diff/1/ui/gfx/geometry/rect.h#newc...
ui/gfx/geometry/rect.h:210: return a > 0 && b > std::numeric_limits<int>::max()
- a;
danakj: I feel like these are things I'd want to use in rect/vector/point/etc. 
Where would you want these to live?  Maybe these should be called ClampedAdd /
ClampedSubtract?

I haven't performance tested these different functions, but I think having fewer
casts is better.

[enne (OOO), 2016-09-21 20:20:11]

Description was changed from

==========
[wip] stop overflows ONCE AND FOR ALL
==========

to

==========
Fix overflow/underflow in ui/gfx once and for all

ui/gfx/geometry currently has a number of spots where adding or
subtracting two integers can cause overflow or underflow.  Fix this so
that Point/Vector2d/Size/Rect all correctly clamp integer arithmetic to
not fall into undefined overflow or underflow.

The behavior is now that any operation that would have overflowed or
underflowed is now clamped to the max or min integer.  Additionally, for
rects, if their size is large enough to overflow right/bottom, this will
also be clamped.

gfx::Insets is left unchanged as it is only used by ui/views and so is
much more likely to be under user control and not overflow.

This patch also fixes an overflow error in the unit test that happened
in Rect::Inset.

BUG=648214
==========

[enne (OOO), 2016-09-21 20:20:12]

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

[enne (OOO), 2016-09-21 20:20:30]

PTAL

[enne (OOO), 2016-09-21 20:44:40]

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

[commit-bot: I haz the power, 2016-09-21 20:45:02]

Dry run: 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-09-21 20:50:18]

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

[commit-bot: I haz the power, 2016-09-21 20:50:19]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[danakj, 2016-09-21 22:01:36]

LGTM

https://codereview.chromium.org/2354783004/diff/40001/ui/gfx/geometry/rect.cc
File ui/gfx/geometry/rect.cc (right):

https://codereview.chromium.org/2354783004/diff/40001/ui/gfx/geometry/rect.cc...
ui/gfx/geometry/rect.cc:70: set_width(SafeSubtract(width(), SafeAdd(left,
right)));
I think itd be nice to test to show why you went this order of operations/safety

https://codereview.chromium.org/2354783004/diff/40001/ui/gfx/geometry/vector2...
File ui/gfx/geometry/vector2d_unittest.cc (right):

https://codereview.chromium.org/2354783004/diff/40001/ui/gfx/geometry/vector2...
ui/gfx/geometry/vector2d_unittest.cc:292: }  // namespace gfx
nit: whitespace below

https://codereview.chromium.org/2354783004/diff/60001/ui/gfx/geometry/rect.h
File ui/gfx/geometry/rect.h (left):

https://codereview.chromium.org/2354783004/diff/60001/ui/gfx/geometry/rect.h#...
ui/gfx/geometry/rect.h:216: // 3) We cast the values to unsigned int because the
compiler can optimize
I suspect you'll have to preserve this behaviour at which point please preserve
the comment. Bots will show.

[danakj, 2016-09-21 22:02:08]

https://codereview.chromium.org/2354783004/diff/60001/ui/gfx/geometry/rect.h
File ui/gfx/geometry/rect.h (left):

https://codereview.chromium.org/2354783004/diff/60001/ui/gfx/geometry/rect.h#...
ui/gfx/geometry/rect.h:216: // 3) We cast the values to unsigned int because the
compiler can optimize
On 2016/09/21 22:01:36, danakj wrote:
> I suspect you'll have to preserve this behaviour at which point please
preserve
> the comment. Bots will show.

It might not fail until landing tho, maybe double check the patch that added
this comment to see where the error came from.

[enne (OOO), 2016-09-21 22:10:27]

> It might not fail until landing tho, maybe double check the patch that added
this comment to see where the error came from.

It failed on tryjobs, so will just lean on the cq.  See:
https://codereview.chromium.org/2268423003

https://codereview.chromium.org/2354783004/diff/40001/ui/gfx/geometry/rect.cc
File ui/gfx/geometry/rect.cc (right):

https://codereview.chromium.org/2354783004/diff/40001/ui/gfx/geometry/rect.cc...
ui/gfx/geometry/rect.cc:70: set_width(SafeSubtract(width(), SafeAdd(left,
right)));
On 2016/09/21 at 22:01:36, danakj wrote:
> I think itd be nice to test to show why you went this order of
operations/safety

Done.

[enne (OOO), 2016-09-21 22:10:35]

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

[commit-bot: I haz the power, 2016-09-21 22:11:09]

Dry run: 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-09-21 22:30:04]

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

[commit-bot: I haz the power, 2016-09-21 22:30:05]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[enne (OOO), 2016-09-21 23:08:30]

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

[commit-bot: I haz the power, 2016-09-21 23:09:09]

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

[enne (OOO), 2016-09-22 00:15:55]

The CQ bit was unchecked by enne@chromium.org

[enne (OOO), 2016-09-22 00:16:01]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2016-09-22 00:16:01]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2354783004/#ps120001
(title: "Fix for GCC")

[commit-bot: I haz the power, 2016-09-22 00:16:29]

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-09-22 01:22:12]

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

[commit-bot: I haz the power, 2016-09-22 01:22:14]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[enne (OOO), 2016-09-23 00:11:30]

enne@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[enne (OOO), 2016-09-23 00:17:06]

+dmazzoni for some changes to chromeos accessibility unit tests that were
depending on integer underflow

https://codereview.chromium.org/2354783004/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/accessibility/accessibility_highlight_manager.cc
(right):

https://codereview.chromium.org/2354783004/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/accessibility_highlight_manager.cc:21:
(INT_MAX - 1000, INT_MAX - 1000, 0, 0));
Surprisingly, AccessibilityHighlightManagerTest.TestFocusRingDrawsOrangePixels
depends on the fact that the offscreen rect is a large *positive* number when
inset by 40 pixels.  This is an integer underflow and undefined behavior.

If I make it INT_MIN + 1000, the test also fails but with a different error
(wrong pixels).  Therefore, it appears that this test relies on very specific
offscreen point values.  So, I've made it use large positive numbers with enough
of an offset that it won't overflow when inset by margins.

I'm kind of surprised that there's not some more straight forward way to convey
the information that the focus ring or caret should be invisible instead of just
placing it very far away.

[enne (OOO), 2016-09-23 00:17:40]

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

[commit-bot: I haz the power, 2016-09-23 00:18:12]

Dry run: 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-09-23 01:49:24]

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

[commit-bot: I haz the power, 2016-09-23 01:49:25]

Dry run: This issue passed the CQ dry run.

[enne (OOO), 2016-09-30 19:00:08]

enne@chromium.org changed reviewers:
+ dtseng@chromium.org

[enne (OOO), 2016-09-30 19:00:08]

+dtseng as alternate dmazzoni for chrome/browser/chromeos/accessibility

[enne (OOO), 2016-10-03 17:16:17]

enne@chromium.org changed reviewers:
+ plundblad@chromium.org

[enne (OOO), 2016-10-03 17:16:50]

+plundblad as an alternate dtseng/dmazzoni for
chrome/browser/chromeos/accessibility

Anybody? ;;

[dmazzoni, 2016-10-04 22:29:32]

https://codereview.chromium.org/2354783004/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/accessibility/accessibility_highlight_manager.cc
(right):

https://codereview.chromium.org/2354783004/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/accessibility_highlight_manager.cc:21:
(INT_MAX - 1000, INT_MAX - 1000, 0, 0));
On 2016/09/23 00:17:06, enne wrote:
> Surprisingly, AccessibilityHighlightManagerTest.TestFocusRingDrawsOrangePixels
> depends on the fact that the offscreen rect is a large *positive* number when
> inset by 40 pixels.  This is an integer underflow and undefined behavior.
> 
> If I make it INT_MIN + 1000, the test also fails but with a different error
> (wrong pixels).  Therefore, it appears that this test relies on very specific
> offscreen point values.  So, I've made it use large positive numbers with
enough
> of an offset that it won't overflow when inset by margins.
> 
> I'm kind of surprised that there's not some more straight forward way to
convey
> the information that the focus ring or caret should be invisible instead of
just
> placing it very far away.

Working on a cleaner fix for this here:

https://codereview.chromium.org/2388093004/

[enne (OOO), 2016-10-05 17:06:42]

Looks like that fix landed, so I'll land this without those test changes. 
Thanks again!  I didn't even know where to start with that code.

[enne (OOO), 2016-10-05 17:09:28]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2016-10-05 17:09:28]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2354783004/#ps160001
(title: "Remove accessibility test changes")

[commit-bot: I haz the power, 2016-10-05 17:09:50]

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

[dmazzoni, 2016-10-05 18:00:37]

On 2016/10/05 17:06:42, enne wrote:
> Looks like that fix landed, so I'll land this without those test changes. 
> Thanks again!  I didn't even know where to start with that code.

You're very welcome. Thanks for catching the undefined behavior,
it was worth making it cleaner.

[commit-bot: I haz the power, 2016-10-05 19:10:55]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-10-05 19:13:29]

Description was changed from

==========
Fix overflow/underflow in ui/gfx once and for all

ui/gfx/geometry currently has a number of spots where adding or
subtracting two integers can cause overflow or underflow.  Fix this so
that Point/Vector2d/Size/Rect all correctly clamp integer arithmetic to
not fall into undefined overflow or underflow.

The behavior is now that any operation that would have overflowed or
underflowed is now clamped to the max or min integer.  Additionally, for
rects, if their size is large enough to overflow right/bottom, this will
also be clamped.

gfx::Insets is left unchanged as it is only used by ui/views and so is
much more likely to be under user control and not overflow.

This patch also fixes an overflow error in the unit test that happened
in Rect::Inset.

BUG=648214
==========

to

==========
Fix overflow/underflow in ui/gfx once and for all

ui/gfx/geometry currently has a number of spots where adding or
subtracting two integers can cause overflow or underflow.  Fix this so
that Point/Vector2d/Size/Rect all correctly clamp integer arithmetic to
not fall into undefined overflow or underflow.

The behavior is now that any operation that would have overflowed or
underflowed is now clamped to the max or min integer.  Additionally, for
rects, if their size is large enough to overflow right/bottom, this will
also be clamped.

gfx::Insets is left unchanged as it is only used by ui/views and so is
much more likely to be under user control and not overflow.

This patch also fixes an overflow error in the unit test that happened
in Rect::Inset.

BUG=648214

Committed: https://crrev.com/8c14bf7d37bec879a5caa162f2c72a303da1a6e0
Cr-Commit-Position: refs/heads/master@{#423243}
==========

[commit-bot: I haz the power, 2016-10-05 19:13:30]

Patchset 9 (id:??) landed as
https://crrev.com/8c14bf7d37bec879a5caa162f2c72a303da1a6e0
Cr-Commit-Position: refs/heads/master@{#423243}