Add LLVM fuzzer: Skia color space and color transform

testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc

Index: testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc
diff --git a/testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc b/testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc
new file mode 100644
index 0000000000000000000000000000000000000000..3385d4eabfe20c1154094c48888b5443a815dcf1
--- /dev/null
+++ b/testing/libfuzzer/fuzzers/skia_color_space_fuzzer.cc
@@ -0,0 +1,100 @@
+// Copyright 2017 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.
+
+#include <stddef.h>
+#include <stdint.h>
+
+#include "base/logging.h"
+#include "base/macros.h"
+#include "testing/libfuzzer/fuzzers/color_space_data.h"
+#include "third_party/skia/include/core/SkColorSpace.h"
+#include "third_party/skia/include/core/SkColorSpaceXform.h"
+
+static constexpr unsigned kPixels = 2048 / 4;
+
+static const char* RandomSource() {
+  DCHECK_LT(kPixels * 4, arraysize(srgb_data));
+  // Use srgb_data as a source of kPixels * 4 read-only random data bytes.
+  return srgb_data + arraysize(srgb_data) - kPixels * 4;
+}
+
+static sk_sp<SkColorSpace> test;
+static sk_sp<SkColorSpace> srgb;
+
+static void ColorTransform(size_t size, bool input) {
+  DCHECK(test.get());
+
+  auto transform = input ? SkColorSpaceXform::New(test.get(), srgb.get())
+                         : SkColorSpaceXform::New(srgb.get(), test.get());
+  if (!transform)
+    return;
+
+  uint32_t data[kPixels * 4];

[Noel Gordon, 2017/04/04 12:23:16]

This is a fine source of random pixel data, and contains whatever is on the data
stack at this location.

If this fuzzer also gets run in MSAN though, I suspect that MSAN might complain
that the data is uninitialized, if/when it is read by the color transform code
during transform->apply().

How do we tell MSAN to stfu about that, and just accept the uninitialized reads
from the data variable?

[mmoroz, 2017/04/04 13:43:06]

I'm afraid that we cannot rely on this approach because:
1) all the testcases generated by the fuzzer should be fully reproducible if we
re-run them, i.e. we cannot use a random data from a random memory region
2) this might be looking as a hacky though efficient solution, but we shouldn't
use bad coding patterns anyway

Since you always need a constant number of bytes here, I'd recommend to simply
use first |4 * kPixels| bytes of |data| coming from fuzzer.

[Noel Gordon, 2017/04/05 14:14:07]

IC, we can fix that ...
I created a deterministic "random" pixel generator.
Note the input data can be less than 4 * kPixels.

[mmoroz, 2017/04/06 08:26:37]

Right, but something like:
if (size < 4 * kPixels) return 0;

in the beginning of LLVMFuzzerTestOneInput function will help. LibFuzzer is able
to quickly understand that inputs shorter than that limit do no make sense and
won't spend time on them. 

+  if ((size & 7) == 7)
+    memcpy(data, RandomSource(), kPixels * 4);
+
+  const auto color = SkColorSpaceXform::ColorFormat(size & 7);
+  const auto alpha = SkAlphaType(size & 3);
+
+  transform->apply(color, data, color, data, kPixels, alpha);
+}
+
+template <typename T>
+static T* FuzzyPointer(const uint8_t* data, size_t size) {
+  constexpr size_t kAddress = 256;
+  if (size < kAddress + sizeof(T) + 64) {
+    static T object;
+    return &object;
+  } else {
+    uintptr_t address = (uintptr_t)&data[kAddress];
+    return (T*)(address & ~31);  // 32-bit aligned.

[mmoroz, 2017/04/04 13:43:06]

Coudl you please use c++ cast here?

[Noel Gordon, 2017/04/05 14:14:07]

Removed all this.

+  }
+}
+
+extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
+  constexpr size_t kSizeLimit = 4 * 1024 * 1024;

[mmoroz, 2017/04/04 13:43:06]

As a follow-up to comment on line #33, |kSizeLimit| here and in GN config should
be increased by |4 * kPixels|. Also, it would make sense to have a lower limit
check, i.e. |if (size <= 4 * kPixels)| .

[Noel Gordon, 2017/04/05 14:14:08]

The new "random" pixel generator is independent of the size of input data, and
so does not depend on kSizeLimit either.
Added a lower limit that is sensible for color profiles (128).

+  if (size > kSizeLimit)
+    return 0;
+
+  test = SkColorSpace::MakeICC(data, size);

[mmoroz, 2017/04/04 13:43:06]

As first |4 * kPixels| bytes of |data| will be used for initialization of data
used in |ColorTransform| function, we should do here something like:

std::vector<uint8_t> buffer(data + 4 * kPixels, buffer + size - 4 * kPixels);

and then use |buffer.data()| and |buffer.size()| instead of |data| and |size|.
That's important to have these bytes in a separate buffer to be able catch
out-of-bound access to the left. Thus, we cannot just use |data + 4 * kPixels|
and |size - 4 * kPixels| without creating a separate buffer.

[Noel Gordon, 2017/04/05 14:14:07]

With the new design, I'm not sure any of this applies.  In thinking about a png
decoder, for example, which creates many internal structures to read/write data
from, I be surprised if a fuzzer could not spot any out-of-bounds reads or
writes on said structures (none of which are wrapped in a std::vector).

[mmoroz, 2017/04/06 08:31:26]

If the target creates internal structures with their own buffers, this is fine.
Unless there are bugs in the initialization code which creates those internal
structures.

What I'm speaking about is cases like the following one:

void VulnerableTarget(data, size) {
  <...>
  data[-1] = 0; <-- OOB access
}

LLVMFuzzerTestOneInput(data, size) {
  if (size < 5) return 0;
  <...>
  VulnerableTarget(data + 5, size - 5);
}


In this case, the OOB access won't be detected because it happens inside the
accessible memory actually.

+  if (!test)
+    return 0;
+
+  switch (size & 7) {  // Create profile to transform pixels to/from.

[mmoroz, 2017/04/04 13:43:07]

Nice trick, but it would be better to avoid relying only on |size| here,
otherwise you would never get an input of length |8 * n| to be processed by
cases other than case 0, and so forth.

Our current recommendation for such cases is to use |std::hash| value calculated
from |data|, e.g.
https://cs.chromium.org/search/?q=file:fuzzer.*.cc+std::hash+package:%5Echrom...

[Noel Gordon, 2017/04/05 14:14:07]

Not some a trick. The distribution of size & 7 tends to uniform as the corpus
grows. The hash idea seems fine, but it does add measurable overhead which
decreases exec/s ...
Hmmm: looking at the link, copying the input data into a std::string<> just to
compute a std::hash wastes too time and is maybe not a good idea for color
profile data.  

A better approach is to 1) use base::StringPiece and 2) compute a fixed-sized
hash (we know that input data size is at least 128 now) to maintain performance
and provide a reasonable hash.

[mmoroz, 2017/04/06 08:26:37]

The distribution of size & 7 doesn't matter because all inputs of the following
sizes:
8, 16, 24, 32, ..., 1024, ..., 2048, ... 8 * n

will never go into any of the cases except of the case 0.

The same applies to inputs of sizes (1 + 8 * n) as they will always visit only
case 1 branch, and so on. This is bad for coverage.


If hash is a performance concern here, you can extract one byte from the data
for flags randomization (example:
https://cs.chromium.org/chromium/src/third_party/sqlite/fuzz/sqlite3_prepare_...)

StringPieceHash looks fine to me as well.

+    case 0:
+      srgb = SkColorSpace::MakeICC(adobe_data, arraysize(adobe_data));
+      break;
+    case 1:
+      srgb = SkColorSpace::MakeICC(srgb_para, arraysize(srgb_para));
+      break;
+    case 2:
+      srgb = SkColorSpace::MakeSRGBLinear();
+      break;
+    case 3:
+      srgb = SkColorSpace::MakeRGB(
+          *FuzzyPointer<SkColorSpaceTransferFn>(data, size),
+          SkColorSpace::Gamut::kAdobeRGB_Gamut);
+      break;
+    case 4:
+      srgb = SkColorSpace::MakeRGB(
+          SkColorSpace::RenderTargetGamma::kSRGB_RenderTargetGamma,
+          *FuzzyPointer<SkMatrix44>(data, size));
+      break;
+    case 5:
+      srgb = SkColorSpace::MakeRGB(
+          SkColorSpace::RenderTargetGamma::kSRGB_RenderTargetGamma,
+          SkColorSpace::Gamut::kDCIP3_D65_Gamut);
+      break;
+    default:
+      srgb = SkColorSpace::MakeICC(srgb_data, arraysize(srgb_data));
+      break;
+  }
+
+  ColorTransform(size, true);
+  ColorTransform(size, false);
+
+  test.reset();
+  srgb.reset();
+  return 0;
+}