Eric and I decided to open a separate bug for this, to potentially implement this patch into the unit-test upstream. This bug shall remain hidden until further action has taken place. Usage: $ export MOZ_WEBRTC_TESTS=1 $ export DYLD_LIBRARY_PATH=<OBJDIR>dist/lib Supported Environment Variables $ export RTP_FUZZER $ export RTCP_FUZZER $ export RANDOM_SEED Deactivate $ unset RTP_FUZZER $ unset RTCP_FUZZER Execution $ <OBJDIR>media/webrtc/signaling/test/mediaconduit_unittests Edit/Compile $ media/webrtc/signaling/test/mediaconduit_unittests.cpp $ cd <OBJDIR>/media/webrtc/signaling/test && make mediaconduit Tested with m-c changeset: 114084:0d373cf880fd

Comment on attachment 684834 [details] [diff] [review] RTP-RTCP_fuzzer_v1 Review of attachment 684834 [details] [diff] [review]: ----------------------------------------------------------------- A bunch of suggested improvements below. ::: media/webrtc/signaling/test/mediaconduit_unittests.cpp @@ +407,5 @@ > * to the conduit for eventual decoding and rendering. > */ > +unsigned int RandomNumber(unsigned int max) > +{ > + return (unsigned int)(rand() % max); You might consider using the PRNG in this code so that you can guarantee deterministic results in case someone else uses rand(). Please use C++-style casts throughout this patch. @@ +412,5 @@ > +} > + > +template <class T> T RandomNumberRange(T min, T max) > +{ > + T x = (T)rand() % (max - min) + min; Suggest a compile-time assert here that sizeof(T) <= sizeof(int). This ensures you are getting the full range of the PRNG. @@ +420,5 @@ > +void Hexdump(unsigned char *buf, int len) > +{ > + int i, j; > + > + for (i = 0; i < len; i += 16) { Ordinarily, I would suggest restructuring this code to build one line at a time and then emit it. But in test code it doesn't really matter. @@ +458,5 @@ > ~FakeMediaTransport() > { > } > > + const void *MutatePacket(const void *data, int len, const char* type) Suggest changing this code to take "uint8_t *data" and then change the caller to do the const-cast. Better yet, honestly, would be to have the caller copy the data. Generally un-consting a const variable so that you can mutate it is really bad news as you're explicitly violating the interface contract. I usually try to use const_cast only to bridge interface boundaries where I know that the other side will not modify the data, but just isn't const correct. @@ +465,5 @@ > + std::cerr << "Seed: " << seed << std::endl; > + std::cerr << "Packet: " << std::endl; > + unsigned char *packet = (unsigned char*)data; > + Hexdump(packet, len); > + Blank space. @@ +468,5 @@ > + Hexdump(packet, len); > + > + uint32_t maxMutations = RandomNumber(len); > + for (uint32_t i = 0; i < maxMutations; ++i) { > + uint32_t position = RandomNumberRange<uint32_t>(0, len); Don't we want int rather than uint32_t because otherwise we are doing an unsigned-signed conversion. @@ +469,5 @@ > + > + uint32_t maxMutations = RandomNumber(len); > + for (uint32_t i = 0; i < maxMutations; ++i) { > + uint32_t position = RandomNumberRange<uint32_t>(0, len); > + *(packet+position) = RandomNumberRange<unsigned char>(0, 255); suggest packet[position] @@ +485,5 @@ > if(mAudio) > { > + if (isRTPFuzzerEnabled) > + { > + const void *packet = MutatePacket(data,len, "ReceiveRTPPacketAudio"); I would suggest copying into a temporary. We know (through external knowledge) that WebRTC will never produce a packet bigger than the Ethernet MTU. So, have a member variable like so: unsigned char mBuffer[1500]; Then here do: if (isRTPFuzzerEnabled) { MOZ_ASSERT(len <= sizeof(mBuffer)); int temp_len = std::min(len, sizeof(mBuffer)); memcpy(mBuffer, data, temp_len); MutatePacket(mBuffer, temp_len); data = mBuffer; len = temp_len; } And then you won't need the else. Also, you can push this code up above the if (mAudio), because you are replacing data, thus making the code simpler. @@ +844,5 @@ > { > // This test can cause intermittent oranges on the builders > CHECK_ENVIRONMENT_FLAG("MOZ_WEBRTC_TESTS") > > + // Initialize RTP/RTCP Fuzzer I would suggest that instead of using environment variables, you simply make these a standard part of the test harness. I.e., add a member function to TransportConduitTest like: void EnableFuzzing(bool aRTP, bool aRTCP); And then you can simply have new tests, like: TEST_F(TransportConduitTest, TestDummyAudioWithTransportFuzzingRTP) { EnableFuzzing(true, false); TestDummyAudioAndTransport(); } That way people can just use the standard gtest controls. And you want to just fuzz but do it a lot? No problem: mediaconduit_unittests '--gtest_filter=*Fuzz*' --gtest_repeat=100 I would suggest also making the seed an argument rather than an env variable, but that's more of a taste thing.

Thanks for reviewing Eric. I have applied those recommendations to the new patch. New command to execute: <OBJDIR>/media/webrtc/signaling/test/mediaconduit_unittests --gtest_filter=*FuzzingRTCP* --gtest_repeat=1000 --seed 123456 Whereby --seed is optional and gtest_filter can be adjusted to FuzzingRTP, FuzzingRTCP or FuzzingRTPAndRTCP.

Comment on attachment 685106 [details] [diff] [review] RTP-RTCP_fuzzer_v2 Review of attachment 685106 [details] [diff] [review]: ----------------------------------------------------------------- This is much improved. One real bug and one potential bug below, plus a bunch of minor comments. ::: media/webrtc/signaling/test/mediaconduit_unittests.cpp @@ +29,5 @@ > +//RTP/RTCP Fuzzer > +#define MAX_SIZE_MTU 1500 > +unsigned int seed = 0; > +bool isRTPFuzzerEnabled; > +bool isRTCPFuzzerEnabled; These two should be member variables of TransportConduitTest or, perahaps of FakeMediaTransport but frobbed via TCTest. They will also need sensible initializors (false, false). Note this is currently a bug since if you run the entire test harness on repeat, it will misbehave in the non-fuzzing tests. @@ +408,5 @@ > * to the conduit for eventual decoding and rendering. > */ > +uint32_t RandomNumber(int max) > +{ > + return static_cast<uint32_t>(random() % max); random(3) returns long, so probably this should return long as well. @@ +413,5 @@ > +} > + > +template <class T> T RandomNumberRange(T min, T max) > +{ > + MOZ_ASSERT(sizeof(T) <= sizeof(int)); This should be "long" for the reasons above. Also MOZ_ASSERT(max < LONG_MAX) and MOZ_ASSERT(min < max). @@ +414,5 @@ > + > +template <class T> T RandomNumberRange(T min, T max) > +{ > + MOZ_ASSERT(sizeof(T) <= sizeof(int)); > + T x = static_cast<T>(random() % (max - min) + min); This assignment is unnecessary, since you can just do "return static_cast<>..." @@ +460,5 @@ > ~FakeMediaTransport() > { > } > > + const void *MutatePacket(unsigned char*packet, int len, const char* type) You never use the return value of MutatePacket How about have it return void. @@ +485,5 @@ > + packet[position] = '\xff'; > + } > + } > + > + cerr << "Packet (Mutated Fields: " << maxMutations << "): " << endl; Note that this potentially returns the wrong answer if you coincidentally mutate the same field twice. @@ +496,5 @@ > { > ++numPkts; > + unsigned char mBuffer[MAX_SIZE_MTU]; > + MOZ_ASSERT(len <= sizeof(mBuffer)); > + memset(mBuffer, 0, sizeof(mBuffer)); What is this memset for? @@ +503,3 @@ > if(mAudio) > { > + if (isRTPFuzzerEnabled) If you pull these above the mAudio check you could save some duplication. @@ +523,5 @@ > virtual nsresult SendRtcpPacket(const void* data, int len) > { > + unsigned char mBuffer[MAX_SIZE_MTU]; > + MOZ_ASSERT(len <= sizeof(mBuffer)); > + memset(mBuffer, 0, sizeof(mBuffer)); Why the memset? @@ +911,5 @@ > + return 1; > + } > + } > + } > + srandom(seed); This does not behave the way you would expect in the case of repeats. Do you want to refactor it so that you re-srandom() on every individual test? (You do this with SetUp in the test class).

(In reply to Eric Rescorla from comment #3) > @@ +413,5 @@ > > +} > > + > > +template <class T> T RandomNumberRange(T min, T max) > > +{ > > + MOZ_ASSERT(sizeof(T) <= sizeof(int)); > > This should be "long" for the reasons above. > > Also MOZ_ASSERT(max < LONG_MAX) and MOZ_ASSERT(min < max). I'd add MOZ_ASSERT(min >= 0) as well. > > @@ +496,5 @@ > > { > > ++numPkts; > > + unsigned char mBuffer[MAX_SIZE_MTU]; > > + MOZ_ASSERT(len <= sizeof(mBuffer)); > > + memset(mBuffer, 0, sizeof(mBuffer)); > > What is this memset for? I'm guessing general cleanliness and to allow easier visibility of what's been modified in a debugger (without actually looking at the code block, so I could be wrong!)

I used memset() to prevent having uninitialized memory in the buffer. I have attached a new version with the latest recommendations applied. The command to run the fuzzer has not changed.

I had a basic question ... What are the mutations on the RTP/RTCP packet data supposed to test with respect to the conduit .. Sorry if I am missing something fundamental here

Comment on attachment 685648 [details] [diff] [review] RTP-RTCP_fuzzer_v3 Review of attachment 685648 [details] [diff] [review]: ----------------------------------------------------------------- r- for me due to the "how do I find which packet caused what error issue" (i.e. fuzz on receive not send.) ::: media/webrtc/signaling/test/mediaconduit_unittests.cpp @@ +479,5 @@ > + isRTPFuzzerEnabled = aRTP; > + isRTCPFuzzerEnabled = aRTCP; > + } > + > + void MutatePacket(unsigned char*packet, int len) char *packet, or if the file prefers this style char* packet @@ +482,5 @@ > + > + void MutatePacket(unsigned char*packet, int len) > + { > + cerr << "Seed: " << seed << endl; > + cerr << "Packet:" << endl; If this is used with data in both directions, we won't be able to tell which direction this is for except very indirectly (RTP header values in the packet dump). @@ +484,5 @@ > + { > + cerr << "Seed: " << seed << endl; > + cerr << "Packet:" << endl; > + > + Hexdump(packet, len); Realize that's a TON of data for video, and a lot for audio. But it's handy for tracking down why it failed. Note that the failure will be on the OTHER side, so if this is used in a non-loopback way, you need to look at the *other* side for the data - and then you may not be able to figure out which packet killed it, as the sender will happily continue. Thus, I think it should mutate (and log) on *reception* (after decrypt), not on send. @@ +499,5 @@ > + case 1: > + packet[position] = '\x00'; > + break; > + default: > + packet[position] = '\xff'; So, 50% 0xFF, 25% 0x00, 25% 0-0xFF Mutations like this will likely cause toss-away in the RTP layer (if they hit the RTP header, or many mutations in RTCP) due to violating constraints. Still useful though, especially as a start. A more detailed fuzzer would know reasonable mutations of RTP/RTCP so as to stress more than the "invalid packet" logic, and also have a mode to fuzz codec data only or RTP/RTCP data only.

Christoph are still planing on landing this, or can we close it?

Nä, the approach wasn't good and was only run as WebRTC got built into Firefox. We should stick with LibFuzzer now.