A simple fuzzer for HttpServer, with limited coverage of WebSocket.

A simple fuzzer for HttpServer, with limited coverage of WebSocket.

BUG=606428
Review-Url: https://codereview.chromium.org/2649983003
Cr-Commit-Position: refs/heads/master@{#446980}
Committed: https://chromium.googlesource.com/chromium/src/+/5e6e19b1a3b499719ebd2243869adbb8e4271350

[Maks Orlovich, 2017-01-23 20:49:47]

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

[commit-bot: I haz the power, 2017-01-23 20:50:12]

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, 2017-01-23 22:08:42]

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

[commit-bot: I haz the power, 2017-01-23 22:08:45]

Dry run: This issue passed the CQ dry run.

[Maks Orlovich, 2017-01-24 15:05:16]

morlovich@chromium.org changed reviewers:
+ mmenke@chromium.org

[Maks Orlovich, 2017-01-24 15:05:16]

So I think this is far along enough now to get some feedback.

One thing I am really not sure of is how to best deal with websocket --- I can
probably cover it better with a few more seed testcases to cover e.g. the length
handling and the like, but I am wondering about the proper handling of deflate
mode --- doesn't seem like making the fuzzer go through zlib's search space is
appropriate in this case.

[mmenke, 2017-01-24 16:29:17]

Looking good

https://codereview.chromium.org/2649983003/diff/1/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/2649983003/diff/1/net/BUILD.gn#newcode2097
net/BUILD.gn:2097: libfuzzer_options = [ "max_len=256" ]
Not needed - the default max length is significantly longer (And varies over
iteration), and I don't think there's a need to shorten it?

https://codereview.chromium.org/2649983003/diff/1/net/server/http_server_fuzz...
File net/server/http_server_fuzzer.cc (right):

https://codereview.chromium.org/2649983003/diff/1/net/server/http_server_fuzz...
net/server/http_server_fuzzer.cc:19: should_accept_webp_ =
data_provider_->ConsumeBool();
This variable name is confusing.  webp is the name of an image format.
should_accept_web_socket_?

https://codereview.chromium.org/2649983003/diff/1/net/server/http_server_fuzz...
net/server/http_server_fuzzer.cc:24: void OnConnect(int connection_id) override
{}
Randomly close the socket or not here?

https://codereview.chromium.org/2649983003/diff/1/net/server/http_server_fuzz...
net/server/http_server_fuzzer.cc:26: const net::HttpServerRequestInfo& info)
override {}
Should we randomly write data or close the connection here?

https://codereview.chromium.org/2649983003/diff/1/net/server/http_server_fuzz...
net/server/http_server_fuzzer.cc:36: void OnWebSocketMessage(int connection_id,
const std::string& data) override {
Should we randomly write data or close the connection here?

https://codereview.chromium.org/2649983003/diff/1/net/server/http_server_fuzz...
net/server/http_server_fuzzer.cc:42: base::FuzzedDataProvider* data_provider_;
nit:

base::FuzzedDataProvider* const

(Meaning the address pointed at don't change).

https://codereview.chromium.org/2649983003/diff/1/net/server/http_server_fuzz...
net/server/http_server_fuzzer.cc:44: bool should_accept_webp_;
add this to the initializer list and make it a const?

https://codereview.chromium.org/2649983003/diff/1/net/server/http_server_fuzz...
net/server/http_server_fuzzer.cc:44: bool should_accept_webp_;
DISALLOW_COPY_AND_ASSIGN + include base/macros.h

https://codereview.chromium.org/2649983003/diff/1/net/server/http_server_fuzz...
net/server/http_server_fuzzer.cc:53: net::BoundTestNetLog bound_test_net_log;
Should just be using TestNetLog (Then don't need the bound().net_log() stuff,
can just pass in its address directly)

https://codereview.chromium.org/2649983003/diff/1/net/server/http_server_fuzz...
net/server/http_server_fuzzer.cc:59: CHECK_EQ(net::OK,
include net/base/net_errors.h and base/logging.h

https://codereview.chromium.org/2649983003/diff/1/net/server/http_server_fuzz...
net/server/http_server_fuzzer.cc:62: net::TestCompletionCallback callback;  //
provides the message loop
The comment isn't correct.  It can spin the message loop, but the message loop
itself comes from net/base/fuzzer_test_support.cc.

https://codereview.chromium.org/2649983003/diff/1/net/server/http_server_fuzz...
net/server/http_server_fuzzer.cc:66: CHECK_EQ(net::OK,
callback.WaitForResult());
Why check the result?

Also, I don't think we really get anything from using a TestCompletionCallback,
instead of just using a RunLoop directly.

https://codereview.chromium.org/2649983003/diff/1/net/socket/fuzzed_server_so...
File net/socket/fuzzed_server_socket.cc (right):

https://codereview.chromium.org/2649983003/diff/1/net/socket/fuzzed_server_so...
net/socket/fuzzed_server_socket.cc:26: DCHECK(!listen_called_);
nit:  include base/logging.h

https://codereview.chromium.org/2649983003/diff/1/net/socket/fuzzed_server_so...
net/socket/fuzzed_server_socket.cc:29: return OK;
include net_errors.h

https://codereview.chromium.org/2649983003/diff/1/net/socket/fuzzed_server_so...
net/socket/fuzzed_server_socket.cc:46: return ERR_IO_PENDING;
Accept can technically succeed synchronously, can't it?  Not exactly likely,
admittedly (And adding it probably doesn't expand test coverage much, over what
unittests already get us).

https://codereview.chromium.org/2649983003/diff/1/net/socket/fuzzed_server_so...
net/socket/fuzzed_server_socket.cc:58: callback.Run(OK);
I guess the test fixture wouldn't exit properly on sync, async accept errors?

https://codereview.chromium.org/2649983003/diff/1/net/socket/fuzzed_server_so...
File net/socket/fuzzed_server_socket.h (right):

https://codereview.chromium.org/2649983003/diff/1/net/socket/fuzzed_server_so...
net/socket/fuzzed_server_socket.h:8: #include <stdint.h>
I don't think this is being used in this header?

https://codereview.chromium.org/2649983003/diff/1/net/socket/fuzzed_server_so...
net/socket/fuzzed_server_socket.h:58:
DISALLOW_COPY_AND_ASSIGN(FuzzedServerSocket);
nit:  Blank line before DISALLOW_COPY_AND_ASSIGN

[Maks Orlovich, 2017-01-24 18:34:19]

Working on most of the rest of the comments, flushing the stuff I had a question
on and a few most trivial things.

https://codereview.chromium.org/2649983003/diff/1/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/2649983003/diff/1/net/BUILD.gn#newcode2097
net/BUILD.gn:2097: libfuzzer_options = [ "max_len=256" ]
On 2017/01/24 16:29:16, mmenke wrote:
> Not needed - the default max length is significantly longer (And varies over
> iteration), and I don't think there's a need to shorten it?

The default if I run it from command-line is 64; is it different for
ClusterFuzz?

(Coincidentally, is there an easy way of running the fuzzer with the flags
provided here?)

https://codereview.chromium.org/2649983003/diff/1/net/server/http_server_fuzz...
File net/server/http_server_fuzzer.cc (right):

https://codereview.chromium.org/2649983003/diff/1/net/server/http_server_fuzz...
net/server/http_server_fuzzer.cc:19: should_accept_webp_ =
data_provider_->ConsumeBool();
On 2017/01/24 16:29:16, mmenke wrote:
> This variable name is confusing.  webp is the name of an image format.
> should_accept_web_socket_?

Err, yeah. My past life shining through.

https://codereview.chromium.org/2649983003/diff/1/net/server/http_server_fuzz...
net/server/http_server_fuzzer.cc:42: base::FuzzedDataProvider* data_provider_;
On 2017/01/24 16:29:16, mmenke wrote:
> nit:
> 
> base::FuzzedDataProvider* const
> 
> (Meaning the address pointed at don't change).

Done.

https://codereview.chromium.org/2649983003/diff/1/net/server/http_server_fuzz...
net/server/http_server_fuzzer.cc:44: bool should_accept_webp_;
On 2017/01/24 16:29:16, mmenke wrote:
> DISALLOW_COPY_AND_ASSIGN + include base/macros.h

Done.

[mmenke, 2017-01-24 19:03:42]

https://codereview.chromium.org/2649983003/diff/1/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/2649983003/diff/1/net/BUILD.gn#newcode2097
net/BUILD.gn:2097: libfuzzer_options = [ "max_len=256" ]
On 2017/01/24 18:34:18, morlovich1 wrote:
> On 2017/01/24 16:29:16, mmenke wrote:
> > Not needed - the default max length is significantly longer (And varies over
> > iteration), and I don't think there's a need to shorten it?
> 
> The default if I run it from command-line is 64; is it different for
> ClusterFuzz?

Yes.  I think ClusterFuzz randomly varies it on a per-run basis from 1k to 4k. 
It's in docs somewhere, not positive of the exact values, but certainly over 1k.

> (Coincidentally, is there an easy way of running the fuzzer with the flags
> provided here?)

Not that I'm aware of.

[mmenke, 2017-01-24 19:43:58]

Two more comments.

https://codereview.chromium.org/2649983003/diff/1/net/server/http_server_fuzz...
File net/server/http_server_fuzzer.cc (right):

https://codereview.chromium.org/2649983003/diff/1/net/server/http_server_fuzz...
net/server/http_server_fuzzer.cc:26: const net::HttpServerRequestInfo& info)
override {}
On 2017/01/24 16:29:16, mmenke wrote:
> Should we randomly write data or close the connection here?

(Also, may want to do this multiple times, and synchronously and asynchronously)

https://codereview.chromium.org/2649983003/diff/1/net/server/http_server_fuzz...
net/server/http_server_fuzzer.cc:38: void OnClose(int connection_id) override {
callback_.Run(net::OK); }
Why is the socket guaranteed to be closed?

[Maks Orlovich, 2017-01-25 14:54:37]

The boring stuff (w/o the underlying CL, since I am still debugging some things)

https://codereview.chromium.org/2649983003/diff/1/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/2649983003/diff/1/net/BUILD.gn#newcode2097
net/BUILD.gn:2097: libfuzzer_options = [ "max_len=256" ]
Removed then.

https://codereview.chromium.org/2649983003/diff/1/net/server/http_server_fuzz...
File net/server/http_server_fuzzer.cc (right):

https://codereview.chromium.org/2649983003/diff/1/net/server/http_server_fuzz...
net/server/http_server_fuzzer.cc:24: void OnConnect(int connection_id) override
{}
On 2017/01/24 16:29:16, mmenke wrote:
> Randomly close the socket or not here?

Done.

https://codereview.chromium.org/2649983003/diff/1/net/server/http_server_fuzz...
net/server/http_server_fuzzer.cc:26: const net::HttpServerRequestInfo& info)
override {}
On 2017/01/24 16:29:16, mmenke wrote:
> Should we randomly write data or close the connection here?

Done.

https://codereview.chromium.org/2649983003/diff/1/net/server/http_server_fuzz...
net/server/http_server_fuzzer.cc:36: void OnWebSocketMessage(int connection_id,
const std::string& data) override {
On 2017/01/24 16:29:16, mmenke wrote:
> Should we randomly write data or close the connection here?

Done.

https://codereview.chromium.org/2649983003/diff/1/net/server/http_server_fuzz...
net/server/http_server_fuzzer.cc:53: net::BoundTestNetLog bound_test_net_log;
On 2017/01/24 16:29:16, mmenke wrote:
> Should just be using TestNetLog (Then don't need the bound().net_log() stuff,
> can just pass in its address directly)

Done.

https://codereview.chromium.org/2649983003/diff/1/net/server/http_server_fuzz...
net/server/http_server_fuzzer.cc:59: CHECK_EQ(net::OK,
On 2017/01/24 16:29:16, mmenke wrote:
> include net/base/net_errors.h and base/logging.h

Done.

https://codereview.chromium.org/2649983003/diff/1/net/server/http_server_fuzz...
net/server/http_server_fuzzer.cc:62: net::TestCompletionCallback callback;  //
provides the message loop
On 2017/01/24 16:29:16, mmenke wrote:
> The comment isn't correct.  It can spin the message loop, but the message loop
> itself comes from net/base/fuzzer_test_support.cc.

Spinning is what I meant; the sloppy comment is because not doing it was one of
the first things I did wrong; but at any rate w/RunLoop, as you suggested below,
its significance is clear.

https://codereview.chromium.org/2649983003/diff/1/net/server/http_server_fuzz...
net/server/http_server_fuzzer.cc:66: CHECK_EQ(net::OK,
callback.WaitForResult());
On 2017/01/24 16:29:16, mmenke wrote:
> Why check the result?
> 
> Also, I don't think we really get anything from using a
TestCompletionCallback,
> instead of just using a RunLoop directly.

Switched to RunLoop --- which made the result issue moot.

[Maks Orlovich, 2017-01-25 15:00:07]

> On 2017/01/24 16:29:16, mmenke wrote:
> > Should we randomly write data or close the connection here?
> 
> (Also, may want to do this multiple times, and synchronously and
asynchronously)

So... I am having a lot of difficulty with this because of competition for input
between 
the socket and the control making it either hard to produce frames that actually
hit the websocket path 
or make us run out of bits by the time I get to something like
OnWebSocketMessage 
(since that requires the socket to simulate having just enough bytes to have a
valid WebSocket upgrade + 
 WebSocket frame, and have enough bits left in underlying source to feed the
conditional).
It seems to help to read stuff in advance and have "\0x00\x00\x00\0x00" and
"\0x00\x00\x00\0x01" in the dictionary,
but that doesn't really work for multiple replies. I would appreciate any advice
you can give on this...


> 
>
https://codereview.chromium.org/2649983003/diff/1/net/server/http_server_fuzz...
> net/server/http_server_fuzzer.cc:38: void OnClose(int connection_id) override
{
> callback_.Run(net::OK); }
> Why is the socket guaranteed to be closed?

Eventually the underlying FuzzedSocket will run out of data, and issue an error,
which will 
cause HttpServer to close the connection.

[mmenke, 2017-01-25 16:19:12]

On 2017/01/25 15:00:07, morlovich1 wrote:
> > On 2017/01/24 16:29:16, mmenke wrote:
> > > Should we randomly write data or close the connection here?
> > 
> > (Also, may want to do this multiple times, and synchronously and
> asynchronously)
> 
> So... I am having a lot of difficulty with this because of competition for
input
> between 
> the socket and the control making it either hard to produce frames that
actually
> hit the websocket path 
> or make us run out of bits by the time I get to something like
> OnWebSocketMessage 
> (since that requires the socket to simulate having just enough bytes to have a
> valid WebSocket upgrade + 
>  WebSocket frame, and have enough bits left in underlying source to feed the
> conditional).
> It seems to help to read stuff in advance and have "\0x00\x00\x00\0x00" and
> "\0x00\x00\x00\0x01" in the dictionary,
> but that doesn't really work for multiple replies. I would appreciate any
advice
> you can give on this...
> 
> 
> > 
> >
>
https://codereview.chromium.org/2649983003/diff/1/net/server/http_server_fuzz...
> > net/server/http_server_fuzzer.cc:38: void OnClose(int connection_id)
override
> {
> > callback_.Run(net::OK); }
> > Why is the socket guaranteed to be closed?
> 
> Eventually the underlying FuzzedSocket will run out of data, and issue an
error,
> which will 
> cause HttpServer to close the connection.

Ah, you're right.  HttpServer keeps on reading requests and sending them to the
embedder even if the embedder hasn't finished uploading the response to the
previous request.  What could possibly go wrong there?  :)

[mmenke, 2017-01-25 16:27:23]

On 2017/01/25 15:00:07, morlovich1 wrote:
> > On 2017/01/24 16:29:16, mmenke wrote:
> > > Should we randomly write data or close the connection here?
> > 
> > (Also, may want to do this multiple times, and synchronously and
> asynchronously)
> 
> So... I am having a lot of difficulty with this because of competition for
input
> between 
> the socket and the control making it either hard to produce frames that
actually
> hit the websocket path 
> or make us run out of bits by the time I get to something like
> OnWebSocketMessage 
> (since that requires the socket to simulate having just enough bytes to have a
> valid WebSocket upgrade + 
>  WebSocket frame, and have enough bits left in underlying source to feed the
> conditional).
> It seems to help to read stuff in advance and have "\0x00\x00\x00\0x00" and
> "\0x00\x00\x00\0x01" in the dictionary,
> but that doesn't really work for multiple replies. I would appreciate any
advice
> you can give on this...

If you add "\\ " to the dictionary, they might help.  It's used by
FuzzedDataProvider::ConsumeRandomLengthString as a string terminator. 
Experimentally, it works much better than encoding length + bytes separately for
traversing the search space.

[Maks Orlovich, 2017-01-25 19:47:03]

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

[commit-bot: I haz the power, 2017-01-25 19:47:43]

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

[Maks Orlovich, 2017-01-25 20:57:39]

> Ah, you're right.  HttpServer keeps on reading requests and sending them to
the
> embedder even if the embedder hasn't finished uploading the response to the
> previous request.  What could possibly go wrong there?  :)

I think it'll just accidentally invent HTTP pipelining, unless the methods are 
called from the wrong thread?

[Maks Orlovich, 2017-01-25 21:01:02]

> If you add "\\ " to the dictionary, they might help.  It's used by
> FuzzedDataProvider::ConsumeRandomLengthString as a string terminator. 
> Experimentally, it works much better than encoding length + bytes separately
for
> traversing the search space.

This did help, as did having all the WebSocket handshake headers as a single
blob 
in the dictionary rather as separate ones. 

I think I still need to add async options for the ops. The coverage also has
this not producing 
WebSocket replies, which is a bit weird.

[commit-bot: I haz the power, 2017-01-25 23:11:39]

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

[commit-bot: I haz the power, 2017-01-25 23:11:40]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[mmenke, 2017-01-26 13:21:37]

On 2017/01/25 20:57:39, morlovich1 wrote:
> > Ah, you're right.  HttpServer keeps on reading requests and sending them to
> the
> > embedder even if the embedder hasn't finished uploading the response to the
> > previous request.  What could possibly go wrong there?  :)
> 
> I think it'll just accidentally invent HTTP pipelining, unless the methods are

> called from the wrong thread?

Anything that streams the response with multiple async writes would get the
responses jumbled together.  I know DevTools uses multiple async writes for data
it gets from the renderer.  That seems rather bad.

[mmenke, 2017-01-26 13:23:01]

On 2017/01/26 13:21:37, mmenke wrote:
> On 2017/01/25 20:57:39, morlovich1 wrote:
> > > Ah, you're right.  HttpServer keeps on reading requests and sending them
to
> > the
> > > embedder even if the embedder hasn't finished uploading the response to
the
> > > previous request.  What could possibly go wrong there?  :)
> > 
> > I think it'll just accidentally invent HTTP pipelining, unless the methods
are
> 
> > called from the wrong thread?
> 
> Anything that streams the response with multiple async writes would get the
> responses jumbled together.  I know DevTools uses multiple async writes for
data
> it gets from the renderer.  That seems rather bad.

Devtools may be smart enough to wait until one reply is sent before sending the
next, not sure, but the API really should force consumers to do that, rather
than on relying them all to implement it themselves.

[morlovich, 2017-01-26 14:22:06]

> > Anything that streams the response with multiple async writes would get the
> > responses jumbled together.  I know DevTools uses multiple async writes for
> data
> > it gets from the renderer.  That seems rather bad.
> 
> Devtools may be smart enough to wait until one reply is sent before sending
the
> next, not sure, but the API really should force consumers to do that, rather
> than on relying them all to implement it themselves.

I don't think this is actually a problem --- if I can trust codesearch, that is;
don't know how reliable its cross-references are in Chromeland.

The only HTTP API that's streaming is SendRaw and that's not used outside
net/server. 
Everything else is single-shot (there is no chunked encoding implemented in 
HttpServer at all, after all, which would require a streaming client to do
it...).

So may be the answer might be to make SendRaw private and add some friendships
or something?

There is also SendOverWebSocket, of course, which is used in devtools, but that
seems OK with the connection switched over to the protocol?

(Apologies for using the other account, can't seem to make my laptop's security
key
 work for the chromium one...)

[mmenke, 2017-01-26 20:23:36]

On 2017/01/26 14:22:06, morlovich wrote:
> > > Anything that streams the response with multiple async writes would get
the
> > > responses jumbled together.  I know DevTools uses multiple async writes
for
> > data
> > > it gets from the renderer.  That seems rather bad.
> > 
> > Devtools may be smart enough to wait until one reply is sent before sending
> the
> > next, not sure, but the API really should force consumers to do that, rather
> > than on relying them all to implement it themselves.
> 
> I don't think this is actually a problem --- if I can trust codesearch, that
is;
> don't know how reliable its cross-references are in Chromeland.

Ah, devtools uses WebSockets, so presumably that's why it avoids the issue I was
concerned about.

> The only HTTP API that's streaming is SendRaw and that's not used outside
> net/server. 
> Everything else is single-shot (there is no chunked encoding implemented in 
> HttpServer at all, after all, which would require a streaming client to do
> it...).
> 
> So may be the answer might be to make SendRaw private and add some friendships
> or something?

I'd actually like to go the opposite direction - encourage consumers to stream
data to us instead of sending it to us in massive chunks, and have them tell us
when they're done sending a response.  It would be a beefier API, but the fact
is that some consumers have run up against the memory limit and so need to
increase it - creating a huge in-memory buffer is a horrible solution to
anything, and we should provide an API that gives viable alternatives to that
(Or better yet, makes it impossible to do that, or at least forces consumers to
allocate their own huge buffers, instead of using one that's built-in).  The
current API doesn't even provide any visibility to how much data, if any, is in
the send buffer, so consumers are left to just stuff everything into it that
they can.

> There is also SendOverWebSocket, of course, which is used in devtools, but
that
> seems OK with the connection switched over to the protocol?
> 
> (Apologies for using the other account, can't seem to make my laptop's
security
> key
>  work for the chromium one...)

[mmenke, 2017-01-26 21:16:58]

Is this ready for another pass?  I had thought not, but now I'm less sure.

[morlovich, 2017-01-26 21:23:46]

On 2017/01/26 21:16:58, mmenke wrote:
> Is this ready for another pass?  I had thought not, but now I'm less sure.

Probably? It doesn't deal with async invocation, but I am not sure the
action_flags_
approach is the right one, and that would build on that (in a pretty
straightforward
way, I think).

[mmenke, 2017-01-26 21:24:52]

On 2017/01/26 21:23:46, morlovich wrote:
> On 2017/01/26 21:16:58, mmenke wrote:
> > Is this ready for another pass?  I had thought not, but now I'm less sure.
> 
> Probably? It doesn't deal with async invocation, but I am not sure the
> action_flags_
> approach is the right one, and that would build on that (in a pretty
> straightforward
> way, I think).

Ok, sorry for not getting to it today.  I'll do pass tomorrow.

[mmenke, 2017-01-27 17:08:53]

This looks pretty good to me.  Other than my comments below, just wonder about
calling back into the server asynchronously.

https://codereview.chromium.org/2649983003/diff/20001/net/server/http_server_...
File net/server/http_server_fuzzer.cc (right):

https://codereview.chromium.org/2649983003/diff/20001/net/server/http_server_...
net/server/http_server_fuzzer.cc:43: "text/html");
nit:  Use braces when the body of an if takes up multiple lines.

https://codereview.chromium.org/2649983003/diff/20001/net/server/http_server_...
net/server/http_server_fuzzer.cc:51: server_->AcceptWebSocket(connection_id,
info);
Should we close the connection if we don't accept this?

I'm not rightly sure what state we're in if we neither accept the websocket
request nor close the connection.  (Same is also true in the HTTP request...if
we get one request, and don't send a response....We just wait for another? 
That's weird).

https://codereview.chromium.org/2649983003/diff/20001/net/server/http_server_...
net/server/http_server_fuzzer.cc:62:
data_provider_->ConsumeRandomLengthString(64));
nit:  Braces

https://codereview.chromium.org/2649983003/diff/20001/net/server/http_server_...
net/server/http_server_fuzzer.cc:95:
server_socket->ListenWithAddressAndPort("127.0.0.1", 80, 5));
Should just use port 0.  This both gets us on an ephemeral port instead of a
named one, which seems like a good practice, and we have no guarantee that port
80 isn't already in use on the local device.

[morlovich, 2017-01-27 17:30:29]

morlovich@google.com changed reviewers:
+ morlovich@google.com

[morlovich, 2017-01-27 17:30:29]

Ack on the {} comments, keeping them around as reminders. 
(And thank you for stating the exact rule).

https://codereview.chromium.org/2649983003/diff/20001/net/server/http_server_...
File net/server/http_server_fuzzer.cc (right):

https://codereview.chromium.org/2649983003/diff/20001/net/server/http_server_...
net/server/http_server_fuzzer.cc:51: server_->AcceptWebSocket(connection_id,
info);
Well, not doing either is basically the case this addresses:
https://chromium.googlesource.com/chromium/src/net/+/62e66169d64ab6f44118a99f...

... so it definitely feels worth covering. May also be worth testing a coin-flip
of close, of course.

It's also an easy API misuse, or at least I did so myself: it's easy to provide
empty implementations of WebSocket methods when thinking of HTTP only, and
having that not crash based on remote input seems desirable...

https://codereview.chromium.org/2649983003/diff/20001/net/server/http_server_...
net/server/http_server_fuzzer.cc:95:
server_socket->ListenWithAddressAndPort("127.0.0.1", 80, 5));
On 2017/01/27 17:08:53, mmenke (Out Feb 4 to March 5) wrote:
> Should just use port 0.  This both gets us on an ephemeral port instead of a
> named one, which seems like a good practice, and we have no guarantee that
port
> 80 isn't already in use on the local device.

This isn't actually listening, though (being a FuzzedServerSocket), just
pretends to be for purposes of GetLocalAddress() --- so does this matter?

[mmenke, 2017-01-27 17:34:59]

I think this is worth landing (With the braces, and a close path in the
OnWebSocketConnect call).  We can then add testing of the async stuff, if we're
going to, afterwards.  Sound good?

https://codereview.chromium.org/2649983003/diff/20001/net/server/http_server_...
File net/server/http_server_fuzzer.cc (right):

https://codereview.chromium.org/2649983003/diff/20001/net/server/http_server_...
net/server/http_server_fuzzer.cc:51: server_->AcceptWebSocket(connection_id,
info);
On 2017/01/27 17:30:29, morlovich wrote:
> Well, not doing either is basically the case this addresses:
>
https://chromium.googlesource.com/chromium/src/net/+/62e66169d64ab6f44118a99f...
> 
> ... so it definitely feels worth covering. May also be worth testing a
coin-flip
> of close, of course.
> 
> It's also an easy API misuse, or at least I did so myself: it's easy to
provide
> empty implementations of WebSocket methods when thinking of HTTP only, and
> having that not crash based on remote input seems desirable...
> 
> 

Hrm...Good point.  Seems like we're in an indeterminate state if we don't accept
the WebSocket...but yea, test should cover that case, as long as the API allows
it.

https://codereview.chromium.org/2649983003/diff/20001/net/server/http_server_...
net/server/http_server_fuzzer.cc:95:
server_socket->ListenWithAddressAndPort("127.0.0.1", 80, 5));
On 2017/01/27 17:30:29, morlovich wrote:
> On 2017/01/27 17:08:53, mmenke (Out Feb 4 to March 5) wrote:
> > Should just use port 0.  This both gets us on an ephemeral port instead of a
> > named one, which seems like a good practice, and we have no guarantee that
> port
> > 80 isn't already in use on the local device.
> 
> This isn't actually listening, though (being a FuzzedServerSocket), just
> pretends to be for purposes of GetLocalAddress() --- so does this matter?

Oh, right, it doesn't.

[mmenke, 2017-01-27 17:40:14]

https://codereview.chromium.org/2649983003/diff/20001/net/server/http_server_...
File net/server/http_server_fuzzer.cc (right):

https://codereview.chromium.org/2649983003/diff/20001/net/server/http_server_...
net/server/http_server_fuzzer.cc:51: server_->AcceptWebSocket(connection_id,
info);
On 2017/01/27 17:34:59, mmenke (Out Feb 4 to March 5) wrote:
> On 2017/01/27 17:30:29, morlovich wrote:
> > Well, not doing either is basically the case this addresses:
> >
>
https://chromium.googlesource.com/chromium/src/net/+/62e66169d64ab6f44118a99f...
> > 
> > ... so it definitely feels worth covering. May also be worth testing a
> coin-flip
> > of close, of course.
> > 
> > It's also an easy API misuse, or at least I did so myself: it's easy to
> provide
> > empty implementations of WebSocket methods when thinking of HTTP only, and
> > having that not crash based on remote input seems desirable...
> > 
> > 
> 
> Hrm...Good point.  Seems like we're in an indeterminate state if we don't
accept
> the WebSocket...but yea, test should cover that case, as long as the API
allows
> it.

Well, "indeterminant" might not be the word - we'd still be reading data, but
would return FRAME_ERROR if any data was actually received...And the consumer
could accept the connection at any time, if it so desired, until then.

[Maks Orlovich, 2017-01-27 17:57:29]

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

[commit-bot: I haz the power, 2017-01-27 17:58:00]

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

[Maks Orlovich, 2017-01-27 18:04:57]

On 2017/01/27 17:34:59, mmenke (Out Feb 4 to March 5) wrote:
> I think this is worth landing (With the braces, and a close path in the
> OnWebSocketConnect call).  We can then add testing of the async stuff, if
we're
> going to, afterwards.  Sound good?

Very much so --- I would love to see what the clusterfuzz version of the
coverage 
data looks like... 

(And done).

[Maks Orlovich, 2017-01-27 18:17:15]

https://codereview.chromium.org/2649983003/diff/20001/net/server/http_server_...
File net/server/http_server_fuzzer.cc (right):

https://codereview.chromium.org/2649983003/diff/20001/net/server/http_server_...
net/server/http_server_fuzzer.cc:43: "text/html");
On 2017/01/27 17:08:53, mmenke (Out Feb 4 to March 5) wrote:
> nit:  Use braces when the body of an if takes up multiple lines.

Done.

https://codereview.chromium.org/2649983003/diff/20001/net/server/http_server_...
net/server/http_server_fuzzer.cc:62:
data_provider_->ConsumeRandomLengthString(64));
On 2017/01/27 17:08:53, mmenke (Out Feb 4 to March 5) wrote:
> nit:  Braces

Done.

[mmenke, 2017-01-27 18:19:18]

LGTM!

[commit-bot: I haz the power, 2017-01-27 18:38:38]

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

[commit-bot: I haz the power, 2017-01-27 18:38:39]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Maks Orlovich, 2017-01-27 18:54:22]

The CQ bit was checked by morlovich@chromium.org

[commit-bot: I haz the power, 2017-01-27 18:54:49]

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, 2017-01-27 19:34:15]

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

[commit-bot: I haz the power, 2017-01-27 19:34:18]

Failed to apply patch for net/BUILD.gn:
While running git apply --index -p1;
  error: patch failed: net/BUILD.gn:5623
  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
b9b637983975dfe1f4860fff8866482489ded82a..46d22d091b09e7d9f4370502e3ad83f2550115f7
100644
--- a/net/BUILD.gn
+++ b/net/BUILD.gn
@@ -5151,6 +5151,8 @@ source_set("net_fuzzer_test_support") {
     "filter/fuzzed_source_stream.h",
     "socket/fuzzed_datagram_client_socket.cc",
     "socket/fuzzed_datagram_client_socket.h",
+    "socket/fuzzed_server_socket.cc",
+    "socket/fuzzed_server_socket.h",
     "socket/fuzzed_socket.cc",
     "socket/fuzzed_socket.h",
     "socket/fuzzed_socket_factory.cc",
@@ -5623,6 +5625,21 @@
fuzzer_test("net_http_transport_security_state_static_fuzzer") {
       "data/fuzzer_dictionaries/net_http_transport_security_state_fuzzer.dict"
 }
 
+fuzzer_test("net_http_server_fuzzer") {
+  sources = [
+    "server/http_server_fuzzer.cc",
+  ]
+  deps = [
+    ":http_server",
+    ":net_fuzzer_test_support",
+    ":test_support",
+    "//base",
+    "//net",
+  ]
+  dict = "data/fuzzer_dictionaries/net_http_server_fuzzer.dict"
+  seed_corpus = "data/fuzzer_data/http_server_requests/"
+}
+
 if (host_toolchain == current_toolchain && !is_proto_quic) {
   executable("domain_security_preload_generator") {
     sources = [

[Maks Orlovich, 2017-01-29 23:31:24]

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

[commit-bot: I haz the power, 2017-01-29 23:31:38]

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, 2017-01-30 00:50:21]

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

[commit-bot: I haz the power, 2017-01-30 00:50:22]

Dry run: This issue passed the CQ dry run.

[Maks Orlovich, 2017-01-30 14:34:13]

The CQ bit was checked by morlovich@chromium.org

[Maks Orlovich, 2017-01-30 14:34:14]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/2649983003/#ps60001
(title: "Rebased, looked like doing it automatically failed")

[commit-bot: I haz the power, 2017-01-30 14:34:26]

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, 2017-01-30 14:38:20]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485786853858240,
"parent_rev": "aab3deacd994b91cf50594577b4c8e0329cdfee1", "commit_rev":
"5e6e19b1a3b499719ebd2243869adbb8e4271350"}

[commit-bot: I haz the power, 2017-01-30 14:38:51]

Description was changed from

==========
A simple fuzzer for HttpServer, with limited coverage of WebSocket.

BUG=606428
==========

to

==========
A simple fuzzer for HttpServer, with limited coverage of WebSocket.

BUG=606428

Review-Url: https://codereview.chromium.org/2649983003
Cr-Commit-Position: refs/heads/master@{#446980}
Committed:
https://chromium.googlesource.com/chromium/src/+/5e6e19b1a3b499719ebd2243869a...
==========

[commit-bot: I haz the power, 2017-01-30 14:38:53]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/5e6e19b1a3b499719ebd2243869a...