Implement a fuzzer for skia paths.

Implement a fuzzer for skia paths.

BUG=

Committed: https://crrev.com/cf781994211e4439ec06af08eacf01ffbfe9801f
Cr-Commit-Position: refs/heads/master@{#400461}

[Martin Barbella, 2016-06-01 23:34:13]

Description was changed from

==========
Implement a fuzzer for skia paths.

BUG=
==========

to

==========
Implement a fuzzer for skia paths.

BUG=
==========

[Martin Barbella, 2016-06-01 23:34:13]

mbarbella@chromium.org changed reviewers:
+ mmoroz@chromium.org

[Martin Barbella, 2016-06-01 23:43:36]

Max, would you mind doing an initial review of this?

https://codereview.chromium.org/2036523002/diff/20001/skia/tools/path_fuzzer/...
File skia/tools/path_fuzzer/path_fuzzer.cc (right):

https://codereview.chromium.org/2036523002/diff/20001/skia/tools/path_fuzzer/...
skia/tools/path_fuzzer/path_fuzzer.cc:15: template <typename T>
I'm assuming this comes up a lot in other fuzzers using libfuzzer. Do we have a
more general way to handle this? I suppose it's easy enough to implement that it
doesn't really matter.

https://codereview.chromium.org/2036523002/diff/20001/skia/tools/path_fuzzer/...
skia/tools/path_fuzzer/path_fuzzer.cc:27: SkPath path;
SkPath::readFromMemory actually looks takes data and a size, but it didn't seem
to work well here. Performance was quite a bit worse than this implementation,
though I'm not sure why, and I like being able to mix in other calls while
reading data. I'm also not totally sure that it can handle arbitrary input.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/s...
for reference.

[mmoroz, 2016-06-02 10:05:22]

https://codereview.chromium.org/2036523002/diff/20001/skia/tools/path_fuzzer/...
File skia/tools/path_fuzzer/path_fuzzer.cc (right):

https://codereview.chromium.org/2036523002/diff/20001/skia/tools/path_fuzzer/...
skia/tools/path_fuzzer/path_fuzzer.cc:15: template <typename T>
On 2016/06/01 23:43:36, Martin Barbella wrote:
> I'm assuming this comes up a lot in other fuzzers using libfuzzer. Do we have
a
> more general way to handle this? I suppose it's easy enough to implement that
it
> doesn't really matter.

I've seen a similar thing in some of networking fuzzers, but we don't have a
common solution for this.

https://codereview.chromium.org/2036523002/diff/40001/skia/BUILD.gn
File skia/BUILD.gn (right):

https://codereview.chromium.org/2036523002/diff/40001/skia/BUILD.gn#newcode764
skia/BUILD.gn:764: fuzzer_test("path_fuzzer") {
May be let's have more explicit name like "skia_path_fuzzer"?

https://codereview.chromium.org/2036523002/diff/40001/skia/tools/path_fuzzer/...
File skia/tools/path_fuzzer/path_fuzzer.cc (right):

https://codereview.chromium.org/2036523002/diff/40001/skia/tools/path_fuzzer/...
skia/tools/path_fuzzer/path_fuzzer.cc:17: if (*size < sizeof(value))
Shouldn't we use sizeof(T) here and below? sizeof(value) is a pointer size, not
size of a type.

https://codereview.chromium.org/2036523002/diff/40001/skia/tools/path_fuzzer/...
skia/tools/path_fuzzer/path_fuzzer.cc:26: extern "C" int
LLVMFuzzerTestOneInput(uint8_t* data, size_t size) {
We usually have "const uint8_t* data". I guess we don't change data bytes here
and can have "const" as well?

https://codereview.chromium.org/2036523002/diff/40001/skia/tools/path_fuzzer/...
skia/tools/path_fuzzer/path_fuzzer.cc:33: return 1;
AFAIK, target function should always return 0 for now. Other values are reserved
for future usage.

[mmoroz, 2016-06-02 10:13:39]

May we use an array or vector of |std::function<bool(uint8_t*, size_t,
SkPath&)>| instead of large switch statement and hardcoded constants, like:

while (read<uint8_t>(&data, &size, &operation)) {
    size_t offset = operation % functionsArray.size();
    if (functionsArray[offset](data, size, path))
        return 0;
}

I think it would be easier to extend and refactor then. However I'm not sure
what is the best way to pass |dst_path| and |paint| for case 9 and 10 - we may
pass them in all functions just to have a common signature or may use some
global pointers.

[mmoroz, 2016-06-02 10:18:38]

https://codereview.chromium.org/2036523002/diff/40001/skia/tools/path_fuzzer/...
File skia/tools/path_fuzzer/path_fuzzer.cc (right):

https://codereview.chromium.org/2036523002/diff/40001/skia/tools/path_fuzzer/...
skia/tools/path_fuzzer/path_fuzzer.cc:13: const int OPERATION_COUNT = 11;
Constant names are CamelCase with "k" prefix without underscores:
https://google.github.io/styleguide/cppguide.html#Constant_Names

[inferno, 2016-06-02 14:28:53]

inferno@chromium.org changed reviewers:
+ aizatsky@chromium.org, inferno@chromium.org, kcc@chromium.org

[inferno, 2016-06-02 14:28:54]

I am really excited to see api related sequence fuzzing than just calling one
function (previous libfuzzers). Lets check how well this works out.

[mmoroz, 2016-06-02 15:23:36]

On 2016/06/02 14:28:54, inferno wrote:
> I am really excited to see api related sequence fuzzing than just calling one
> function (previous libfuzzers). Lets check how well this works out.

I've just realized that corpus will not be "consistent" if we try to update this
fuzzer in future.

Let's say we have the following file in the corpus:
0xOB, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0xFF, ...

With current version it will go into |case 0:|, read two |SkSkalar| values and
call |path.lineTo(a, b);|. Then it will go to |case 2:| (0xFF % 11 = 2), and so
on.

Assume that then we add one more case into the switch. Instead of |case 0:| the
testcase above will go to that new |case 11:| which will differ from |case 0:|
and corpus will be parsed in other way. This won't not good, since libFuzzer
stores only interesting inputs and they should cause the same behavior in the
target over the time.

[Martin Barbella, 2016-06-02 16:14:09]

On 2016/06/02 15:23:36, mmoroz wrote:
> On 2016/06/02 14:28:54, inferno wrote:
> > I am really excited to see api related sequence fuzzing than just calling
one
> > function (previous libfuzzers). Lets check how well this works out.
> 
> I've just realized that corpus will not be "consistent" if we try to update
this
> fuzzer in future.
> 
> Let's say we have the following file in the corpus:
> 0xOB, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0xFF, ...
> 
> With current version it will go into |case 0:|, read two |SkSkalar| values and
> call |path.lineTo(a, b);|. Then it will go to |case 2:| (0xFF % 11 = 2), and
so
> on.
> 
> Assume that then we add one more case into the switch. Instead of |case 0:|
the
> testcase above will go to that new |case 11:| which will differ from |case 0:|
> and corpus will be parsed in other way. This won't not good, since libFuzzer
> stores only interesting inputs and they should cause the same behavior in the
> target over the time.

That's a good point. I'm not sure how much I want to complicate this for the
sake of backwards compatibility, though. I'll think about it before uploading
the next version.

I realized that a few of my recent changes actually seemed to hurt coverage (it
no longer finds the bug the initial version hit easily), so I need to rethink
some of this a bit. No need for anyone else to review this yet.

[Martin Barbella, 2016-06-02 19:48:24]

Thanks for the initial review. Addressed comments and got it back to the point
where it can at least find the one issue I mentioned, though I'm sure it can
still be improved.

I think this version also mostly addresses the concerns related to backwards
compatibility. I took another look at SkPath::readFromMemory, it's definitely
not a viable option. Calls using the path were moved to after path construction
and don't read anything else from |data|, and the format for constructing the
path shouldn't change (if it did, we'd likely have to throw the corpus away
regardless).

https://codereview.chromium.org/2036523002/diff/40001/skia/BUILD.gn
File skia/BUILD.gn (right):

https://codereview.chromium.org/2036523002/diff/40001/skia/BUILD.gn#newcode764
skia/BUILD.gn:764: fuzzer_test("path_fuzzer") {
On 2016/06/02 10:05:22, mmoroz wrote:
> May be let's have more explicit name like "skia_path_fuzzer"?

Done.

https://codereview.chromium.org/2036523002/diff/40001/skia/tools/path_fuzzer/...
File skia/tools/path_fuzzer/path_fuzzer.cc (right):

https://codereview.chromium.org/2036523002/diff/40001/skia/tools/path_fuzzer/...
skia/tools/path_fuzzer/path_fuzzer.cc:13: const int OPERATION_COUNT = 11;
On 2016/06/02 10:18:38, mmoroz wrote:
> Constant names are CamelCase with "k" prefix without underscores:
> https://google.github.io/styleguide/cppguide.html#Constant_Names

Done.

https://codereview.chromium.org/2036523002/diff/40001/skia/tools/path_fuzzer/...
skia/tools/path_fuzzer/path_fuzzer.cc:17: if (*size < sizeof(value))
On 2016/06/02 10:05:22, mmoroz wrote:
> Shouldn't we use sizeof(T) here and below? sizeof(value) is a pointer size,
not
> size of a type.

Done.

https://codereview.chromium.org/2036523002/diff/40001/skia/tools/path_fuzzer/...
skia/tools/path_fuzzer/path_fuzzer.cc:26: extern "C" int
LLVMFuzzerTestOneInput(uint8_t* data, size_t size) {
On 2016/06/02 10:05:22, mmoroz wrote:
> We usually have "const uint8_t* data". I guess we don't change data bytes here
> and can have "const" as well?

Done.

https://codereview.chromium.org/2036523002/diff/40001/skia/tools/path_fuzzer/...
skia/tools/path_fuzzer/path_fuzzer.cc:33: return 1;
On 2016/06/02 10:05:22, mmoroz wrote:
> AFAIK, target function should always return 0 for now. Other values are
reserved
> for future usage.

Done.

[mmoroz, 2016-06-03 13:52:14]

LGTM

Left a minor suggestion

https://codereview.chromium.org/2036523002/diff/100001/skia/tools/path_fuzzer...
File skia/tools/path_fuzzer/path_fuzzer.cc (right):

https://codereview.chromium.org/2036523002/diff/100001/skia/tools/path_fuzzer...
skia/tools/path_fuzzer/path_fuzzer.cc:127: auto
surface(SkSurface::MakeRasterN32Premul(w + 1, h + 1));
Since MakeRasterN32Premul() accepts int arguments, looks like there is no chance
to get an overflow with current data types. But just a suggestion to be totally
safe (e.g. when someone changes w and h types), may be w | 1 and h | 1?

[Martin Barbella, 2016-06-03 20:10:39]

I'm still playing around with this a bit locally. I'll add an owner for review
some time next week.

https://codereview.chromium.org/2036523002/diff/100001/skia/tools/path_fuzzer...
File skia/tools/path_fuzzer/path_fuzzer.cc (right):

https://codereview.chromium.org/2036523002/diff/100001/skia/tools/path_fuzzer...
skia/tools/path_fuzzer/path_fuzzer.cc:127: auto
surface(SkSurface::MakeRasterN32Premul(w + 1, h + 1));
On 2016/06/03 13:52:14, mmoroz wrote:
> Since MakeRasterN32Premul() accepts int arguments, looks like there is no
chance
> to get an overflow with current data types. But just a suggestion to be
totally
> safe (e.g. when someone changes w and h types), may be w | 1 and h | 1?

Went with x ? x : 1 instead in the extremely unlikely event that a bug requires
an even size. Good point, though.

[Martin Barbella, 2016-06-09 22:29:53]

mbarbella@chromium.org changed reviewers:
+ reed@google.com

[Martin Barbella, 2016-06-09 22:29:53]

reed: Would you mind taking a look or suggesting another reviewer?

I'm specifically looking for input on whether or not there are any other
interesting things we could do after creating the path, or if anything we're
doing now wouldn't be likely to lead to interesting coverage.

[Martin Barbella, 2016-06-09 22:30:39]

mbarbella@chromium.org changed reviewers:
- aizatsky@chromium.org, kcc@chromium.org

[reed1, 2016-06-10 13:32:26]

reed@google.com changed reviewers:
+ caryclark@google.com, kjlubick@google.com

[reed1, 2016-06-10 13:32:49]

I don't understand the comment/problem with readFromMemory. Can you explain?

[reed1, 2016-06-10 13:43:14]

Lets chat about this. The code reads fine, but Skia itself can't compile this,
so I think it should not live in skia's repo.

[Martin Barbella, 2016-06-10 17:29:06]

On 2016/06/10 13:43:14, reed1 wrote:
> Lets chat about this. The code reads fine, but Skia itself can't compile this,
> so I think it should not live in skia's repo.

I'm fine with moving this to testing/libfuzzer/fuzzers. Kostya, Mike: let me
know if you feel otherwise.

Regarding readFromMemory, it looks like it has a number of checks that would
fail in a debug build, which we may want to use for fuzzing at some point. If we
did this, those issues don't seem like they'd be particularly interesting. The
more immediate problem is that since it reads verbCount, pointCount, and
conicCount as uint32s, it can lead to some very large allocations which slow
down the fuzzing considerably, and can lead to a lot of non-interesting paths
where the counts don't line up properly. I can update the comment to be clearer.

[reed1, 2016-06-10 18:30:09]

Thanks for the explanation.

I ask mostly because we are fuzzing entire pictures, and those can contain
paths, and the paths are stored using writeToMemory. Thus we will be
fuzz-testing readMemory buffers ourselves, which I recognize is different /
separate from fuzz-testing the path API itself.

[aizatsky, 2016-06-10 18:37:47]

On 2016/06/10 at 17:29:06, mbarbella wrote:
> On 2016/06/10 13:43:14, reed1 wrote:
> > Lets chat about this. The code reads fine, but Skia itself can't compile
this,
> > so I think it should not live in skia's repo.
> 
> I'm fine with moving this to testing/libfuzzer/fuzzers. Kostya, Mike: let me
know if you feel otherwise.
> 

Ideally skia should pull these definitions the same way it gets //build/
configuration. What's the mechanism for that?

[Martin Barbella, 2016-06-17 17:01:52]

For now I've moved this to testing/libfuzzer.

[inferno, 2016-06-17 17:22:42]

lgtm.

I think we can improve this later by trying some more interesting values with
higher probability, e.g. a data point could be used to pick like 0, Max, Min and
also, we need to see how we can test on the nonfinite ones.

    const SkScalar inf = SK_ScalarInfinity;
    const SkScalar negInf = SK_ScalarNegativeInfinity;
    const SkScalar nan = SK_ScalarNaN;

[Martin Barbella, 2016-06-17 17:27:37]

The CQ bit was checked by mbarbella@chromium.org

[Martin Barbella, 2016-06-17 17:27:37]

The patchset sent to the CQ was uploaded after l-g-t-m from mmoroz@chromium.org
Link to the patchset: https://codereview.chromium.org/2036523002/#ps140001
(title: "Move to testing/libfuzzer")

[commit-bot: I haz the power, 2016-06-17 17:28:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2036523002/140001

[commit-bot: I haz the power, 2016-06-17 18:23:57]

Description was changed from

==========
Implement a fuzzer for skia paths.

BUG=
==========

to

==========
Implement a fuzzer for skia paths.

BUG=
==========

[commit-bot: I haz the power, 2016-06-17 18:23:58]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-06-17 18:25:34]

Description was changed from

==========
Implement a fuzzer for skia paths.

BUG=
==========

to

==========
Implement a fuzzer for skia paths.

BUG=

Committed: https://crrev.com/cf781994211e4439ec06af08eacf01ffbfe9801f
Cr-Commit-Position: refs/heads/master@{#400461}
==========

[commit-bot: I haz the power, 2016-06-17 18:25:35]

Patchset 8 (id:??) landed as
https://crrev.com/cf781994211e4439ec06af08eacf01ffbfe9801f
Cr-Commit-Position: refs/heads/master@{#400461}