media: Add fuzzer test for bit_reader

media: Add fuzzer test for bit_reader

Also fixes a problem in bit_reader_core where skipping off the end of the buffer doesn't properly updated the bits used, so it appears that some bits are still available but any subsequent operation fails.

TEST=fuzzer test runs locally

Committed: https://crrev.com/976753e8ed554d074083b3cd3c093ae1960776ab
Cr-Commit-Position: refs/heads/master@{#380205}

[jrummell, 2016-03-02 22:29:19]

jrummell@chromium.org changed reviewers:
+ dalecurtis@chromium.org, kcc@chromium.org

[jrummell, 2016-03-02 22:29:20]

PTAL. Another fuzzer test, this time for bit_reader.

[DaleCurtis, 2016-03-03 00:16:35]

https://codereview.chromium.org/1754523004/diff/1/media/base/bit_reader_fuzze...
File media/base/bit_reader_fuzzertest.cc (right):

https://codereview.chromium.org/1754523004/diff/1/media/base/bit_reader_fuzze...
media/base/bit_reader_fuzzertest.cc:22: for (size_t i = 0; i < size &&
reader.bits_available(); ++i) {
Why restrict to size here? Seems you want to keep reading and just use i/8 for
your read or skip check? DetermineNumBits() could also take bits remaining and
clamp so that you don't need l.35. to l.39

https://codereview.chromium.org/1754523004/diff/1/media/base/bit_reader_fuzze...
media/base/bit_reader_fuzzertest.cc:25: // Read up to 32 bits. This may fail if
there is not enough bits
Why 32 instead of say 64?

[jrummell, 2016-03-03 23:11:25]

Patchset #2 (id:20001) has been deleted

[jrummell, 2016-03-03 23:21:08]

Description was changed from

==========
media: Add fuzzer test for bit_reader

TEST=fuzzer test runs locally
==========

to

==========
media: Add fuzzer test for bit_reader

Also fixes a problem in bit_reader_core where skipping off the end of the buffer
doesn't properly updated the bits used, so it appears that some bits are still
available but any subsequent operation fails.

TEST=fuzzer test runs locally
==========

[jrummell, 2016-03-03 23:21:38]

Updated.

https://codereview.chromium.org/1754523004/diff/1/media/base/bit_reader_fuzze...
File media/base/bit_reader_fuzzertest.cc (right):

https://codereview.chromium.org/1754523004/diff/1/media/base/bit_reader_fuzze...
media/base/bit_reader_fuzzertest.cc:22: for (size_t i = 0; i < size &&
reader.bits_available(); ++i) {
On 2016/03/03 00:16:35, DaleCurtis wrote:
> Why restrict to size here? Seems you want to keep reading and just use i/8 for
> your read or skip check? DetermineNumBits() could also take bits remaining and
> clamp so that you don't need l.35. to l.39

I was using the bytes in |data| to determine the operation to do. I didn't want
to read off the end of |data|. However, I've switched it to use a random number
generator, so this is no longer an issue.

Note that now the data bytes are not really used (other than to generate the
random number seed).

https://codereview.chromium.org/1754523004/diff/1/media/base/bit_reader_fuzze...
media/base/bit_reader_fuzzertest.cc:25: // Read up to 32 bits. This may fail if
there is not enough bits
On 2016/03/03 00:16:35, DaleCurtis wrote:
> Why 32 instead of say 64?

Updated to use a bigger range.

[DaleCurtis, 2016-03-04 18:54:36]

lgtm

https://codereview.chromium.org/1754523004/diff/40001/media/base/bit_reader_f...
File media/base/bit_reader_fuzzertest.cc (right):

https://codereview.chromium.org/1754523004/diff/40001/media/base/bit_reader_f...
media/base/bit_reader_fuzzertest.cc:24: // read/skip in a reproducible way
(given the same |data|).
Add a comment that Hash() is used to ensure the seed varies significantly over
minor changes within the source material.

https://codereview.chromium.org/1754523004/diff/40001/media/base/bit_reader_f...
media/base/bit_reader_fuzzertest.cc:33: if
(!reader.ReadBits(DetermineNumBits(rnd.Rand(), 1, 64), &value))
I think this is clearer w/o the helper function actually: ReadBits(rnd.Rand() %
64 + 1, &value)

https://codereview.chromium.org/1754523004/diff/40001/media/base/bit_reader_f...
media/base/bit_reader_fuzzertest.cc:37: if
(!reader.SkipBits(DetermineNumBits(rnd.Rand(), 1, 128)))
Ditto:

ReadBits(rnd.Rand() % 128 + 1);

[jrummell, 2016-03-04 23:47:13]

Updated.

https://codereview.chromium.org/1754523004/diff/40001/media/base/bit_reader_f...
File media/base/bit_reader_fuzzertest.cc (right):

https://codereview.chromium.org/1754523004/diff/40001/media/base/bit_reader_f...
media/base/bit_reader_fuzzertest.cc:24: // read/skip in a reproducible way
(given the same |data|).
On 2016/03/04 18:54:36, DaleCurtis wrote:
> Add a comment that Hash() is used to ensure the seed varies significantly over
> minor changes within the source material.

Done.

https://codereview.chromium.org/1754523004/diff/40001/media/base/bit_reader_f...
media/base/bit_reader_fuzzertest.cc:33: if
(!reader.ReadBits(DetermineNumBits(rnd.Rand(), 1, 64), &value))
On 2016/03/04 18:54:36, DaleCurtis wrote:
> I think this is clearer w/o the helper function actually: ReadBits(rnd.Rand()
%
> 64 + 1, &value)

Done.

https://codereview.chromium.org/1754523004/diff/40001/media/base/bit_reader_f...
media/base/bit_reader_fuzzertest.cc:37: if
(!reader.SkipBits(DetermineNumBits(rnd.Rand(), 1, 128)))
On 2016/03/04 18:54:36, DaleCurtis wrote:
> Ditto:
> 
> ReadBits(rnd.Rand() % 128 + 1);

Done.

[jrummell, 2016-03-09 00:23:00]

jrummell@chromium.org changed reviewers:
+ aizatsky@chromium.org, mmoroz@chromium.org

[jrummell, 2016-03-09 00:23:01]

+mmoroz@ and aizatsky@ in case they have any comments.

[mmoroz, 2016-03-09 10:20:49]

LGTM

https://codereview.chromium.org/1754523004/diff/60001/media/base/bit_reader_f...
File media/base/bit_reader_fuzzertest.cc (right):

https://codereview.chromium.org/1754523004/diff/60001/media/base/bit_reader_f...
media/base/bit_reader_fuzzertest.cc:5: #include <stddef.h>
Could you please include <stdint.h> too? May be it looks a sort of redundant,
but we try to do it for all fuzzers to have |size_t| and |uint8_t| defined
independently on other headers included.

[jrummell, 2016-03-09 19:08:21]

Thanks for the reviews.

https://codereview.chromium.org/1754523004/diff/60001/media/base/bit_reader_f...
File media/base/bit_reader_fuzzertest.cc (right):

https://codereview.chromium.org/1754523004/diff/60001/media/base/bit_reader_f...
media/base/bit_reader_fuzzertest.cc:5: #include <stddef.h>
On 2016/03/09 10:20:48, mmoroz wrote:
> Could you please include <stdint.h> too? May be it looks a sort of redundant,
> but we try to do it for all fuzzers to have |size_t| and |uint8_t| defined
> independently on other headers included.

Done.

[jrummell, 2016-03-09 19:08:29]

The CQ bit was checked by jrummell@chromium.org

[jrummell, 2016-03-09 19:08:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org, mmoroz@chromium.org
Link to the patchset: https://codereview.chromium.org/1754523004/#ps80001
(title: "add include")

[commit-bot: I haz the power, 2016-03-09 19:08:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1754523004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1754523004/80001

[commit-bot: I haz the power, 2016-03-09 20:36:33]

Description was changed from

==========
media: Add fuzzer test for bit_reader

Also fixes a problem in bit_reader_core where skipping off the end of the buffer
doesn't properly updated the bits used, so it appears that some bits are still
available but any subsequent operation fails.

TEST=fuzzer test runs locally
==========

to

==========
media: Add fuzzer test for bit_reader

Also fixes a problem in bit_reader_core where skipping off the end of the buffer
doesn't properly updated the bits used, so it appears that some bits are still
available but any subsequent operation fails.

TEST=fuzzer test runs locally
==========

[commit-bot: I haz the power, 2016-03-09 20:36:34]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-03-09 20:38:48]

Description was changed from

==========
media: Add fuzzer test for bit_reader

Also fixes a problem in bit_reader_core where skipping off the end of the buffer
doesn't properly updated the bits used, so it appears that some bits are still
available but any subsequent operation fails.

TEST=fuzzer test runs locally
==========

to

==========
media: Add fuzzer test for bit_reader

Also fixes a problem in bit_reader_core where skipping off the end of the buffer
doesn't properly updated the bits used, so it appears that some bits are still
available but any subsequent operation fails.

TEST=fuzzer test runs locally

Committed: https://crrev.com/976753e8ed554d074083b3cd3c093ae1960776ab
Cr-Commit-Position: refs/heads/master@{#380205}
==========

[commit-bot: I haz the power, 2016-03-09 20:38:49]

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