Setting up an RTP input fuzzer for NetEq

Setting up an RTP input fuzzer for NetEq

This CL introduces a new fuzzer target neteq_rtp_fuzzer that
manipulates the RTP header fields before inserting the packets into
NetEq. A few helper classes are also introduced.

BUG=webrtc:5447
NOTRY=True

Committed: https://crrev.com/2d273f1e97cd5030ed1686f27ce1118291b66395
Cr-Commit-Position: refs/heads/master@{#14103}

[hlundin-webrtc, 2016-09-06 08:15:08]

henrik.lundin@webrtc.org changed reviewers:
+ ivoc@webrtc.org, kjellander@webrtc.org

[hlundin-webrtc, 2016-09-06 08:15:09]

Adding a new fuzzer test for NetEq.

Please, review as follows.
ivoc: webrtc/modules/audio_coding
kjellander: webrtc/test/fuzzers

Thanks!

[kjellander_webrtc, 2016-09-06 12:12:55]

webrtc/test/fuzzers: lgtm but I don't know much about the code in
	webrtc/test/fuzzers/neteq_rtp_fuzzer.cc so it's better somebody else review
that.

[hlundin-webrtc, 2016-09-06 12:15:19]

On 2016/09/06 12:12:55, kjellander_webrtc wrote:
> webrtc/test/fuzzers: lgtm but I don't know much about the code in
> 	webrtc/test/fuzzers/neteq_rtp_fuzzer.cc so it's better somebody else review
> that.

Let's have ivoc look at webrtc/test/fuzzers/neteq_rtp_fuzzer.cc too. Please.

[ivoc, 2016-09-07 11:19:25]

https://codereview.webrtc.org/2315633002/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/tools/encode_neteq_input.cc (right):

https://codereview.webrtc.org/2315633002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/tools/encode_neteq_input.cc:64:
packet_data_->payload.Clear();
Is this really needed, considering that packet_data_ was just constructed?

https://codereview.webrtc.org/2315633002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/tools/encode_neteq_input.cc:70:
rtc::CheckedDivExact(encoder_->SampleRateHz(), 100);
I think we could replace the 100 with 1000/kOutputPeriodMs for a little more
genericity, WDYT?

https://codereview.webrtc.org/2315633002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/tools/encode_neteq_input.cc:86:
next_packet_time_ms_ += num_blocks * 10;
Seems like another good place to use kOutputPeriodMs

https://codereview.webrtc.org/2315633002/diff/1/webrtc/test/fuzzers/neteq_rtp...
File webrtc/test/fuzzers/neteq_rtp_fuzzer.cc (right):

https://codereview.webrtc.org/2315633002/diff/1/webrtc/test/fuzzers/neteq_rtp...
webrtc/test/fuzzers/neteq_rtp_fuzzer.cc:39: FuzzHeader();
FuzzHeader() is called from PopPacket, so I think this can be removed.

https://codereview.webrtc.org/2315633002/diff/1/webrtc/test/fuzzers/neteq_rtp...
webrtc/test/fuzzers/neteq_rtp_fuzzer.cc:54: FuzzHeader();
Same here

https://codereview.webrtc.org/2315633002/diff/1/webrtc/test/fuzzers/neteq_rtp...
webrtc/test/fuzzers/neteq_rtp_fuzzer.cc:76: packet_->header.header.payloadType
+=
Why is this an addition instead of an assignment? Won't it create a bias towards
higher payload types?

https://codereview.webrtc.org/2315633002/diff/1/webrtc/test/fuzzers/neteq_rtp...
webrtc/test/fuzzers/neteq_rtp_fuzzer.cc:136: void FuzzOneInput(const uint8_t*
data, size_t size) {
What is the point of this function?

[hlundin-webrtc, 2016-09-07 11:43:09]

Thanks for the comments. Please, take a look again.

https://codereview.webrtc.org/2315633002/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/tools/encode_neteq_input.cc (right):

https://codereview.webrtc.org/2315633002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/tools/encode_neteq_input.cc:64:
packet_data_->payload.Clear();
On 2016/09/07 11:19:25, ivoc wrote:
> Is this really needed, considering that packet_data_ was just constructed?

Done.

https://codereview.webrtc.org/2315633002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/tools/encode_neteq_input.cc:70:
rtc::CheckedDivExact(encoder_->SampleRateHz(), 100);
On 2016/09/07 11:19:25, ivoc wrote:
> I think we could replace the 100 with 1000/kOutputPeriodMs for a little more
> genericity, WDYT?

Done.

https://codereview.webrtc.org/2315633002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/tools/encode_neteq_input.cc:86:
next_packet_time_ms_ += num_blocks * 10;
On 2016/09/07 11:19:25, ivoc wrote:
> Seems like another good place to use kOutputPeriodMs

Done.

https://codereview.webrtc.org/2315633002/diff/1/webrtc/test/fuzzers/neteq_rtp...
File webrtc/test/fuzzers/neteq_rtp_fuzzer.cc (right):

https://codereview.webrtc.org/2315633002/diff/1/webrtc/test/fuzzers/neteq_rtp...
webrtc/test/fuzzers/neteq_rtp_fuzzer.cc:39: FuzzHeader();
On 2016/09/07 11:19:25, ivoc wrote:
> FuzzHeader() is called from PopPacket, so I think this can be removed.

No, this calls PopPacket on the input_, which does not call FuzzHeader. This is
a way of preloading the first packet.

https://codereview.webrtc.org/2315633002/diff/1/webrtc/test/fuzzers/neteq_rtp...
webrtc/test/fuzzers/neteq_rtp_fuzzer.cc:54: FuzzHeader();
On 2016/09/07 11:19:25, ivoc wrote:
> Same here

Same reply.

https://codereview.webrtc.org/2315633002/diff/1/webrtc/test/fuzzers/neteq_rtp...
webrtc/test/fuzzers/neteq_rtp_fuzzer.cc:76: packet_->header.header.payloadType
+=
On 2016/09/07 11:19:25, ivoc wrote:
> Why is this an addition instead of an assignment? Won't it create a bias
towards
> higher payload types?

packet_->header.header.payloadType is a uint8_t, so it will wrap. And it is
capped in the next line, too.

But I guess I should change all of these to assignments instead of additions.

https://codereview.webrtc.org/2315633002/diff/1/webrtc/test/fuzzers/neteq_rtp...
webrtc/test/fuzzers/neteq_rtp_fuzzer.cc:136: void FuzzOneInput(const uint8_t*
data, size_t size) {
On 2016/09/07 11:19:25, ivoc wrote:
> What is the point of this function? 

This is the entry point for the fuzzer, like the main function for a regular
executable.

[ivoc, 2016-09-07 11:55:06]

LGTM.

[hlundin-webrtc, 2016-09-07 11:56:46]

Description was changed from

==========
Setting up an RTP input fuzzer for NetEq

This CL introduces a new fuzzer target neteq_rtp_fuzzer that
manipulates the RTP header fields before inserting the packets into
NetEq. A few helper classes are also introduced.

BUG=webrtc:5447
==========

to

==========
Setting up an RTP input fuzzer for NetEq

This CL introduces a new fuzzer target neteq_rtp_fuzzer that
manipulates the RTP header fields before inserting the packets into
NetEq. A few helper classes are also introduced.

BUG=webrtc:5447
NOTRY=True
==========

[hlundin-webrtc, 2016-09-07 12:41:48]

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

[hlundin-webrtc, 2016-09-07 12:41:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
kjellander@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2315633002/#ps20001 (title:
"After review")

[commit-bot: I haz the power, 2016-09-07 12:41:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-09-07 12:57:28]

Description was changed from

==========
Setting up an RTP input fuzzer for NetEq

This CL introduces a new fuzzer target neteq_rtp_fuzzer that
manipulates the RTP header fields before inserting the packets into
NetEq. A few helper classes are also introduced.

BUG=webrtc:5447
NOTRY=True
==========

to

==========
Setting up an RTP input fuzzer for NetEq

This CL introduces a new fuzzer target neteq_rtp_fuzzer that
manipulates the RTP header fields before inserting the packets into
NetEq. A few helper classes are also introduced.

BUG=webrtc:5447
NOTRY=True
==========

[commit-bot: I haz the power, 2016-09-07 12:57:29]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-07 12:57:37]

Description was changed from

==========
Setting up an RTP input fuzzer for NetEq

This CL introduces a new fuzzer target neteq_rtp_fuzzer that
manipulates the RTP header fields before inserting the packets into
NetEq. A few helper classes are also introduced.

BUG=webrtc:5447
NOTRY=True
==========

to

==========
Setting up an RTP input fuzzer for NetEq

This CL introduces a new fuzzer target neteq_rtp_fuzzer that
manipulates the RTP header fields before inserting the packets into
NetEq. A few helper classes are also introduced.

BUG=webrtc:5447
NOTRY=True

Committed: https://crrev.com/2d273f1e97cd5030ed1686f27ce1118291b66395
Cr-Commit-Position: refs/heads/master@{#14103}
==========

[commit-bot: I haz the power, 2016-09-07 12:57:38]

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

[hlundin-webrtc, 2016-09-08 12:00:21]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.webrtc.org/2328483002/ by henrik.lundin@webrtc.org.

The reason for reverting is: Broke all Chromium libFuzzer builds
https://bugs.chromium.org/p/chromium/issues/detail?id=645069
.