Add fuzzer for (A)PNG decoder

Add fuzzer for (A)PNG decoder

Implement a fuzzer for the PNG decoder. It uses three animated png images
which I created myself, and three existing static PNG images in WebKits
LayoutTest resources as a seed corpus.

It works for both the current PNGImageDecoder, which only supports
decoding static PNGs, and the new PNGImageDecoder which also supports
decoding animated PNGs. This is achieved by having both static and
animated PNG images in the seed corpus.

BUG=437662

[joostouwerling, 2016-12-15 21:27:38]

Description was changed from

==========
Implement fuzzer for APNG decoder
==========

to

==========
Implement a fuzzer for APNG decoder
==========

[joostouwerling, 2016-12-15 21:28:18]

Description was changed from

==========
Implement a fuzzer for APNG decoder
==========

to

==========
Implement a fuzzer for the animated PNG decoder. It uses three images
which I created myself as a seed corpus.
==========

[joostouwerling, 2016-12-15 21:29:37]

Description was changed from

==========
Implement a fuzzer for the animated PNG decoder. It uses three images
which I created myself as a seed corpus.
==========

to

==========
Implement a fuzzer for the animated PNG decoder. It uses three images
which I created myself as a seed corpus.

BUG=437662
==========

[joostouwerling, 2016-12-15 21:29:52]

joostouwerling@google.com changed reviewers:
+ scroggo@chromium.org

[joostouwerling, 2016-12-15 21:32:16]

Description was changed from

==========
Implement a fuzzer for the animated PNG decoder. It uses three images
which I created myself as a seed corpus.

BUG=437662
==========

to

==========
Implement a fuzzer for the animated PNG decoder. It uses three images
which I created myself as a seed corpus.

It also works for the current PNGImageDecoder, since APNG chunks will
be ignored by the current decoder. But the seed corpus only contains
animated png images.

BUG=437662
==========

[joostouwerling, 2016-12-16 19:11:53]

Description was changed from

==========
Implement a fuzzer for the animated PNG decoder. It uses three images
which I created myself as a seed corpus.

It also works for the current PNGImageDecoder, since APNG chunks will
be ignored by the current decoder. But the seed corpus only contains
animated png images.

BUG=437662
==========

to

==========
Implement a fuzzer for the PNG decoder. It uses three animated png images
which I created myself, and three existing static PNG images in WebKits
LayoutTest resources as a seed corpus.

It works for both the current PNGImageDecoder, which only supports decoding
static PNGs, and the new PNGImageDecoder which also supports decoding animated
PNGs. This is achieved by having both static and animated PNG images in the seed
corpus.

BUG=437662
==========

[joostouwerling, 2016-12-16 19:12:08]

Description was changed from

==========
Implement a fuzzer for the PNG decoder. It uses three animated png images
which I created myself, and three existing static PNG images in WebKits
LayoutTest resources as a seed corpus.

It works for both the current PNGImageDecoder, which only supports decoding
static PNGs, and the new PNGImageDecoder which also supports decoding animated
PNGs. This is achieved by having both static and animated PNG images in the seed
corpus.

BUG=437662
==========

to

==========
Implement a fuzzer for the PNG decoder. It uses three animated png images
which I created myself, and three existing static PNG images in WebKits
LayoutTest resources as a seed corpus.

It works for both the current PNGImageDecoder, which only supports
decoding static PNGs, and the new PNGImageDecoder which also supports
decoding animated PNGs. This is achieved by having both static and
animated PNG images in the seed corpus.

BUG=437662
==========

[joostouwerling, 2016-12-16 19:55:56]

joostouwerling@google.com changed reviewers:
+ noel@chromium.org

[joostouwerling, 2016-12-16 19:55:56]

Adding noel@ for review.

[Noel Gordon, 2017-01-03 02:43:50]

Description was changed from

==========
Implement a fuzzer for the PNG decoder. It uses three animated png images
which I created myself, and three existing static PNG images in WebKits
LayoutTest resources as a seed corpus.

It works for both the current PNGImageDecoder, which only supports
decoding static PNGs, and the new PNGImageDecoder which also supports
decoding animated PNGs. This is achieved by having both static and
animated PNG images in the seed corpus.

BUG=437662
==========

to

==========
Add fuzzer for (A)PNG decoder

Implement a fuzzer for the PNG decoder. It uses three animated png images
which I created myself, and three existing static PNG images in WebKits
LayoutTest resources as a seed corpus.

It works for both the current PNGImageDecoder, which only supports
decoding static PNGs, and the new PNGImageDecoder which also supports
decoding animated PNGs. This is achieved by having both static and
animated PNG images in the seed corpus.

BUG=437662
==========

[Noel Gordon, 2017-01-03 04:12:39]

Thanks for working on this.  Some minor matters:

https://codereview.chromium.org/2578263002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/PngFuzzer.cpp (right):

https://codereview.chromium.org/2578263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/PngFuzzer.cpp:4: 
location: third_party/WebKit/Source/platform/PngFuzzer.cpp

Would it makes sense to put this fuzzer in directory
third_party/WebKit/Source/platform/image-decoders ?

That could be done as a TODO if we decide to go that way.

https://codereview.chromium.org/2578263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/PngFuzzer.cpp:20: #include
"public/platform/WebString.h"
nit: do you need the following includes?

+#include "public/platform/WebIconSizesParser.h"
+#include "public/platform/WebSize.h"
+#include "public/platform/WebString.h"

https://codereview.chromium.org/2578263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/PngFuzzer.cpp:27: alphaOption,
ColorBehavior::transformToTargetForTesting(),
/me curious: do any of your seed images have a color profile?

https://codereview.chromium.org/2578263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/PngFuzzer.cpp:32: return
createDecoder(ImageDecoder::AlphaNotPremultiplied);
The default code path used by Blink would be _ImageDecoder::AlphaPremultiplied_.
 Could we fuzz that code path too, somehow?

Also have createDecoder() and and override of it, threw me on initial reading. 
Perhaps ditch createDecoder(), and call createDecoder(alphaOption) version
directly from LLVMFuzzerTestOneInput?

https://codereview.chromium.org/2578263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/PngFuzzer.cpp:37: // in the image.
Not sure about the value of this comment.  "It parses the frame count and then
tries to decode each frame
in the image." just repeats what the code does, for example.

"If this does not crash," ... made me curious: why would this fuzz test crash?

https://codereview.chromium.org/2578263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/PngFuzzer.cpp:41:
decoder->setData(buffer.get(), true);
optional: up to you, but I usually write this as

   bool allDataReceived = true;
   decoder->setData(buffer.get(), allDataReceived);

to remind me that I'm sending all the encode image data into the image decoder.

[mmoroz, 2017-01-03 08:18:36]

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

[mmoroz, 2017-01-03 08:18:36]

Left some suggestions / comments, LGTM from fuzzing side :)

https://codereview.chromium.org/2578263002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/BUILD.gn (right):

https://codereview.chromium.org/2578263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/BUILD.gn:2021: fuzzer_test("png_fuzzer") {
Would you mind naming it a bit more explicitly? For example, `blink_png_fuzzer`,
or `png_image_decoder_fuzzer`? I'm asking, because we have many fuzz targets.
Giving short names to them will collide soon (For example, we have
`libpng_read_fuzzer` which also could be named `png_fuzzer`, but we are tending
to have specific names to avoid ambiguity).

https://codereview.chromium.org/2578263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/BUILD.gn:2030: seed_corpus =
"//third_party/WebKit/LayoutTests/images/resources/pngfuzz"
That's awesome that you're adding a seed corpus. I suggest you also add a
dictionary. You can use existing png.dict used by some other fuzzers
(https://cs.chromium.org/chromium/src/testing/libfuzzer/fuzzers/BUILD.gn?q=png...).

https://codereview.chromium.org/2578263002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/PngFuzzer.cpp (right):

https://codereview.chromium.org/2578263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/PngFuzzer.cpp:11: // ./out/Fuzz/png_fuzzer
third_party/WebKit/LayoutTests/images/resources/pngfuzz
This command will write new files into
`third_party/WebKit/LayoutTests/images/resources/pngfuzz`. To keep it
consistent, we can also run it with:

`./out/Fuzz/png_fuzzer ~/another_dir_to_store_corpus
third_party/WebKit/LayoutTests/images/resources/pngfuzz`, so the fuzzer will
read both directories passed, but all new generated testcases will go into
`~/another_dir_to_store_corpus`.

https://codereview.chromium.org/2578263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/PngFuzzer.cpp:37: // in the image.
On 2017/01/03 04:12:39, noel gordon wrote:
> Not sure about the value of this comment.  "It parses the frame count and then
> tries to decode each frame
> in the image." just repeats what the code does, for example.
> 
> "If this does not crash," ... made me curious: why would this fuzz test crash?

Technically, this function will be called by libFuzzer (a fuzzing engine).
ClusterFuzz is an infrastructure running and managing all Chrome fuzzers,
including libFuzzer-based ones (like this one).


This fuzz test will crash if there is a bug in decoder that can be triggered
with some input data.

[scroggo_chromium, 2017-01-03 18:24:15]

Joost's internship has ended, so I am picking up where he left off. I've
uploaded a new issue at https://codereview.chromium.org/2603303003/. Patch set 1
looks just like this one, and patch set 2 addresses the comments here.

https://codereview.chromium.org/2578263002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/BUILD.gn (right):

https://codereview.chromium.org/2578263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/BUILD.gn:2021: fuzzer_test("png_fuzzer") {
On 2017/01/03 08:18:36, mmoroz wrote:
> Would you mind naming it a bit more explicitly? For example,
`blink_png_fuzzer`,
> or `png_image_decoder_fuzzer`? I'm asking, because we have many fuzz targets.
> Giving short names to them will collide soon (For example, we have
> `libpng_read_fuzzer` which also could be named `png_fuzzer`, but we are
tending
> to have specific names to avoid ambiguity).

Done.

https://codereview.chromium.org/2578263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/BUILD.gn:2030: seed_corpus =
"//third_party/WebKit/LayoutTests/images/resources/pngfuzz"
On 2017/01/03 08:18:36, mmoroz wrote:
> That's awesome that you're adding a seed corpus. I suggest you also add a
> dictionary. You can use existing png.dict used by some other fuzzers
>
(https://cs.chromium.org/chromium/src/testing/libfuzzer/fuzzers/BUILD.gn?q=png...).
> 

Done. Should I also add "//cc/test/data" to the seed_corpus? (Is there a way to
have multiple seed corpora?)

https://codereview.chromium.org/2578263002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/PngFuzzer.cpp (right):

https://codereview.chromium.org/2578263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/PngFuzzer.cpp:4: 
On 2017/01/03 04:12:39, noel gordon wrote:
> location: third_party/WebKit/Source/platform/PngFuzzer.cpp
> 
> Would it makes sense to put this fuzzer in directory
> third_party/WebKit/Source/platform/image-decoders ?
> 
> That could be done as a TODO if we decide to go that way.

I do not have a strong preference. Added the TODO. If we decide to go that way
before submitting, I'll upload that change as its own patch set.

https://codereview.chromium.org/2578263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/PngFuzzer.cpp:11: // ./out/Fuzz/png_fuzzer
third_party/WebKit/LayoutTests/images/resources/pngfuzz
On 2017/01/03 08:18:36, mmoroz wrote:
> This command will write new files into
> `third_party/WebKit/LayoutTests/images/resources/pngfuzz`. To keep it
> consistent, we can also run it with:
> 
> `./out/Fuzz/png_fuzzer ~/another_dir_to_store_corpus
> third_party/WebKit/LayoutTests/images/resources/pngfuzz`, so the fuzzer will
> read both directories passed, but all new generated testcases will go into
> `~/another_dir_to_store_corpus`.
> 

Added a comment regarding this.

https://codereview.chromium.org/2578263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/PngFuzzer.cpp:20: #include
"public/platform/WebString.h"
On 2017/01/03 04:12:39, noel gordon wrote:
> nit: do you need the following includes?
> 
> +#include "public/platform/WebIconSizesParser.h"
> +#include "public/platform/WebSize.h"
> +#include "public/platform/WebString.h"

No. Removed.

https://codereview.chromium.org/2578263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/PngFuzzer.cpp:27: alphaOption,
ColorBehavior::transformToTargetForTesting(),
On 2017/01/03 04:12:39, noel gordon wrote:
> /me curious: do any of your seed images have a color profile?

oval.png is sRGB. The rest do not.

https://codereview.chromium.org/2578263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/PngFuzzer.cpp:32: return
createDecoder(ImageDecoder::AlphaNotPremultiplied);
On 2017/01/03 04:12:39, noel gordon wrote:
> The default code path used by Blink would be
_ImageDecoder::AlphaPremultiplied_.

Good point. Will default to ImageDecoder::AlphaPremultiplied, to match Blink.

>  Could we fuzz that code path too, somehow?

I'm sure there's a way to make cluster fuzz test both. Added a TODO for now.
Will investigate.

> 
> Also have createDecoder() and and override of it, threw me on initial reading.

> Perhaps ditch createDecoder(), and call createDecoder(alphaOption) version
> directly from LLVMFuzzerTestOneInput?

Agreed. Done.

https://codereview.chromium.org/2578263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/PngFuzzer.cpp:37: // in the image.
On 2017/01/03 08:18:36, mmoroz wrote:
> On 2017/01/03 04:12:39, noel gordon wrote:
> > Not sure about the value of this comment.  "It parses the frame count and
then
> > tries to decode each frame
> > in the image." just repeats what the code does, for example.
> > 
> > "If this does not crash," ... made me curious: why would this fuzz test
crash?
> 
> Technically, this function will be called by libFuzzer (a fuzzing engine).
> ClusterFuzz is an infrastructure running and managing all Chrome fuzzers,
> including libFuzzer-based ones (like this one).
> 
> 
> This fuzz test will crash if there is a bug in decoder that can be triggered
> with some input data.

Removed the comment.

https://codereview.chromium.org/2578263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/PngFuzzer.cpp:41:
decoder->setData(buffer.get(), true);
On 2017/01/03 04:12:39, noel gordon wrote:
> optional: up to you, but I usually write this as
> 
>    bool allDataReceived = true;
>    decoder->setData(buffer.get(), allDataReceived);
> 
> to remind me that I'm sending all the encode image data into the image
decoder.

Done.

[Noel Gordon, 2017-01-03 23:57:16]

https://codereview.chromium.org/2578263002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/PngFuzzer.cpp (right):

https://codereview.chromium.org/2578263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/PngFuzzer.cpp:4: 
On 2017/01/03 18:24:15, scroggo_chromium wrote:
> On 2017/01/03 04:12:39, noel gordon wrote:
> > location: third_party/WebKit/Source/platform/PngFuzzer.cpp
> > 
> > Would it makes sense to put this fuzzer in directory
> > third_party/WebKit/Source/platform/image-decoders ?
> > 
> > That could be done as a TODO if we decide to go that way.
> 
> I do not have a strong preference. Added the TODO. If we decide to go that way
> before submitting, I'll upload that change as its own patch set.

My thinking was if more fuzzers are coming / are being added, then a random
collection of fuzzer test source files end up living in Platform root,
third_party/WebKit/Source/platform  ...  Anyho, there's a TODO about it, which
is fine by me.

[Noel Gordon, 2017-01-03 23:59:02]

On 2017/01/03 18:24:15, scroggo_chromium wrote:
> Joost's internship has ended, so I am picking up where he left off. I've
> uploaded a new issue at https://codereview.chromium.org/2603303003/.

Right, thanks for picking it up.  Over to the new patch ...