Prevent overflows when using gamma_alloc_size

Prevent overflows when using gamma_alloc_size

BUG=636268
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2230163002

Committed: https://skia.googlesource.com/skia/+/40ff5fe59b77b0b3e34467cc2f8666e4e88356f9

[Giwan Go, 2016-08-10 08:42:56]

Description was changed from

==========
Prevent overflows when using gamma_alloc_size

BUG=636268
==========

to

==========
Prevent overflows when using gamma_alloc_size

BUG=636268
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2230163002
==========

[msarett, 2016-08-10 17:24:50]

msarett@google.com changed reviewers:
+ msarett@google.com, reed@google.com

[msarett, 2016-08-10 17:24:52]

Thanks for this fix!  Looks good to me plus some comments to match the Skia
style guide.

FWIW, I intend to add some limits on the "max" size of gamma tables.  That way,
we give up sooner if a table is ridiculously large.  Just not sure what that
"max" should be.

Regardless, I think we are right to check for integer overflow here.

https://codereview.chromium.org/2230163002/diff/20001/src/core/SkColorSpace_I...
File src/core/SkColorSpace_ICC.cpp (right):

https://codereview.chromium.org/2230163002/diff/20001/src/core/SkColorSpace_I...
src/core/SkColorSpace_ICC.cpp:791: return_if_false(safe_add(allocSize,
gamma_alloc_size(rType, rData), &allocSize), "bad size");
nit: Lines should be less than 100 chars.

nit: Change error message to "SkGammas struct is too large to allocate".

https://codereview.chromium.org/2230163002/diff/20001/src/core/SkColorSpace_I...
src/core/SkColorSpace_ICC.cpp:824: return_if_false(safe_add(allocSize,
gamma_alloc_size(rType, rData), &allocSize), "bad size");
Same nits on these three lines as well.

https://codereview.chromium.org/2230163002/diff/20001/src/core/SkColorSpace_I...
src/core/SkColorSpace_ICC.cpp:977: return_null("bad size");
"SkGammas struct is too large to allocate"

https://codereview.chromium.org/2230163002/diff/20001/src/core/SkColorSpace_I...
src/core/SkColorSpace_ICC.cpp:1013: !safe_add(allocSize, gamma_alloc_size(bType,
bData), &allocSize)) {
nit: Move brace to it's own line

https://codereview.chromium.org/2230163002/diff/20001/src/core/SkColorSpace_I...
src/core/SkColorSpace_ICC.cpp:1014: return_null("bad size");
"SkGammas struct is too large to allocate"

[Giwan Go, 2016-08-11 11:18:06]

On 2016/08/10 17:24:52, msarett wrote:
> Thanks for this fix!  Looks good to me plus some comments to match the Skia
> style guide.
> 
> FWIW, I intend to add some limits on the "max" size of gamma tables.  That
way,
> we give up sooner if a table is ridiculously large.  Just not sure what that
> "max" should be.
> 
> Regardless, I think we are right to check for integer overflow here.
> 
>
https://codereview.chromium.org/2230163002/diff/20001/src/core/SkColorSpace_I...
> File src/core/SkColorSpace_ICC.cpp (right):
> 
>
https://codereview.chromium.org/2230163002/diff/20001/src/core/SkColorSpace_I...
> src/core/SkColorSpace_ICC.cpp:791: return_if_false(safe_add(allocSize,
> gamma_alloc_size(rType, rData), &allocSize), "bad size");
> nit: Lines should be less than 100 chars.
> 
> nit: Change error message to "SkGammas struct is too large to allocate".
> 
>
https://codereview.chromium.org/2230163002/diff/20001/src/core/SkColorSpace_I...
> src/core/SkColorSpace_ICC.cpp:824: return_if_false(safe_add(allocSize,
> gamma_alloc_size(rType, rData), &allocSize), "bad size");
> Same nits on these three lines as well.
> 
>
https://codereview.chromium.org/2230163002/diff/20001/src/core/SkColorSpace_I...
> src/core/SkColorSpace_ICC.cpp:977: return_null("bad size");
> "SkGammas struct is too large to allocate"
> 
>
https://codereview.chromium.org/2230163002/diff/20001/src/core/SkColorSpace_I...
> src/core/SkColorSpace_ICC.cpp:1013: !safe_add(allocSize,
gamma_alloc_size(bType,
> bData), &allocSize)) {
> nit: Move brace to it's own line
> 
>
https://codereview.chromium.org/2230163002/diff/20001/src/core/SkColorSpace_I...
> src/core/SkColorSpace_ICC.cpp:1014: return_null("bad size");
> "SkGammas struct is too large to allocate"

Updated!

[msarett, 2016-08-11 12:53:35]

lgtm

[msarett, 2016-08-11 13:06:34]

The CQ bit was checked by msarett@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-11 13:06:43]

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-08-11 13:35:22]

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

[commit-bot: I haz the power, 2016-08-11 13:35:22]

Dry run: This issue passed the CQ dry run.

[Giwan Go, 2016-08-14 08:39:38]

The CQ bit was checked by gogil@stealien.com

[commit-bot: I haz the power, 2016-08-14 08:39:45]

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-14 08:50:15]

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

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

Try jobs failed on following builders:
  Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-GN-Trybot on
master.client.skia (JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...)

[Giwan Go, 2016-08-14 09:02:55]

The CQ bit was checked by gogil@stealien.com

[commit-bot: I haz the power, 2016-08-14 09:03:06]

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-14 09:12:43]

Description was changed from

==========
Prevent overflows when using gamma_alloc_size

BUG=636268
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2230163002
==========

to

==========
Prevent overflows when using gamma_alloc_size

BUG=636268
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2230163002

Committed:
https://skia.googlesource.com/skia/+/40ff5fe59b77b0b3e34467cc2f8666e4e88356f9
==========

[commit-bot: I haz the power, 2016-08-14 09:12:43]

Committed patchset #3 (id:40001) as
https://skia.googlesource.com/skia/+/40ff5fe59b77b0b3e34467cc2f8666e4e88356f9