Add an HttpStreamParser fuzzer.

net/http/http_stream_parser_fuzzer.cc

Index: net/http/http_stream_parser_fuzzer.cc
diff --git a/net/http/http_stream_parser_fuzzer.cc b/net/http/http_stream_parser_fuzzer.cc
new file mode 100644
index 0000000000000000000000000000000000000000..7e539efd64e8cd9bef4a7c82cce740571dcdb757
--- /dev/null
+++ b/net/http/http_stream_parser_fuzzer.cc
@@ -0,0 +1,123 @@
+// 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 "net/http/http_stream_parser.h"
+
+#include <stddef.h>
+#include <stdint.h>
+
+#include <algorithm>
+#include <string>
+#include <vector>
+
+#include "base/logging.h"
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/message_loop/message_loop.h"
+#include "base/numerics/safe_conversions.h"
+#include "net/base/address_list.h"
+#include "net/base/io_buffer.h"
+#include "net/base/net_errors.h"
+#include "net/base/test_completion_callback.h"
+#include "net/http/http_request_headers.h"
+#include "net/http/http_request_info.h"
+#include "net/http/http_response_info.h"
+#include "net/log/net_log.h"
+#include "net/log/test_net_log.h"
+#include "net/socket/client_socket_handle.h"
+#include "net/socket/socket_test_util.h"
+#include "url/gurl.h"
+
+// Entry point for LibFuzzer.
+extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {

[eroman, 2016/03/26 01:39:29]

It would be helpful to have a high-level overview of what this fuzzer does here.

There is good documentation inside about the individual steps taking (thanks!),
but I still recommend a short overview of how the stream parser is being driven.

[mmenke, 2016/03/28 15:26:26]

Added a short comment.  Happy to add more, just not sure what else is worth
saying.

+  // Needed for thread checks and waits.
+  base::MessageLoopForIO message_loop;

[eroman, 2016/03/26 01:39:28]

I wonder if it would be faster to extract the message_loop to a singleton, since
LLVMFuzzerTestOneInput() is going to be called repeatedly.

Can explore that later I guess.

[mmenke, 2016/03/28 15:26:26]

I think MessageLoops aren't too heavy weight?  Only kernel object they need is a
lock.

Experimentally, making it a global made exec/s per sec go from a varying
1100-1200 runs/sec to a more consistent 1250.

+
+  net::MockWrite writes[] = {
+      net::MockWrite(net::ASYNC, 0, "GET / HTTP/1.1\r\n\r\n"),
+  };
+
+  // Break the buffer into a sequence of variable sized sync and async

[eroman, 2016/03/28 22:54:57]

optional nit:

"sync and async reads of various sizes"

+  // reads.  Use the last bytes of |data| exclusively for determining
+  // the size of type of each read.

[eroman, 2016/03/26 01:39:28]

"size of type" ==> "size and type"

[mmenke, 2016/03/28 15:26:26]

Done.

+  std::vector<net::MockRead> reads;
+  // Sequence number for socket operations.
+  int last_sequence_number = 0;
+  // Each read needs a one byte seed to determine read size and whether it
+  // should be sync or async, so if there's only one byte left unused, can't use
+  // it here.
+  while (size > 1) {
+    size_t read_seed = data[size - 1];

[eroman, 2016/03/26 01:39:29]

Max can advise on what works best for the fuzzer.

I wonder if it would be advantageous to increase the ways in which the chunk
sizes and types are determined. Right now if I understand, it is highly
dependent on the last byte of the data, and then the rest follow from that.

Some ideas (don't trust me though, listen to whatever Max suggests):

  * Pull this byte out of the data that is fed to parser so it can change
independently, and can generate many permutations of chunks for the same data
processed

  * Mix in data from other bytes of the input, to increase ways of modifying the
chunking. Like perhaps the first byte, or the previous read_seed.
XOR this with some other bytes from the data to increase the combinations
reachable. Like for instance pulling a byte out of the input (not feeding to
parser) and using that to influence this, or XORing with he previous read_seed.

[mmenke, 2016/03/28 15:26:26]

You're misunderstanding the code.  This code uses the last bytes exclusively for
read sizes - they don't appear in any read.  This avoids dual use data , which
I'm highly skeptical of.  Just consider a chunked-encoded HTTP stream.  Both the
first and last bytes are always the same.

Could also use a hash, but I figure this is simpler.  It does affect how we walk
throguh the search space a bit, though.

[eroman, 2016/03/28 22:54:57]

I see, I had indeed read the code wrong.

The new comments help; I think your approach is sound.

+    size--;
+    net::IoMode io_mode = net::ASYNC;
+    // High order byte determines IoMode.

[eroman, 2016/03/26 01:39:29]

"high order byte" ==> "high order bit"

[mmenke, 2016/03/28 15:26:26]

Done.

+    if (read_seed & 0x80)

[eroman, 2016/03/26 01:39:28]

Just thinking out loud, but if we are generating lots of ASCII data this will be
less likely to be hit.

I wonder if taking the least significant bit instead would give more uniformity.

For then getting the read_size doesn't really matter if that bit is re-used, can
still find it as (0x3F & read_seed).

[mmenke, 2016/03/28 15:26:26]

Done.  Since these bytes are not used as part of the body, not sure if they have
an ASCII bias, though given the transforms the fuzzer applies, perhaps they do.

+      io_mode = net::SYNCHRONOUS;
+
+    // Low order 6 bytes determine how many bytes are returned by the read.

[eroman, 2016/03/26 01:39:29]

bytes ==> bits

[mmenke, 2016/03/28 15:26:26]

Done.

+    int read_size = static_cast<int>(std::min(1 + (0x3F & read_seed), size));
+    reads.push_back(net::MockRead(io_mode, reinterpret_cast<const char*>(data),
+                                  read_size, ++last_sequence_number));
+
+    data += read_size;
+    size -= read_size;
+  }
+
+  // Use whether or not there's a left over byte to determine IoMode of final
+  // operation.
+  net::IoMode io_mode = net::ASYNC;
+  if (size > 0)

[eroman, 2016/03/26 01:39:28]

Isn't |size| going to be 0 after the while-loop above?

Alternately for getting a single bit worth of data, Max suggested to me in the
past this pattern:

if (size > 1) {
  if ((data[0] ^ data[size - 1]) & 1)
    io_mode = net::SYNCHRONOUS;
}

[mmenke, 2016/03/28 15:26:26]

data[0] here is almost always 'H', and the last byte can have special properties
now (Well...I guess not).  Note that the above loop ends when there are 0 or 1
remaining bytes, since if there's 1, can't use it is a code for how many bytes
to read.  That having been said, because of the std::min above, it's usually 0. 
There's still some bits left over, though...Let's use them.

[mmenke, 2016/03/28 22:42:50]

Erm...that "special properties / I guess not" was not well explained.  For
chunked-encoding, the final chunk is always 0\r\n\r\n, I believe (We may allow
0\n\n?).  Of course, there can be data *after* the last chunk, so it may not
have special properties afterall.  Seems most robust just to use separate bytes
only for the meta-data, though.

+    io_mode = net::SYNCHRONOUS;
+  reads.push_back(net::MockRead(io_mode, net::ERR_CONNECTION_CLOSED,
+                                ++last_sequence_number));
+  net::SequencedSocketData socket_data(reads.data(), reads.size(), writes,
+                                       arraysize(writes));
+  socket_data.set_connect_data(net::MockConnect(net::SYNCHRONOUS, net::OK));
+
+  scoped_ptr<net::MockTCPClientSocket> socket(
+      new net::MockTCPClientSocket(net::AddressList(), nullptr, &socket_data));
+
+  net::TestCompletionCallback callback;
+  CHECK_EQ(net::OK, socket->Connect(callback.callback()));
+
+  net::ClientSocketHandle socket_handle;
+  socket_handle.SetSocket(std::move(socket));
+
+  net::HttpRequestInfo request_info;
+  request_info.method = "GET";
+  request_info.url = GURL("http://localhost/");
+
+  scoped_refptr<net::GrowableIOBuffer> read_buffer(new net::GrowableIOBuffer());
+  // Use a NetLog that listens to events, to get coverage of logging
+  // callbacks.
+  net::BoundTestNetLog net_log;
+  net::HttpStreamParser parser(&socket_handle, &request_info, read_buffer.get(),
+                               net_log.bound());
+
+  net::HttpResponseInfo response_info;
+  int result =
+      parser.SendRequest("GET / HTTP/1.1\r\n", net::HttpRequestHeaders(),
+                         &response_info, callback.callback());
+  CHECK_EQ(net::OK, callback.GetResult(result));
+
+  result = parser.ReadResponseHeaders(callback.callback());
+  result = callback.GetResult(result);
+
+  if (result != net::OK)
+    return 0;
+
+  while (true) {
+    // 64 exactly matches the maximum amount of data returned by a single
+    // MockRead, as created above.
+    scoped_refptr<net::IOBufferWithSize> io_buffer(

[eroman, 2016/03/26 01:39:29]

Would it make sense to allocate this once outside of the while-loop ?

[mmenke, 2016/03/28 15:26:26]

Freeing it is more likely to catch a use-after-free, though, which is why I went
with this approach.  So a bit of a tradeoff of coverage vs performance here.

+        new net::IOBufferWithSize(64));
+    result = parser.ReadResponseBody(io_buffer.get(), io_buffer->size(),
+                                     callback.callback());
+    if (callback.GetResult(result) <= 0)
+      break;
+  }
+
+  return 0;
+}