Add fuzzer for (A)PNG decoder

third_party/WebKit/Source/platform/PngFuzzer.cpp

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+

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

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.

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

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.

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

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.

+// Compile with:
+// gn gen out/Fuzz '--args=use_libfuzzer=true is_asan=true
+// is_debug=false is_ubsan_security=true' --check
+// ninja -C out/Fuzz png_fuzzer
+//
+// Run with:
+// ./out/Fuzz/png_fuzzer third_party/WebKit/LayoutTests/images/resources/pngfuzz

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

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`.

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

Added a comment regarding this.

+//
+// For more details, see
+// https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/README.md
+
+#include "platform/image-decoders/png/PNGImageDecoder.cpp"
+#include "platform/testing/BlinkFuzzerTestSupport.h"
+#include "public/platform/WebIconSizesParser.h"
+#include "public/platform/WebSize.h"
+#include "public/platform/WebString.h"

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

nit: do you need the following includes?

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

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

No. Removed.

+
+namespace blink {
+
+std::unique_ptr<ImageDecoder> createDecoder(
+    ImageDecoder::AlphaOption alphaOption) {
+  return WTF::wrapUnique(new PNGImageDecoder(
+      alphaOption, ColorBehavior::transformToTargetForTesting(),

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

/me curious: do any of your seed images have a color profile?

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

oval.png is sRGB. The rest do not.

+      ImageDecoder::noDecodedImageByteLimit));
+}
+
+std::unique_ptr<ImageDecoder> createDecoder() {
+  return createDecoder(ImageDecoder::AlphaNotPremultiplied);

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

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?

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

Good point. Will default to ImageDecoder::AlphaPremultiplied, to match Blink.
I'm sure there's a way to make cluster fuzz test both. Added a TODO for now.
Will investigate.
Agreed. Done.

+}
+
+// This function will be called by ClusterFuzz. If this does not crash, the
+// test passes. It parses the frame count and then tries to decode each frame
+// in the image.

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

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?

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

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]

Removed the comment.

+int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
+  auto buffer = SharedBuffer::create(data, size);
+  auto decoder = createDecoder();
+  decoder->setData(buffer.get(), true);

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

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.

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

Done.

+  decoder->frameCount();
+  if (decoder->failed())
+    return 0;
+  for (size_t frame = 0; frame < decoder->frameCount(); frame++) {
+    decoder->frameBufferAtIndex(frame);
+    if (decoder->failed())
+      return 0;
+  }
+  return 0;
+}
+
+}  // namespace blink
+
+extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
+  return blink::LLVMFuzzerTestOneInput(data, size);
+}
+
+extern "C" int LLVMFuzzerInitialize(int* argc, char*** argv) {
+  blink::InitializeBlinkFuzzTest(argc, argv);
+  return 0;
+}