Add a URLRequest FTP fuzzer.

Add a URLRequest FTP fuzzer.

Also add a new method to FuzzedDataProvider to provide a random length
string, with a max length. Generating the string this way allows for
the fuzzer to mutate it more easily than the previous pattern of
picking length from the end of input and then choosing that many
characters from the start of the input.

FuzzedSockets are switched to use the new method.  It will cause them
to need to rebuild their corpuses, but should result in faster
convergence, longer term.

BUG=660869
NOPRESUBMIT=true

Committed: https://crrev.com/a7da0714bb1e387b30298047a9fa81c0ed2a2d13
Cr-Commit-Position: refs/heads/master@{#433652}

[mmenke, 2016-11-01 18:25:12]

Patchset #2 (id:20001) has been deleted

[mmenke, 2016-11-01 18:35:42]

mmenke@chromium.org changed reviewers:
+ csharrison@chromium.org, eroman@chromium.org

[mmenke, 2016-11-01 18:35:43]

[+eroman]: Please review everything.
[+csharrison]: Please review FuzzedDataProvider.

Not sure a test corpus is really needed - running with just the dictionary does
eventually reach the point of successfully completing a request in a reasonable
amount of time (Though it does take quite a while), but figure it's nice to give
it a place to start, given how many responses are needed to get to that point.
Also, the string fuzzing code means you can actually create things by hand
(Though still a bit of a pain.  Fuzzed HostResolver still requires NULLs and
such, too).

https://codereview.chromium.org/2469813002/diff/60001/net/socket/fuzzed_socke...
File net/socket/fuzzed_socket.cc (right):

https://codereview.chromium.org/2469813002/diff/60001/net/socket/fuzzed_socke...
net/socket/fuzzed_socket.cc:277: net_error_ = result;
This is a bug fix.  Basically, if connect succeeded asynchronously,
IsConnected[AndIdle] would always return false.  May be worth considering some
way to simulate the socket being disconnected / having data first during that
call, but beyond the scope of this CL.

[mmenke, 2016-11-01 18:36:13]

Patchset #2 (id:40001) has been deleted

[mmenke, 2016-11-01 18:37:48]

Oh, and I did compare the new FuzzedSocket code against the old one, on the old
URLRequestFuzzer.  It seemed to get code coverage a lot more quickly, though I
didn't run the comparison that long.

[Charlie Harrison, 2016-11-01 19:02:06]

https://codereview.chromium.org/2469813002/diff/60001/base/test/fuzzed_data_p...
File base/test/fuzzed_data_provider.h (right):

https://codereview.chromium.org/2469813002/diff/60001/base/test/fuzzed_data_p...
base/test/fuzzed_data_provider.h:37: // input data, returns what remains of the
input. Designed to be more stable with respect to inserting characters than
picking a random length and then consuming that many bytes with ConsumeBytes().
wrap to 80 cols.

https://codereview.chromium.org/2469813002/diff/60001/base/test/fuzzed_data_p...
base/test/fuzzed_data_provider.h:38: std::string
ConsumeRandomLengthString(size_t max_length);
Can you document that this looks for the magic string "\\"? Some consumers might
care about that. It would also be nice to explain why this is more stable, as it
isn't obvious (explanation can go in definition).

[mmenke, 2016-11-01 19:14:29]

Thanks for the feedback!

https://codereview.chromium.org/2469813002/diff/60001/base/test/fuzzed_data_p...
File base/test/fuzzed_data_provider.h (right):

https://codereview.chromium.org/2469813002/diff/60001/base/test/fuzzed_data_p...
base/test/fuzzed_data_provider.h:37: // input data, returns what remains of the
input. Designed to be more stable with respect to inserting characters than
picking a random length and then consuming that many bytes with ConsumeBytes().
On 2016/11/01 19:02:05, Charlie Harrison wrote:
> wrap to 80 cols.

Done.  Oops.  Had to bypass the whine due to CR's in the corpus, which made me
miss the 80 col warning.

https://codereview.chromium.org/2469813002/diff/60001/base/test/fuzzed_data_p...
base/test/fuzzed_data_provider.h:38: std::string
ConsumeRandomLengthString(size_t max_length);
On 2016/11/01 19:02:05, Charlie Harrison wrote:
> Can you document that this looks for the magic string "\\"? Some consumers
might
> care about that. It would also be nice to explain why this is more stable, as
it
> isn't obvious (explanation can go in definition).

I don't think it's useful to consumers unless we document all behavior here (The
ConsumeUint and friends pulling from the back of the input, for example).  Do we
want to do that?  I've added docs to the cc file, can move them, if we decide to
document all low level details in this file.

[Charlie Harrison, 2016-11-01 19:16:52]

fuzzed_data_provider lgtm. I don't think we need to expose those implementation
details in the header, but having them somewhere is good :)

[eroman, 2016-11-14 21:02:20]

LGTM.

Typo in the CL description:

> than the previous patter of pick

https://codereview.chromium.org/2469813002/diff/110001/base/test/fuzzed_data_...
File base/test/fuzzed_data_provider.cc (right):

https://codereview.chromium.org/2469813002/diff/110001/base/test/fuzzed_data_...
base/test/fuzzed_data_provider.cc:58: // Reads bytes from start of |data|. Maps
"\\" to "\", and maps "\" followed
|data| --> |remaining_data_| ?

[mmenke, 2016-11-14 21:06:59]

Description was changed from

==========
Add a URLRequest FTP fuzzer.

Also add a new method to FuzzedDataProvider to provide a random length
string, with a max length. The way the string is generated allows for
the fuzzer to mutate it more easily than the previous patter of pick
length from end of input and then choose that many characters from the
start of the input.

FuzzedSockets are switched to use the new method.  It will cause them
to need to rebuild their corpuses, but should result in faster
convergence, longer term.

BUG=660869
==========

to

==========
Add a URLRequest FTP fuzzer.

Also add a new method to FuzzedDataProvider to provide a random length
string, with a max length. Generating the string this way allows for
the fuzzer to mutate it more easily than the previous pattern of
picking length from the end of input and then choosing that many
characters from the start of the input.

FuzzedSockets are switched to use the new method.  It will cause them
to need to rebuild their corpuses, but should result in faster
convergence, longer term.

BUG=660869
==========

[mmenke, 2016-11-14 21:12:09]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[mmenke, 2016-11-14 21:13:12]

On 2016/11/14 21:02:20, eroman (slow) wrote:
> Typo in the CL description:
> 
> > than the previous patter of pick

Fixed, thanks!

https://codereview.chromium.org/2469813002/diff/110001/base/test/fuzzed_data_...
File base/test/fuzzed_data_provider.cc (right):

https://codereview.chromium.org/2469813002/diff/110001/base/test/fuzzed_data_...
base/test/fuzzed_data_provider.cc:58: // Reads bytes from start of |data|. Maps
"\\" to "\", and maps "\" followed
On 2016/11/14 21:02:20, eroman (slow) wrote:
> |data| --> |remaining_data_| ?

Done.

[commit-bot: I haz the power, 2016-11-14 21:14:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mmenke, 2016-11-14 21:14:46]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-14 21:15:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mmenke, 2016-11-14 21:18:57]

mmenke@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

[mmenke, 2016-11-14 21:18:58]

phajdan.jr:  Please review base/test/fuzzed_data_provider.*.  Would use thestig,
as he's reviewed this code before, but his status indicates he's not available
for reviews, currently.

[commit-bot: I haz the power, 2016-11-14 21:21:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-14 21:21:26]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mmenke, 2016-11-14 21:55:04]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-14 21:55:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-14 22:35:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-14 22:35:35]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Paweł Hajdan Jr., 2016-11-15 13:38:50]

https://codereview.chromium.org/2469813002/diff/150001/base/test/fuzzed_data_...
File base/test/fuzzed_data_provider.cc (right):

https://codereview.chromium.org/2469813002/diff/150001/base/test/fuzzed_data_...
base/test/fuzzed_data_provider.cc:58: // Reads bytes from start of
|remaining_data_|. Maps "\\" to "\", and maps "\"
Why do we care about backslashes?

Is this the right level of abstraction for this logic?

[mmenke, 2016-11-15 13:43:48]

https://codereview.chromium.org/2469813002/diff/150001/base/test/fuzzed_data_...
File base/test/fuzzed_data_provider.cc (right):

https://codereview.chromium.org/2469813002/diff/150001/base/test/fuzzed_data_...
base/test/fuzzed_data_provider.cc:58: // Reads bytes from start of
|remaining_data_|. Maps "\\" to "\", and maps "\"
On 2016/11/15 13:38:50, Paweł Hajdan Jr. wrote:
> Why do we care about backslashes?
> 
> Is this the right level of abstraction for this logic?

I'm not following.  We need a random string that can contain any character, with
a length from 1 to max_length.  We also want it to be relatively stable against
mutation, allowing the fuzzer to insert characters into it (Which length
prefix/suffix + characters doesn't do).

The simplest way to do this is with a magic sequence of characters that mean end
of string, but then that necessitates using an escape sequence of some sort, to
allow any string to be encoded.

[mmenke, 2016-11-15 13:46:59]

https://codereview.chromium.org/2469813002/diff/150001/base/test/fuzzed_data_...
File base/test/fuzzed_data_provider.cc (right):

https://codereview.chromium.org/2469813002/diff/150001/base/test/fuzzed_data_...
base/test/fuzzed_data_provider.cc:58: // Reads bytes from start of
|remaining_data_|. Maps "\\" to "\", and maps "\"
On 2016/11/15 13:43:48, mmenke wrote:
> On 2016/11/15 13:38:50, Paweł Hajdan Jr. wrote:
> > Why do we care about backslashes?
> > 
> > Is this the right level of abstraction for this logic?
> 
> I'm not following.  We need a random string that can contain any character,
with
> a length from 1 to max_length.  We also want it to be relatively stable
against
> mutation, allowing the fuzzer to insert characters into it (Which length
> prefix/suffix + characters doesn't do).
> 
> The simplest way to do this is with a magic sequence of characters that mean
end
> of string, but then that necessitates using an escape sequence of some sort,
to
> allow any string to be encoded.

Sorry, "From length 0 to max_length".

[mmenke, 2016-11-15 13:55:33]

https://codereview.chromium.org/2469813002/diff/150001/base/test/fuzzed_data_...
File base/test/fuzzed_data_provider.cc (right):

https://codereview.chromium.org/2469813002/diff/150001/base/test/fuzzed_data_...
base/test/fuzzed_data_provider.cc:58: // Reads bytes from start of
|remaining_data_|. Maps "\\" to "\", and maps "\"
On 2016/11/15 13:46:59, mmenke wrote:
> On 2016/11/15 13:43:48, mmenke wrote:
> > On 2016/11/15 13:38:50, Paweł Hajdan Jr. wrote:
> > > Why do we care about backslashes?
> > > 
> > > Is this the right level of abstraction for this logic?
> > 
> > I'm not following.  We need a random string that can contain any character,
> with
> > a length from 1 to max_length.  We also want it to be relatively stable
> against
> > mutation, allowing the fuzzer to insert characters into it (Which length
> > prefix/suffix + characters doesn't do).
> > 
> > The simplest way to do this is with a magic sequence of characters that mean
> end
> > of string, but then that necessitates using an escape sequence of some sort,
> to
> > allow any string to be encoded.
> 
> Sorry, "From length 0 to max_length".

And, just so we're on the same page, the fuzzers generate random input (Based on
previous inputs that were interesting, and possibly a dictionary to help with
the search), send them to the fuzzer code, which then can use this class to
break up the input into bite-sized chunks.  Consumers don't need to use these
escape characters directly themselves.

In my tests, this method resulted in faster exploration of the search space than
picking out a length from the fuzzed data, and then creating a string of exactly
that many characters.

[Paweł Hajdan Jr., 2016-11-21 17:21:42]

LGTM

[mmenke, 2016-11-21 17:22:31]

On 2016/11/21 17:21:42, Paweł Hajdan Jr. wrote:
> LGTM

Thanks!

[mmenke, 2016-11-21 17:22:37]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2016-11-21 17:22:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
csharrison@chromium.org, eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2469813002/#ps150001
(title: "Fix merge of net/BUILD.gn")

[commit-bot: I haz the power, 2016-11-21 17:23:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-21 17:31:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-21 17:31:50]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[mmenke, 2016-11-21 17:37:58]

It's complainint about windows line endings in the fuzzer input files (Which FTP
requires).  Going to go ahead and skip presubmit checks.

[mmenke, 2016-11-21 17:39:38]

Description was changed from

==========
Add a URLRequest FTP fuzzer.

Also add a new method to FuzzedDataProvider to provide a random length
string, with a max length. Generating the string this way allows for
the fuzzer to mutate it more easily than the previous pattern of
picking length from the end of input and then choosing that many
characters from the start of the input.

FuzzedSockets are switched to use the new method.  It will cause them
to need to rebuild their corpuses, but should result in faster
convergence, longer term.

BUG=660869
==========

to

==========
Add a URLRequest FTP fuzzer.

Also add a new method to FuzzedDataProvider to provide a random length
string, with a max length. Generating the string this way allows for
the fuzzer to mutate it more easily than the previous pattern of
picking length from the end of input and then choosing that many
characters from the start of the input.

FuzzedSockets are switched to use the new method.  It will cause them
to need to rebuild their corpuses, but should result in faster
convergence, longer term.

BUG=660869
NOPRESUBMIT=true
==========

[mmenke, 2016-11-21 17:39:49]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2016-11-21 17:40:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-21 18:33:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-21 18:33:47]

Failed to apply patch for net/BUILD.gn:
While running git apply --index -p1;
  error: patch failed: net/BUILD.gn:1592
  error: net/BUILD.gn: patch does not apply

Patch:       net/BUILD.gn
Index: net/BUILD.gn
diff --git a/net/BUILD.gn b/net/BUILD.gn
index
6ed970836b1703428d2ab7cdd08f7e4b3884e1c9..959c68e5b9c9d7449ed138fdb96455ba9ef9284b
100644
--- a/net/BUILD.gn
+++ b/net/BUILD.gn
@@ -1592,6 +1592,8 @@ source_set("net_fuzzer_test_support") {
 
   sources = [
     "base/fuzzer_test_support.cc",
+    "dns/fuzzed_host_resolver.cc",
+    "dns/fuzzed_host_resolver.h",
     "filter/fuzzed_source_stream.cc",
     "filter/fuzzed_source_stream.h",
     "socket/fuzzed_socket.cc",
@@ -1788,8 +1790,6 @@ fuzzer_test("net_dns_hosts_parse_fuzzer") {
 
 fuzzer_test("net_host_resolver_impl_fuzzer") {
   sources = [
-    "dns/fuzzed_host_resolver.cc",
-    "dns/fuzzed_host_resolver.h",
     "dns/host_resolver_impl_fuzzer.cc",
   ]
   deps = [
@@ -1982,6 +1982,20 @@ fuzzer_test("net_socks5_client_socket_fuzzer") {
   ]
 }
 
+fuzzer_test("net_url_request_ftp_fuzzer") {
+  sources = [
+    "url_request/url_request_ftp_fuzzer.cc",
+  ]
+  deps = [
+    ":net_fuzzer_test_support",
+    ":test_support",
+    "//base",
+    "//net",
+  ]
+  dict = "data/fuzzer_dictionaries/net_url_request_ftp_fuzzer.dict"
+  seed_corpus = "data/fuzzer_data/net_url_request_ftp_fuzzer/"
+}
+
 fuzzer_test("net_url_request_fuzzer") {
   sources = [
     "url_request/url_request_fuzzer.cc",

[mmenke, 2016-11-21 19:57:17]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2016-11-21 19:57:18]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org,
csharrison@chromium.org, phajdan.jr@chromium.org
Link to the patchset: https://codereview.chromium.org/2469813002/#ps170001
(title: "merge")

[commit-bot: I haz the power, 2016-11-21 19:58:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-21 21:12:34]

CQ is committing da patch.

Bot data: {"patchset_id": 170001, "attempt_start_ts": 1479758237577950,
"parent_rev": "fb779f01d1ec8e1c8913e3eb71224f36f302cf42", "commit_rev":
"07873e758dd69b81db924bad64aee3b5f08aefaa"}

[commit-bot: I haz the power, 2016-11-21 21:13:04]

Description was changed from

==========
Add a URLRequest FTP fuzzer.

Also add a new method to FuzzedDataProvider to provide a random length
string, with a max length. Generating the string this way allows for
the fuzzer to mutate it more easily than the previous pattern of
picking length from the end of input and then choosing that many
characters from the start of the input.

FuzzedSockets are switched to use the new method.  It will cause them
to need to rebuild their corpuses, but should result in faster
convergence, longer term.

BUG=660869
NOPRESUBMIT=true
==========

to

==========
Add a URLRequest FTP fuzzer.

Also add a new method to FuzzedDataProvider to provide a random length
string, with a max length. Generating the string this way allows for
the fuzzer to mutate it more easily than the previous pattern of
picking length from the end of input and then choosing that many
characters from the start of the input.

FuzzedSockets are switched to use the new method.  It will cause them
to need to rebuild their corpuses, but should result in faster
convergence, longer term.

BUG=660869
NOPRESUBMIT=true
==========

[commit-bot: I haz the power, 2016-11-21 21:13:06]

Committed patchset #8 (id:170001)

[commit-bot: I haz the power, 2016-11-21 21:16:27]

Description was changed from

==========
Add a URLRequest FTP fuzzer.

Also add a new method to FuzzedDataProvider to provide a random length
string, with a max length. Generating the string this way allows for
the fuzzer to mutate it more easily than the previous pattern of
picking length from the end of input and then choosing that many
characters from the start of the input.

FuzzedSockets are switched to use the new method.  It will cause them
to need to rebuild their corpuses, but should result in faster
convergence, longer term.

BUG=660869
NOPRESUBMIT=true
==========

to

==========
Add a URLRequest FTP fuzzer.

Also add a new method to FuzzedDataProvider to provide a random length
string, with a max length. Generating the string this way allows for
the fuzzer to mutate it more easily than the previous pattern of
picking length from the end of input and then choosing that many
characters from the start of the input.

FuzzedSockets are switched to use the new method.  It will cause them
to need to rebuild their corpuses, but should result in faster
convergence, longer term.

BUG=660869
NOPRESUBMIT=true

Committed: https://crrev.com/a7da0714bb1e387b30298047a9fa81c0ed2a2d13
Cr-Commit-Position: refs/heads/master@{#433652}
==========

[commit-bot: I haz the power, 2016-11-21 21:16:29]

Patchset 8 (id:??) landed as
https://crrev.com/a7da0714bb1e387b30298047a9fa81c0ed2a2d13
Cr-Commit-Position: refs/heads/master@{#433652}