Add fuzzer for ParseURLHostnameToAddress to //net.

Add fuzzer for ParseURLHostnameToAddress to //net.

BUG=

Committed: https://crrev.com/9299ef9b548194db44baba9d70c05831de91edc6
Cr-Commit-Position: refs/heads/master@{#415987}

[mmoroz, 2016-08-30 13:19:55]

Description was changed from

==========
Add fuzzer for ParseURLHostnameToAddress to //net.

BUG=
==========

to

==========
Add fuzzer for ParseURLHostnameToAddress to //net.

BUG=
==========

[mmoroz, 2016-08-30 13:19:58]

mmoroz@chromium.org changed reviewers:
+ aizatsky@chromium.org, eroman@chromium.org, ochang@chromium.org

[mmoroz, 2016-08-30 13:26:58]

On 2016/08/30 13:19:58, mmoroz wrote:
> mailto:mmoroz@chromium.org changed reviewers:
> + mailto:aizatsky@chromium.org, mailto:eroman@chromium.org,
mailto:ochang@chromium.org

I was looking through the codebase and noticed that some test code for
"WebSocket::Connect()" looks like a potential fuzz target.
I followed the sequence of function calls from
https://cs.chromium.org/chromium/src/chrome/test/chromedriver/net/websocket.c...

but removed ResolveHost() part.

[eroman, 2016-08-30 17:24:22]

lgtm

https://codereview.chromium.org/2291133002/diff/1/net/base/parse_url_hostname...
File net/base/parse_url_hostname_to_address_fuzzer.cc (right):

https://codereview.chromium.org/2291133002/diff/1/net/base/parse_url_hostname...
net/base/parse_url_hostname_to_address_fuzzer.cc:34: uint16_t port = CRC16(data,
size);
Please document why using a hash for the port (rather than either a fixed port,
or consuming it directly from input).

I am assuming your thinking is there is value in varying the port, but not
enough to enumerate it as part of fuzz input?

https://codereview.chromium.org/2291133002/diff/1/net/base/parse_url_hostname...
net/base/parse_url_hostname_to_address_fuzzer.cc:38: for (auto endpoint :
addresses) {
nit: const auto&

https://codereview.chromium.org/2291133002/diff/1/net/base/parse_url_hostname...
net/base/parse_url_hostname_to_address_fuzzer.cc:43: if
(base::JSONWriter::Write(endpoints, &json)) {
Why the dependency on constructing a base::Value and JSONWriter?

https://codereview.chromium.org/2291133002/diff/1/net/data/fuzzer_data/hostna...
File net/data/fuzzer_data/hostnames/ipv4.txt (right):

https://codereview.chromium.org/2291133002/diff/1/net/data/fuzzer_data/hostna...
net/data/fuzzer_data/hostnames/ipv4.txt:1: 192.168.246.137
If you want other data, these are all valid IP addresses too (as crazy as they
seem)

192.168.9.1.2
0300.0250.00.01
0xC0.0Xa8.0x0.0x1
192
0xC0a80001
030052000001
192.168.0.1.
0000000000000300.0x00000000000000fF.00000000000000001
[::ffff:192.168.0.1]
[::ffff:192.1.2]
[::ffff:0xC0.0Xa8.0x0.0x1]
[0:0::0:0:8]


I expect this fuzz tester will mostly be exploring GURLs parsing space (which is
true of a great many net unit tests).

[aizatsky, 2016-08-30 18:18:31]

https://codereview.chromium.org/2291133002/diff/1/net/base/parse_url_hostname...
File net/base/parse_url_hostname_to_address_fuzzer.cc (right):

https://codereview.chromium.org/2291133002/diff/1/net/base/parse_url_hostname...
net/base/parse_url_hostname_to_address_fuzzer.cc:34: uint16_t port = CRC16(data,
size);
On 2016/08/30 at 17:24:21, eroman wrote:
> Please document why using a hash for the port (rather than either a fixed
port, or consuming it directly from input).
> 
> I am assuming your thinking is there is value in varying the port, but not
enough to enumerate it as part of fuzz input?

If don't need CRC16 specifically, I suggest you use std::hash. 

(https://cs.chromium.org/chromium/src/third_party/icu/fuzzers/fuzzer_utils.h?r...)

https://codereview.chromium.org/2291133002/diff/1/net/base/parse_url_hostname...
net/base/parse_url_hostname_to_address_fuzzer.cc:43: if
(base::JSONWriter::Write(endpoints, &json)) {
On 2016/08/30 at 17:24:21, eroman wrote:
> Why the dependency on constructing a base::Value and JSONWriter?

+1. Simply calling ToString on endpoint should be enough. No need to fuzz JSON
writer.

[mmoroz, 2016-08-31 11:18:25]

https://codereview.chromium.org/2291133002/diff/1/net/base/parse_url_hostname...
File net/base/parse_url_hostname_to_address_fuzzer.cc (right):

https://codereview.chromium.org/2291133002/diff/1/net/base/parse_url_hostname...
net/base/parse_url_hostname_to_address_fuzzer.cc:34: uint16_t port = CRC16(data,
size);
On 2016/08/30 17:24:21, eroman wrote:
> Please document why using a hash for the port (rather than either a fixed
port,
> or consuming it directly from input).
> 
> I am assuming your thinking is there is value in varying the port, but not
> enough to enumerate it as part of fuzz input?

Yes, exactly! Added a comment on that.

https://codereview.chromium.org/2291133002/diff/1/net/base/parse_url_hostname...
net/base/parse_url_hostname_to_address_fuzzer.cc:34: uint16_t port = CRC16(data,
size);
On 2016/08/30 18:18:31, aizatsky wrote:
> On 2016/08/30 at 17:24:21, eroman wrote:
> > Please document why using a hash for the port (rather than either a fixed
> port, or consuming it directly from input).
> > 
> > I am assuming your thinking is there is value in varying the port, but not
> enough to enumerate it as part of fuzz input?
> 
> If don't need CRC16 specifically, I suggest you use std::hash. 
> 
>
(https://cs.chromium.org/chromium/src/third_party/icu/fuzzers/fuzzer_utils.h?r...)

Thanks for the suggestion. Done!

https://codereview.chromium.org/2291133002/diff/1/net/base/parse_url_hostname...
net/base/parse_url_hostname_to_address_fuzzer.cc:38: for (auto endpoint :
addresses) {
On 2016/08/30 17:24:21, eroman wrote:
> nit: const auto&

Done.

https://codereview.chromium.org/2291133002/diff/1/net/base/parse_url_hostname...
net/base/parse_url_hostname_to_address_fuzzer.cc:43: if
(base::JSONWriter::Write(endpoints, &json)) {
On 2016/08/30 18:18:31, aizatsky wrote:
> On 2016/08/30 at 17:24:21, eroman wrote:
> > Why the dependency on constructing a base::Value and JSONWriter?
> 
> +1. Simply calling ToString on endpoint should be enough. No need to fuzz JSON
> writer.

Done.

https://codereview.chromium.org/2291133002/diff/1/net/data/fuzzer_data/hostna...
File net/data/fuzzer_data/hostnames/ipv4.txt (right):

https://codereview.chromium.org/2291133002/diff/1/net/data/fuzzer_data/hostna...
net/data/fuzzer_data/hostnames/ipv4.txt:1: 192.168.246.137
On 2016/08/30 17:24:22, eroman wrote:
> If you want other data, these are all valid IP addresses too (as crazy as they
> seem)
> 
> 192.168.9.1.2
> 0300.0250.00.01
> 0xC0.0Xa8.0x0.0x1
> 192
> 0xC0a80001
> 030052000001
> 192.168.0.1.
> 0000000000000300.0x00000000000000fF.00000000000000001
> [::ffff:192.168.0.1]
> [::ffff:192.1.2]
> [::ffff:0xC0.0Xa8.0x0.0x1]
> [0:0::0:0:8]
> 
> 
> I expect this fuzz tester will mostly be exploring GURLs parsing space (which
is
> true of a great many net unit tests).

Nice set! Added all of them, thanks!

[aizatsky, 2016-08-31 18:15:40]

https://codereview.chromium.org/2291133002/diff/40001/net/base/parse_url_host...
File net/base/parse_url_hostname_to_address_fuzzer.cc (right):

https://codereview.chromium.org/2291133002/diff/40001/net/base/parse_url_host...
net/base/parse_url_hostname_to_address_fuzzer.cc:26: base::ListValue endpoints;
I meant it even like this:

  ...
  for (const auto& endpoint : addresses) {
      endpoint.ToStringWithoutPort();
  }
  return 0;

no need for json at all.

[eroman, 2016-08-31 18:23:51]

https://codereview.chromium.org/2291133002/diff/40001/net/base/parse_url_host...
File net/base/parse_url_hostname_to_address_fuzzer.cc (right):

https://codereview.chromium.org/2291133002/diff/40001/net/base/parse_url_host...
net/base/parse_url_hostname_to_address_fuzzer.cc:26: base::ListValue endpoints;
On 2016/08/31 18:15:40, aizatsky wrote:
> I meant it even like this:
> 
>   ...
>   for (const auto& endpoint : addresses) {
>       endpoint.ToStringWithoutPort();
>   }
>   return 0;
> 
> no need for json at all.

+1

Is your concern is that the compiler will be smart enough to optimize away the
inside body of the loop, or do you actually want to test the
ToStringWithoutPort() functionality as well?

If it is just the former, you can add this:

   base::debug::Alias(&addresses);

Which should prevent the compiler from getting too clever.

[eroman, 2016-08-31 18:28:33]

https://codereview.chromium.org/2291133002/diff/40001/net/base/parse_url_host...
File net/base/parse_url_hostname_to_address_fuzzer.cc (right):

https://codereview.chromium.org/2291133002/diff/40001/net/base/parse_url_host...
net/base/parse_url_hostname_to_address_fuzzer.cc:26: base::ListValue endpoints;
On 2016/08/31 18:23:50, eroman wrote:
> On 2016/08/31 18:15:40, aizatsky wrote:
> > I meant it even like this:
> > 
> >   ...
> >   for (const auto& endpoint : addresses) {
> >       endpoint.ToStringWithoutPort();
> >   }
> >   return 0;
> > 
> > no need for json at all.
> 
> +1
> 
> Is your concern is that the compiler will be smart enough to optimize away the
> inside body of the loop, or do you actually want to test the
> ToStringWithoutPort() functionality as well?
> 
> If it is just the former, you can add this:
> 
>    base::debug::Alias(&addresses);
> 
> Which should prevent the compiler from getting too clever.

(FWIW I agree that calling ToStringWithPort() is a good idea)

[mmoroz, 2016-09-01 10:46:21]

https://codereview.chromium.org/2291133002/diff/40001/net/base/parse_url_host...
File net/base/parse_url_hostname_to_address_fuzzer.cc (right):

https://codereview.chromium.org/2291133002/diff/40001/net/base/parse_url_host...
net/base/parse_url_hostname_to_address_fuzzer.cc:26: base::ListValue endpoints;
On 2016/08/31 18:28:33, eroman wrote:
> On 2016/08/31 18:23:50, eroman wrote:
> > On 2016/08/31 18:15:40, aizatsky wrote:
> > > I meant it even like this:
> > > 
> > >   ...
> > >   for (const auto& endpoint : addresses) {
> > >       endpoint.ToStringWithoutPort();
> > >   }
> > >   return 0;
> > > 
> > > no need for json at all.
> > 
> > +1
> > 
> > Is your concern is that the compiler will be smart enough to optimize away
the
> > inside body of the loop, or do you actually want to test the
> > ToStringWithoutPort() functionality as well?
> > 
> > If it is just the former, you can add this:
> > 
> >    base::debug::Alias(&addresses);
> > 
> > Which should prevent the compiler from getting too clever.
> 
> (FWIW I agree that calling ToStringWithPort() is a good idea)

Ah, sorry for misunderstanding from my side! Looks like ToString() method (which
Mike suggested initially, I guess) does the same as ToStringWithPort() plus adds
the port. I would like to test it as well.

Regarding compiler optimizations, I've checked the binary with IDA and verified
that the loop's body is not removed!

[mmoroz, 2016-09-01 11:12:13]

Patchset #5 (id:80001) has been deleted

[aizatsky, 2016-09-01 15:56:36]

lgtm

[aizatsky, 2016-09-01 15:57:54]

> Regarding compiler optimizations, I've checked the binary with IDA and
verified that the loop's body is not removed!

the ToString() method usually has lots of potential heap allocations (big
strings). I doubt compiler can easily optimize it out.

[mmoroz, 2016-09-01 16:41:59]

The CQ bit was checked by mmoroz@chromium.org

[mmoroz, 2016-09-01 16:41:59]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2291133002/#ps60001
(title: "Call ToString() for every endpoint, remove base::ListValue and second
loop.")

[commit-bot: I haz the power, 2016-09-01 16:42:16]

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-09-01 17:37:31]

Description was changed from

==========
Add fuzzer for ParseURLHostnameToAddress to //net.

BUG=
==========

to

==========
Add fuzzer for ParseURLHostnameToAddress to //net.

BUG=
==========

[commit-bot: I haz the power, 2016-09-01 17:37:34]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-01 17:40:03]

Description was changed from

==========
Add fuzzer for ParseURLHostnameToAddress to //net.

BUG=
==========

to

==========
Add fuzzer for ParseURLHostnameToAddress to //net.

BUG=

Committed: https://crrev.com/9299ef9b548194db44baba9d70c05831de91edc6
Cr-Commit-Position: refs/heads/master@{#415987}
==========

[commit-bot: I haz the power, 2016-09-01 17:40:04]

Patchset 4 (id:??) landed as
https://crrev.com/9299ef9b548194db44baba9d70c05831de91edc6
Cr-Commit-Position: refs/heads/master@{#415987}