Fix ImageFilter fuzzer issue

Fix ImageFilter fuzzer issue

What appears to be happening in this fuzz is that a paint index inside the picture of an SkPictureImageFilter is getting changed to be out of range.

BUG=skia:5192
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1893423002

Committed: https://skia.googlesource.com/skia/+/9ca06c4b00bfb9bb1a7f352efd264185e5a95fbc

[robertphillips, 2016-04-18 16:57:46]

Description was changed from

==========
Fix ImageFilter fuzzer issue

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

to

==========
Fix ImageFilter fuzzer issue

BUG=skia:5192
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[robertphillips, 2016-04-18 16:59:16]

Description was changed from

==========
Fix ImageFilter fuzzer issue

BUG=skia:5192
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Fix ImageFilter fuzzer issue

What appears to be happening in this fuzz is that a paint index inside the
picture of an SkPictureImageFilter is getting changed to be out of range.

BUG=skia:5192
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[robertphillips, 2016-04-18 16:59:16]

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

[robertphillips, 2016-04-18 17:00:15]

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

[commit-bot: I haz the power, 2016-04-18 17:00:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1893423002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1893423002/20001

[commit-bot: I haz the power, 2016-04-18 17:10:23]

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

[commit-bot: I haz the power, 2016-04-18 17:10:24]

Dry run: This issue passed the CQ dry run.

[reed1, 2016-04-18 17:53:49]

Wonder if we should change the param type to readbuffer, and also mark the
buffer as invalid if we encounter this.

[robertphillips, 2016-04-18 22:46:27]

PTAL, here is what a SkReadBuffer approach would look like

[reed1, 2016-04-19 12:54:11]

https://codereview.chromium.org/1893423002/diff/60001/src/core/SkPictureData.h
File src/core/SkPictureData.h (right):

https://codereview.chromium.org/1893423002/diff/60001/src/core/SkPictureData....
src/core/SkPictureData.h:92: if (index < 0 || index >= fBitmaps.count()) {
Seems fine, but a little repetitive.

One condensation could be:

return reader->validate(index < 0 || index >= fBitmaps.count()) ?
fBitmaps[index] : fEmptyBitmap;

And/or, we might add a helper to ReadBuffer...

bool validateIndex(int index, int count) so it will just read

return reader->validateIndex(index, fBitmaps.count()) ? fBitmaps[index] :
fEmptyBitmap;

https://codereview.chromium.org/1893423002/diff/60001/src/core/SkPicturePlayb...
File src/core/SkPicturePlayback.cpp (right):

https://codereview.chromium.org/1893423002/diff/60001/src/core/SkPicturePlayb...
src/core/SkPicturePlayback.cpp:112: void
SkPicturePlayback::handleOp(SkReadBuffer* reader,
If it is easier (not saying either way), you can always extract the "Reader32"
from reader above the switch, and use it in each clause. That will reduce the
diff, but not necessarily make for better code per-se.

https://codereview.chromium.org/1893423002/diff/60001/src/core/SkReadBuffer.h
File src/core/SkReadBuffer.h (right):

https://codereview.chromium.org/1893423002/diff/60001/src/core/SkReadBuffer.h...
src/core/SkReadBuffer.h:212: virtual void makeInvalid() { }
is this still needed?

https://codereview.chromium.org/1893423002/diff/60001/src/core/SkValidatingRe...
File src/core/SkValidatingReadBuffer.h (right):

https://codereview.chromium.org/1893423002/diff/60001/src/core/SkValidatingRe...
src/core/SkValidatingReadBuffer.h:66: void makeInvalid() override { fError =
true; }
needed?

[robertphillips, 2016-04-19 14:34:42]

https://codereview.chromium.org/1893423002/diff/60001/src/core/SkPictureData.h
File src/core/SkPictureData.h (right):

https://codereview.chromium.org/1893423002/diff/60001/src/core/SkPictureData....
src/core/SkPictureData.h:92: if (index < 0 || index >= fBitmaps.count()) {
On 2016/04/19 12:54:11, reed1 wrote:
> Seems fine, but a little repetitive.
> 
> One condensation could be:
> 
> return reader->validate(index < 0 || index >= fBitmaps.count()) ?
> fBitmaps[index] : fEmptyBitmap;
> 
> And/or, we might add a helper to ReadBuffer...
> 
> bool validateIndex(int index, int count) so it will just read
> 
> return reader->validateIndex(index, fBitmaps.count()) ? fBitmaps[index] :
> fEmptyBitmap;

Done.

https://codereview.chromium.org/1893423002/diff/60001/src/core/SkPicturePlayb...
File src/core/SkPicturePlayback.cpp (right):

https://codereview.chromium.org/1893423002/diff/60001/src/core/SkPicturePlayb...
src/core/SkPicturePlayback.cpp:112: void
SkPicturePlayback::handleOp(SkReadBuffer* reader,
On 2016/04/19 12:54:11, reed1 wrote:
> If it is easier (not saying either way), you can always extract the "Reader32"
> from reader above the switch, and use it in each clause. That will reduce the
> diff, but not necessarily make for better code per-se.

In this instance I think we should let the SkReadBuffer have its way so that all
its hardening can take effect.

https://codereview.chromium.org/1893423002/diff/60001/src/core/SkReadBuffer.h
File src/core/SkReadBuffer.h (right):

https://codereview.chromium.org/1893423002/diff/60001/src/core/SkReadBuffer.h...
src/core/SkReadBuffer.h:212: virtual void makeInvalid() { }
On 2016/04/19 12:54:11, reed1 wrote:
> is this still needed?

Done.

https://codereview.chromium.org/1893423002/diff/60001/src/core/SkValidatingRe...
File src/core/SkValidatingReadBuffer.h (right):

https://codereview.chromium.org/1893423002/diff/60001/src/core/SkValidatingRe...
src/core/SkValidatingReadBuffer.h:66: void makeInvalid() override { fError =
true; }
On 2016/04/19 12:54:11, reed1 wrote:
> needed?

Done.

[robertphillips, 2016-04-20 11:39:23]

ping

[reed1, 2016-04-20 13:26:37]

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

[reed1, 2016-04-20 13:26:38]

+matt who is also tweaking buffers for flattenables

https://codereview.chromium.org/1893423002/diff/80001/src/core/SkPicturePlayb...
File src/core/SkPicturePlayback.cpp (right):

https://codereview.chromium.org/1893423002/diff/80001/src/core/SkPicturePlayb...
src/core/SkPicturePlayback.cpp:148: reader->readRect(&rect);
I presume we don't think the efficiency of skip is worth keeping it? (here and
below)

https://codereview.chromium.org/1893423002/diff/80001/src/core/SkReadBuffer.h
File src/core/SkReadBuffer.h (right):

https://codereview.chromium.org/1893423002/diff/80001/src/core/SkReadBuffer.h...
src/core/SkReadBuffer.h:105: void setOffset(size_t offset) {
fReader.setOffset(offset); }
who needs this, why?

[robertphillips, 2016-04-20 17:09:40]

https://codereview.chromium.org/1893423002/diff/80001/src/core/SkPicturePlayb...
File src/core/SkPicturePlayback.cpp (right):

https://codereview.chromium.org/1893423002/diff/80001/src/core/SkPicturePlayb...
src/core/SkPicturePlayback.cpp:148: reader->readRect(&rect);
On 2016/04/20 13:26:37, reed1 wrote:
> I presume we don't think the efficiency of skip is worth keeping it? (here and
> below)

I'm actually surprised skip isn't protected in SkReadBuffer. AFAICT its only
class-external use is in SkPaint::unflatten and even there it seems a bit dodgy.

https://codereview.chromium.org/1893423002/diff/80001/src/core/SkReadBuffer.h
File src/core/SkReadBuffer.h (right):

https://codereview.chromium.org/1893423002/diff/80001/src/core/SkReadBuffer.h...
src/core/SkReadBuffer.h:105: void setOffset(size_t offset) {
fReader.setOffset(offset); }
On 2016/04/20 13:26:37, reed1 wrote:
> who needs this, why?

SkPicturePlayback::handleOp uses it as an optimization to skip to the restore
operation if everything is clipped out (for clipRect, clipPath, clipRRect and
clipRegion).

I have added a validating override.

[robertphillips, 2016-04-20 18:03:49]

PTAL - now w/o setOffset

[reed1, 2016-04-20 18:15:29]

lgtm

matt, do you see any conflicts etc. w.r.t. your CL on flattenables?

[msarett, 2016-04-20 18:16:35]

Looks fine to me lgtm

[robertphillips, 2016-04-20 18:32:09]

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

[commit-bot: I haz the power, 2016-04-20 18:32:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1893423002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1893423002/120001

[commit-bot: I haz the power, 2016-04-20 18:41:51]

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

[commit-bot: I haz the power, 2016-04-20 18:41:52]

Dry run: This issue passed the CQ dry run.

[robertphillips, 2016-04-20 18:42:32]

The CQ bit was checked by robertphillips@google.com

[commit-bot: I haz the power, 2016-04-20 18:42:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1893423002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1893423002/120001

[commit-bot: I haz the power, 2016-04-20 18:43:35]

Description was changed from

==========
Fix ImageFilter fuzzer issue

What appears to be happening in this fuzz is that a paint index inside the
picture of an SkPictureImageFilter is getting changed to be out of range.

BUG=skia:5192
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Fix ImageFilter fuzzer issue

What appears to be happening in this fuzz is that a paint index inside the
picture of an SkPictureImageFilter is getting changed to be out of range.

BUG=skia:5192
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/9ca06c4b00bfb9bb1a7f352efd264185e5a95fbc
==========

[commit-bot: I haz the power, 2016-04-20 18:43:36]

Committed patchset #7 (id:120001) as
https://skia.googlesource.com/skia/+/9ca06c4b00bfb9bb1a7f352efd264185e5a95fbc