Add --serial mode to dm, runs unit tests serially

Add --solo mode to dm, runs unit tests alone

This mode is useful for tests that use a lot of memory (and
should not be run multi-threaded).

Also adds regression test for bug fix in:
https://codereview.chromium.org/2230163002

This particular test will be run using --solo.

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

Committed: https://skia.googlesource.com/skia/+/bcae9d3d15d34a59d279c34e187e6101975500c0

[msarett, 2016-08-14 21:14:08]

Description was changed from

==========
Add regression test

BUG:636268
==========

to

==========
Add regression test

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

[msarett, 2016-08-14 22:00:02]

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

[msarett, 2016-08-14 22:04:48]

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

[commit-bot: I haz the power, 2016-08-14 22:04:51]

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-14 22:22:22]

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

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

Dry run: This issue passed the CQ dry run.

[msarett, 2016-08-14 23:01:44]

Description was changed from

==========
Add regression test

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

to

==========
Add regression test

Original bug fix was in:
https://codereview.chromium.org/2230163002

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

[Giwan Go, 2016-08-15 04:58:17]

lgtm

[msarett, 2016-08-15 16:41:00]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2016-08-15 16:41: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-15 16:42:03]

Description was changed from

==========
Add regression test

Original bug fix was in:
https://codereview.chromium.org/2230163002

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

to

==========
Add regression test

Original bug fix was in:
https://codereview.chromium.org/2230163002

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

Committed:
https://skia.googlesource.com/skia/+/bcae9d3d15d34a59d279c34e187e6101975500c0
==========

[commit-bot: I haz the power, 2016-08-15 16:42:04]

Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/bcae9d3d15d34a59d279c34e187e6101975500c0

[msarett, 2016-08-16 11:54:09]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/2243403003/ by msarett@google.com.

The reason for reverting is: Nexus 9 and Nexus Player failures..

[Giwan Go, 2016-08-16 12:38:32]

On 2016/08/16 11:54:09, msarett wrote:
> A revert of this CL (patchset #2 id:20001) has been created in
> https://codereview.chromium.org/2243403003/ by mailto:msarett@google.com.
> 
> The reason for reverting is: Nexus 9 and Nexus Player failures..

What does 'failures' mean?
By any chance, OOM in SkColorSpace::NewICC?

[msarett, 2016-08-16 12:40:32]

On 2016/08/16 12:38:32, GiWan Go wrote:
> On 2016/08/16 11:54:09, msarett wrote:
> > A revert of this CL (patchset #2 id:20001) has been created in
> > https://codereview.chromium.org/2243403003/ by mailto:msarett@google.com.
> > 
> > The reason for reverting is: Nexus 9 and Nexus Player failures..
> 
> What does 'failures' mean?
> By any chance, OOM in SkColorSpace::NewICC?

I believe that these are OOM errors.  I just want to confirm.  Most likely,
I will disable the test on these devices and re-land.

[msarett, 2016-08-16 12:40:51]

Description was changed from

==========
Add regression test

Original bug fix was in:
https://codereview.chromium.org/2230163002

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

Committed:
https://skia.googlesource.com/skia/+/bcae9d3d15d34a59d279c34e187e6101975500c0
==========

to

==========
Add regression test

Original bug fix was in:
https://codereview.chromium.org/2230163002

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

Committed:
https://skia.googlesource.com/skia/+/bcae9d3d15d34a59d279c34e187e6101975500c0
==========

[Giwan Go, 2016-08-16 12:43:29]

On 2016/08/16 12:40:32, msarett wrote:
> On 2016/08/16 12:38:32, GiWan Go wrote:
> > On 2016/08/16 11:54:09, msarett wrote:
> > > A revert of this CL (patchset #2 id:20001) has been created in
> > > https://codereview.chromium.org/2243403003/ by mailto:msarett@google.com.
> > > 
> > > The reason for reverting is: Nexus 9 and Nexus Player failures..
> > 
> > What does 'failures' mean?
> > By any chance, OOM in SkColorSpace::NewICC?
> 
> I believe that these are OOM errors.  I just want to confirm.  Most likely,
> I will disable the test on these devices and re-land.

I think.. This testcase only run in 32bit build of skia.

[msarett, 2016-08-16 13:11:31]

On 2016/08/16 12:43:29, GiWan Go wrote:
> On 2016/08/16 12:40:32, msarett wrote:
> > On 2016/08/16 12:38:32, GiWan Go wrote:
> > > On 2016/08/16 11:54:09, msarett wrote:
> > > > A revert of this CL (patchset #2 id:20001) has been created in
> > > > https://codereview.chromium.org/2243403003/ by
mailto:msarett@google.com.
> > > > 
> > > > The reason for reverting is: Nexus 9 and Nexus Player failures..
> > > 
> > > What does 'failures' mean?
> > > By any chance, OOM in SkColorSpace::NewICC?
> > 
> > I believe that these are OOM errors.  I just want to confirm.  Most likely,
> > I will disable the test on these devices and re-land.
> 
> I think.. This testcase only run in 32bit build of skia.

Good point, this doesn't really test anything on 64-bit.  Unfortunately, I
think I'll still need to disable on Nexus Player.

[Giwan Go, 2016-08-16 13:32:41]

On 2016/08/16 13:11:31, msarett wrote:
> On 2016/08/16 12:43:29, GiWan Go wrote:
> > On 2016/08/16 12:40:32, msarett wrote:
> > > On 2016/08/16 12:38:32, GiWan Go wrote:
> > > > On 2016/08/16 11:54:09, msarett wrote:
> > > > > A revert of this CL (patchset #2 id:20001) has been created in
> > > > > https://codereview.chromium.org/2243403003/ by
> mailto:msarett@google.com.
> > > > > 
> > > > > The reason for reverting is: Nexus 9 and Nexus Player failures..
> > > > 
> > > > What does 'failures' mean?
> > > > By any chance, OOM in SkColorSpace::NewICC?
> > > 
> > > I believe that these are OOM errors.  I just want to confirm.  Most
likely,
> > > I will disable the test on these devices and re-land.
> > 
> > I think.. This testcase only run in 32bit build of skia.
> 
> Good point, this doesn't really test anything on 64-bit.  Unfortunately, I
> think I'll still need to disable on Nexus Player.

Oh, Nexus Player has just 1GB of RAM.
Put on the spot.

[msarett, 2016-08-16 13:51:30]

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

[commit-bot: I haz the power, 2016-08-16 13:51:35]

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

[mtklein, 2016-08-16 13:53:01]

On 2016/08/16 13:32:41, GiWan Go wrote:
> On 2016/08/16 13:11:31, msarett wrote:
> > On 2016/08/16 12:43:29, GiWan Go wrote:
> > > On 2016/08/16 12:40:32, msarett wrote:
> > > > On 2016/08/16 12:38:32, GiWan Go wrote:
> > > > > On 2016/08/16 11:54:09, msarett wrote:
> > > > > > A revert of this CL (patchset #2 id:20001) has been created in
> > > > > > https://codereview.chromium.org/2243403003/ by
> > mailto:msarett@google.com.
> > > > > > 
> > > > > > The reason for reverting is: Nexus 9 and Nexus Player failures..
> > > > > 
> > > > > What does 'failures' mean?
> > > > > By any chance, OOM in SkColorSpace::NewICC?
> > > > 
> > > > I believe that these are OOM errors.  I just want to confirm.  Most
> likely,
> > > > I will disable the test on these devices and re-land.
> > > 
> > > I think.. This testcase only run in 32bit build of skia.
> > 
> > Good point, this doesn't really test anything on 64-bit.  Unfortunately, I
> > think I'll still need to disable on Nexus Player.
> 
> Oh, Nexus Player has just 1GB of RAM.
> Put on the spot.

How much RAM does this regression test need?
If we're talking 100s of MB or higher, we should probably not run it with other
tests.  That's just asking for confusing DM runs down the line.

[Giwan Go, 2016-08-16 14:07:20]

On 2016/08/16 13:53:01, mtklein wrote:
> On 2016/08/16 13:32:41, GiWan Go wrote:
> > On 2016/08/16 13:11:31, msarett wrote:
> > > On 2016/08/16 12:43:29, GiWan Go wrote:
> > > > On 2016/08/16 12:40:32, msarett wrote:
> > > > > On 2016/08/16 12:38:32, GiWan Go wrote:
> > > > > > On 2016/08/16 11:54:09, msarett wrote:
> > > > > > > A revert of this CL (patchset #2 id:20001) has been created in
> > > > > > > https://codereview.chromium.org/2243403003/ by
> > > mailto:msarett@google.com.
> > > > > > > 
> > > > > > > The reason for reverting is: Nexus 9 and Nexus Player failures..
> > > > > > 
> > > > > > What does 'failures' mean?
> > > > > > By any chance, OOM in SkColorSpace::NewICC?
> > > > > 
> > > > > I believe that these are OOM errors.  I just want to confirm.  Most
> > likely,
> > > > > I will disable the test on these devices and re-land.
> > > > 
> > > > I think.. This testcase only run in 32bit build of skia.
> > > 
> > > Good point, this doesn't really test anything on 64-bit.  Unfortunately, I
> > > think I'll still need to disable on Nexus Player.
> > 
> > Oh, Nexus Player has just 1GB of RAM.
> > Put on the spot.
> 
> How much RAM does this regression test need?
> If we're talking 100s of MB or higher, we should probably not run it with
other
> tests.  That's just asking for confusing DM runs down the line.

This testcase uses 1.4GB(700MB*2) of memory.

[msarett, 2016-08-16 14:36:48]

On 2016/08/16 13:53:01, mtklein wrote:
> On 2016/08/16 13:32:41, GiWan Go wrote:
> > On 2016/08/16 13:11:31, msarett wrote:
> > > On 2016/08/16 12:43:29, GiWan Go wrote:
> > > > On 2016/08/16 12:40:32, msarett wrote:
> > > > > On 2016/08/16 12:38:32, GiWan Go wrote:
> > > > > > On 2016/08/16 11:54:09, msarett wrote:
> > > > > > > A revert of this CL (patchset #2 id:20001) has been created in
> > > > > > > https://codereview.chromium.org/2243403003/ by
> > > mailto:msarett@google.com.
> > > > > > > 
> > > > > > > The reason for reverting is: Nexus 9 and Nexus Player failures..
> > > > > > 
> > > > > > What does 'failures' mean?
> > > > > > By any chance, OOM in SkColorSpace::NewICC?
> > > > > 
> > > > > I believe that these are OOM errors.  I just want to confirm.  Most
> > likely,
> > > > > I will disable the test on these devices and re-land.
> > > > 
> > > > I think.. This testcase only run in 32bit build of skia.
> > > 
> > > Good point, this doesn't really test anything on 64-bit.  Unfortunately, I
> > > think I'll still need to disable on Nexus Player.
> > 
> > Oh, Nexus Player has just 1GB of RAM.
> > Put on the spot.
> 
> How much RAM does this regression test need?
> If we're talking 100s of MB or higher, we should probably not run it with
other
> tests.  That's just asking for confusing DM runs down the line.

Do we have a way to mark tests like these?

[msarett, 2016-08-16 14:37:16]

msarett@google.com changed reviewers:
+ mtklein@google.com

[mtklein, 2016-08-16 14:42:11]

On 2016/08/16 14:36:48, msarett wrote:
> On 2016/08/16 13:53:01, mtklein wrote:
> > On 2016/08/16 13:32:41, GiWan Go wrote:
> > > On 2016/08/16 13:11:31, msarett wrote:
> > > > On 2016/08/16 12:43:29, GiWan Go wrote:
> > > > > On 2016/08/16 12:40:32, msarett wrote:
> > > > > > On 2016/08/16 12:38:32, GiWan Go wrote:
> > > > > > > On 2016/08/16 11:54:09, msarett wrote:
> > > > > > > > A revert of this CL (patchset #2 id:20001) has been created in
> > > > > > > > https://codereview.chromium.org/2243403003/ by
> > > > mailto:msarett@google.com.
> > > > > > > > 
> > > > > > > > The reason for reverting is: Nexus 9 and Nexus Player failures..
> > > > > > > 
> > > > > > > What does 'failures' mean?
> > > > > > > By any chance, OOM in SkColorSpace::NewICC?
> > > > > > 
> > > > > > I believe that these are OOM errors.  I just want to confirm.  Most
> > > likely,
> > > > > > I will disable the test on these devices and re-land.
> > > > > 
> > > > > I think.. This testcase only run in 32bit build of skia.
> > > > 
> > > > Good point, this doesn't really test anything on 64-bit.  Unfortunately,
I
> > > > think I'll still need to disable on Nexus Player.
> > > 
> > > Oh, Nexus Player has just 1GB of RAM.
> > > Put on the spot.
> > 
> > How much RAM does this regression test need?
> > If we're talking 100s of MB or higher, we should probably not run it with
> other
> > tests.  That's just asking for confusing DM runs down the line.
> 
> Do we have a way to mark tests like these?

No.  I think you'd want to run it as a second pass, like the Valgrind bot does. 
(A single bot task runs dm 2-3 times IIRC.)

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

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

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

Dry run: This issue passed the CQ dry run.

[msarett, 2016-08-16 19:11:00]

msarett@google.com changed reviewers:
+ borenet@google.com

[msarett, 2016-08-16 19:11:01]

> > > How much RAM does this regression test need?
> > > If we're talking 100s of MB or higher, we should probably not run it with
> > other
> > > tests.  That's just asking for confusing DM runs down the line.
> > 
> > 1.4 GB.  Do we have a way to mark tests like these?
> 
> No.  I think you'd want to run it as a second pass, like the Valgrind bot
does. 
> (A single bot task runs dm 2-3 times IIRC.)

Talked to Eric about this a bit.  Sounds like valgrind is
the only bot that runs dm more than once.  And I think we
want to drop this behavior once we start sharding tests
to different machines.

On the other hand, I would agree that running this in
parallel with other tests is going to lead to annoying
flakes and failures.

Eric suggested that maybe dm should run a set of tests
serially before spawning threads?  This test would be a
good candidate for that.  Eric, do you think that's
something we'll implement in future?  If so, I'm ok if we
decide not to land this test until that option is
available.

[msarett, 2016-08-16 19:15:16]

On 2016/08/16 19:11:01, msarett wrote:
> > > > How much RAM does this regression test need?
> > > > If we're talking 100s of MB or higher, we should probably not run it
with
> > > other
> > > > tests.  That's just asking for confusing DM runs down the line.
> > > 
> > > 1.4 GB.  Do we have a way to mark tests like these?
> > 
> > No.  I think you'd want to run it as a second pass, like the Valgrind bot
> does. 
> > (A single bot task runs dm 2-3 times IIRC.)
> 
> Talked to Eric about this a bit.  Sounds like valgrind is
> the only bot that runs dm more than once.  And I think we
> want to drop this behavior once we start sharding tests
> to different machines.
> 
> On the other hand, I would agree that running this in
> parallel with other tests is going to lead to annoying
> flakes and failures.
> 
> Eric suggested that maybe dm should run a set of tests
> serially before spawning threads?  This test would be a
> good candidate for that.  Eric, do you think that's
> something we'll implement in future?  If so, I'm ok if we
> decide not to land this test until that option is
> available.

Replying to myself.  I'll go ahead and add a feature to
dm to run a few tests serially before kicking off all
the threads.

[msarett, 2016-08-16 22:32:25]

Patchset #4 (id:60001) has been deleted

[msarett, 2016-08-16 22:34:14]

Description was changed from

==========
Add regression test

Original bug fix was in:
https://codereview.chromium.org/2230163002

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

Committed:
https://skia.googlesource.com/skia/+/bcae9d3d15d34a59d279c34e187e6101975500c0
==========

to

==========
Add --serial mode to dm, runs unit tests serially

This mode is useful for tests that use a lot of memory (and
should not be run multi-threaded).

Also adds regression test for bug fix in:
https://codereview.chromium.org/2230163002

This particular test will be run using --serial.

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

Committed:
https://skia.googlesource.com/skia/+/bcae9d3d15d34a59d279c34e187e6101975500c0
==========

[msarett, 2016-08-16 22:35:05]

Please take a look.  I've added the --serial flag to dm
to run unit tests serially.

[mtklein, 2016-08-16 23:54:07]

https://codereview.chromium.org/2243143002/diff/80001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/2243143002/diff/80001/dm/DM.cpp#newcode74
dm/DM.cpp:74: DEFINE_string2(serial, s, nullptr,
Why don't we call this flag and the array and so on something that makes it
stand apart a bit more from the existing serial tests?

E.g.  --solo

https://codereview.chromium.org/2243143002/diff/80001/dm/DM.cpp#newcode1239
dm/DM.cpp:1239: if (name_in_string_array(FLAGS_serial, test.name)) {
Probably better as

if (FLAGS_serial.contains(test.name)) { ... }

https://codereview.chromium.org/2243143002/diff/80001/infra/bots/recipes/swar...
File infra/bots/recipes/swarm_test.py (right):

https://codereview.chromium.org/2243143002/diff/80001/infra/bots/recipes/swar...
infra/bots/recipes/swarm_test.py:310: match.append('~ColorSpaceCRBug')
Is this still going to be necessary once we've made the test run in complete
isolation?  I don't suppose we can construct a smaller test case?

https://codereview.chromium.org/2243143002/diff/80001/tests/ColorSpaceTest.cpp
File tests/ColorSpaceTest.cpp (right):

https://codereview.chromium.org/2243143002/diff/80001/tests/ColorSpaceTest.cp...
tests/ColorSpaceTest.cpp:264: #if defined(SK_CPU_X86) || defined(SK_CPU_ARM32)
Do we really need to not have the test exist?  Can't we just have it here and
not run it?

[msarett, 2016-08-17 12:23:24]

Description was changed from

==========
Add --serial mode to dm, runs unit tests serially

This mode is useful for tests that use a lot of memory (and
should not be run multi-threaded).

Also adds regression test for bug fix in:
https://codereview.chromium.org/2230163002

This particular test will be run using --serial.

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

Committed:
https://skia.googlesource.com/skia/+/bcae9d3d15d34a59d279c34e187e6101975500c0
==========

to

==========
Add --solo mode to dm, runs unit tests alone

This mode is useful for tests that use a lot of memory (and
should not be run multi-threaded).

Also adds regression test for bug fix in:
https://codereview.chromium.org/2230163002

This particular test will be run using --solo.

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

Committed:
https://skia.googlesource.com/skia/+/bcae9d3d15d34a59d279c34e187e6101975500c0
==========

[msarett, 2016-08-17 12:40:14]

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

[commit-bot: I haz the power, 2016-08-17 12:40:22]

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

[msarett, 2016-08-17 12:40:45]

https://codereview.chromium.org/2243143002/diff/80001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/2243143002/diff/80001/dm/DM.cpp#newcode74
dm/DM.cpp:74: DEFINE_string2(serial, s, nullptr,
On 2016/08/16 23:54:06, mtklein wrote:
> Why don't we call this flag and the array and so on something that makes it
> stand apart a bit more from the existing serial tests?
> 
> E.g.  --solo

sgtm was struggling for a name.

https://codereview.chromium.org/2243143002/diff/80001/dm/DM.cpp#newcode1239
dm/DM.cpp:1239: if (name_in_string_array(FLAGS_serial, test.name)) {
On 2016/08/16 23:54:06, mtklein wrote:
> Probably better as
> 
> if (FLAGS_serial.contains(test.name)) { ... }

Ahh yes, done.

https://codereview.chromium.org/2243143002/diff/80001/infra/bots/recipes/swar...
File infra/bots/recipes/swarm_test.py (right):

https://codereview.chromium.org/2243143002/diff/80001/infra/bots/recipes/swar...
infra/bots/recipes/swarm_test.py:310: match.append('~ColorSpaceCRBug')
On 2016/08/16 23:54:06, mtklein wrote:
> Is this still going to be necessary once we've made the test run in complete
> isolation?  I don't suppose we can construct a smaller test case?

I'll run the trybot, but I think we'll still need it.

Unforuntately, the bug only reproduces if we have 3 giant gamma tables - large
enough to overflow uint32_t when their sizes are added together.  And if the ICC
profile isn't large enough to contain the tables, we reject it early.  So we
need a really large ICC profile.

https://codereview.chromium.org/2243143002/diff/80001/tests/ColorSpaceTest.cpp
File tests/ColorSpaceTest.cpp (right):

https://codereview.chromium.org/2243143002/diff/80001/tests/ColorSpaceTest.cp...
tests/ColorSpaceTest.cpp:264: #if defined(SK_CPU_X86) || defined(SK_CPU_ARM32)
On 2016/08/16 23:54:06, mtklein wrote:
> Do we really need to not have the test exist?  Can't we just have it here and
> not run it?

Sure, sgtm

[mtklein, 2016-08-17 12:45:12]

https://codereview.chromium.org/2243143002/diff/80001/infra/bots/recipes/swar...
File infra/bots/recipes/swarm_test.py (right):

https://codereview.chromium.org/2243143002/diff/80001/infra/bots/recipes/swar...
infra/bots/recipes/swarm_test.py:310: match.append('~ColorSpaceCRBug')
On 2016/08/17 12:40:45, msarett wrote:
> On 2016/08/16 23:54:06, mtklein wrote:
> > Is this still going to be necessary once we've made the test run in complete
> > isolation?  I don't suppose we can construct a smaller test case?
> 
> I'll run the trybot, but I think we'll still need it.
> 
> Unforuntately, the bug only reproduces if we have 3 giant gamma tables - large
> enough to overflow uint32_t when their sizes are added together.  And if the
ICC
> profile isn't large enough to contain the tables, we reject it early.  So we
> need a really large ICC profile.

So you're saying it allocates 2 giant tables, then the third tips it over?

Can we write the parsing to check the sizes of all its allocations first before
doing any?

[msarett, 2016-08-17 12:51:47]

https://codereview.chromium.org/2243143002/diff/80001/infra/bots/recipes/swar...
File infra/bots/recipes/swarm_test.py (right):

https://codereview.chromium.org/2243143002/diff/80001/infra/bots/recipes/swar...
infra/bots/recipes/swarm_test.py:310: match.append('~ColorSpaceCRBug')
On 2016/08/17 12:45:12, mtklein wrote:
> On 2016/08/17 12:40:45, msarett wrote:
> > On 2016/08/16 23:54:06, mtklein wrote:
> > > Is this still going to be necessary once we've made the test run in
complete
> > > isolation?  I don't suppose we can construct a smaller test case?
> > 
> > I'll run the trybot, but I think we'll still need it.
> > 
> > Unforuntately, the bug only reproduces if we have 3 giant gamma tables -
large
> > enough to overflow uint32_t when their sizes are added together.  And if the
> ICC
> > profile isn't large enough to contain the tables, we reject it early.  So we
> > need a really large ICC profile.
> 
> So you're saying it allocates 2 giant tables, then the third tips it over?
> 
> Can we write the parsing to check the sizes of all its allocations first
before
> doing any?

This is actually what we do - the large memory use is from the decompressed ICC
profile.  We allocate gammas in one block of memory - so we haven't allocated
anything yet.  The bug fix prevents us from trying to allocate if we overflow
when adding sizeof(gamma1) + sizeof(gamma2) + sizeof(gamma3).

FWIW, I intend to write another CL that rejects all tables that are greater than
<some large number of entries>.  That may incidentally also fix this bug, but I
think the overflow checks are important on their own.

[mtklein, 2016-08-17 12:55:17]

> This is actually what we do - the large memory use is from the decompressed
ICC
> profile.  We allocate gammas in one block of memory - so we haven't allocated
> anything yet.  The bug fix prevents us from trying to allocate if we overflow
> when adding sizeof(gamma1) + sizeof(gamma2) + sizeof(gamma3).
> 
> FWIW, I intend to write another CL that rejects all tables that are greater
than
> <some large number of entries>.  That may incidentally also fix this bug, but
I
> think the overflow checks are important on their own.

Is it possible to test this until then with something that's got large enough
gammas but small enough everything else that it doesn't have to be an unusual
test?  Then when the full-force reject logic comes in, you can switch back to
this (presumably wild?) .png.

[commit-bot: I haz the power, 2016-08-17 13:01:22]

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

[commit-bot: I haz the power, 2016-08-17 13:01:23]

Dry run: This issue passed the CQ dry run.

[msarett, 2016-08-17 13:08:40]

On 2016/08/17 12:55:17, mtklein wrote:
> > This is actually what we do - the large memory use is from the decompressed
> ICC
> > profile.  We allocate gammas in one block of memory - so we haven't
allocated
> > anything yet.  The bug fix prevents us from trying to allocate if we
overflow
> > when adding sizeof(gamma1) + sizeof(gamma2) + sizeof(gamma3).
> > 
> > FWIW, I intend to write another CL that rejects all tables that are greater
> than
> > <some large number of entries>.  That may incidentally also fix this bug,
but
> I
> > think the overflow checks are important on their own.
> 
> Is it possible to test this until then with something that's got large enough
> gammas but small enough everything else that it doesn't have to be an unusual
> test?  Then when the full-force reject logic comes in, you can switch back to
> this (presumably wild?) .png.

A smaller test would probably be fine to test the full force reject logic.

To test specifically this overflow bug, we need a large test.  I believe I could
trim the test down to less than 400 MB.  It will just be a bit tricky because
I'll need to generate a giant custom ICC profile, then encode it to PNG.  Does
that sound better?  Or we can just not have this test?  I don't feel strongly.

[mtklein, 2016-08-17 13:10:20]

On 2016/08/17 13:08:40, msarett wrote:
> On 2016/08/17 12:55:17, mtklein wrote:
> > > This is actually what we do - the large memory use is from the
decompressed
> > ICC
> > > profile.  We allocate gammas in one block of memory - so we haven't
> allocated
> > > anything yet.  The bug fix prevents us from trying to allocate if we
> overflow
> > > when adding sizeof(gamma1) + sizeof(gamma2) + sizeof(gamma3).
> > > 
> > > FWIW, I intend to write another CL that rejects all tables that are
greater
> > than
> > > <some large number of entries>.  That may incidentally also fix this bug,
> but
> > I
> > > think the overflow checks are important on their own.
> > 
> > Is it possible to test this until then with something that's got large
enough
> > gammas but small enough everything else that it doesn't have to be an
unusual
> > test?  Then when the full-force reject logic comes in, you can switch back
to
> > this (presumably wild?) .png.
> 
> A smaller test would probably be fine to test the full force reject logic.
> 
> To test specifically this overflow bug, we need a large test.  I believe I
could
> trim the test down to less than 400 MB.  It will just be a bit tricky because
> I'll need to generate a giant custom ICC profile, then encode it to PNG.  Does
> that sound better?  Or we can just not have this test?  I don't feel strongly.

I think I'm still not clear why we can't delay all allocation until after we've
decided it's safe.  If the profile is 4 or 700 MB, can we not add that into our
overflow check?

[Giwan Go, 2016-08-18 02:17:49]

On 2016/08/17 13:10:20, mtklein wrote:
> On 2016/08/17 13:08:40, msarett wrote:
> > On 2016/08/17 12:55:17, mtklein wrote:
> > > > This is actually what we do - the large memory use is from the
> decompressed
> > > ICC
> > > > profile.  We allocate gammas in one block of memory - so we haven't
> > allocated
> > > > anything yet.  The bug fix prevents us from trying to allocate if we
> > overflow
> > > > when adding sizeof(gamma1) + sizeof(gamma2) + sizeof(gamma3).
> > > > 
> > > > FWIW, I intend to write another CL that rejects all tables that are
> greater
> > > than
> > > > <some large number of entries>.  That may incidentally also fix this
bug,
> > but
> > > I
> > > > think the overflow checks are important on their own.
> > > 
> > > Is it possible to test this until then with something that's got large
> enough
> > > gammas but small enough everything else that it doesn't have to be an
> unusual
> > > test?  Then when the full-force reject logic comes in, you can switch back
> to
> > > this (presumably wild?) .png.
> > 
> > A smaller test would probably be fine to test the full force reject logic.
> > 
> > To test specifically this overflow bug, we need a large test.  I believe I
> could
> > trim the test down to less than 400 MB.  It will just be a bit tricky
because
> > I'll need to generate a giant custom ICC profile, then encode it to PNG. 
Does
> > that sound better?  Or we can just not have this test?  I don't feel
strongly.
> 
> I think I'm still not clear why we can't delay all allocation until after
we've
> decided it's safe.  If the profile is 4 or 700 MB, can we not add that into
our
> overflow check?

Please take a look at libqcms.
There is a good check code.
https://skia.googlesource.com/skia.git/+/master/third_party/qcms/src/iccread....

#define MAX_PROFILE_SIZE 1024*1024*4
#define MAX_TAG_COUNT 1024

[Giwan Go, 2016-08-18 02:25:02]

On 2016/08/18 02:17:49, GiWan Go wrote:
> On 2016/08/17 13:10:20, mtklein wrote:
> > On 2016/08/17 13:08:40, msarett wrote:
> > > On 2016/08/17 12:55:17, mtklein wrote:
> > > > > This is actually what we do - the large memory use is from the
> > decompressed
> > > > ICC
> > > > > profile.  We allocate gammas in one block of memory - so we haven't
> > > allocated
> > > > > anything yet.  The bug fix prevents us from trying to allocate if we
> > > overflow
> > > > > when adding sizeof(gamma1) + sizeof(gamma2) + sizeof(gamma3).
> > > > > 
> > > > > FWIW, I intend to write another CL that rejects all tables that are
> > greater
> > > > than
> > > > > <some large number of entries>.  That may incidentally also fix this
> bug,
> > > but
> > > > I
> > > > > think the overflow checks are important on their own.
> > > > 
> > > > Is it possible to test this until then with something that's got large
> > enough
> > > > gammas but small enough everything else that it doesn't have to be an
> > unusual
> > > > test?  Then when the full-force reject logic comes in, you can switch
back
> > to
> > > > this (presumably wild?) .png.
> > > 
> > > A smaller test would probably be fine to test the full force reject logic.
> > > 
> > > To test specifically this overflow bug, we need a large test.  I believe I
> > could
> > > trim the test down to less than 400 MB.  It will just be a bit tricky
> because
> > > I'll need to generate a giant custom ICC profile, then encode it to PNG. 
> Does
> > > that sound better?  Or we can just not have this test?  I don't feel
> strongly.
> > 
> > I think I'm still not clear why we can't delay all allocation until after
> we've
> > decided it's safe.  If the profile is 4 or 700 MB, can we not add that into
> our
> > overflow check?
> 
> Please take a look at libqcms.
> There is a good check code.
>
https://skia.googlesource.com/skia.git/+/master/third_party/qcms/src/iccread....
> 
> #define MAX_PROFILE_SIZE 1024*1024*4
> #define MAX_TAG_COUNT 1024

But, The 700MB of memory is still allocated in libpng.

[msarett, 2016-08-18 15:29:11]

On 2016/08/18 02:25:02, GiWan Go wrote:
> On 2016/08/18 02:17:49, GiWan Go wrote:
> > On 2016/08/17 13:10:20, mtklein wrote:
> > > On 2016/08/17 13:08:40, msarett wrote:
> > > > On 2016/08/17 12:55:17, mtklein wrote:
> > > > > > This is actually what we do - the large memory use is from the
> > > decompressed
> > > > > ICC
> > > > > > profile.  We allocate gammas in one block of memory - so we haven't
> > > > allocated
> > > > > > anything yet.  The bug fix prevents us from trying to allocate if we
> > > > overflow
> > > > > > when adding sizeof(gamma1) + sizeof(gamma2) + sizeof(gamma3).
> > > > > > 
> > > > > > FWIW, I intend to write another CL that rejects all tables that are
> > > greater
> > > > > than
> > > > > > <some large number of entries>.  That may incidentally also fix this
> > bug,
> > > > but
> > > > > I
> > > > > > think the overflow checks are important on their own.
> > > > > 
> > > > > Is it possible to test this until then with something that's got large
> > > enough
> > > > > gammas but small enough everything else that it doesn't have to be an
> > > unusual
> > > > > test?  Then when the full-force reject logic comes in, you can switch
> back
> > > to
> > > > > this (presumably wild?) .png.
> > > > 
> > > > A smaller test would probably be fine to test the full force reject
logic.
> > > > 
> > > > To test specifically this overflow bug, we need a large test.  I believe
I
> > > could
> > > > trim the test down to less than 400 MB.  It will just be a bit tricky
> > because
> > > > I'll need to generate a giant custom ICC profile, then encode it to PNG.

> > Does
> > > > that sound better?  Or we can just not have this test?  I don't feel
> > strongly.
> > > 
> > > I think I'm still not clear why we can't delay all allocation until after
> > we've
> > > decided it's safe.  If the profile is 4 or 700 MB, can we not add that
into
> > our
> > > overflow check?
> > 
> > Please take a look at libqcms.
> > There is a good check code.
> >
>
https://skia.googlesource.com/skia.git/+/master/third_party/qcms/src/iccread....
> > 
> > #define MAX_PROFILE_SIZE 1024*1024*4
> > #define MAX_TAG_COUNT 1024
> 
> But, The 700MB of memory is still allocated in libpng.

I don't feel strongly that we need a test for this specific code path, given
that we should be short-circuiting this error case with some sanity checks up
front.

I plan to right a CL that includes these sanity checks along with some (smaller)
test cases that exercise them.