fix RTCP firewall hole punch pkt format

I happened to notice that Wireshark complained about this packet.

This was using a RTCP SR but omitting the mandatory sender info block.
Just switch to using an RR which does not include this block and test
that it validates via our own logic.

Reassuringly, ffmpeg also uses an RR in its `ff_rtp_send_punch_packets`.

CHANGELOG.md

@@ -1,6 +1,7 @@
 ## unreleased
 
 *   fix H.265 SPS parsing flaw that affected the Tenda CP3PRO camera.
+*   fix format of RTCP packet used for firewall hole punching.
 
 ## `v0.4.12` (2025-01-28)
 

src/client/mod.rs

@@ -2023,6 +2023,22 @@ fn note_stale_live555_data(
     });
 }
 
+#[rustfmt::skip]
+const HOLE_PUNCH_RTP: [u8; 12] = [
+    2 << 6,     // version=2 + p=0 + x=0 + cc=0
+    0,          // m=0 + pt=0
+    0, 0,       // sequence number=0
+    0, 0, 0, 0, // timestamp=0 0,
+    0, 0, 0, 0, // ssrc=0
+];
+#[rustfmt::skip]
+const HOLE_PUNCH_RTCP: [u8; 8] = [
+    2 << 6,     // version=2 + p=0 + rc=0
+    201,        // pt=200 (reception report)
+    0, 1,       // length=1 (in 4-byte words minus 1)
+    0, 0, 0, 0, // ssrc=0 (bogus but we don't know the ssrc reliably yet)
+];
+
 /// Sends dummy RTP and RTCP packets to punch a hole in connection-tracking
 /// firewalls.
 ///
@@ -2034,23 +2050,8 @@ fn note_stale_live555_data(
 /// Note this is insufficient for NAT traversal; the NAT firewall must be
 /// RTSP-aware to rewrite the Transport header's client_ports.
 async fn punch_firewall_hole(sockets: &UdpSockets) -> Result<(), std::io::Error> {
-    #[rustfmt::skip]
-    const DUMMY_RTP: [u8; 12] = [
-        2 << 6,     // version=2 + p=0 + x=0 + cc=0
-        0,          // m=0 + pt=0
-        0, 0,       // sequence number=0
-        0, 0, 0, 0, // timestamp=0 0,
-        0, 0, 0, 0, // ssrc=0
-    ];
-    #[rustfmt::skip]
-    const DUMMY_RTCP: [u8; 8] = [
-        2 << 6,     // version=2 + p=0 + rc=0
-        200,        // pt=200 (reception report)
-        0, 1,       // length=1 (in 4-byte words minus 1)
-        0, 0, 0, 0, // ssrc=0 (bogus but we don't know the ssrc reliably yet)
-    ];
-    sockets.rtp.send(&DUMMY_RTP[..]).await?;
-    sockets.rtcp.send(&DUMMY_RTCP[..]).await?;
+    sockets.rtp.send(&HOLE_PUNCH_RTP[..]).await?;
+    sockets.rtcp.send(&HOLE_PUNCH_RTCP[..]).await?;
     Ok(())
 }
 
@@ -3194,4 +3195,19 @@ mod tests {
         let stale_sessions = group.stale_sessions();
         assert_send(group.await_stale_sessions(&stale_sessions));
     }
+
+    #[test]
+    fn validate_hole_punch_rtp() {
+        let (pkt_ref, _) = crate::rtp::RawPacket::new(Bytes::from_static(&HOLE_PUNCH_RTP)).unwrap();
+        assert_eq!(pkt_ref.payload_type(), 0);
+    }
+
+    #[test]
+    fn validate_hole_punch_rtcp() {
+        let (pkt_ref, _) = crate::rtcp::PacketRef::parse(&HOLE_PUNCH_RTCP).unwrap();
+        assert!(matches!(
+            pkt_ref.as_typed().unwrap(),
+            Some(crate::rtcp::TypedPacketRef::ReceiverReport(_))
+        ));
+    }
 }

src/rtcp.rs

@@ -135,6 +135,7 @@ impl<'a> Iterator for CompoundPacketIterator<'a> {
 #[non_exhaustive]
 pub enum TypedPacketRef<'a> {
     SenderReport(SenderReportRef<'a>),
+    ReceiverReport(ReceiverReportRef<'a>),
 }
 
 /// A sender report, as defined in
@@ -192,7 +193,7 @@ impl<'a> SenderReportRef<'a> {
                 count, pkt.payload_end
             ));
         }
-        Ok(SenderReportRef(pkt))
+        Ok(Self(pkt))
     }
 
     pub fn ssrc(&self) -> u32 {
@@ -216,6 +217,67 @@ impl<'a> std::ops::Deref for SenderReportRef<'a> {
     }
 }
 
+/// A receiver report, as defined in
+/// [RFC 3550 section 6.4.2](https://datatracker.ietf.org/doc/html/rfc3550#section-6.4.2).
+///
+/// ```text
+///         0                   1                   2                   3
+///         0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
+///        +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+/// header |V=2|P|    RC   |   PT=RR=201   |             length            |
+///        +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+///        |                     SSRC of packet sender                     |
+///        +=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+
+/// report |                 SSRC_1 (SSRC of first source)                 |
+/// block  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+///   1    | fraction lost |       cumulative number of packets lost       |
+///        +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+///        |           extended highest sequence number received           |
+///        +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+///        |                      interarrival jitter                      |
+///        +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+///        |                         last SR (LSR)                         |
+///        +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+///        |                   delay since last SR (DLSR)                  |
+///        +=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+
+/// report |                 SSRC_2 (SSRC of second source)                |
+/// block  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+///   2    :                               ...                             :
+///        +=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+=+
+///        |                  profile-specific extensions                  |
+///        +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+/// ```
+///
+pub struct ReceiverReportRef<'a>(PacketRef<'a>);
+
+impl<'a> ReceiverReportRef<'a> {
+    fn validate(pkt: PacketRef<'a>) -> Result<Self, String> {
+        let count = usize::from(pkt.count());
+        const HEADER_LEN: usize = 8;
+        const REPORT_BLOCK_LEN: usize = 24;
+        let expected_len = HEADER_LEN + (count * REPORT_BLOCK_LEN);
+        if pkt.payload_end < expected_len {
+            return Err(format!(
+                "RTCP RR has invalid count={} with unpadded_byte_len={}",
+                count, pkt.payload_end
+            ));
+        }
+        Ok(Self(pkt))
+    }
+
+    pub fn ssrc(&self) -> u32 {
+        u32::from_be_bytes(self.0.buf[4..8].try_into().unwrap())
+    }
+}
+
+impl<'a> std::ops::Deref for ReceiverReportRef<'a> {
+    type Target = PacketRef<'a>;
+
+    fn deref(&self) -> &Self::Target {
+        &self.0
+    }
+}
+
 /// A generic packet, not parsed as any particular payload type.
 ///
 /// This only interprets the leading four bytes:
@@ -304,6 +366,9 @@ impl<'a> PacketRef<'a> {
             200 => Ok(Some(TypedPacketRef::SenderReport(
                 SenderReportRef::validate(self)?,
             ))),
+            201 => Ok(Some(TypedPacketRef::ReceiverReport(
+                ReceiverReportRef::validate(self)?,
+            ))),
             _ => Ok(None),
         }
     }