Fix integer overflow in software compositor

Fix integer overflow in software compositor

Ensure that the size mapped from the renderer
process for the software frame is not less than
expected due to integer overflow.

BUG=348332
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257417

[ccameron, 2014-03-13 23:15:01]

Is this an appropriate fix?

[Tom Sepez, 2014-03-14 00:10:02]

On 2014/03/13 23:15:01, ccameron1 wrote:
> Is this an appropriate fix?
Assigning to author of safe integer package to verify.

[jschuh, 2014-03-14 06:14:12]

https://codereview.chromium.org/196283018/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/software_frame_manager.cc (right):

https://codereview.chromium.org/196283018/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/software_frame_manager.cc:103:
base::CheckedNumeric<size_t>(frame_data->size.height());
This is fine, but is there a good reason not to have overflow checks in
gfx::SizeBase::GetArea() and gfx::SizeBase::Expand()? We can just have integer
overflow saturate to numeric_limits<Type>::max(), to preserve the current
interface (and it parallels flooring the negative values to zero elsewhere in
the class).

[ccameron, 2014-03-14 19:38:14]

Filed an issue on that bug. If we saturate there, we'll still need to patch up
the "4 * " here. I'll wait until that bug is resolved (should it turn out to be
controversial) and then finish this one up.

https://codereview.chromium.org/196283018/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/software_frame_manager.cc (right):

https://codereview.chromium.org/196283018/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/software_frame_manager.cc:103:
base::CheckedNumeric<size_t>(frame_data->size.height());
On 2014/03/14 06:14:12, Justin Schuh wrote:
> This is fine, but is there a good reason not to have overflow checks in
> gfx::SizeBase::GetArea() and gfx::SizeBase::Expand()? We can just have integer
> overflow saturate to numeric_limits<Type>::max(), to preserve the current
> interface (and it parallels flooring the negative values to zero elsewhere in
> the class).

Filed issue 352761 on it.

[jschuh, 2014-03-14 19:54:28]

On 2014/03/14 19:38:14, ccameron1 wrote:
> Filed an issue on that bug. If we saturate there, we'll still need to patch up
> the "4 * " here. I'll wait until that bug is resolved (should it turn out to
be
> controversial) and then finish this one up.
> 
>
https://codereview.chromium.org/196283018/diff/1/content/browser/renderer_hos...
> File content/browser/renderer_host/software_frame_manager.cc (right):
> 
>
https://codereview.chromium.org/196283018/diff/1/content/browser/renderer_hos...
> content/browser/renderer_host/software_frame_manager.cc:103:
> base::CheckedNumeric<size_t>(frame_data->size.height());
> On 2014/03/14 06:14:12, Justin Schuh wrote:
> > This is fine, but is there a good reason not to have overflow checks in
> > gfx::SizeBase::GetArea() and gfx::SizeBase::Expand()? We can just have
integer
> > overflow saturate to numeric_limits<Type>::max(), to preserve the current
> > interface (and it parallels flooring the negative values to zero elsewhere
in
> > the class).
> 
> Filed issue 352761 on it.

okay, lgtm on this and we'll follow up on the bug.

[ccameron, 2014-03-14 19:59:56]

Thanks -- I'll have a patch for the other bug up for discussion in a bit. Btw,
can you point me to the Expand function you mentioned?

[ccameron, 2014-03-14 19:59:59]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2014-03-14 20:01:54]

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

[commit-bot: I haz the power, 2014-03-14 20:05:34]

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

[commit-bot: I haz the power, 2014-03-14 20:05:34]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg

[ccameron, 2014-03-14 20:19:31]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2014-03-14 20:20:47]

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

[commit-bot: I haz the power, 2014-03-14 20:24:10]

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

[commit-bot: I haz the power, 2014-03-14 20:24:10]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg

[ccameron, 2014-03-17 11:26:34]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2014-03-17 11:26:39]

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

[commit-bot: I haz the power, 2014-03-17 13:40:36]

Change committed as 257417