Add ability to fuzz images and skps to fuzz binary

fuzz/fuzz.cpp

Index: fuzz/fuzz.cpp
diff --git a/fuzz/fuzz.cpp b/fuzz/fuzz.cpp
index 343e25b45303527a8a5cbfb5cc33bcebfb747afa..037c3b8a546c807929deeb428e8c26b8eea0e96a 100644
--- a/fuzz/fuzz.cpp
+++ b/fuzz/fuzz.cpp
@@ -6,33 +6,160 @@
  */
 
 #include "Fuzz.h"
+#include "SkCanvas.h"
+#include "SkCodec.h"
 #include "SkCommandLineFlags.h"
+#include "SkData.h"
+#include "SkImage.h"
+#include "SkImageEncoder.h"
+#include "SkMallocPixelRef.h"
+#include "SkPicture.h"
+#include "SkStream.h"
+
 #include <signal.h>
 #include <stdlib.h>
+#include <cmath>
 
-DEFINE_string2(bytes, b, "", "A path to a file containing fuzzed bytes.");
+DEFINE_string2(bytes, b, "", "A path to a file.");

[mtklein, 2016/01/20 14:49:06]

?  Do they not contain fuzzed bytes any more?

[kjlubick, 2016/01/20 19:21:58]

Done.

 DEFINE_string2(match, m, "", "The usual --match, applied to DEF_FUZZ names.");
 
+DEFINE_string(mode, "api", "How to interpret --bytes, either 'image', 'skp', or 'api'.");

[mtklein, 2016/01/20 14:49:06]

We may want to make this type, not mode, so that we can give it a short version
too (-t)?

[kjlubick, 2016/01/20 19:21:58]

Done.

+DEFINE_string(dump, "", "If not empty, dump 'image' or 'skp' modes as a PNG with this name.");
+
+void printUsage(char*);

[mtklein, 2016/01/20 14:49:06]

It is a good habit to get into make functions (really, all globals) static when
possible.  As you've written it here, you're giving off an implicit signal that
these functions are defined in another file.

[mtklein, 2016/01/20 14:49:06]

It's another good habit to take your arguments with as little power as needed. 
Here, you can pass argv[0] as a const char*.

[kjlubick, 2016/01/20 19:21:58]

Done.

+int runSingleTest(SkData*);

[mtklein, 2016/01/20 14:49:06]

Let's make these more symmetric:

static int fuzz_api(SkData*);
static int fuzz_img(SkData*);
static int fuzz_skp(SkData*);

You can interpret an SkData as an SkStream using SkMemoryStream.

[kjlubick, 2016/01/20 19:21:58]

Done.

+int decodeImage(SkData*);
+int decodeSkp();
+
 int main(int argc, char** argv) {
     SkCommandLineFlags::Parse(argc, argv);
 
-    if (FLAGS_bytes.isEmpty()) {

[mtklein, 2016/01/20 14:49:06]

I think you need to rebase your patch.

Are you intentionally removing the --bytes == this binary default?

[kjlubick, 2016/01/20 19:21:58]

Whoops, just needed a rebase.

-        SkDebugf("Usage: %s -b <path/to/fuzzed.data> [-m pattern]\n", argv[0]);
+    if (FLAGS_bytes.isEmpty() || FLAGS_mode.isEmpty()) {
+        printUsage(argv[0]);

[mtklein, 2016/01/20 14:49:06]

You might consider writing this as:

static int usage(const char* argv0) {
   SkDebugf("...", argv0);
   return 1;
}

so you can just write 
   return usage();

[scroggo, 2016/01/20 18:46:19]

SkCommandLineFlags has a "usage" string, but it only gets printed if you run
your program with "-h" or "--help". Maybe there should be an option to print it
from code?

[kjlubick, 2016/01/20 19:21:57]

That would be nice.

[kjlubick, 2016/01/20 19:21:58]

Done.

         return 1;
     }
+
+    if (0 == strcmp(FLAGS_mode[0], "skp")) {
+        return decodeSkp();
+    }
+
     SkAutoTUnref<SkData> bytes(SkData::NewFromFileName(FLAGS_bytes[0]));
+    if (!bytes) {
+        SkDebugf("Could not read %s\n", FLAGS_bytes[0]);
+        return 2;
+    }
+    if (0 == strcmp(FLAGS_mode[0], "api")) {

[mtklein, 2016/01/20 14:49:06]

This is fine, but we probably don't need to be this precise.  E.g.

switch (FLAGS_mode[0][0]) {
   case 'a': return fuzz_api(bytes);
   case 'c':
   case 'i': return fuzz_img(bytes);
   case 's': return fuzz_skp(bytes);
}

return usage();

[kjlubick, 2016/01/20 19:21:58]

Done, although I'm not sure what the c is for.

+        return runSingleTest(bytes);
+    } else if (0 == strcmp(FLAGS_mode[0], "image")) {
+        return decodeImage(bytes);
+    }
+    printUsage(argv[0]);
+    return 1;
+}
 
+void printUsage(char* name) {
+    SkDebugf("Usage: %s --mode <mode> -b <path/to/file> [-m pattern]\n", name);

[mtklein, 2016/01/20 14:49:07]

It seems like this one would be more useful if defined at the same time it's
declared, up a the top of the file near the flags, or even defined inside
main():

SkCommandLineFlags::Parse(argc, argv);
auto usage = [&] {
    SkDebugf("...", argv[0]);
    return 1;
};

then later,
   return usage();

[kjlubick, 2016/01/20 19:21:58]

Done.

+}
+
+int runSingleTest(SkData* bytes) {
     for (auto r = SkTRegistry<Fuzzable>::Head(); r; r = r->next()) {
         auto fuzzable = r->factory();
         if (!SkCommandLineFlags::ShouldSkip(FLAGS_match, fuzzable.name)) {
             SkDebugf("Fuzzing %s...\n", fuzzable.name);
             Fuzz fuzz(bytes);
             fuzzable.fn(&fuzz);
+            return 0;
         }
     }
     return 0;

[mtklein, 2016/01/20 14:49:06]

1?

[kjlubick, 2016/01/20 19:21:58]

Done.

 }
 
+int decodeImage(SkData* bytes) {
+    SkAutoTDelete<SkCodec> codec(SkCodec::NewFromData(bytes));

[mtklein, 2016/01/20 14:49:06]

This can't really be the easiest way to use SkCodec, right?

Perhaps we should leave these as just

static int decode_img(SkData* data) {
   // TODO(scroggo)
   return 1;
}

static int decode_skp(SkData* data) {
   // TODO(mtklein)
  return 1;
}

?

[scroggo, 2016/01/20 18:46:19]

This looks about right. It's complicated in part by having the option for an
index8 bitmap and zeroing the memory.

[kjlubick, 2016/01/20 19:21:58]

I went over it with scroggo@ yesterday, and this is what I understood to be the
proper way.

+    if (nullptr == codec.get()) {
+        SkDebugf("Couldn't create codec.");
+        return 3;
+    }
+
+    SkImageInfo decodeInfo = codec->getInfo();
+    // Construct a color table for the decode if necessary
+    SkAutoTUnref<SkColorTable> colorTable(nullptr);
+    SkPMColor* colorPtr = nullptr;
+    int* colorCountPtr = nullptr;
+    int maxColors = 256;
+    if (kIndex_8_SkColorType == decodeInfo.colorType()) {
+        SkPMColor colors[256];
+        colorTable.reset(new SkColorTable(colors, maxColors));
+        colorPtr = const_cast<SkPMColor*>(colorTable->readColors());
+        colorCountPtr = &maxColors;
+    }
+
+    SkBitmap bitmap;
+    SkMallocPixelRef::ZeroedPRFactory zeroFactory;
+    SkCodec::Options options;
+    options.fZeroInitialized = SkCodec::kYes_ZeroInitialized;
+
+    if (!bitmap.tryAllocPixels(decodeInfo, &zeroFactory, nullptr)) {
+        SkDebugf("Could not allocate memory.  Image might be too large (%d x %d)",
+                    decodeInfo.width(), decodeInfo.height());
+        return 4;
+    }
+
+    switch (codec->getPixels(decodeInfo, bitmap.getPixels(), bitmap.rowBytes(), &options,
+            colorPtr, colorCountPtr)) {
+        case SkCodec::kSuccess:
+            SkDebugf("Success!\n");
+            break;
+        case SkCodec::kIncompleteInput:
+            SkDebugf("Partial Success\n");
+            break;
+        case SkCodec::kInvalidConversion:
+            SkDebugf("Incompatible colortype conversion");
+            return 5;
+        default:
+            // Everything else is considered a failure.
+            SkDebugf("Couldn't getPixels.");
+            return 6;
+    }
+
+
+    if (!FLAGS_dump.isEmpty()) {
+        SkImageEncoder::EncodeFile(FLAGS_dump[0], bitmap, SkImageEncoder::kPNG_Type, 100);
+        SkDebugf("Dumped to %s\n", FLAGS_dump[0]);
+    }
+    return 0;
+}
+
+static const SkRect kSKPViewport = {0,0, 1000,1000};
+
+int decodeSkp() {
+    SkAutoTDelete<SkStream> stream(SkStream::NewFromFile(FLAGS_bytes[0]));
+    if (!stream) {
+        SkDebugf("Couldn't read %s.", FLAGS_bytes[0]);
+        return 2;
+    }
+    SkDebugf("Decoding\n");
+    SkAutoTUnref<SkPicture> pic(SkPicture::CreateFromStream(stream));
+    if (!pic) {
+        SkDebugf("Couldn't decode as a picture.\n");
+        return 3;
+    }
+    SkDebugf("Rendering\n");
+    SkBitmap bitmap;
+    if (!FLAGS_dump.isEmpty()) {
+        SkIRect size = pic->cullRect().roundOut();
+        bitmap.allocN32Pixels(size.width(), size.height());
+    }
+    SkCanvas canvas(bitmap);
+    canvas.clipRect(kSKPViewport);

[mtklein, 2016/01/20 14:49:06]

Let's not do this?  If we're allocating space for the whole .skp, let's draw the
whole .skp.  If not, let's minimize the allocation to fit the intersection of
the clip and the .skp's dimensions.

[kjlubick, 2016/01/20 19:21:58]

Done.

+    canvas.drawPicture(pic);
+    SkDebugf("Decoded and rendered an SkPicture!\n");
+    if (!FLAGS_dump.isEmpty()) {
+        SkImageEncoder::EncodeFile(FLAGS_dump[0], bitmap, SkImageEncoder::kPNG_Type, 100);

[mtklein, 2016/01/20 14:49:06]

Seems like we've got a bunch of common logic for .skps and images.  I wonder if
we can make them yet more common using
SkCodecImageGenerator::NewFromEncodedCodec() /
SkImageGenerator::NewFromPicture()?

(Could we get Codec in there a few more times? :P)

[kjlubick, 2016/01/20 19:21:57]

Deduped.

+        SkDebugf("Dumped to %s\n", FLAGS_dump[0]);
+    }
+    return 0;
+}
 
 Fuzz::Fuzz(SkData* bytes) : fBytes(SkSafeRef(bytes)), fNextByte(0) {}