URLRequest fuzzer.

net/socket/fuzzed_socket.cc

Index: net/socket/fuzzed_socket.cc
diff --git a/net/socket/fuzzed_socket.cc b/net/socket/fuzzed_socket.cc
index c00faafd3f655729cbdbdcd10fabceed1cdd8317..df2ba649413c8686b3bacffc25cdba86425379e5 100644
--- a/net/socket/fuzzed_socket.cc
+++ b/net/socket/fuzzed_socket.cc
@@ -8,13 +8,21 @@
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/thread_task_runner_handle.h"
+#include "net/base/fuzzed_data_provider.h"
 #include "net/base/io_buffer.h"
 
 namespace net {
 
 namespace {
 
-// Subset of the socket errors that can be returned by normal socket reads /
+// Some of the socket errors that can be returned by normal socket connection
+// attempts.
+const Error kConnectErrors[] = {
+    ERR_CONNECTION_RESET,     ERR_CONNECTION_CLOSED, ERR_FAILED,
+    ERR_CONNECTION_TIMED_OUT, ERR_ACCESS_DENIED,     ERR_CONNECTION_REFUSED,
+    ERR_ADDRESS_UNREACHABLE};
+
+// Some of the socket errors that can be returned by normal socket reads /
 // writes. The first one is returned when no more input data remains, so it's
 // one of the most common ones.
 const Error kReadWriteErrors[] = {ERR_CONNECTION_CLOSED, ERR_FAILED,
@@ -22,11 +30,11 @@ const Error kReadWriteErrors[] = {ERR_CONNECTION_CLOSED, ERR_FAILED,
 
 }  // namespace
 
-FuzzedSocket::FuzzedSocket(const uint8_t* data,
-                           size_t data_size,
-                           const BoundNetLog& bound_net_log)
-    : data_(reinterpret_cast<const char*>(data), data_size),
-      bound_net_log_(bound_net_log),
+FuzzedSocket::FuzzedSocket(FuzzedDataProvider* data_provider,
+                           net::NetLog* net_log)
+    : data_provider_(data_provider),
+      bound_net_log_(BoundNetLog::Make(net_log, NetLog::SOURCE_SOCKET)),
+      remote_address_(IPEndPoint(IPAddress(127, 0, 0, 1), 80)),

[eroman, 2016/04/22 22:07:10]

Or alternately IPAddress::IPv4Localhost()

[mmenke, 2016/04/27 19:53:25]

Handy.  Done.

       weak_factory_(this) {}
 
 FuzzedSocket::~FuzzedSocket() {}
@@ -34,6 +42,7 @@ FuzzedSocket::~FuzzedSocket() {}
 int FuzzedSocket::Read(IOBuffer* buf,
                        int buf_len,
                        const CompletionCallback& callback) {
+  DCHECK(!connect_pending_);
   DCHECK(!read_pending_);
 
   bool sync;
@@ -44,24 +53,20 @@ int FuzzedSocket::Read(IOBuffer* buf,
     result = net_error_;
     sync = !error_pending_;
   } else {
-    // Otherwise, use |data_|.
-    uint8_t random_val = ConsumeUint8FromData();
-    sync = !!(random_val & 0x01);
-    result = random_val >> 1;
+    // Otherwise, use |data_provider_|.
+    sync = !!data_provider_->ConsumeBits(1);

[eroman, 2016/04/22 22:07:11]

How about adding a wrapper method:

bool FuzzedDatazProvider::ConsumeBool() {
  return !!ConsumeBits(1);
}

Since I could see this being useful in a number of places.

I guess the downside is it makes it a bit less obvious how many bits are being
used.
i.e. it is less obvious why the following ConsumeBits(7) is awesome.

[mmenke, 2016/04/27 19:53:25]

Hrm...We could make consume bits always round to the nearest multiple of 8 bits,
instead of trying to minimize number of consumed bits.  WDYT?  We could waste
more data, but I don't think that's a huge deal.  I've switched to doing that.

+    result = data_provider_->ConsumeBits(7);

[eroman, 2016/04/22 22:07:11]

Could combine this with the next line:

result = std::min(buf_len, data_provider_->ConsumeBits(7));

although perhaps less clear given the style used here.

[mmenke, 2016/04/27 19:53:25]

You mean std::min(static_cast<uint32_t>(buf_len),
data_provider_->ConsumeBits(7));

Which is why I didn't do it - casts, and templates in general, are just ugly,
though this does, admittedly, do the same cast implicitly.

     if (result > buf_len)
       result = buf_len;
-    // Can't read more data than is available in |data_|.
-    if (static_cast<size_t>(result) > data_.length())
-      result = data_.length();
+
+    if (result > 0)
+      result = data_provider_->ConsumeBytes(buf->data(), result);
 
     if (result == 0) {
       net_error_ = ConsumeReadWriteErrorFromData();
       result = net_error_;
       if (!sync)
         error_pending_ = true;
-    } else {
-      memcpy(buf->data(), data_.data(), result);
-      data_ = data_.substr(result);
     }
   }
 
@@ -86,6 +91,7 @@ int FuzzedSocket::Read(IOBuffer* buf,
 int FuzzedSocket::Write(IOBuffer* buf,
                         int buf_len,
                         const CompletionCallback& callback) {
+  DCHECK(!connect_pending_);
   DCHECK(!write_pending_);
 
   bool sync;
@@ -97,9 +103,8 @@ int FuzzedSocket::Write(IOBuffer* buf,
     sync = !error_pending_;
   } else {
     // Otherwise, use |data_|.
-    uint8_t random_val = ConsumeUint8FromData();
-    sync = !!(random_val & 0x01);
-    result = random_val >> 1;
+    sync = !!data_provider_->ConsumeBits(1);

[eroman, 2016/04/22 22:07:11]

Same suggestiong (ConsumeBool())

+    result = data_provider_->ConsumeBits(7);
     if (result > buf_len)
       result = buf_len;
     if (result == 0) {
@@ -134,19 +139,46 @@ int FuzzedSocket::SetSendBufferSize(int32_t size) {
 int FuzzedSocket::Connect(const CompletionCallback& callback) {
   // Sockets can normally be reused, but don't support it here.
   DCHECK_NE(net_error_, OK);
+  DCHECK(!connect_pending_);
   DCHECK(!read_pending_);
   DCHECK(!write_pending_);
   DCHECK(!error_pending_);
   DCHECK(!total_bytes_read_);
   DCHECK(!total_bytes_written_);
 
-  net_error_ = OK;
-  return OK;
+  int sync = true;

[eroman, 2016/04/22 22:07:10]

Why not a bool?

+  Error result = OK;
+  if (fuzz_connect_result_) {
+    // Decide if sync or async. Use async, if no data is left.
+    sync = !!data_provider_->ConsumeBits(1);
+    // Decide if the connect succeeds or not. Fail, if no data is left.
+    bool is_error = !data_provider_->ConsumeBits(1);
+
+    // Remove 6 bits, so a full byte as been consumed. If the connect is going

[eroman, 2016/04/22 22:07:11]

as been --> has been.

Also note I don't think it is worth documenting too much about the details on
that a "full byte" was consumed. 

With the new design where the data provider saves the unused bits, at most the
fuzz test wastes 7 bits, even when doing lots of small bit reads.

So meh, consumers can just read how much data they want and call it a day :)

+    // to fail, use them to decide the error code. Otherwise, throw them away.
+    uint32_t error_bits = data_provider_->ConsumeBits(6);

[eroman, 2016/04/22 22:07:11]

Why not just Consume these 6 bits when |is_error| ? 

[mmenke, 2016/04/25 15:09:19]

See my other comments on how the fuzzer traverses input, above.

+    if (is_error)
+      result = kConnectErrors[error_bits % arraysize(kConnectErrors)];

[eroman, 2016/04/22 22:07:11]

Note: would have been sufficient to consume 3 bits (since there are just 8
connect errors).

Might be interesting to have a helper for that:

// returns a number in range [0, max), consuming ceil(log_2 max) bits.
FuzzedDataProvider::ConsumeNumberInRange(size_t max);

+  }
+
+  if (sync) {
+    net_error_ = result;
+    return result;
+  }
+
+  connect_pending_ = true;
+  if (result != OK)
+    error_pending_ = true;
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::Bind(&FuzzedSocket::OnConnectComplete,
+                            weak_factory_.GetWeakPtr(), callback, result));
+  return ERR_IO_PENDING;
 }
 
 void FuzzedSocket::Disconnect() {
   net_error_ = ERR_CONNECTION_CLOSED;
   weak_factory_.InvalidateWeakPtrs();
+  connect_pending_ = false;
   read_pending_ = false;
   write_pending_ = false;
   error_pending_ = false;
@@ -212,17 +244,9 @@ int64_t FuzzedSocket::GetTotalReceivedBytes() const {
   return total_bytes_read_;
 }
 
-uint8_t FuzzedSocket::ConsumeUint8FromData() {
-  size_t length = data_.length();
-  if (!length)
-    return 0;
-  uint8_t out = data_[length - 1];
-  data_ = data_.substr(0, length - 1);
-  return out;
-}
-
 Error FuzzedSocket::ConsumeReadWriteErrorFromData() {
-  return kReadWriteErrors[ConsumeUint8FromData() % arraysize(kReadWriteErrors)];
+  return kReadWriteErrors[data_provider_->ConsumeBits(8) %

[eroman, 2016/04/22 22:07:11]

Ditto here -- 8 bits is more than we need. Not a problem per se, but does mean
we are throwing away bits from the fuzzed data which could slow it down
(although I am sure by a negligable amount).

Could consider helper ConsumeNumberInRange() here too.

(The corpus already won't be completely stable to changes made to
kReadWriteErrors).

+                          arraysize(kReadWriteErrors)];
 }
 
 void FuzzedSocket::OnReadComplete(const CompletionCallback& callback,
@@ -249,4 +273,13 @@ void FuzzedSocket::OnWriteComplete(const CompletionCallback& callback,
   callback.Run(result);
 }
 
+void FuzzedSocket::OnConnectComplete(const CompletionCallback& callback,
+                                     int result) {
+  CHECK(connect_pending_);
+  connect_pending_ = false;
+  if (result < 0)
+    error_pending_ = false;
+  callback.Run(result);
+}
+
 }  // namespace net