Add a fuzzer for GzipSourceStream

Add a fuzzer for GzipSourceStream

This CL adds a fuzzer for GzipSourceStream.

BUG=602424
Committed: https://crrev.com/758373306f70943b63c8f276a2e3f6af28f0d9df
Cr-Commit-Position: refs/heads/master@{#428742}

[xunjieli, 2016-10-27 19:02:13]

Description was changed from

==========
Add a fuzzer for GzipSourceStream

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

to

==========
Add a fuzzer for GzipSourceStream

This CL adds a fuzzer for GzipSourceStream.

BUG=602424
==========

[xunjieli, 2016-10-27 19:05:02]

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

[xunjieli, 2016-10-27 19:05:02]

Hi Matt, this is basically copied over from the fuzzed stream socket. I compiled
and ran the fuzzer, and it seems to run. Not sure what else is needed. Could you
advise? Thanks!

[xunjieli, 2016-10-27 19:05:13]

Patchset #1 (id:1) has been deleted

[mmenke, 2016-10-27 19:12:30]

On 2016/10/27 19:05:02, xunjieli wrote:
> Hi Matt, this is basically copied over from the fuzzed stream socket. I
compiled
> and ran the fuzzer, and it seems to run. Not sure what else is needed. Could
you
> advise? Thanks!

Going to do a pass now, but just FYI, I'll be off tomorrow.

[mmenke, 2016-10-27 19:31:12]

Nice looking fuzzer, great that it can easily be applied to brotli (Not sure
about SDCH).

https://codereview.chromium.org/2460633002/diff/20001/net/filter/fuzzed_sourc...
File net/filter/fuzzed_source_stream.cc (right):

https://codereview.chromium.org/2460633002/diff/20001/net/filter/fuzzed_sourc...
net/filter/fuzzed_source_stream.cc:35: DCHECK(!read_pending_);
Maybe DCHECK that Read isn't called after we're returned a result <= 0?

https://codereview.chromium.org/2460633002/diff/20001/net/filter/fuzzed_sourc...
net/filter/fuzzed_source_stream.cc:38: int result =
data_provider_->ConsumeUint8();
I suggest just doing:

int result = data_provider_->ConsumeUint32InRange(0, buf_len);
std::string data = data_provider_->ConsumeBytes(result);

if (data.size() > 0) {
  ...
} else {
  ...
}

It's a little simpler (Allows having one if/else instead of 3 if's), and it
allows us to fill the buffer (Unless it's longer than the max size of
DataProvider's input).

Should probably also add a CHECK that buf_len >= 0, though.

https://codereview.chromium.org/2460633002/diff/20001/net/filter/fuzzed_sourc...
net/filter/fuzzed_source_stream.cc:46: std::copy(data.data(), data.data() +
result, buf->data());
I think we need <algorithm> for std::copy?

https://codereview.chromium.org/2460633002/diff/20001/net/filter/fuzzed_sourc...
net/filter/fuzzed_source_stream.cc:46: std::copy(data.data(), data.data() +
result, buf->data());
optional:  Could be a little more rigorous by not copying data until
OnReadComplete in the async case.  FuzzedSocket doesn't do this, I believe, but
perhaps it should.

https://codereview.chromium.org/2460633002/diff/20001/net/filter/fuzzed_sourc...
File net/filter/fuzzed_source_stream.h (right):

https://codereview.chromium.org/2460633002/diff/20001/net/filter/fuzzed_sourc...
net/filter/fuzzed_source_stream.h:24: // |data_provider| is used as to determine
behavior of the FuzzedSourceStream.
nit:  -as

https://codereview.chromium.org/2460633002/diff/20001/net/filter/fuzzed_sourc...
net/filter/fuzzed_source_stream.h:26:
FuzzedSourceStream(base::FuzzedDataProvider* data_provider);
nit:  Explicit

https://codereview.chromium.org/2460633002/diff/20001/net/filter/fuzzed_sourc...
net/filter/fuzzed_source_stream.h:30: int Read(IOBuffer* dest_buffer,
nit:  Forward declare IOBuffer

https://codereview.chromium.org/2460633002/diff/20001/net/filter/gzip_source_...
File net/filter/gzip_source_stream_fuzzer.cc (right):

https://codereview.chromium.org/2460633002/diff/20001/net/filter/gzip_source_...
net/filter/gzip_source_stream_fuzzer.cc:25: net::SourceStream::TYPE_GZIP);
Fuzz the type?  Or, alternatively, just run the fuzzer against all 3 types on
each input.

https://codereview.chromium.org/2460633002/diff/20001/net/filter/gzip_source_...
net/filter/gzip_source_stream_fuzzer.cc:27: scoped_refptr<net::GrowableIOBuffer>
read_buffer(new net::GrowableIOBuffer());
Not used.

[xunjieli, 2016-10-27 20:00:59]

Thanks for the review! I will address the comments tomorrow or next week. I am
not in a hurry. I just want to get the Fuzzit swag :) have a great weekend.

[xunjieli, 2016-10-28 15:21:48]

Thanks!

https://codereview.chromium.org/2460633002/diff/20001/net/filter/fuzzed_sourc...
File net/filter/fuzzed_source_stream.cc (right):

https://codereview.chromium.org/2460633002/diff/20001/net/filter/fuzzed_sourc...
net/filter/fuzzed_source_stream.cc:35: DCHECK(!read_pending_);
On 2016/10/27 19:31:11, mmenke (OOO 10-28) wrote:
> Maybe DCHECK that Read isn't called after we're returned a result <= 0?

Done.

https://codereview.chromium.org/2460633002/diff/20001/net/filter/fuzzed_sourc...
net/filter/fuzzed_source_stream.cc:38: int result =
data_provider_->ConsumeUint8();
On 2016/10/27 19:31:11, mmenke (OOO 10-28) wrote:
> I suggest just doing:
> 
> int result = data_provider_->ConsumeUint32InRange(0, buf_len);
> std::string data = data_provider_->ConsumeBytes(result);
> 
> if (data.size() > 0) {
>   ...
> } else {
>   ...
> }
> 
> It's a little simpler (Allows having one if/else instead of 3 if's), and it
> allows us to fill the buffer (Unless it's longer than the max size of
> DataProvider's input).
> 
> Should probably also add a CHECK that buf_len >= 0, though.

Done.

https://codereview.chromium.org/2460633002/diff/20001/net/filter/fuzzed_sourc...
net/filter/fuzzed_source_stream.cc:46: std::copy(data.data(), data.data() +
result, buf->data());
On 2016/10/27 19:31:11, mmenke (OOO 10-28) wrote:
> I think we need <algorithm> for std::copy?

Done.

https://codereview.chromium.org/2460633002/diff/20001/net/filter/fuzzed_sourc...
net/filter/fuzzed_source_stream.cc:46: std::copy(data.data(), data.data() +
result, buf->data());
On 2016/10/27 19:31:11, mmenke (OOO 10-28) wrote:
> optional:  Could be a little more rigorous by not copying data until
> OnReadComplete in the async case.  FuzzedSocket doesn't do this, I believe,
but
> perhaps it should.

Done.

https://codereview.chromium.org/2460633002/diff/20001/net/filter/fuzzed_sourc...
File net/filter/fuzzed_source_stream.h (right):

https://codereview.chromium.org/2460633002/diff/20001/net/filter/fuzzed_sourc...
net/filter/fuzzed_source_stream.h:24: // |data_provider| is used as to determine
behavior of the FuzzedSourceStream.
On 2016/10/27 19:31:11, mmenke (OOO 10-28) wrote:
> nit:  -as

Done.

https://codereview.chromium.org/2460633002/diff/20001/net/filter/fuzzed_sourc...
net/filter/fuzzed_source_stream.h:26:
FuzzedSourceStream(base::FuzzedDataProvider* data_provider);
On 2016/10/27 19:31:11, mmenke (OOO 10-28) wrote:
> nit:  Explicit

Done.

https://codereview.chromium.org/2460633002/diff/20001/net/filter/fuzzed_sourc...
net/filter/fuzzed_source_stream.h:30: int Read(IOBuffer* dest_buffer,
On 2016/10/27 19:31:11, mmenke (OOO 10-28) wrote:
> nit:  Forward declare IOBuffer

It's forward declared.

https://codereview.chromium.org/2460633002/diff/20001/net/filter/gzip_source_...
File net/filter/gzip_source_stream_fuzzer.cc (right):

https://codereview.chromium.org/2460633002/diff/20001/net/filter/gzip_source_...
net/filter/gzip_source_stream_fuzzer.cc:25: net::SourceStream::TYPE_GZIP);
On 2016/10/27 19:31:12, mmenke (OOO 10-28) wrote:
> Fuzz the type?  Or, alternatively, just run the fuzzer against all 3 types on
> each input.

Done.

https://codereview.chromium.org/2460633002/diff/20001/net/filter/gzip_source_...
net/filter/gzip_source_stream_fuzzer.cc:27: scoped_refptr<net::GrowableIOBuffer>
read_buffer(new net::GrowableIOBuffer());
On 2016/10/27 19:31:11, mmenke (OOO 10-28) wrote:
> Not used.

Done.

[mmenke, 2016-10-31 14:59:52]

LGTM, thanks for writing this fuzzer!

https://codereview.chromium.org/2460633002/diff/40001/net/filter/fuzzed_sourc...
File net/filter/fuzzed_source_stream.h (right):

https://codereview.chromium.org/2460633002/diff/40001/net/filter/fuzzed_sourc...
net/filter/fuzzed_source_stream.h:37: const std::string& fuzzed_data,
Should probably include <string> here instead of the CC file.

https://codereview.chromium.org/2460633002/diff/40001/net/filter/fuzzed_sourc...
net/filter/fuzzed_source_stream.h:38: scoped_refptr<IOBuffer> read_buf,
include base/memory/ref_counted.h

https://codereview.chromium.org/2460633002/diff/40001/net/filter/gzip_source_...
File net/filter/gzip_source_stream_fuzzer.cc (right):

https://codereview.chromium.org/2460633002/diff/40001/net/filter/gzip_source_...
net/filter/gzip_source_stream_fuzzer.cc:23: net::SourceStream::SourceType
gzip_types[] = {
nit:  const / kGzipTypes

[xunjieli, 2016-10-31 15:14:56]

Thanks!

https://codereview.chromium.org/2460633002/diff/40001/net/filter/fuzzed_sourc...
File net/filter/fuzzed_source_stream.h (right):

https://codereview.chromium.org/2460633002/diff/40001/net/filter/fuzzed_sourc...
net/filter/fuzzed_source_stream.h:37: const std::string& fuzzed_data,
On 2016/10/31 14:59:52, mmenke (Busy Nov 2-3) wrote:
> Should probably include <string> here instead of the CC file.

Done. Thanks for point that out. I recently learned about "git cl lint" which
will identify problems like this. I will make sure to run it in the future.

https://codereview.chromium.org/2460633002/diff/40001/net/filter/fuzzed_sourc...
net/filter/fuzzed_source_stream.h:38: scoped_refptr<IOBuffer> read_buf,
On 2016/10/31 14:59:52, mmenke (Busy Nov 2-3) wrote:
> include base/memory/ref_counted.h

Done.

https://codereview.chromium.org/2460633002/diff/40001/net/filter/gzip_source_...
File net/filter/gzip_source_stream_fuzzer.cc (right):

https://codereview.chromium.org/2460633002/diff/40001/net/filter/gzip_source_...
net/filter/gzip_source_stream_fuzzer.cc:23: net::SourceStream::SourceType
gzip_types[] = {
On 2016/10/31 14:59:52, mmenke (Busy Nov 2-3) wrote:
> nit:  const / kGzipTypes

Done.

[xunjieli, 2016-10-31 15:15:14]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2016-10-31 15:15:15]

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/2460633002/#ps60001
(title: "Address comments")

[commit-bot: I haz the power, 2016-10-31 15:15:29]

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-10-31 16:51:50]

Description was changed from

==========
Add a fuzzer for GzipSourceStream

This CL adds a fuzzer for GzipSourceStream.

BUG=602424
==========

to

==========
Add a fuzzer for GzipSourceStream

This CL adds a fuzzer for GzipSourceStream.

BUG=602424
==========

[commit-bot: I haz the power, 2016-10-31 16:51:51]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-10-31 17:15:58]

Description was changed from

==========
Add a fuzzer for GzipSourceStream

This CL adds a fuzzer for GzipSourceStream.

BUG=602424
==========

to

==========
Add a fuzzer for GzipSourceStream

This CL adds a fuzzer for GzipSourceStream.

BUG=602424

Committed: https://crrev.com/758373306f70943b63c8f276a2e3f6af28f0d9df
Cr-Commit-Position: refs/heads/master@{#428742}
==========

[commit-bot: I haz the power, 2016-10-31 17:15:59]

Patchset 3 (id:??) landed as
https://crrev.com/758373306f70943b63c8f276a2e3f6af28f0d9df
Cr-Commit-Position: refs/heads/master@{#428742}