Update the current RTP parser fuzzer to handle header extensions.

webrtc/test/fuzzers/rtp_packet_fuzzer.cc

Index: webrtc/test/fuzzers/rtp_packet_fuzzer.cc
diff --git a/webrtc/test/fuzzers/rtp_packet_fuzzer.cc b/webrtc/test/fuzzers/rtp_packet_fuzzer.cc
index a9efdb96ecfe4f7a77873f54edbb55108c9497f9..726208af836d83673215608db055fb34a9b0868e 100644
--- a/webrtc/test/fuzzers/rtp_packet_fuzzer.cc
+++ b/webrtc/test/fuzzers/rtp_packet_fuzzer.cc
@@ -7,13 +7,34 @@
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
+
 #include "webrtc/modules/rtp_rtcp/source/rtp_packet_received.h"
+#include "webrtc/modules/rtp_rtcp/source/rtp_header_extension.h"
+#include "webrtc/modules/rtp_rtcp/source/rtp_header_extensions.h"
 
 namespace webrtc {
-
 void FuzzOneInput(const uint8_t* data, size_t size) {
-  RtpPacketReceived packet;
+  if (size <= 1)
+    return;
+
+  // We decide which header extensions to register by reading one byte

[danilchap, 2016/06/27 11:06:17]

The way parsing is written, registering all extensions shouldn't leave any
codepath unchecked as long as we support less than 14 extensions.

[katrielc, 2016/07/04 07:15:31]

What about an out-of-bounds read that reads too far into the buffer? That's
legal unless no other headers and data are present, right?

[danilchap, 2016/07/04 08:43:55]

I'm not sure how not registering extension can help this case:
not registering extension @id=1 is same as registering same extension at @id=2
(and registering no other extension at @id=1), so with 6 extensions some ids
will still be free and parser still should behave when they appear: should skip
them when length provided correctly, should fail the parse when not.

They only code path that is not covering is behavior of GetExtension function
when extension is not registered, but that should be covered by unit test, not
fuzzer.

On the other side.... that's about current implementation. Future implementation
might be different and may fail differently with non-registered extensions.

+  // from the beginning of |data| and interpreting it as a bitmask
+  // over the RTPExtensionType enum. That byte shouldn't also be part
+  // of the packet, so we adjust |data| and |size| to remove it.
+  std::bitset<8> extensionMask(data[0]);
+  data++;
+  size--;
+
+  RtpPacketReceived::ExtensionManager extensions;

[pbos-webrtc, 2016/06/26 21:49:59]

Make a compile-time assert on kRtpExtensionNumberOfExtensions being <= size of
extensionMask, so that we have to update this fuzzer if we add more extensions.

[katrielc, 2016/07/04 07:15:31]

Done.

+  for (int i = 1; i < kRtpExtensionNumberOfExtensions; i++) {
+    // Skip i=0 which is kRtpExtensionNone i.e. not an actual extension.

[pbos-webrtc, 2016/06/26 21:49:59]

From zero, but skip if it's kRtpExtensionNone explicitly, instead of relying on
declaration order.

e.g. RTPExtensionType extension_type = static_cast<RTPExtensionType>(i);

if (extension_type == kRtpExtensionNone)
  continue;

...

[katrielc, 2016/07/04 07:15:31]

Done.

+    if (extensionMask[i]) {
+      // We use i as the ID; it's used in negotiation so not relevant.
+      extensions.Register(static_cast<RTPExtensionType>(i), i);
+    }
+  }
 
+  RtpPacketReceived packet(&extensions);
   packet.Parse(data, size);
 
   // Call packet accessors because they have extra checks.
@@ -23,7 +44,39 @@ void FuzzOneInput(const uint8_t* data, size_t size) {
   packet.Timestamp();
   packet.Ssrc();
   packet.Csrcs();
-}
 
+  // Each extension has its own getter. Call all the ones we registered above.

[pbos-webrtc, 2016/06/26 21:49:59]

Should this have defined behavior on non-registered extensions?

[danilchap, 2016/06/27 11:06:17]

Yes. both for non-registered and non-present extensions GetExtension should
return false.
Which mean it might be better to replace loop with set of GetExtension, ignoring
'registered' status.

[katrielc, 2016/07/04 07:15:31]

Acknowledged.

+  for (int i = 1; i < kRtpExtensionNumberOfExtensions; i++) {
+    if (extensionMask[i]) {
+      switch (i) {

[pbos-webrtc, 2016/06/26 21:49:59]

cast to RTPExtensionType here instead of to int below.

[katrielc, 2016/07/04 07:15:31]

Done.

+        case (int)kRtpExtensionTransmissionTimeOffset:
+          int32_t offset;
+          packet.GetExtension<TransmissionOffset>(&offset);
+          break;
+        case (int)kRtpExtensionAudioLevel:
+          bool voice_activity;
+          uint8_t audio_level;
+          packet.GetExtension<AudioLevel>(&voice_activity, &audio_level);
+          break;
+        case (int)kRtpExtensionAbsoluteSendTime:
+          uint32_t sendtime;
+          packet.GetExtension<AbsoluteSendTime>(&sendtime);
+          break;
+        case (int)kRtpExtensionVideoRotation:
+          uint8_t rotation;
+          packet.GetExtension<VideoOrientation>(&rotation);
+          break;
+        case (int)kRtpExtensionTransportSequenceNumber:
+          uint16_t seqnum;
+          packet.GetExtension<TransportSequenceNumber>(&seqnum);
+          break;
+        case (int)kRtpExtensionPlayoutDelay:
+          // TODO(danilchap) Add this once it's written.

[pbos-webrtc, 2016/06/26 21:49:59]

TODO(katrielc):

[katrielc, 2016/07/04 07:15:31]

Done, though I'm leaving in two weeks...

[danilchap, 2016/07/04 08:43:55]

That doesn't matter here: TODO name the person who wrote it, not the person who
would do it.

+          break;
+        default:
+          break;

[pbos-webrtc, 2016/06/26 21:49:59]

Remove default, or assert false on it.

[katrielc, 2016/07/04 07:15:31]

Done.

+      }
+    }
+  }
+}
 }  // namespace webrtc
-