Reland: cc: Fix size_t to int truncations in layers/ output/ playback/ quads/

Reland: cc: Fix size_t to int truncations in layers/ output/ playback/ quads/

This patch fixes size_t to int truncations in layers/, output/,
playback/, and quads/ directories in cc/.

R=danakj
BUG=167187
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel;tryserver.chromium.win:win_chromium_gn_x64_dbg

Committed: https://crrev.com/0d5963315aa03e6ebb20351f13d3d517ca14d816
Cr-Commit-Position: refs/heads/master@{#333153}

Committed: https://crrev.com/91e2309abe88d9a333deaf4714d0c48f875ba472
Cr-Commit-Position: refs/heads/master@{#333362}

[vmpstr, 2015-06-02 00:01:13]

Please take a look.

[danakj, 2015-06-02 00:06:27]

LGTM

https://codereview.chromium.org/1158433010/diff/1/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/1158433010/diff/1/cc/layers/layer.cc#newcode347
cc/layers/layer.cc:347: NOTREACHED();
Oh, yuck. Can you just DCHECK(reference_it != end) and not return?

[vmpstr, 2015-06-02 00:21:11]

https://codereview.chromium.org/1158433010/diff/1/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/1158433010/diff/1/cc/layers/layer.cc#newcode347
cc/layers/layer.cc:347: NOTREACHED();
On 2015/06/02 00:06:27, danakj wrote:
> Oh, yuck. Can you just DCHECK(reference_it != end) and not return?

Done.

[vmpstr, 2015-06-02 00:46:57]

The CQ bit was checked by vmpstr@chromium.org

[vmpstr, 2015-06-02 00:46:57]

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/1158433010/#ps60001
(title: "compile_fix")

[commit-bot: I haz the power, 2015-06-02 00:55:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1158433010/60001

[commit-bot: I haz the power, 2015-06-02 01:42:04]

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

[commit-bot: I haz the power, 2015-06-02 01:42:05]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[vmpstr, 2015-06-02 18:03:11]

The CQ bit was checked by vmpstr@chromium.org

[vmpstr, 2015-06-02 18:03:12]

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/1158433010/#ps80001
(title: "compile fix")

[commit-bot: I haz the power, 2015-06-02 18:04:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1158433010/80001

[commit-bot: I haz the power, 2015-06-02 19:10:48]

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

[commit-bot: I haz the power, 2015-06-02 19:10:50]

Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[vmpstr, 2015-06-02 20:20:57]

There were a few compiler errors that I had to fix, so this patch looks a bit
different than the original. Please take another look.

[danakj, 2015-06-02 20:40:50]

LGTM

[vmpstr, 2015-06-02 20:41:17]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2015-06-02 20:43:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1158433010/100001

[vmpstr, 2015-06-02 20:56:05]

vmpstr@chromium.org changed reviewers:
+ mkwst@chromium.org

[vmpstr, 2015-06-02 20:56:05]

+mkwst for cc_messages. Please take a look.

[vmpstr, 2015-06-03 23:39:16]

vmpstr@chromium.org changed reviewers:
+ jschuh@chromium.org

[vmpstr, 2015-06-03 23:39:17]

+jschuh as well, mkwst or jschuh, please take a look at cc_messages.

[jschuh, 2015-06-04 00:18:17]

Converting these indeces into signed integers creates the potential for invalid
comparisons and truncations at other layers. Why not plumb the size_t through
since it's the correct type for an index into an array?

[vmpstr, 2015-06-04 00:22:15]

On 2015/06/04 00:18:17, jschuh wrote:
> Converting these indeces into signed integers creates the potential for
invalid
> comparisons and truncations at other layers. Why not plumb the size_t through
> since it's the correct type for an index into an array?

Are there specific instances you are concerned about or just in general instead
of a static_cast<int>? This patch changes some ints to size_ts as well.

[jschuh, 2015-06-04 03:01:00]

On 2015/06/04 00:22:15, vmpstr wrote:
> On 2015/06/04 00:18:17, jschuh wrote:
> > Converting these indeces into signed integers creates the potential for
> invalid
> > comparisons and truncations at other layers. Why not plumb the size_t
through
> > since it's the correct type for an index into an array?
> 
> Are there specific instances you are concerned about or just in general
instead
> of a static_cast<int>? This patch changes some ints to size_ts as well.

There's two things that concern me. First, using static_cast rather than
base::checked_cast for integral conversions is likely to hide real truncation
bugs (of which we've had many in chrome). Second, a negative value is never
legal as an index, so if one makes it through it's almost certain to be a
security vulnerability (another recurring bug in Chrome). Neither of these are a
concern if you can verify that negative values and overflows are impossible, but
I'm inherently suspicious of that assumption. From my perspective, it's just
safer and cleaner to use size_t consistently and avoid potentially dangerous
conversions back and forth between ints.

[vmpstr, 2015-06-04 17:09:38]

> There's two things that concern me. First, using static_cast rather than
> base::checked_cast for integral conversions is likely to hide real truncation
> bugs (of which we've had many in chrome). Second, a negative value is never
> legal as an index, so if one makes it through it's almost certain to be a
> security vulnerability (another recurring bug in Chrome). Neither of these are
a
> concern if you can verify that negative values and overflows are impossible,
but
> I'm inherently suspicious of that assumption. From my perspective, it's just
> safer and cleaner to use size_t consistently and avoid potentially dangerous
> conversions back and forth between ints.

If I can categorize the changes in this patch, then this would be the list:

- Covert render_pass index to be size_t (instead of an int).
  Affected files:
  - delegated_renderer_layer_impl.h
  - delegated_renderer_layer_impl.cc
  - delegated_renderer_layer_impl_unittest.cc
  - render_pass.cc
  - render_pass_draw_quad.cc
  - render_pass_id.h
  - surface_aggregator_test_helpers.h
  - cc_messages.cc

- Convert child index to be size_t (instead of an int).
  Affected files:
  - layer.h
  - layer.cc

- Remove int size, and replace with bool empty
  Affected files:
  - layer.cc

- Change layer index to be size_t (instead of an int).
  Affected files:
  - layer_iterator.h

- Hash_function cast to int to resolve ambiguity of pair hashing functions.
  Affected files:
  - render_pass.h
  - render_pass_id.cc

==============

- static_cast for kGraphWidth in HUD layer
  Affected files:
  - heads_up_display_layer_impl.cc

- tracing/uma static_cast size_t to int (since we don't have size_t ints in
tracing)
  Affected files:
  - layer_impl.cc
  - direct_renderer.cc
  - display_item_list.cc

- gl->DrawElements static_cast to int
  Affected files:
  - gl_renderer.cc


From my understanding, the concern is all of the items below ========, is that
correct? I've changed indices to be size_ts in this patch, not the other way
around. If so, I'll be happy to undo those changes. If not, please let me know
if I'm missing something more subtle here.

[danakj, 2015-06-04 17:11:12]

On 2015/06/04 00:18:17, jschuh wrote:
> Converting these indeces into signed integers creates the potential for
invalid
> comparisons and truncations at other layers. Why not plumb the size_t through
> since it's the correct type for an index into an array?

Which case are you worried about here? This comment isn't attached to anything
in particular. I think I agree with your position here generally so I'm
confused.

In cc_messages, it is turning an int into a size_t.

[vmpstr, 2015-06-05 00:21:48]

Patchset #9 (id:160001) has been deleted

[vmpstr, 2015-06-05 00:21:54]

Patchset #9 (id:180001) has been deleted

[vmpstr, 2015-06-05 01:03:00]

Please take another look. After an offline discussion, we agreed to use
saturated_cast/checked_cast/CheckedNumeric where appropriate, so the latest
patch set attempts to do just that.

Let me know if something still looks off, and I'll be happy to fix it.

https://codereview.chromium.org/1158433010/diff/220001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/1158433010/diff/220001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:2237: 6 *
base::checked_cast<int>(draw_cache_.matrix_data.size()),
This is the only checked_cast that I'm using, most other places are
saturated_cast. Let me know if this should change. The code around this is
mostly static_casting everything...

[jschuh, 2015-06-05 14:08:25]

lgtm with a comment for a follow up

https://codereview.chromium.org/1158433010/diff/220001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/1158433010/diff/220001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:2237: 6 *
base::checked_cast<int>(draw_cache_.matrix_data.size()),
On 2015/06/05 01:03:00, vmpstr wrote:
> This is the only checked_cast that I'm using, most other places are
> saturated_cast. Let me know if this should change. The code around this is
> mostly static_casting everything...

Yeah, it looks like the other locations should be using checked or saturated
cast, but that's fine for a follow on.

[danakj, 2015-06-05 18:22:30]

LGTM

https://codereview.chromium.org/1158433010/diff/220001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/1158433010/diff/220001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:2237: 6 *
base::checked_cast<int>(draw_cache_.matrix_data.size()),
matrix_data.size() is at most 8 (line 2299). Having a CHECK compiled in here
seems wasteful, and this code is running every frame. I think this should be
DCHECK and static_cast IMO.

wtb base::dchecked_cast heh.

[vmpstr, 2015-06-05 19:27:34]

https://codereview.chromium.org/1158433010/diff/220001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/1158433010/diff/220001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:2237: 6 *
base::checked_cast<int>(draw_cache_.matrix_data.size()),
On 2015/06/05 18:22:29, danakj wrote:
> matrix_data.size() is at most 8 (line 2299). Having a CHECK compiled in here
> seems wasteful, and this code is running every frame. I think this should be
> DCHECK and static_cast IMO.
> 
> wtb base::dchecked_cast heh.

Ok, I changed this to a DCHECK. I think if this is to be fixed, we should just
do it in one pass for all instances of static_cast in gl_renderer.cc

[vmpstr, 2015-06-05 19:31:16]

The CQ bit was checked by vmpstr@chromium.org

[vmpstr, 2015-06-05 19:31:17]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
jschuh@chromium.org
Link to the patchset: https://codereview.chromium.org/1158433010/#ps260001
(title: " ")

[commit-bot: I haz the power, 2015-06-05 19:31:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1158433010/260001

[commit-bot: I haz the power, 2015-06-05 21:18:55]

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

[commit-bot: I haz the power, 2015-06-05 21:18:56]

Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[vmpstr, 2015-06-05 21:43:45]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2015-06-05 21:44:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1158433010/260001

[commit-bot: I haz the power, 2015-06-05 22:27:57]

Committed patchset #12 (id:260001)

[commit-bot: I haz the power, 2015-06-05 22:28:56]

Patchset 12 (id:??) landed as
https://crrev.com/0d5963315aa03e6ebb20351f13d3d517ca14d816
Cr-Commit-Position: refs/heads/master@{#333153}

[vmpstr, 2015-06-05 23:01:41]

A revert of this CL (patchset #12 id:260001) has been created in
https://codereview.chromium.org/1155553006/ by vmpstr@chromium.org.

The reason for reverting is: Broke windows build.
http://build.chromium.org/p/chromium.win/builders/Win%20x64%20GN%20%28dbg%29/....

[vmpstr, 2015-06-06 00:10:42]

+sky, please take a look at the mojo bits (mojo/converters and
components/view_manager)

[vmpstr, 2015-06-06 00:11:34]

vmpstr@chromium.org changed reviewers:
+ sky@chromium.org

[vmpstr, 2015-06-06 00:11:37]

+sky for real (please take a look at mojo parts of the change)

[sky, 2015-06-08 14:53:14]

https://codereview.chromium.org/1158433010/diff/280001/components/view_manage...
File components/view_manager/public/interfaces/quads.mojom (right):

https://codereview.chromium.org/1158433010/diff/280001/components/view_manage...
components/view_manager/public/interfaces/quads.mojom:23: uint32 index;
Seems wrong to have this and RenderPassId::index difference types. Should this
be uint64 to match the RenderPassId::index? Or should  RenderPassId::index be
uint32_t to match this?

[vmpstr, 2015-06-08 18:59:27]

https://codereview.chromium.org/1158433010/diff/280001/components/view_manage...
File components/view_manager/public/interfaces/quads.mojom (right):

https://codereview.chromium.org/1158433010/diff/280001/components/view_manage...
components/view_manager/public/interfaces/quads.mojom:23: uint32 index;
On 2015/06/08 14:53:13, sky wrote:
> Seems wrong to have this and RenderPassId::index difference types. Should this
> be uint64 to match the RenderPassId::index? Or should  RenderPassId::index be
> uint32_t to match this?

The problem is that I think the correct type for RenderPassId::index is size_t,
since it's an index into a vector. For mojo, I was following a similar
situation:
https://code.google.com/p/chromium/codesearch#chromium/src/mojo/converters/su...
(a dcheck and a static_cast from size_t to uint32_t). 

Making this uint64 would probably cause more truncation issues when used as an
index on systems where size_t is 4 bytes...

Do you know if there are any plans to support size_t in mojom files? That would
probably be the best solution if we want the types to match perfectly.

[sky, 2015-06-08 20:01:14]

I don't think we would ever add size_t as it depends upon compile time flags,
which in theory could differ from process to process. LGTM

[vmpstr, 2015-06-08 21:45:28]

The CQ bit was checked by vmpstr@chromium.org

[vmpstr, 2015-06-08 21:45:29]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
jschuh@chromium.org
Link to the patchset: https://codereview.chromium.org/1158433010/#ps300001
(title: " ")

[commit-bot: I haz the power, 2015-06-08 21:45:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1158433010/300001

[commit-bot: I haz the power, 2015-06-08 21:45:58]

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

[commit-bot: I haz the power, 2015-06-08 21:46:00]

Transient error: Too many "tryserver.blink:linux_blink_rel,
tryserver.chromium.win:win_chromium_gn_x64_dbg" delimiters in ":".
Correct syntax is "tryserver:bot1,bot2;tryserver2:bot3,bot4;".

[vmpstr, 2015-06-08 21:46:45]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2015-06-08 21:47:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1158433010/300001

[commit-bot: I haz the power, 2015-06-08 22:24:24]

Committed patchset #14 (id:300001)

[commit-bot: I haz the power, 2015-06-08 22:25:26]

Patchset 14 (id:??) landed as
https://crrev.com/91e2309abe88d9a333deaf4714d0c48f875ba472
Cr-Commit-Position: refs/heads/master@{#333362}

[Nico, 2015-07-31 17:57:13]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2015-07-31 17:57:15]

https://codereview.chromium.org/1158433010/diff/300001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/1158433010/diff/300001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:2243:
static_cast<size_t>(std::numeric_limits<int>::max()) / 6u);
shouldn't this check that size() is lessequal than 8? StaticGeometry only sets
up indices for at most 8 triangles as far as I can tell.

[vmpstr, 2015-07-31 18:29:31]

https://codereview.chromium.org/1158433010/diff/300001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/1158433010/diff/300001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:2243:
static_cast<size_t>(std::numeric_limits<int>::max()) / 6u);
On 2015/07/31 17:57:15, Nico (hiding) wrote:
> shouldn't this check that size() is lessequal than 8? StaticGeometry only sets
> up indices for at most 8 triangles as far as I can tell.

Yeah, I think that's clearly a much tighter bound. The intent of this check is
to simply guard against overflow in a call below, so the code is free to have
larger size, if at all possible, as long as it doesn't cause an overflow. But
you're absolutely right that we should probably use as tight of a bound as we
can here.