Vary the parameters to WebSocketDeflateStream in fuzzer

Vary the parameters to WebSocketDeflateStream in fuzzer

Previously, the fuzzer for WebSocketDeflateStream always initialised it with
default parameters. Vary the initialisation parameters in order to increase
coverage.

BUG=693496
R=yhirano

Review-Url: https://codereview.chromium.org/2706433003
Cr-Commit-Position: refs/heads/master@{#453580}
Committed: https://chromium.googlesource.com/chromium/src/+/1c2d77f89b0296318db4239d657a191bf4c7cad2

[yhirano, 2017-02-24 12:15:05]

lgtm

https://codereview.chromium.org/2706433003/diff/1/net/websockets/websocket_de...
File net/websockets/websocket_deflate_stream_fuzzer.cc (right):

https://codereview.chromium.org/2706433003/diff/1/net/websockets/websocket_de...
net/websockets/websocket_deflate_stream_fuzzer.cc:77: base::FuzzedDataProvider*
fuzzed_data_provider_;
[optional] I'd prefer having this as a unique_ptr, but it's up to you.

https://codereview.chromium.org/2706433003/diff/1/net/websockets/websocket_de...
net/websockets/websocket_deflate_stream_fuzzer.cc:100:
CHECK(parameters.Initialize(params, &failure_message)) << failure_message;
Is DCHECK enough?

[mmoroz, 2017-02-24 12:38:37]

mmoroz@chromium.org changed reviewers:
+ mmoroz@chromium.org

[mmoroz, 2017-02-24 12:38:38]

Left some suggestions, but LGTM and thanks for upgrading fuzz targets!

https://codereview.chromium.org/2706433003/diff/1/net/websockets/websocket_de...
File net/websockets/websocket_deflate_stream_fuzzer.cc (right):

https://codereview.chromium.org/2706433003/diff/1/net/websockets/websocket_de...
net/websockets/websocket_deflate_stream_fuzzer.cc:82: uint8_t flags =
fuzzed_data_provider.ConsumeUint8();
I would like to suggest to consume more bits here, but only if that's possible
that we will have more flags in the future. Assuming that one day we will
consume more than 8 bits here, existing corpus would become mostly useless.

https://codereview.chromium.org/2706433003/diff/1/net/websockets/websocket_de...
net/websockets/websocket_deflate_stream_fuzzer.cc:100:
CHECK(parameters.Initialize(params, &failure_message)) << failure_message;
On 2017/02/24 12:15:05, yhirano wrote:
> Is DCHECK enough?

DCHECK will be enabled only in ASan Debug build. Other configurations are
Release ones. If this CHECK is expected to catch real bugs, this is exactly what
we need.

https://codereview.chromium.org/2706433003/diff/1/net/websockets/websocket_de...
net/websockets/websocket_deflate_stream_fuzzer.cc:113: extern "C" int
LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
It might be useful to add something like |if (size < N) return 0;| in the
beginning of the fuzz target, where N is minimum amount of bytes we need to
generate parameters. Looks like N is 10 here. That would let libFuzzer to know
that inputs shorter than 10 bytes are useless.

[Adam Rice, 2017-02-27 04:00:04]

yhirano, could you double-check my calculation of MIN_BYTES_TO_CREATE_A_FRAME
and MIN_USEFUL_SIZE if you have time?

https://codereview.chromium.org/2706433003/diff/1/net/websockets/websocket_de...
File net/websockets/websocket_deflate_stream_fuzzer.cc (right):

https://codereview.chromium.org/2706433003/diff/1/net/websockets/websocket_de...
net/websockets/websocket_deflate_stream_fuzzer.cc:77: base::FuzzedDataProvider*
fuzzed_data_provider_;
On 2017/02/24 12:15:05, yhirano wrote:
> [optional] I'd prefer having this as a unique_ptr, but it's up to you.

I'm going to stick with a raw pointer here, so that I can continue to allocate
FuzzedDataProvider on the stack. It's not a big optimisation, given how many
other memory allocations we do, but it's easy to do.

https://codereview.chromium.org/2706433003/diff/1/net/websockets/websocket_de...
net/websockets/websocket_deflate_stream_fuzzer.cc:82: uint8_t flags =
fuzzed_data_provider.ConsumeUint8();
On 2017/02/24 12:38:37, mmoroz wrote:
> I would like to suggest to consume more bits here, but only if that's possible
> that we will have more flags in the future. Assuming that one day we will
> consume more than 8 bits here, existing corpus would become mostly useless.

Done.

https://codereview.chromium.org/2706433003/diff/1/net/websockets/websocket_de...
net/websockets/websocket_deflate_stream_fuzzer.cc:100:
CHECK(parameters.Initialize(params, &failure_message)) << failure_message;
On 2017/02/24 12:38:37, mmoroz wrote:
> On 2017/02/24 12:15:05, yhirano wrote:
> > Is DCHECK enough?
> 
> DCHECK will be enabled only in ASan Debug build. Other configurations are
> Release ones. If this CHECK is expected to catch real bugs, this is exactly
what
> we need.

This is a grey area. My conclusion is that we're really only testing for a bug
in the fuzzer here, so DCHECK is appropriate. We have unit tests for whether the
parsing done by WebSocketDeflateParameters works properly.

https://codereview.chromium.org/2706433003/diff/1/net/websockets/websocket_de...
net/websockets/websocket_deflate_stream_fuzzer.cc:113: extern "C" int
LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
On 2017/02/24 12:38:37, mmoroz wrote:
> It might be useful to add something like |if (size < N) return 0;| in the
> beginning of the fuzz target, where N is minimum amount of bytes we need to
> generate parameters. Looks like N is 10 here. That would let libFuzzer to know
> that inputs shorter than 10 bytes are useless.

That's a good tip, thanks. Because ConsumeUint32InRange() only consumes 1 byte
for the ranges we use, I ended up with N = 4.

[mmoroz, 2017-02-27 09:35:31]

Cool! LGTM

[yhirano, 2017-02-28 11:35:02]

lgtm

[Adam Rice, 2017-02-28 12:44:41]

The CQ bit was checked by ricea@chromium.org

[commit-bot: I haz the power, 2017-02-28 12:44:58]

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-02-28 12:58:23]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488285881372400,
"parent_rev": "c1a6b6e44607e30f833bd01cefeb43da05545851", "commit_rev":
"1c2d77f89b0296318db4239d657a191bf4c7cad2"}

[commit-bot: I haz the power, 2017-02-28 12:59:03]

Description was changed from

==========
Vary the parameters to WebSocketDeflateStream in fuzzer

Previously, the fuzzer for WebSocketDeflateStream always initialised it with
default parameters. Vary the initialisation parameters in order to increase
coverage.

BUG=693496
R=yhirano
==========

to

==========
Vary the parameters to WebSocketDeflateStream in fuzzer

Previously, the fuzzer for WebSocketDeflateStream always initialised it with
default parameters. Vary the initialisation parameters in order to increase
coverage.

BUG=693496
R=yhirano

Review-Url: https://codereview.chromium.org/2706433003
Cr-Commit-Position: refs/heads/master@{#453580}
Committed:
https://chromium.googlesource.com/chromium/src/+/1c2d77f89b0296318db4239d657a...
==========

[commit-bot: I haz the power, 2017-02-28 12:59:04]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/1c2d77f89b0296318db4239d657a...