Fuzzer tests for AudioDecoder's DecodeRedundant and IncomingPacket

Fuzzer tests for AudioDecoder's DecodeRedundant and IncomingPacket

This CL adds new fuzzer tests for the DecodeRedundant and
IncomingPacket methods of AudioDecoder. In practice, only Opus has
DecodeRedundant, and only iSAC has IncomingPacket. Did some minor work
to generalize the helper function reading values from the fuzzed
input.

BUG=webrtc:5306
R=pbos@webrtc.org
NOTRY=true

Committed: https://crrev.com/fd2be2718d984792b949b186ff11ceaf256723d0
Cr-Commit-Position: refs/heads/master@{#11533}

[hlundin-webrtc, 2016-01-20 12:24:32]

Patchset #1 (id:1) has been deleted

[hlundin-webrtc, 2016-01-20 12:24:59]

pbos, please, take a look.

https://codereview.webrtc.org/1607173003/diff/20001/webrtc/test/fuzzers/audio...
File webrtc/test/fuzzers/audio_decoder_fuzzer.cc (right):

https://codereview.webrtc.org/1607173003/diff/20001/webrtc/test/fuzzers/audio...
webrtc/test/fuzzers/audio_decoder_fuzzer.cc:63: while (packet_len != 0 &&
packet_len + 2 <= remaining_size) {
(Turned out to be a bad idea to subtract from a size_t before knowing the value
of it...)

[pbos-webrtc, 2016-01-20 13:16:36]

pbos@webrtc.org changed reviewers:
+ terelius@webrtc.org

[pbos-webrtc, 2016-01-20 13:16:36]

+R terelius@ Who I think might have some input on type safety.

https://codereview.webrtc.org/1607173003/diff/20001/webrtc/test/fuzzers/audio...
File webrtc/test/fuzzers/audio_decoder_fuzzer.cc (right):

https://codereview.webrtc.org/1607173003/diff/20001/webrtc/test/fuzzers/audio...
webrtc/test/fuzzers/audio_decoder_fuzzer.cc:19: rtc::Optional<uint64_t>
ReadNBytes(const uint8_t* data, size_t size, size_t N) {
This sounds like memcpy, can you rename this to "ParseInt" or smth?

https://codereview.webrtc.org/1607173003/diff/20001/webrtc/test/fuzzers/audio...
webrtc/test/fuzzers/audio_decoder_fuzzer.cc:37: T ReadTCheckedAndIncrement(const
uint8_t** data, size_t* size) {
Can we do something like ReadUint? this sounds like it'd read something more
generally, like a struct.

terelius@: Can we enforce that T is an integer type somehow:?

https://codereview.webrtc.org/1607173003/diff/20001/webrtc/test/fuzzers/audio...
webrtc/test/fuzzers/audio_decoder_fuzzer.cc:63: while (packet_len != 0 &&
packet_len + 2 <= remaining_size) {
On 2016/01/20 12:24:59, hlundin-webrtc wrote:
> (Turned out to be a bad idea to subtract from a size_t before knowing the
value
> of it...)

:D

[pbos-webrtc, 2016-01-20 13:16:51]

Also, run linux_libfuzzer_rel for the tryjob

[terelius, 2016-01-20 13:47:28]

https://codereview.webrtc.org/1607173003/diff/20001/webrtc/test/fuzzers/audio...
File webrtc/test/fuzzers/audio_decoder_fuzzer.cc (right):

https://codereview.webrtc.org/1607173003/diff/20001/webrtc/test/fuzzers/audio...
webrtc/test/fuzzers/audio_decoder_fuzzer.cc:37: T ReadTCheckedAndIncrement(const
uint8_t** data, size_t* size) {
On 2016/01/20 13:16:36, pbos-webrtc wrote:
> Can we do something like ReadUint? this sounds like it'd read something more
> generally, like a struct.
> 
> terelius@: Can we enforce that T is an integer type somehow:?

You could do a static_assert on std::numeric_limits<T>::is_integer which is true
for the built-in integer types including bool, but false for floating point
numbers and types which do not explicitly specialize numeric_limits.

Depending on what you want to allow and whether the C++11 type_traits library is
ok to use, I think std::is_fundamental might be better. As far as I can tell,
the existing code would work fine with floating point types too.

[terelius, 2016-01-20 13:50:06]

https://codereview.webrtc.org/1607173003/diff/20001/webrtc/test/fuzzers/audio...
File webrtc/test/fuzzers/audio_decoder_fuzzer.cc (right):

https://codereview.webrtc.org/1607173003/diff/20001/webrtc/test/fuzzers/audio...
webrtc/test/fuzzers/audio_decoder_fuzzer.cc:37: T ReadTCheckedAndIncrement(const
uint8_t** data, size_t* size) {
On 2016/01/20 13:47:27, terelius wrote:
> On 2016/01/20 13:16:36, pbos-webrtc wrote:
> > Can we do something like ReadUint? this sounds like it'd read something more
> > generally, like a struct.
> > 
> > terelius@: Can we enforce that T is an integer type somehow:?
> 
> You could do a static_assert on std::numeric_limits<T>::is_integer which is
true
> for the built-in integer types including bool, but false for floating point
> numbers and types which do not explicitly specialize numeric_limits.
> 
> Depending on what you want to allow and whether the C++11 type_traits library
is
> ok to use, I think std::is_fundamental might be better. As far as I can tell,
> the existing code would work fine with floating point types too.

Hm, actually it won't. std::numeric_limits<T>::is_integer is probably the best
option then.

[hlundin-webrtc, 2016-01-21 15:18:04]

Updates after review

[hlundin-webrtc, 2016-01-21 15:18:51]

Thanks! Please take a look again.

https://codereview.webrtc.org/1607173003/diff/20001/webrtc/test/fuzzers/audio...
File webrtc/test/fuzzers/audio_decoder_fuzzer.cc (right):

https://codereview.webrtc.org/1607173003/diff/20001/webrtc/test/fuzzers/audio...
webrtc/test/fuzzers/audio_decoder_fuzzer.cc:19: rtc::Optional<uint64_t>
ReadNBytes(const uint8_t* data, size_t size, size_t N) {
On 2016/01/20 13:16:36, pbos-webrtc wrote:
> This sounds like memcpy, can you rename this to "ParseInt" or smth?

Sure can do. But it is not really memcpy, depending on platform endianness,
right?

https://codereview.webrtc.org/1607173003/diff/20001/webrtc/test/fuzzers/audio...
webrtc/test/fuzzers/audio_decoder_fuzzer.cc:37: T ReadTCheckedAndIncrement(const
uint8_t** data, size_t* size) {
On 2016/01/20 13:50:06, terelius wrote:
> On 2016/01/20 13:47:27, terelius wrote:
> > On 2016/01/20 13:16:36, pbos-webrtc wrote:
> > > Can we do something like ReadUint? this sounds like it'd read something
more
> > > generally, like a struct.
> > > 
> > > terelius@: Can we enforce that T is an integer type somehow:?
> > 
> > You could do a static_assert on std::numeric_limits<T>::is_integer which is
> true
> > for the built-in integer types including bool, but false for floating point
> > numbers and types which do not explicitly specialize numeric_limits.
> > 
> > Depending on what you want to allow and whether the C++11 type_traits
library
> is
> > ok to use, I think std::is_fundamental might be better. As far as I can
tell,
> > the existing code would work fine with floating point types too.
> 
> Hm, actually it won't. std::numeric_limits<T>::is_integer is probably the best
> option then.

Done.

[pbos-webrtc, 2016-01-21 15:31:24]

https://codereview.webrtc.org/1607173003/diff/40001/webrtc/test/fuzzers/audio...
File webrtc/test/fuzzers/audio_decoder_fuzzer.cc (right):

https://codereview.webrtc.org/1607173003/diff/40001/webrtc/test/fuzzers/audio...
webrtc/test/fuzzers/audio_decoder_fuzzer.cc:26: uint64_t val = 0;
Actually can you use webrtc::ByteReader instead? Just check that sizeof() is
fine to read first.

[hlundin-webrtc, 2016-01-22 13:58:57]

https://codereview.webrtc.org/1607173003/diff/40001/webrtc/test/fuzzers/audio...
File webrtc/test/fuzzers/audio_decoder_fuzzer.cc (right):

https://codereview.webrtc.org/1607173003/diff/40001/webrtc/test/fuzzers/audio...
webrtc/test/fuzzers/audio_decoder_fuzzer.cc:26: uint64_t val = 0;
On 2016/01/21 15:31:24, pbos-webrtc wrote:
> Actually can you use webrtc::ByteReader instead? Just check that sizeof() is
> fine to read first.

That works. Except there is a bug in ByteReader. Fixed in
https://codereview.webrtc.org/1615653011/, which obviously must land before this
CL.

[terelius, 2016-01-25 13:16:34]

lgtm

[pbos-webrtc, 2016-01-25 13:53:08]

https://codereview.webrtc.org/1607173003/diff/60001/webrtc/test/fuzzers/audio...
File webrtc/test/fuzzers/audio_decoder_fuzzer.cc (right):

https://codereview.webrtc.org/1607173003/diff/60001/webrtc/test/fuzzers/audio...
webrtc/test/fuzzers/audio_decoder_fuzzer.cc:22: size_t
PacketSizeFromTwoBytes(const uint8_t* data, size_t size) {
Imo. remove this function and do as below:

https://codereview.webrtc.org/1607173003/diff/60001/webrtc/test/fuzzers/audio...
webrtc/test/fuzzers/audio_decoder_fuzzer.cc:28: T
ReadIntCheckedAndIncrement(const uint8_t** data, size_t* size) {
bool ParseInt(T* int, size_t size, const uint8_t** data, size_t* remaining_size)

https://codereview.webrtc.org/1607173003/diff/60001/webrtc/test/fuzzers/audio...
webrtc/test/fuzzers/audio_decoder_fuzzer.cc:55: while (packet_len != 0 &&
packet_len + 2 <= remaining_size) {
while (ParseInt(&packet_len, 2, data_ptr, &remaining_size) && packet_len <=
remaining_size)

here and below

https://codereview.webrtc.org/1607173003/diff/60001/webrtc/test/fuzzers/audio...
webrtc/test/fuzzers/audio_decoder_fuzzer.cc:100: const size_t header_len =
sizeof(uint16_t) + 2 * sizeof(uint32_t);
kHeaderLength

https://codereview.webrtc.org/1607173003/diff/60001/webrtc/test/fuzzers/audio...
webrtc/test/fuzzers/audio_decoder_fuzzer.cc:105:
ReadIntCheckedAndIncrement<uint16_t>(&data_ptr, &remaining_size);
if (!ParseInt).. (break; or rtp_sequence_number = 0)

[hlundin-webrtc, 2016-01-25 15:47:09]

Wow. Such improve. Very generalization. PTAL.

https://codereview.webrtc.org/1607173003/diff/60001/webrtc/test/fuzzers/audio...
File webrtc/test/fuzzers/audio_decoder_fuzzer.cc (right):

https://codereview.webrtc.org/1607173003/diff/60001/webrtc/test/fuzzers/audio...
webrtc/test/fuzzers/audio_decoder_fuzzer.cc:22: size_t
PacketSizeFromTwoBytes(const uint8_t* data, size_t size) {
On 2016/01/25 13:53:07, pbos-webrtc wrote:
> Imo. remove this function and do as below:

Done.

https://codereview.webrtc.org/1607173003/diff/60001/webrtc/test/fuzzers/audio...
webrtc/test/fuzzers/audio_decoder_fuzzer.cc:28: T
ReadIntCheckedAndIncrement(const uint8_t** data, size_t* size) {
On 2016/01/25 13:53:07, pbos-webrtc wrote:
> bool ParseInt(T* int, size_t size, const uint8_t** data, size_t*
remaining_size)

Done, except that I omitted your second argument (size), and changed the order
of the remaining ones.

https://codereview.webrtc.org/1607173003/diff/60001/webrtc/test/fuzzers/audio...
webrtc/test/fuzzers/audio_decoder_fuzzer.cc:55: while (packet_len != 0 &&
packet_len + 2 <= remaining_size) {
On 2016/01/25 13:53:07, pbos-webrtc wrote:
> while (ParseInt(&packet_len, 2, data_ptr, &remaining_size) && packet_len <=
> remaining_size)
> 
> here and below

Done.

https://codereview.webrtc.org/1607173003/diff/60001/webrtc/test/fuzzers/audio...
webrtc/test/fuzzers/audio_decoder_fuzzer.cc:100: const size_t header_len =
sizeof(uint16_t) + 2 * sizeof(uint32_t);
On 2016/01/25 13:53:07, pbos-webrtc wrote:
> kHeaderLength

Done.

https://codereview.webrtc.org/1607173003/diff/60001/webrtc/test/fuzzers/audio...
webrtc/test/fuzzers/audio_decoder_fuzzer.cc:105:
ReadIntCheckedAndIncrement<uint16_t>(&data_ptr, &remaining_size);
On 2016/01/25 13:53:07, pbos-webrtc wrote:
> if (!ParseInt).. (break; or rtp_sequence_number = 0)

Done.

[hlundin-webrtc, 2016-01-26 13:01:55]

https://codereview.webrtc.org/1607173003/diff/60001/webrtc/test/fuzzers/audio...
File webrtc/test/fuzzers/audio_decoder_fuzzer.cc (right):

https://codereview.webrtc.org/1607173003/diff/60001/webrtc/test/fuzzers/audio...
webrtc/test/fuzzers/audio_decoder_fuzzer.cc:28: T
ReadIntCheckedAndIncrement(const uint8_t** data, size_t* size) {
On 2016/01/25 15:47:09, hlundin-webrtc wrote:
> On 2016/01/25 13:53:07, pbos-webrtc wrote:
> > bool ParseInt(T* int, size_t size, const uint8_t** data, size_t*
> remaining_size)
> 
> Done, except that I omitted your second argument (size), and changed the order
> of the remaining ones.

And when I say "omitted" I mean "moved it to be a template parameter".

[pbos-webrtc, 2016-01-26 14:18:12]

almost done I think, sorry for the slow reply

https://codereview.webrtc.org/1607173003/diff/80001/webrtc/test/fuzzers/audio...
File webrtc/test/fuzzers/audio_decoder_fuzzer.cc (right):

https://codereview.webrtc.org/1607173003/diff/80001/webrtc/test/fuzzers/audio...
webrtc/test/fuzzers/audio_decoder_fuzzer.cc:62: // that it call DecodeRedundant
instead of Decode.
Can you refactor this into one method with an enum separating the difference?
FuzzAudioDecoder(kReundant, ...) ?

https://codereview.webrtc.org/1607173003/diff/80001/webrtc/test/fuzzers/audio...
webrtc/test/fuzzers/audio_decoder_fuzzer.cc:82: // This function is similar to
FuzzAudioDecoder, but also reads fuzzed data into
Same as above if possible.

[pbos-webrtc, 2016-01-26 14:19:08]

https://codereview.webrtc.org/1607173003/diff/80001/webrtc/test/fuzzers/audio...
File webrtc/test/fuzzers/audio_decoder_fuzzer.cc (right):

https://codereview.webrtc.org/1607173003/diff/80001/webrtc/test/fuzzers/audio...
webrtc/test/fuzzers/audio_decoder_fuzzer.cc:82: // This function is similar to
FuzzAudioDecoder, but also reads fuzzed data into
On 2016/01/26 14:18:12, pbos-webrtc wrote:
> Same as above if possible.

Not very possible, ignore this. :)

[hlundin-webrtc, 2016-01-26 22:28:32]

Fold normal and redundant fuzzer functions

[hlundin-webrtc, 2016-01-26 22:31:04]

Fold normal and redundant fuzzer functions

[hlundin-webrtc, 2016-01-26 22:31:22]

Patchset #5 (id:100001) has been deleted

[hlundin-webrtc, 2016-01-26 22:32:31]

Here we go again... :)

https://codereview.webrtc.org/1607173003/diff/80001/webrtc/test/fuzzers/audio...
File webrtc/test/fuzzers/audio_decoder_fuzzer.cc (right):

https://codereview.webrtc.org/1607173003/diff/80001/webrtc/test/fuzzers/audio...
webrtc/test/fuzzers/audio_decoder_fuzzer.cc:62: // that it call DecodeRedundant
instead of Decode.
On 2016/01/26 14:18:12, pbos-webrtc wrote:
> Can you refactor this into one method with an enum separating the difference?
> FuzzAudioDecoder(kReundant, ...) ?

Done.

https://codereview.webrtc.org/1607173003/diff/80001/webrtc/test/fuzzers/audio...
webrtc/test/fuzzers/audio_decoder_fuzzer.cc:82: // This function is similar to
FuzzAudioDecoder, but also reads fuzzed data into
On 2016/01/26 14:19:08, pbos-webrtc wrote:
> On 2016/01/26 14:18:12, pbos-webrtc wrote:
> > Same as above if possible.
> 
> Not very possible, ignore this. :)

Acknowledged.

[pbos-webrtc, 2016-01-27 15:20:03]

lgtm with nit

https://codereview.webrtc.org/1607173003/diff/120001/webrtc/test/fuzzers/audi...
File webrtc/test/fuzzers/audio_decoder_fuzzer.h (right):

https://codereview.webrtc.org/1607173003/diff/120001/webrtc/test/fuzzers/audi...
webrtc/test/fuzzers/audio_decoder_fuzzer.h:35: void
FuzzAudioDecoderRedundant(const uint8_t* data,
Remove this one?

[hlundin-webrtc, 2016-02-08 14:00:20]

Remove FuzzAudioDecoderRedundant from h-file

[hlundin-webrtc, 2016-02-08 14:08:46]

https://codereview.webrtc.org/1607173003/diff/120001/webrtc/test/fuzzers/audi...
File webrtc/test/fuzzers/audio_decoder_fuzzer.h (right):

https://codereview.webrtc.org/1607173003/diff/120001/webrtc/test/fuzzers/audi...
webrtc/test/fuzzers/audio_decoder_fuzzer.h:35: void
FuzzAudioDecoderRedundant(const uint8_t* data,
On 2016/01/27 15:20:03, pbos-webrtc wrote:
> Remove this one?

D'oh!
Done.

[hlundin-webrtc, 2016-02-09 07:59:01]

Description was changed from

==========
Fuzzer tests for AudioDecoder's DecodeRedundant and IncomingPacket

This CL adds new fuzzer tests for the DecodeRedundant and
IncomingPacket methods of AudioDecoder. In practice, only Opus has
DecodeRedundant, and only iSAC has IncomingPacket. Did some minor work
to generalize the helper function reading values from the fuzzed
input.

BUG=webrtc:5306
R=pbos@webrtc.org
==========

to

==========
Fuzzer tests for AudioDecoder's DecodeRedundant and IncomingPacket

This CL adds new fuzzer tests for the DecodeRedundant and
IncomingPacket methods of AudioDecoder. In practice, only Opus has
DecodeRedundant, and only iSAC has IncomingPacket. Did some minor work
to generalize the helper function reading values from the fuzzed
input.

BUG=webrtc:5306
R=pbos@webrtc.org
NOTRY=true
==========

[hlundin-webrtc, 2016-02-09 07:59:08]

The CQ bit was checked by henrik.lundin@webrtc.org

[hlundin-webrtc, 2016-02-09 07:59:09]

The patchset sent to the CQ was uploaded after l-g-t-m from terelius@webrtc.org,
pbos@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1607173003/#ps140001 (title:
"Remove FuzzAudioDecoderRedundant from h-file")

[commit-bot: I haz the power, 2016-02-09 07:59:17]

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

[commit-bot: I haz the power, 2016-02-09 08:00:27]

Description was changed from

==========
Fuzzer tests for AudioDecoder's DecodeRedundant and IncomingPacket

This CL adds new fuzzer tests for the DecodeRedundant and
IncomingPacket methods of AudioDecoder. In practice, only Opus has
DecodeRedundant, and only iSAC has IncomingPacket. Did some minor work
to generalize the helper function reading values from the fuzzed
input.

BUG=webrtc:5306
R=pbos@webrtc.org
NOTRY=true
==========

to

==========
Fuzzer tests for AudioDecoder's DecodeRedundant and IncomingPacket

This CL adds new fuzzer tests for the DecodeRedundant and
IncomingPacket methods of AudioDecoder. In practice, only Opus has
DecodeRedundant, and only iSAC has IncomingPacket. Did some minor work
to generalize the helper function reading values from the fuzzed
input.

BUG=webrtc:5306
R=pbos@webrtc.org
NOTRY=true
==========

[commit-bot: I haz the power, 2016-02-09 08:00:28]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2016-02-09 08:00:43]

Description was changed from

==========
Fuzzer tests for AudioDecoder's DecodeRedundant and IncomingPacket

This CL adds new fuzzer tests for the DecodeRedundant and
IncomingPacket methods of AudioDecoder. In practice, only Opus has
DecodeRedundant, and only iSAC has IncomingPacket. Did some minor work
to generalize the helper function reading values from the fuzzed
input.

BUG=webrtc:5306
R=pbos@webrtc.org
NOTRY=true
==========

to

==========
Fuzzer tests for AudioDecoder's DecodeRedundant and IncomingPacket

This CL adds new fuzzer tests for the DecodeRedundant and
IncomingPacket methods of AudioDecoder. In practice, only Opus has
DecodeRedundant, and only iSAC has IncomingPacket. Did some minor work
to generalize the helper function reading values from the fuzzed
input.

BUG=webrtc:5306
R=pbos@webrtc.org
NOTRY=true

Committed: https://crrev.com/fd2be2718d984792b949b186ff11ceaf256723d0
Cr-Commit-Position: refs/heads/master@{#11533}
==========

[commit-bot: I haz the power, 2016-02-09 08:00:44]

Patchset 6 (id:??) landed as
https://crrev.com/fd2be2718d984792b949b186ff11ceaf256723d0
Cr-Commit-Position: refs/heads/master@{#11533}