Add fuzzer to test Fuzz URLRequestDataJob

net/url_request/url_request_data_job_fuzzer.cc

+// 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.
+
+#include <algorithm>
+#include <memory>
+
+#include "base/memory/ptr_util.h"
+#include "base/run_loop.h"
+#include "base/strings/stringprintf.h"
+#include "net/base/fuzzed_data_provider.h"
+#include "net/http/http_request_headers.h"
+#include "net/url_request/data_protocol_handler.h"
+#include "net/url_request/url_request_job_factory_impl.h"
+#include "net/url_request/url_request_test_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+std::string GenerateRandomRange(const char* bytes, size_t size) {

[eroman, 2016/04/25 23:51:30]

Please document.

This isn't necessarily generating valid range headers right? In that case, why
generate the header here rather than have it discovered by the fuzzer?

[Charlie Harrison, 2016/04/26 12:58:40]

The goal is to give some structure so the fuzzer doesn't have to learn about
range requests. This means the fuzzer learns about the real bugs in the range
request implementation that isn't just valid header parsing.

I'm fine to remove this though if you think just fuzzing the string is good
enough. In the meantime I've added some comments.

[eroman, 2016/04/27 00:43:01]

I prefer having the fuzzer build the range request.

My thinking is, it may come up with invalid combinations that trigger problems
that we didn't think to test here. For instance what if it has weird bugs when
dealing with hex digits, certain whitespaces, decimal numbers, etc. Rather than
hardcoding what we think is the grammar (or which may be everything today), can
just let the fuzzer discover it.

I am not too concerned with having a larger space to explore, so long as it has
a chance of discovering bugs. You would be surprised what it comes up with after
running long enough :)

Note that we already have separate tests for data:URL parsing itself, so I think
the main value-added of this test is really checking the URLRequest interface
and range stuff that we do for data:URLs.

To be honest, I didn't even expect that range headers should work with data
URLs!

[Charlie Harrison, 2016/04/27 17:39:54]

Sounds good to me. I've removed this code.

+  static std::vector<char> range_vals{'\0', '-', '0', '1', '2', '3', '4',

[eroman, 2016/04/27 00:43:01]

unfortunately our style guide doesn't allow this yet.

[Charlie Harrison, 2016/04/27 17:39:54]

Removed.

+                                      '5',  '6', '7', '8', '9', ','};
+  std::string range = "bytes=";
+  for (size_t i = 0; i < size; i++) {
+    char elt = range_vals[bytes[i] % range_vals.size()];
+    if (elt == '\0')
+      break;
+    range.push_back(elt);
+  }
+  return range;
+}
+
+}  // namespace
+
+// Fuzz different read lengths, different range requests, and different data.

[eroman, 2016/04/25 23:51:31]

Please add a more general description of what the test does. Along the lines
maybe of:

Tests creating and reading to completion a URLRequest with fuzzed data: URLs,
and custom Range headers.

[Charlie Harrison, 2016/04/26 12:58:40]

Done.

+// The structure of the data that the fuzzer generates looks like:
+// |-- 16 bytes of header --|------ data url ------|-- read metadata --|
+extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
+  static const size_t max_length = 256;
+  static const size_t max_range_length = 16;
+  EXPECT_LE(size, max_length);
+
+  // Statically initialize most of the harness, per fuzzing efficiency best
+  // practices. Without these the test runs much slower.
+  static char range_bytes[max_range_length];

[eroman, 2016/04/25 23:51:30]

Rather than N different statics, please extract this to a Singleton.

The test could be a method on the Singleton for ease of accessing all these
member then.

I suggest mainly for readability, but depending how compiler generates the code
might be faster too.

[Charlie Harrison, 2016/04/26 12:58:40]

Done. Good suggestion.

+  static char data_bytes[max_length];
+  static net::TestURLRequestContext context(true);
+  static net::URLRequestJobFactoryImpl job_factory;
+  static net::TestDelegate delegate;
+  static std::vector<size_t> read_lengths;
+  static net::IOBuffer* buf = new net::IOBuffer(size);
+
+  // One off initialization.
+  static bool initialized = false;
+  if (!initialized) {
+    job_factory.SetProtocolHandler(

[eroman, 2016/04/25 23:51:30]

This for instance would just be part of the singleton's constructor.

[Charlie Harrison, 2016/04/26 12:58:39]

Done.

+        "data", base::WrapUnique(new net::DataProtocolHandler()));
+    context.set_job_factory(&job_factory);
+    context.Init();
+    initialized = true;
+  }
+
+  // Per-test initialization.
+  read_lengths.clear();
+  net::FuzzedDataProvider provider(data, size);
+
+  // Generate data for range header. Use 16 bytes to get a good range of header
+  // values, including multiple ranges. Early return if the generated range
+  // header is invalid.
+  size_t range_bytes_size =
+      provider.ConsumeBytes(range_bytes, max_range_length);
+  std::string range = GenerateRandomRange(range_bytes, range_bytes_size);
+  if (!net::HttpUtil::IsValidHeaderValue(range))
+    return 0;
+
+  // Generate a sequence of reads sufficient to read the entire data url.
+  size_t bytes_for_data_url = 0;
+  if (size > range_bytes_size)
+    bytes_for_data_url = size - range_bytes_size;
+
+  size_t simulated_bytes_read = 0;
+  while (simulated_bytes_read < bytes_for_data_url) {
+    size_t read_length = provider.ConsumeBits(8);
+    read_lengths.push_back(read_length);
+    simulated_bytes_read += read_length;
+    // subtract one because we are consuming 8 bits off the end with every read
+    // for metadata.
+    bytes_for_data_url -= 1;

[eroman, 2016/04/25 23:51:30]

This seems like something that could be encapsulated into FuzzedDataProvider
instead.
i.e. would something like:

ConsumeRemainingBytes() work?

[Charlie Harrison, 2016/04/26 12:58:40]

We'll still need to keep track of this length to make sure our reads will fully
cover the remaining bytes. Otherwise we wouldn't know when to stop looping. I've
added a method to on the data provider to make this easier.

+  }
+  size_t data_size = provider.ConsumeBytes(data_bytes, bytes_for_data_url);
+  std::string data_string(data_bytes, data_size);
+
+  // Finally, create a URLRequest with the given data url and start reading from
+  // it.
+  std::unique_ptr<net::URLRequest> request = context.CreateRequest(
+      GURL("data:" + data_string), net::DEFAULT_PRIORITY, &delegate);
+  request->SetExtraRequestHeaderByName("Range", range, true);

[eroman, 2016/04/25 23:51:30]

Might want "no range header" to be a possible input as well. Not sure if invalid
range headers are already treated in that manner.

[Charlie Harrison, 2016/04/26 12:58:39]

Done.

+  request->Start();
+  for (const size_t read_length : read_lengths) {
+    base::RunLoop().RunUntilIdle();
+    int bytes_read = 0;
+    request->Read(buf, read_length, &bytes_read);
+  }
+  return 0;
+}