limit the number of points in SkDashPathEffect::asPoints

limit the number of points in SkDashPathEffect::asPoints

If the length of a line path is sufficiently long relative to the dash
interval, it is possible to cause SkDashPathEffect::asPoints to produce
so many points that it overflows the amount that can fit in an int type,
or otherwise produce non-finite values, i.e. path from (0,0) to (0,9e15)
with a dash interval of 1.

This fixes that by capping the amount of points to a sane limit - in this
case, 1mil, since that limit is also used in utils/SkDashPath.cpp and has
precedent.

Downstream Firefox bug report: https://bugzilla.mozilla.org/show_bug.cgi?id=1287515

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

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

[lsalzman1, 2016-07-20 17:54:03]

Description was changed from

==========
limit the number of points in SkDashPathEffect::asPoints

If the length of a line path is sufficiently long relative to the dash
interval, it is possible to cause SkDashPathEffect::asPoints to produce
so many points that it overflows the amount that can fit in an int type,
or otherwise produce non-finite values, i.e. path from (0,0) to (0,9e15)
with a dash interval of 1.

This fixes that by capping the amount of points to a sane limit - in this
case, 1mil, since that limit is also used in utils/SkDashPath.cpp and has
precedent.

Downstream Firefox bug report:
https://bugzilla.mozilla.org/show_bug.cgi?id=1287515

BUG=skia:
==========

to

==========
limit the number of points in SkDashPathEffect::asPoints

If the length of a line path is sufficiently long relative to the dash
interval, it is possible to cause SkDashPathEffect::asPoints to produce
so many points that it overflows the amount that can fit in an int type,
or otherwise produce non-finite values, i.e. path from (0,0) to (0,9e15)
with a dash interval of 1.

This fixes that by capping the amount of points to a sane limit - in this
case, 1mil, since that limit is also used in utils/SkDashPath.cpp and has
precedent.

Downstream Firefox bug report:
https://bugzilla.mozilla.org/show_bug.cgi?id=1287515

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

[lsalzman1, 2016-07-20 17:55:00]

lsalzman@mozilla.com changed reviewers:
+ caryclark@google.com

[reed1, 2016-07-20 18:06:14]

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

[reed1, 2016-07-20 18:06:15]

https://codereview.chromium.org/2165013002/diff/1/src/effects/SkDashPathEffec...
File src/effects/SkDashPathEffect.cpp (right):

https://codereview.chromium.org/2165013002/diff/1/src/effects/SkDashPathEffec...
src/effects/SkDashPathEffect.cpp:251: // Too many midpoints can cause
results->fNumPoints to overflow or
Alternative: we could just return false if the value is too big, since this is
only an optimization routine to draw via points rather than the normal dashing
mechanism...?

[lsalzman1, 2016-07-20 18:11:43]

On 2016/07/20 18:06:15, reed1 wrote:
>
https://codereview.chromium.org/2165013002/diff/1/src/effects/SkDashPathEffec...
> File src/effects/SkDashPathEffect.cpp (right):
> 
>
https://codereview.chromium.org/2165013002/diff/1/src/effects/SkDashPathEffec...
> src/effects/SkDashPathEffect.cpp:251: // Too many midpoints can cause
> results->fNumPoints to overflow or
> Alternative: we could just return false if the value is too big, since this is
> only an optimization routine to draw via points rather than the normal dashing
> mechanism...?

That's pretty much what this does, since you need to know both the line length
and
the dash interval to predict how many points will actually get allocated. This
seemed
like the safest way to go about it.

[reed1, 2016-07-20 18:29:39]

Doh! You're right. I thought (dumbly) you were just clamping the count. My bad.

I defer to Cary.

[caryclark, 2016-07-20 19:35:33]

Two requests:
Can you move kMaxDashCount into SkDashPath so that both
SkDashPath::InternalFilter and SkDashPathEffect::asPoints can share the same
constant?

Can you add a unit test?

[lsalzman1, 2016-07-21 03:22:48]

On 2016/07/20 19:35:33, caryclark wrote:
> Two requests:
> Can you move kMaxDashCount into SkDashPath so that both
> SkDashPath::InternalFilter and SkDashPathEffect::asPoints can share the same
> constant?
> 
> Can you add a unit test?

Done.

[caryclark, 2016-07-21 12:22:59]

Thanks for the test. With comment addressed, LGTM

https://codereview.chromium.org/2165013002/diff/40001/src/utils/SkDashPath.cpp
File src/utils/SkDashPath.cpp (right):

https://codereview.chromium.org/2165013002/diff/40001/src/utils/SkDashPath.cp...
src/utils/SkDashPath.cpp:179: ptCount = SkTMin(ptCount,
SkDashPath::kMaxDashCount);
Don't need the SkDashPath:: prefix here

[lsalzman1, 2016-07-21 12:33:53]

On 2016/07/21 12:22:59, caryclark wrote:
> Thanks for the test. With comment addressed, LGTM
> 
> https://codereview.chromium.org/2165013002/diff/40001/src/utils/SkDashPath.cpp
> File src/utils/SkDashPath.cpp (right):
> 
>
https://codereview.chromium.org/2165013002/diff/40001/src/utils/SkDashPath.cp...
> src/utils/SkDashPath.cpp:179: ptCount = SkTMin(ptCount,
> SkDashPath::kMaxDashCount);
> Don't need the SkDashPath:: prefix here

The namespace prefix is necessary because that code is in SpecialLineRec,
which is not in the SkDashPath namespace nor is the SkDashPath namespace
imported into that file.

[caryclark, 2016-07-21 12:36:35]

On 2016/07/21 12:33:53, lsalzman1 wrote:
> On 2016/07/21 12:22:59, caryclark wrote:
> > Thanks for the test. With comment addressed, LGTM
> > 
> >
https://codereview.chromium.org/2165013002/diff/40001/src/utils/SkDashPath.cpp
> > File src/utils/SkDashPath.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2165013002/diff/40001/src/utils/SkDashPath.cp...
> > src/utils/SkDashPath.cpp:179: ptCount = SkTMin(ptCount,
> > SkDashPath::kMaxDashCount);
> > Don't need the SkDashPath:: prefix here
> 
> The namespace prefix is necessary because that code is in SpecialLineRec,
> which is not in the SkDashPath namespace nor is the SkDashPath namespace
> imported into that file.

OK, my mistake, LGTM

[lsalzman1, 2016-07-21 16:21:43]

The CQ bit was checked by lsalzman@mozilla.com

[commit-bot: I haz the power, 2016-07-21 16:21:49]

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-07-21 16:38:01]

Description was changed from

==========
limit the number of points in SkDashPathEffect::asPoints

If the length of a line path is sufficiently long relative to the dash
interval, it is possible to cause SkDashPathEffect::asPoints to produce
so many points that it overflows the amount that can fit in an int type,
or otherwise produce non-finite values, i.e. path from (0,0) to (0,9e15)
with a dash interval of 1.

This fixes that by capping the amount of points to a sane limit - in this
case, 1mil, since that limit is also used in utils/SkDashPath.cpp and has
precedent.

Downstream Firefox bug report:
https://bugzilla.mozilla.org/show_bug.cgi?id=1287515

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

to

==========
limit the number of points in SkDashPathEffect::asPoints

If the length of a line path is sufficiently long relative to the dash
interval, it is possible to cause SkDashPathEffect::asPoints to produce
so many points that it overflows the amount that can fit in an int type,
or otherwise produce non-finite values, i.e. path from (0,0) to (0,9e15)
with a dash interval of 1.

This fixes that by capping the amount of points to a sane limit - in this
case, 1mil, since that limit is also used in utils/SkDashPath.cpp and has
precedent.

Downstream Firefox bug report:
https://bugzilla.mozilla.org/show_bug.cgi?id=1287515

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

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

[commit-bot: I haz the power, 2016-07-21 16:38:02]

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