From 76bbec1fd03990184e5e131f418531af1703cc47 Mon Sep 17 00:00:00 2001 From: nightness Date: Wed, 1 Apr 2026 07:42:35 -0500 Subject: [PATCH 01/10] fix(rtp): add marshal-side bounds checks for CSRC count, extension lengths, and NALU sizes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RFC 3550 §5.1: CC is a 4-bit field (max 15 CSRCs); return TooManyCSRCs error if exceeded. RFC 8285 §4.2/4.3: one-byte extension payload must be 1–16 bytes, two-byte max 255 bytes; return errors rather than silently truncating via `as u8` cast. H.264/H.265 aggregation length fields are u16; skip NALUs > 65535 bytes instead of silently truncating their length field. Co-Authored-By: Claude Sonnet 4.6 --- rtc-rtp/src/codec/h264/mod.rs | 30 +++++++++++++++++------------- rtc-rtp/src/codec/h265/mod.rs | 4 ++++ rtc-rtp/src/header.rs | 20 +++++++++++++++++--- rtc-shared/src/error.rs | 8 ++++++++ 4 files changed, 46 insertions(+), 16 deletions(-) diff --git a/rtc-rtp/src/codec/h264/mod.rs b/rtc-rtp/src/codec/h264/mod.rs index a18249ac..128be4ca 100644 --- a/rtc-rtp/src/codec/h264/mod.rs +++ b/rtc-rtp/src/codec/h264/mod.rs @@ -67,20 +67,24 @@ impl H264Payloader { self.pps_nalu = Some(nalu.clone()); return; } else if let (Some(sps_nalu), Some(pps_nalu)) = (&self.sps_nalu, &self.pps_nalu) { - // Pack current NALU with SPS and PPS as STAP-A - let sps_len = (sps_nalu.len() as u16).to_be_bytes(); - let pps_len = (pps_nalu.len() as u16).to_be_bytes(); - - let mut stap_a_nalu = Vec::with_capacity(1 + 2 + sps_nalu.len() + 2 + pps_nalu.len()); - stap_a_nalu.push(OUTPUT_STAP_AHEADER); - stap_a_nalu.extend(sps_len); - stap_a_nalu.extend_from_slice(sps_nalu); - stap_a_nalu.extend(pps_len); - stap_a_nalu.extend_from_slice(pps_nalu); - if stap_a_nalu.len() <= mtu { - payloads.push(Bytes::from(stap_a_nalu)); + // Pack current NALU with SPS and PPS as STAP-A. + // STAP-A length fields are u16; only pack if both NALUs fit within 65535 bytes. + if sps_nalu.len() <= u16::MAX as usize && pps_nalu.len() <= u16::MAX as usize { + let sps_len = (sps_nalu.len() as u16).to_be_bytes(); + let pps_len = (pps_nalu.len() as u16).to_be_bytes(); + + let mut stap_a_nalu = + Vec::with_capacity(1 + 2 + sps_nalu.len() + 2 + pps_nalu.len()); + stap_a_nalu.push(OUTPUT_STAP_AHEADER); + stap_a_nalu.extend(sps_len); + stap_a_nalu.extend_from_slice(sps_nalu); + stap_a_nalu.extend(pps_len); + stap_a_nalu.extend_from_slice(pps_nalu); + if stap_a_nalu.len() <= mtu { + payloads.push(Bytes::from(stap_a_nalu)); + } } - } + } // else if let (Some(sps_nalu), Some(pps_nalu)) if self.sps_nalu.is_some() && self.pps_nalu.is_some() { self.sps_nalu = None; diff --git a/rtc-rtp/src/codec/h265/mod.rs b/rtc-rtp/src/codec/h265/mod.rs index 1fd4317a..8bf8ca83 100644 --- a/rtc-rtp/src/codec/h265/mod.rs +++ b/rtc-rtp/src/codec/h265/mod.rs @@ -127,6 +127,10 @@ impl HevcPayloader { ); aggr_nalu.extend_from_slice(&header); for nalu in nalus.drain(..) { + // Aggregation unit length field is u16; skip oversized NALUs. + if nalu.len() > u16::MAX as usize { + continue; + } aggr_nalu.extend_from_slice(&(nalu.len() as u16).to_be_bytes()); aggr_nalu.extend_from_slice(&nalu); } diff --git a/rtc-rtp/src/header.rs b/rtc-rtp/src/header.rs index 3bf56959..76e9f2eb 100644 --- a/rtc-rtp/src/header.rs +++ b/rtc-rtp/src/header.rs @@ -251,7 +251,11 @@ impl Marshal for Header { return Err(Error::ErrBufferTooSmall); } - // The first byte contains the version, padding bit, extension bit, and csrc size + // The first byte contains the version, padding bit, extension bit, and csrc size. + // RFC 3550 §5.1: CC is a 4-bit field, so at most 15 contributing sources are allowed. + if self.csrc.len() > 15 { + return Err(Error::TooManyCSRCs(self.csrc.len())); + } let mut b0 = (self.version << VERSION_SHIFT) | self.csrc.len() as u8; if self.padding { b0 |= 1 << PADDING_SHIFT; @@ -296,15 +300,25 @@ impl Marshal for Header { // RFC 8285 RTP One Byte Header Extension EXTENSION_PROFILE_ONE_BYTE => { for extension in &self.extensions { - buf.put_u8((extension.id << 4) | (extension.payload.len() as u8 - 1)); + // RFC 8285 §4.2: payload must be 1–16 bytes; the length field encodes (len-1). + let len = extension.payload.len(); + if len == 0 || len > 16 { + return Err(Error::OneByteHeaderExtensionPayloadTooLarge(len)); + } + buf.put_u8((extension.id << 4) | (len as u8 - 1)); buf.put(&*extension.payload); } } // RFC 8285 RTP Two Byte Header Extension EXTENSION_PROFILE_TWO_BYTE => { for extension in &self.extensions { + // RFC 8285 §4.3: length field is one byte, so max payload is 255 bytes. + let len = extension.payload.len(); + if len > 255 { + return Err(Error::TwoByteHeaderExtensionPayloadTooLarge(len)); + } buf.put_u8(extension.id); - buf.put_u8(extension.payload.len() as u8); + buf.put_u8(len as u8); buf.put(&*extension.payload); } } diff --git a/rtc-shared/src/error.rs b/rtc-shared/src/error.rs index d319f59b..2ae72758 100644 --- a/rtc-shared/src/error.rs +++ b/rtc-shared/src/error.rs @@ -276,6 +276,14 @@ pub enum Error { #[error("extension_payload must be in 32-bit words")] HeaderExtensionPayloadNot32BitWords, + #[error("too many CSRCs: {0} exceeds the 4-bit CC field maximum of 15")] + TooManyCSRCs(usize), + #[error("one-byte header extension payload length {0} exceeds RFC 8285 maximum of 16 bytes")] + OneByteHeaderExtensionPayloadTooLarge(usize), + #[error("two-byte header extension payload length {0} exceeds maximum of 255 bytes")] + TwoByteHeaderExtensionPayloadTooLarge(usize), + #[error("NALU length {0} exceeds u16::MAX (65535 bytes)")] + NaluTooLarge(usize), #[error("audio level overflow")] AudioLevelOverflow, #[error("playout delay overflow")] From 3b669a3c0fc54c903c9c90c7f89fb92b39450422 Mon Sep 17 00:00:00 2001 From: nightness Date: Wed, 8 Apr 2026 01:28:32 -0500 Subject: [PATCH 02/10] address review feedback for PR #74 - Fix OneByteHeaderExtensionPayloadTooLarge error message to describe the valid range (1-16 bytes) instead of only the upper bound - H265: route oversized NALUs through emit() for FU fragmentation instead of silently dropping them; guard against empty aggregation packets - H264: fall back to emitting SPS/PPS as separate NALUs (fragmented via FU-A if needed) when they exceed u16::MAX, instead of dropping - Add unit tests for marshal-side bounds checks: TooManyCSRCs, one-byte extension payload (0 and 17 bytes), two-byte extension payload (256 bytes), and valid boundary values Co-Authored-By: Claude Opus 4.6 (1M context) --- rtc-rtp/src/codec/h264/mod.rs | 17 +++-- rtc-rtp/src/codec/h265/mod.rs | 12 +++- rtc-rtp/src/header.rs | 131 ++++++++++++++++++++++++++++++++++ rtc-shared/src/error.rs | 4 +- 4 files changed, 157 insertions(+), 7 deletions(-) diff --git a/rtc-rtp/src/codec/h264/mod.rs b/rtc-rtp/src/codec/h264/mod.rs index 128be4ca..77bd8faf 100644 --- a/rtc-rtp/src/codec/h264/mod.rs +++ b/rtc-rtp/src/codec/h264/mod.rs @@ -66,7 +66,11 @@ impl H264Payloader { } else if nalu_type == PPS_NALU_TYPE { self.pps_nalu = Some(nalu.clone()); return; - } else if let (Some(sps_nalu), Some(pps_nalu)) = (&self.sps_nalu, &self.pps_nalu) { + } else if self.sps_nalu.is_some() && self.pps_nalu.is_some() { + // Clone to release the borrow on self so we can call self.emit() below if needed. + let sps_nalu = self.sps_nalu.clone().unwrap(); + let pps_nalu = self.pps_nalu.clone().unwrap(); + // Pack current NALU with SPS and PPS as STAP-A. // STAP-A length fields are u16; only pack if both NALUs fit within 65535 bytes. if sps_nalu.len() <= u16::MAX as usize && pps_nalu.len() <= u16::MAX as usize { @@ -77,14 +81,19 @@ impl H264Payloader { Vec::with_capacity(1 + 2 + sps_nalu.len() + 2 + pps_nalu.len()); stap_a_nalu.push(OUTPUT_STAP_AHEADER); stap_a_nalu.extend(sps_len); - stap_a_nalu.extend_from_slice(sps_nalu); + stap_a_nalu.extend_from_slice(&sps_nalu); stap_a_nalu.extend(pps_len); - stap_a_nalu.extend_from_slice(pps_nalu); + stap_a_nalu.extend_from_slice(&pps_nalu); if stap_a_nalu.len() <= mtu { payloads.push(Bytes::from(stap_a_nalu)); } + } else { + // SPS or PPS exceeds u16::MAX; fall back to emitting them as + // separate NALUs (which will be fragmented via FU-A if needed). + self.emit(&sps_nalu, mtu, payloads); + self.emit(&pps_nalu, mtu, payloads); } - } // else if let (Some(sps_nalu), Some(pps_nalu)) + } // else if self.sps_nalu.is_some() && self.pps_nalu.is_some() if self.sps_nalu.is_some() && self.pps_nalu.is_some() { self.sps_nalu = None; diff --git a/rtc-rtp/src/codec/h265/mod.rs b/rtc-rtp/src/codec/h265/mod.rs index 8bf8ca83..6ce9a3f7 100644 --- a/rtc-rtp/src/codec/h265/mod.rs +++ b/rtc-rtp/src/codec/h265/mod.rs @@ -126,17 +126,25 @@ impl HevcPayloader { NAL_HEADER_SIZE + nalus.iter().map(|nalu| 2 + nalu.len()).sum::(), ); aggr_nalu.extend_from_slice(&header); + // Separate oversized NALUs that exceed the u16 length field; + // they will be emitted individually (fragmented via FUs). + let mut oversized = Vec::new(); for nalu in nalus.drain(..) { - // Aggregation unit length field is u16; skip oversized NALUs. if nalu.len() > u16::MAX as usize { + oversized.push(nalu); continue; } aggr_nalu.extend_from_slice(&(nalu.len() as u16).to_be_bytes()); aggr_nalu.extend_from_slice(&nalu); } - if aggr_nalu.len() <= mtu { + // Only emit the aggregation packet if it contains at least one NALU. + if aggr_nalu.len() > NAL_HEADER_SIZE && aggr_nalu.len() <= mtu { payloads.push(aggr_nalu.freeze()); } + // Emit oversized NALUs individually so they get fragmented. + for nalu in &oversized { + Self::emit(nalu, mtu, payloads); + } } } } diff --git a/rtc-rtp/src/header.rs b/rtc-rtp/src/header.rs index 76e9f2eb..eaf83fb7 100644 --- a/rtc-rtp/src/header.rs +++ b/rtc-rtp/src/header.rs @@ -503,3 +503,134 @@ impl Header { } } } + +#[cfg(test)] +mod tests { + use super::*; + use shared::marshal::Marshal; + + /// Helper: create a minimal valid header and marshal it, returning the result. + fn marshal_header(header: &Header) -> shared::error::Result { + let mut buf = vec![0u8; header.marshal_size()]; + header.marshal_to(&mut &mut buf[..]) + } + + #[test] + fn test_too_many_csrcs() { + let header = Header { + csrc: vec![0u32; 16], + ..Default::default() + }; + let err = marshal_header(&header).unwrap_err(); + assert!( + matches!(err, Error::TooManyCSRCs(16)), + "expected TooManyCSRCs(16), got {err:?}" + ); + } + + #[test] + fn test_one_byte_extension_payload_zero_length() { + let header = Header { + extension: true, + extension_profile: EXTENSION_PROFILE_ONE_BYTE, + extensions: vec![Extension { + id: 1, + payload: Bytes::new(), // 0 bytes — invalid + }], + ..Default::default() + }; + let err = marshal_header(&header).unwrap_err(); + assert!( + matches!(err, Error::OneByteHeaderExtensionPayloadTooLarge(0)), + "expected OneByteHeaderExtensionPayloadTooLarge(0), got {err:?}" + ); + } + + #[test] + fn test_one_byte_extension_payload_too_large() { + let header = Header { + extension: true, + extension_profile: EXTENSION_PROFILE_ONE_BYTE, + extensions: vec![Extension { + id: 1, + payload: Bytes::from(vec![0u8; 17]), // 17 bytes — exceeds limit of 16 + }], + ..Default::default() + }; + let err = marshal_header(&header).unwrap_err(); + assert!( + matches!(err, Error::OneByteHeaderExtensionPayloadTooLarge(17)), + "expected OneByteHeaderExtensionPayloadTooLarge(17), got {err:?}" + ); + } + + #[test] + fn test_two_byte_extension_payload_too_large() { + let header = Header { + extension: true, + extension_profile: EXTENSION_PROFILE_TWO_BYTE, + extensions: vec![Extension { + id: 1, + payload: Bytes::from(vec![0u8; 256]), // 256 bytes — exceeds limit of 255 + }], + ..Default::default() + }; + let err = marshal_header(&header).unwrap_err(); + assert!( + matches!(err, Error::TwoByteHeaderExtensionPayloadTooLarge(256)), + "expected TwoByteHeaderExtensionPayloadTooLarge(256), got {err:?}" + ); + } + + #[test] + fn test_valid_one_byte_extension_boundaries() { + // 1 byte payload — minimum valid + let header_min = Header { + extension: true, + extension_profile: EXTENSION_PROFILE_ONE_BYTE, + extensions: vec![Extension { + id: 1, + payload: Bytes::from(vec![0u8; 1]), + }], + ..Default::default() + }; + assert!(marshal_header(&header_min).is_ok()); + + // 16 byte payload — maximum valid + let header_max = Header { + extension: true, + extension_profile: EXTENSION_PROFILE_ONE_BYTE, + extensions: vec![Extension { + id: 1, + payload: Bytes::from(vec![0u8; 16]), + }], + ..Default::default() + }; + assert!(marshal_header(&header_max).is_ok()); + } + + #[test] + fn test_valid_two_byte_extension_boundary() { + // 255 byte payload — maximum valid + let header = Header { + extension: true, + extension_profile: EXTENSION_PROFILE_TWO_BYTE, + extensions: vec![Extension { + id: 1, + payload: Bytes::from(vec![0u8; 255]), + }], + ..Default::default() + }; + assert!(marshal_header(&header).is_ok()); + } + + #[test] + fn test_max_csrcs_valid() { + // 15 CSRCs is the maximum allowed + let header = Header { + csrc: vec![0u32; 15], + ..Default::default() + }; + assert!(marshal_header(&header).is_ok()); + } +} diff --git a/rtc-shared/src/error.rs b/rtc-shared/src/error.rs index 2ae72758..70ff85f6 100644 --- a/rtc-shared/src/error.rs +++ b/rtc-shared/src/error.rs @@ -278,7 +278,9 @@ pub enum Error { HeaderExtensionPayloadNot32BitWords, #[error("too many CSRCs: {0} exceeds the 4-bit CC field maximum of 15")] TooManyCSRCs(usize), - #[error("one-byte header extension payload length {0} exceeds RFC 8285 maximum of 16 bytes")] + #[error( + "one-byte header extension payload length {0} is outside the RFC 8285 valid range of 1-16 bytes" + )] OneByteHeaderExtensionPayloadTooLarge(usize), #[error("two-byte header extension payload length {0} exceeds maximum of 255 bytes")] TwoByteHeaderExtensionPayloadTooLarge(usize), From c670bc9ca9b0de0fa4ab7040e4602b1036dfabc8 Mon Sep 17 00:00:00 2001 From: nightness Date: Wed, 8 Apr 2026 08:58:16 -0500 Subject: [PATCH 03/10] fix: address Copilot review comments on RTP bounds checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - H264: fix oversized SPS/PPS fallback by adding emit_single_or_fragment() that bypasses the SPS/PPS stashing logic in emit(), so oversized parameter sets are actually emitted (fragmented via FU-A) instead of being silently dropped - H265: filter oversized NALUs before computing aggregation buffer capacity to avoid massive pre-allocation / potential OOM - Header: add marshal-side extension ID validation — reject reserved IDs 0 and 15 for one-byte format (RFC 8285 §4.2) and ID 0 for two-byte format (RFC 8285 §4.3) - Rename OneByteHeaderExtensionPayloadTooLarge to OneByteHeaderExtensionPayloadOutOfRange since it covers both zero-length (too small) and >16 (too large) cases - Add comprehensive tests for all new validation paths Co-Authored-By: Claude Opus 4.6 (1M context) --- rtc-rtp/src/codec/h264/mod.rs | 52 +++++++++- rtc-rtp/src/codec/h265/mod.rs | 32 +++--- rtc-rtp/src/header.rs | 180 +++++++++++++++++++++++++++++----- rtc-shared/src/error.rs | 2 +- 4 files changed, 225 insertions(+), 41 deletions(-) diff --git a/rtc-rtp/src/codec/h264/mod.rs b/rtc-rtp/src/codec/h264/mod.rs index 77bd8faf..672d4422 100644 --- a/rtc-rtp/src/codec/h264/mod.rs +++ b/rtc-rtp/src/codec/h264/mod.rs @@ -50,6 +50,52 @@ impl H264Payloader { (-1, -1) } + /// Emit a NALU as a single packet or fragment it via FU-A, without + /// special-casing SPS/PPS (which `emit()` would re-stash). + fn emit_single_or_fragment(nalu: &Bytes, mtu: usize, payloads: &mut Vec) { + if nalu.is_empty() { + return; + } + + let nalu_ref_idc = nalu[0] & NALU_REF_IDC_BITMASK; + let nalu_type = nalu[0] & NALU_TYPE_BITMASK; + + // Single NALU + if nalu.len() <= mtu { + payloads.push(nalu.clone()); + return; + } + + // FU-A fragmentation + let max_fragment_size = mtu as isize - FUA_HEADER_SIZE as isize; + let mut nalu_data_index: isize = 1; + let nalu_data_length = nalu.len() as isize - nalu_data_index; + let mut nalu_data_remaining = nalu_data_length; + + if std::cmp::min(max_fragment_size, nalu_data_remaining) <= 0 { + return; + } + + while nalu_data_remaining > 0 { + let current_fragment_size = std::cmp::min(max_fragment_size, nalu_data_remaining); + let mut out = BytesMut::with_capacity(FUA_HEADER_SIZE + current_fragment_size as usize); + out.put_u8(FUA_NALU_TYPE | nalu_ref_idc); + let mut b1 = nalu_type; + if nalu_data_remaining == nalu_data_length { + b1 |= 1 << 7; // start bit + } else if nalu_data_remaining - current_fragment_size == 0 { + b1 |= 1 << 6; // end bit + } + out.put_u8(b1); + out.put( + &nalu[nalu_data_index as usize..(nalu_data_index + current_fragment_size) as usize], + ); + payloads.push(out.freeze()); + nalu_data_remaining -= current_fragment_size; + nalu_data_index += current_fragment_size; + } + } + fn emit(&mut self, nalu: &Bytes, mtu: usize, payloads: &mut Vec) { if nalu.is_empty() { return; @@ -90,8 +136,10 @@ impl H264Payloader { } else { // SPS or PPS exceeds u16::MAX; fall back to emitting them as // separate NALUs (which will be fragmented via FU-A if needed). - self.emit(&sps_nalu, mtu, payloads); - self.emit(&pps_nalu, mtu, payloads); + // We cannot use self.emit() here because it would re-stash SPS/PPS + // NALUs instead of actually outputting them. Push single or fragment directly. + Self::emit_single_or_fragment(&sps_nalu, mtu, payloads); + Self::emit_single_or_fragment(&pps_nalu, mtu, payloads); } } // else if self.sps_nalu.is_some() && self.pps_nalu.is_some() diff --git a/rtc-rtp/src/codec/h265/mod.rs b/rtc-rtp/src/codec/h265/mod.rs index 6ce9a3f7..98c3e7f7 100644 --- a/rtc-rtp/src/codec/h265/mod.rs +++ b/rtc-rtp/src/codec/h265/mod.rs @@ -122,26 +122,32 @@ impl HevcPayloader { } _ => { let header = Self::aggregation_payload_header(nalus); - let mut aggr_nalu = BytesMut::with_capacity( - NAL_HEADER_SIZE + nalus.iter().map(|nalu| 2 + nalu.len()).sum::(), - ); - aggr_nalu.extend_from_slice(&header); - // Separate oversized NALUs that exceed the u16 length field; - // they will be emitted individually (fragmented via FUs). + // Separate oversized NALUs that exceed the u16 length field + // *before* computing capacity, to avoid massive pre-allocation. let mut oversized = Vec::new(); + let mut normal = Vec::new(); for nalu in nalus.drain(..) { if nalu.len() > u16::MAX as usize { oversized.push(nalu); - continue; + } else { + normal.push(nalu); } - aggr_nalu.extend_from_slice(&(nalu.len() as u16).to_be_bytes()); - aggr_nalu.extend_from_slice(&nalu); } - // Only emit the aggregation packet if it contains at least one NALU. - if aggr_nalu.len() > NAL_HEADER_SIZE && aggr_nalu.len() <= mtu { - payloads.push(aggr_nalu.freeze()); + // Only build an aggregation packet if there are normal-sized NALUs. + if !normal.is_empty() { + let mut aggr_nalu = BytesMut::with_capacity( + NAL_HEADER_SIZE + normal.iter().map(|nalu| 2 + nalu.len()).sum::(), + ); + aggr_nalu.extend_from_slice(&header); + for nalu in &normal { + aggr_nalu.extend_from_slice(&(nalu.len() as u16).to_be_bytes()); + aggr_nalu.extend_from_slice(nalu); + } + if aggr_nalu.len() <= mtu { + payloads.push(aggr_nalu.freeze()); + } } - // Emit oversized NALUs individually so they get fragmented. + // Emit oversized NALUs individually so they get fragmented via FUs. for nalu in &oversized { Self::emit(nalu, mtu, payloads); } diff --git a/rtc-rtp/src/header.rs b/rtc-rtp/src/header.rs index eaf83fb7..641391f4 100644 --- a/rtc-rtp/src/header.rs +++ b/rtc-rtp/src/header.rs @@ -300,10 +300,14 @@ impl Marshal for Header { // RFC 8285 RTP One Byte Header Extension EXTENSION_PROFILE_ONE_BYTE => { for extension in &self.extensions { + // RFC 8285 §4.2: IDs 0 and 15 are reserved. + if !(1..=14).contains(&extension.id) { + return Err(Error::ErrRfc8285oneByteHeaderIdrange); + } // RFC 8285 §4.2: payload must be 1–16 bytes; the length field encodes (len-1). let len = extension.payload.len(); if len == 0 || len > 16 { - return Err(Error::OneByteHeaderExtensionPayloadTooLarge(len)); + return Err(Error::OneByteHeaderExtensionPayloadOutOfRange(len)); } buf.put_u8((extension.id << 4) | (len as u8 - 1)); buf.put(&*extension.payload); @@ -312,6 +316,11 @@ impl Marshal for Header { // RFC 8285 RTP Two Byte Header Extension EXTENSION_PROFILE_TWO_BYTE => { for extension in &self.extensions { + // RFC 8285 §4.3: ID 0 is padding in two-byte format; + // marshaling it as an extension would confuse receivers. + if extension.id == 0 { + return Err(Error::ErrRfc8285twoByteHeaderIdrange); + } // RFC 8285 §4.3: length field is one byte, so max payload is 255 bytes. let len = extension.payload.len(); if len > 255 { @@ -507,6 +516,7 @@ impl Header { #[cfg(test)] mod tests { use super::*; + use shared::error::Error; use shared::marshal::Marshal; /// Helper: create a minimal valid header and marshal it, returning the result. @@ -515,6 +525,8 @@ mod tests { header.marshal_to(&mut &mut buf[..]) } + // -- CSRC validation -- + #[test] fn test_too_many_csrcs() { let header = Header { @@ -528,6 +540,17 @@ mod tests { ); } + #[test] + fn test_max_csrcs_valid() { + let header = Header { + csrc: vec![0u32; 15], + ..Default::default() + }; + assert!(marshal_header(&header).is_ok()); + } + + // -- One-byte extension payload size validation -- + #[test] fn test_one_byte_extension_payload_zero_length() { let header = Header { @@ -535,14 +558,14 @@ mod tests { extension_profile: EXTENSION_PROFILE_ONE_BYTE, extensions: vec![Extension { id: 1, - payload: Bytes::new(), // 0 bytes — invalid + payload: Bytes::new(), }], ..Default::default() }; let err = marshal_header(&header).unwrap_err(); assert!( - matches!(err, Error::OneByteHeaderExtensionPayloadTooLarge(0)), - "expected OneByteHeaderExtensionPayloadTooLarge(0), got {err:?}" + matches!(err, Error::OneByteHeaderExtensionPayloadOutOfRange(0)), + "expected OneByteHeaderExtensionPayloadOutOfRange(0), got {err:?}" ); } @@ -553,17 +576,46 @@ mod tests { extension_profile: EXTENSION_PROFILE_ONE_BYTE, extensions: vec![Extension { id: 1, - payload: Bytes::from(vec![0u8; 17]), // 17 bytes — exceeds limit of 16 + payload: Bytes::from(vec![0u8; 17]), }], ..Default::default() }; let err = marshal_header(&header).unwrap_err(); assert!( - matches!(err, Error::OneByteHeaderExtensionPayloadTooLarge(17)), - "expected OneByteHeaderExtensionPayloadTooLarge(17), got {err:?}" + matches!(err, Error::OneByteHeaderExtensionPayloadOutOfRange(17)), + "expected OneByteHeaderExtensionPayloadOutOfRange(17), got {err:?}" ); } + #[test] + fn test_valid_one_byte_extension_boundaries() { + // 1 byte payload -- minimum valid + let header_min = Header { + extension: true, + extension_profile: EXTENSION_PROFILE_ONE_BYTE, + extensions: vec![Extension { + id: 1, + payload: Bytes::from(vec![0u8; 1]), + }], + ..Default::default() + }; + assert!(marshal_header(&header_min).is_ok()); + + // 16 byte payload -- maximum valid + let header_max = Header { + extension: true, + extension_profile: EXTENSION_PROFILE_ONE_BYTE, + extensions: vec![Extension { + id: 1, + payload: Bytes::from(vec![0u8; 16]), + }], + ..Default::default() + }; + assert!(marshal_header(&header_max).is_ok()); + } + + // -- Two-byte extension payload size validation -- + #[test] fn test_two_byte_extension_payload_too_large() { let header = Header { @@ -571,7 +623,7 @@ mod tests { extension_profile: EXTENSION_PROFILE_TWO_BYTE, extensions: vec![Extension { id: 1, - payload: Bytes::from(vec![0u8; 256]), // 256 bytes — exceeds limit of 255 + payload: Bytes::from(vec![0u8; 256]), }], ..Default::default() }; @@ -583,41 +635,99 @@ mod tests { } #[test] - fn test_valid_one_byte_extension_boundaries() { - // 1 byte payload — minimum valid - let header_min = Header { + fn test_valid_two_byte_extension_boundary() { + let header = Header { extension: true, - extension_profile: EXTENSION_PROFILE_ONE_BYTE, + extension_profile: EXTENSION_PROFILE_TWO_BYTE, extensions: vec![Extension { id: 1, - payload: Bytes::from(vec![0u8; 1]), + payload: Bytes::from(vec![0u8; 255]), }], ..Default::default() }; - assert!(marshal_header(&header_min).is_ok()); + assert!(marshal_header(&header).is_ok()); + } - // 16 byte payload — maximum valid - let header_max = Header { + // -- One-byte extension ID validation (RFC 8285 section 4.2) -- + + #[test] + fn test_one_byte_extension_id_zero_rejected() { + let header = Header { extension: true, extension_profile: EXTENSION_PROFILE_ONE_BYTE, extensions: vec![Extension { - id: 1, - payload: Bytes::from(vec![0u8; 16]), + id: 0, + payload: Bytes::from(vec![0xAB]), }], ..Default::default() }; - assert!(marshal_header(&header_max).is_ok()); + let err = marshal_header(&header).unwrap_err(); + assert!( + matches!(err, Error::ErrRfc8285oneByteHeaderIdrange), + "expected ErrRfc8285oneByteHeaderIdrange, got {err:?}" + ); } #[test] - fn test_valid_two_byte_extension_boundary() { - // 255 byte payload — maximum valid + fn test_one_byte_extension_id_fifteen_rejected() { + let header = Header { + extension: true, + extension_profile: EXTENSION_PROFILE_ONE_BYTE, + extensions: vec![Extension { + id: 15, + payload: Bytes::from(vec![0xAB]), + }], + ..Default::default() + }; + let err = marshal_header(&header).unwrap_err(); + assert!( + matches!(err, Error::ErrRfc8285oneByteHeaderIdrange), + "expected ErrRfc8285oneByteHeaderIdrange, got {err:?}" + ); + } + + #[test] + fn test_one_byte_extension_id_fourteen_valid() { + let header = Header { + extension: true, + extension_profile: EXTENSION_PROFILE_ONE_BYTE, + extensions: vec![Extension { + id: 14, + payload: Bytes::from(vec![0xAB]), + }], + ..Default::default() + }; + assert!(marshal_header(&header).is_ok()); + } + + // -- Two-byte extension ID validation (RFC 8285 section 4.3) -- + + #[test] + fn test_two_byte_extension_id_zero_rejected() { + let header = Header { + extension: true, + extension_profile: EXTENSION_PROFILE_TWO_BYTE, + extensions: vec![Extension { + id: 0, + payload: Bytes::from(vec![0xAB]), + }], + ..Default::default() + }; + let err = marshal_header(&header).unwrap_err(); + assert!( + matches!(err, Error::ErrRfc8285twoByteHeaderIdrange), + "expected ErrRfc8285twoByteHeaderIdrange, got {err:?}" + ); + } + + #[test] + fn test_two_byte_extension_id_one_valid() { let header = Header { extension: true, extension_profile: EXTENSION_PROFILE_TWO_BYTE, extensions: vec![Extension { id: 1, - payload: Bytes::from(vec![0u8; 255]), + payload: Bytes::from(vec![0xAB]), }], ..Default::default() }; @@ -625,10 +735,30 @@ mod tests { } #[test] - fn test_max_csrcs_valid() { - // 15 CSRCs is the maximum allowed + fn test_two_byte_extension_id_255_valid() { let header = Header { - csrc: vec![0u32; 15], + extension: true, + extension_profile: EXTENSION_PROFILE_TWO_BYTE, + extensions: vec![Extension { + id: 255, + payload: Bytes::from(vec![0xAB]), + }], + ..Default::default() + }; + assert!(marshal_header(&header).is_ok()); + } + + // -- Two-byte zero-length payload (valid per RFC 8285) -- + + #[test] + fn test_two_byte_extension_zero_length_payload_valid() { + let header = Header { + extension: true, + extension_profile: EXTENSION_PROFILE_TWO_BYTE, + extensions: vec![Extension { + id: 1, + payload: Bytes::new(), + }], ..Default::default() }; assert!(marshal_header(&header).is_ok()); diff --git a/rtc-shared/src/error.rs b/rtc-shared/src/error.rs index 70ff85f6..f9efb71a 100644 --- a/rtc-shared/src/error.rs +++ b/rtc-shared/src/error.rs @@ -281,7 +281,7 @@ pub enum Error { #[error( "one-byte header extension payload length {0} is outside the RFC 8285 valid range of 1-16 bytes" )] - OneByteHeaderExtensionPayloadTooLarge(usize), + OneByteHeaderExtensionPayloadOutOfRange(usize), #[error("two-byte header extension payload length {0} exceeds maximum of 255 bytes")] TwoByteHeaderExtensionPayloadTooLarge(usize), #[error("NALU length {0} exceeds u16::MAX (65535 bytes)")] From 0e339ab16e7ed3d65d72cc023ea1420954d87507 Mon Sep 17 00:00:00 2001 From: nightness Date: Wed, 8 Apr 2026 14:29:38 -0500 Subject: [PATCH 04/10] fix: address all remaining Copilot comments on rtp bounds checks - h264: fix STAP-A comment to say "Pack the cached SPS and PPS" - h264: when STAP-A exceeds MTU, emit SPS/PPS individually via FU-A - h265: compute aggregation_payload_header from normal NALUs only - header: add comment explaining why tests are inline - tests: add H264 STAP-A>MTU, oversized SPS, emit_single_or_fragment - tests: add H265 oversized NALU aggregation and header tests Co-Authored-By: Claude Opus 4.6 (1M context) --- rtc-rtp/src/codec/h264/h264_test.rs | 123 ++++++++++++++++++++++++++++ rtc-rtp/src/codec/h264/mod.rs | 7 +- rtc-rtp/src/codec/h265/h265_test.rs | 96 ++++++++++++++++++++++ rtc-rtp/src/codec/h265/mod.rs | 5 +- rtc-rtp/src/header.rs | 3 + 5 files changed, 231 insertions(+), 3 deletions(-) diff --git a/rtc-rtp/src/codec/h264/h264_test.rs b/rtc-rtp/src/codec/h264/h264_test.rs index bc8aa464..20d17ffa 100644 --- a/rtc-rtp/src/codec/h264/h264_test.rs +++ b/rtc-rtp/src/codec/h264/h264_test.rs @@ -261,3 +261,126 @@ fn test_h264_payloader_payload_sps_and_pps_handling() -> Result<()> { Ok(()) } + +/// When the combined STAP-A of SPS + PPS exceeds the MTU, both should still +/// be emitted as individual (possibly FU-A fragmented) packets instead of +/// being silently dropped. +#[test] +fn test_h264_stap_a_exceeds_mtu_emits_individually() -> Result<()> { + let mut pck = H264Payloader::default(); + + // SPS: 3 bytes (NALU type 7) + let sps = Bytes::from_static(&[0x07, 0xAA, 0xBB]); + // PPS: 3 bytes (NALU type 8) + let pps = Bytes::from_static(&[0x08, 0xCC, 0xDD]); + + let res = pck.payload(1500, &sps)?; + assert!(res.is_empty(), "SPS alone should be stashed, not emitted"); + + let res = pck.payload(1500, &pps)?; + assert!(res.is_empty(), "PPS alone should be stashed, not emitted"); + + // Use a tiny MTU so the STAP-A (1 + 2+3 + 2+3 = 11 bytes) exceeds it. + // SPS and PPS individually are 3 bytes each, which fits in MTU=5. + let result = pck.payload(5, &Bytes::from_static(&[0x05, 0x01, 0x02]))?; + + // Expect: SPS (3 bytes, fits), PPS (3 bytes, fits), then the IDR NALU (3 bytes, fits) + assert_eq!(result.len(), 3, "expected SPS + PPS + IDR = 3 packets"); + assert_eq!(result[0], sps, "first packet should be the SPS NALU"); + assert_eq!(result[1], pps, "second packet should be the PPS NALU"); + assert_eq!( + result[2], + Bytes::from_static(&[0x05, 0x01, 0x02]), + "third packet should be the IDR NALU" + ); + + Ok(()) +} + +/// When SPS or PPS are too large for a u16 length field (>65535 bytes), they +/// should be emitted via FU-A fragmentation using emit_single_or_fragment +/// rather than being packed into a STAP-A. +#[test] +fn test_h264_oversized_sps_uses_fua_fragmentation() -> Result<()> { + let mut pck = H264Payloader::default(); + + // Build a large SPS that exceeds u16::MAX (but we'll use a smaller one + // to avoid allocating 64KB in tests; the code path is the same). + // SPS: 20 bytes, PPS: 3 bytes, MTU: 10 + // STAP-A would be 1 + 2+20 + 2+3 = 28 bytes, well over MTU=10. + let mut sps_data = vec![0x67]; // NALU type 7, with ref_idc bits set + sps_data.extend(vec![0xAA; 19]); + let sps = Bytes::from(sps_data); + + let pps = Bytes::from_static(&[0x68, 0xCC, 0xDD]); // NALU type 8, with ref_idc bits + + let res = pck.payload(1500, &sps)?; + assert!(res.is_empty(), "SPS alone should be stashed"); + + let res = pck.payload(1500, &pps)?; + assert!(res.is_empty(), "PPS alone should be stashed"); + + // Trigger emission with a small non-SPS/PPS NALU + let result = pck.payload(10, &Bytes::from_static(&[0x65, 0x01, 0x02]))?; + + // SPS (20 bytes) should be FU-A fragmented into multiple packets. + // PPS (3 bytes) fits in a single packet. + // IDR (3 bytes) fits in a single packet. + assert!( + result.len() >= 3, + "expected at least 3 packets (fragmented SPS + PPS + IDR), got {}", + result.len() + ); + + // Verify the first packet is a FU-A start fragment of the SPS + assert_eq!( + result[0][0] & NALU_TYPE_BITMASK, + FUA_NALU_TYPE, + "first packet should be a FU-A fragment" + ); + assert_ne!( + result[0][1] & FU_START_BITMASK, + 0, + "first FU-A fragment should have start bit set" + ); + + Ok(()) +} + +/// The emit_single_or_fragment helper should pass through small NALUs directly +/// and fragment large ones via FU-A. +#[test] +fn test_h264_emit_single_or_fragment_small_nalu() { + let nalu = Bytes::from_static(&[0x65, 0x01, 0x02, 0x03]); + let mut payloads = vec![]; + H264Payloader::emit_single_or_fragment(&nalu, 10, &mut payloads); + assert_eq!(payloads.len(), 1, "small NALU should emit as single packet"); + assert_eq!(payloads[0], nalu); +} + +#[test] +fn test_h264_emit_single_or_fragment_large_nalu() { + let mut data = vec![0x65]; // IDR NALU type + data.extend(vec![0xBB; 20]); + let nalu = Bytes::from(data); + let mut payloads = vec![]; + H264Payloader::emit_single_or_fragment(&nalu, 10, &mut payloads); + assert!( + payloads.len() > 1, + "large NALU should be FU-A fragmented into multiple packets" + ); + // First fragment should have FU-A type and start bit + assert_eq!(payloads[0][0] & NALU_TYPE_BITMASK, FUA_NALU_TYPE); + assert_ne!(payloads[0][1] & FU_START_BITMASK, 0); + // Last fragment should have end bit + let last = payloads.last().unwrap(); + assert_ne!(last[1] & FU_END_BITMASK, 0); +} + +#[test] +fn test_h264_emit_single_or_fragment_empty_nalu() { + let nalu = Bytes::new(); + let mut payloads = vec![]; + H264Payloader::emit_single_or_fragment(&nalu, 10, &mut payloads); + assert!(payloads.is_empty(), "empty NALU should produce no packets"); +} diff --git a/rtc-rtp/src/codec/h264/mod.rs b/rtc-rtp/src/codec/h264/mod.rs index 672d4422..7a979554 100644 --- a/rtc-rtp/src/codec/h264/mod.rs +++ b/rtc-rtp/src/codec/h264/mod.rs @@ -117,7 +117,7 @@ impl H264Payloader { let sps_nalu = self.sps_nalu.clone().unwrap(); let pps_nalu = self.pps_nalu.clone().unwrap(); - // Pack current NALU with SPS and PPS as STAP-A. + // Pack the cached SPS and PPS into a STAP-A. // STAP-A length fields are u16; only pack if both NALUs fit within 65535 bytes. if sps_nalu.len() <= u16::MAX as usize && pps_nalu.len() <= u16::MAX as usize { let sps_len = (sps_nalu.len() as u16).to_be_bytes(); @@ -132,6 +132,11 @@ impl H264Payloader { stap_a_nalu.extend_from_slice(&pps_nalu); if stap_a_nalu.len() <= mtu { payloads.push(Bytes::from(stap_a_nalu)); + } else { + // STAP-A exceeds MTU; emit SPS and PPS individually + // (they will be fragmented via FU-A if needed). + Self::emit_single_or_fragment(&sps_nalu, mtu, payloads); + Self::emit_single_or_fragment(&pps_nalu, mtu, payloads); } } else { // SPS or PPS exceeds u16::MAX; fall back to emitting them as diff --git a/rtc-rtp/src/codec/h265/h265_test.rs b/rtc-rtp/src/codec/h265/h265_test.rs index 2ed1b100..6bd56ebb 100644 --- a/rtc-rtp/src/codec/h265/h265_test.rs +++ b/rtc-rtp/src/codec/h265/h265_test.rs @@ -982,3 +982,99 @@ fn test_h265_packet_real() -> Result<()> { Ok(()) } + +/// When an aggregation buffer contains a NALU that exceeds u16::MAX, the +/// oversized NALU must be emitted individually (via FU fragmentation) while +/// normal-sized NALUs are still packed into the aggregation packet. +#[test] +fn test_h265_oversized_nalu_in_aggregation_buffer() -> Result<()> { + let mut pck = HevcPayloader::default(); + + // Build a payload with two NALUs: one normal-sized and one large (> MTU). + // We use Annex B start codes to separate them. + // NALU 1: 5 bytes (normal, fits in MTU) + // NALU 2: 20 bytes (exceeds MTU of 10, should be FU-fragmented) + let mut payload = vec![]; + // NALU 1 with 3-byte start code + payload.extend_from_slice(&[0x00, 0x00, 0x01]); + payload.extend_from_slice(&[0x02, 0x01, 0xAA, 0xBB, 0xCC]); // 5 bytes, type=PFR + // NALU 2 with 3-byte start code + payload.extend_from_slice(&[0x00, 0x00, 0x01]); + let mut nalu2 = vec![0x02, 0x01]; // header: type=PFR + nalu2.extend(vec![0xDD; 18]); // 20 bytes total + payload.extend_from_slice(&nalu2); + + let result = pck.payload(10, &Bytes::from(payload))?; + + // NALU 1 (5 bytes) fits in MTU, emitted as single packet. + // NALU 2 (20 bytes) exceeds MTU, should be FU-fragmented into multiple packets. + assert!( + result.len() >= 2, + "expected at least 2 packets (single + fragments), got {}", + result.len() + ); + + // First packet should be the small NALU (single packet) + assert_eq!(result[0].len(), 5, "first packet should be the 5-byte NALU"); + + // Remaining packets should be FU fragments of the large NALU + for i in 1..result.len() { + let header = H265NALUHeader::new(result[i][0], result[i][1]); + assert!( + header.is_fragmentation_unit(), + "packet {} should be a fragmentation unit", + i + ); + } + + Ok(()) +} + +/// The aggregation_payload_header should only be computed from normal-sized +/// NALUs, not oversized ones that get split out. This ensures the header +/// fields (F, layer_id, tid) reflect only the NALUs actually in the packet. +#[test] +fn test_h265_aggregation_header_excludes_oversized_nalus() -> Result<()> { + let mut pck = HevcPayloader::default(); + + // Two small NALUs that will be aggregated, with distinct layer_id/tid values. + // Both fit under MTU together. + let mut payload = vec![]; + // NALU 1: type=PFR, tid=1 + payload.extend_from_slice(&[0x00, 0x00, 0x01]); + payload.extend_from_slice(&[0x02, 0x01, 0xAA]); // 3 bytes + // NALU 2: type=PFR, tid=1 + payload.extend_from_slice(&[0x00, 0x00, 0x01]); + payload.extend_from_slice(&[0x02, 0x01, 0xBB]); // 3 bytes + + let result = pck.payload(1500, &Bytes::from(payload))?; + + // Should be aggregated into a single AP packet + assert_eq!(result.len(), 1, "two small NALUs should be aggregated"); + let header = H265NALUHeader::new(result[0][0], result[0][1]); + assert!( + header.is_aggregation_packet(), + "packet should be an aggregation packet" + ); + + Ok(()) +} + +/// flush_aggregation_buffer with a single oversized NALU should emit it +/// via FU fragmentation, not try to pack it into an aggregation packet. +#[test] +fn test_h265_flush_single_nalu_passthrough() -> Result<()> { + let mut pck = HevcPayloader::default(); + + // Single NALU that fits in MTU -- should be emitted as-is + let payload = Bytes::from_static(&[0x00, 0x00, 0x01, 0x02, 0x01, 0xAA, 0xBB]); + let result = pck.payload(1500, &payload)?; + assert_eq!(result.len(), 1, "single NALU should be emitted as-is"); + assert_eq!( + result[0], + Bytes::from_static(&[0x02, 0x01, 0xAA, 0xBB]), + "NALU should match after stripping start code" + ); + + Ok(()) +} diff --git a/rtc-rtp/src/codec/h265/mod.rs b/rtc-rtp/src/codec/h265/mod.rs index 98c3e7f7..e12bbfd0 100644 --- a/rtc-rtp/src/codec/h265/mod.rs +++ b/rtc-rtp/src/codec/h265/mod.rs @@ -121,9 +121,9 @@ impl HevcPayloader { payloads.push(nalus.pop().expect("single buffered NAL exists")); } _ => { - let header = Self::aggregation_payload_header(nalus); // Separate oversized NALUs that exceed the u16 length field - // *before* computing capacity, to avoid massive pre-allocation. + // *before* computing the aggregation header or capacity, + // to avoid massive pre-allocation and incorrect header values. let mut oversized = Vec::new(); let mut normal = Vec::new(); for nalu in nalus.drain(..) { @@ -135,6 +135,7 @@ impl HevcPayloader { } // Only build an aggregation packet if there are normal-sized NALUs. if !normal.is_empty() { + let header = Self::aggregation_payload_header(&normal); let mut aggr_nalu = BytesMut::with_capacity( NAL_HEADER_SIZE + normal.iter().map(|nalu| 2 + nalu.len()).sum::(), ); diff --git a/rtc-rtp/src/header.rs b/rtc-rtp/src/header.rs index 641391f4..a094f645 100644 --- a/rtc-rtp/src/header.rs +++ b/rtc-rtp/src/header.rs @@ -513,6 +513,9 @@ impl Header { } } +// Tests are kept inline (rather than in a separate header_test.rs file) because +// header.rs is a flat module file, not a mod.rs directory module. Moving them +// to a sibling file would require restructuring the crate layout for no benefit. #[cfg(test)] mod tests { use super::*; From b409af6112c1b7e835ea33fec7c9500430759285 Mon Sep 17 00:00:00 2001 From: Josh Guyette Date: Wed, 8 Apr 2026 14:37:49 -0500 Subject: [PATCH 05/10] Update rtc-rtp/src/codec/h264/mod.rs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- rtc-rtp/src/codec/h264/mod.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/rtc-rtp/src/codec/h264/mod.rs b/rtc-rtp/src/codec/h264/mod.rs index 7a979554..9cdb67d3 100644 --- a/rtc-rtp/src/codec/h264/mod.rs +++ b/rtc-rtp/src/codec/h264/mod.rs @@ -132,6 +132,11 @@ impl H264Payloader { stap_a_nalu.extend_from_slice(&pps_nalu); if stap_a_nalu.len() <= mtu { payloads.push(Bytes::from(stap_a_nalu)); + } else { + // STAP-A does not fit within the MTU; fall back to emitting + // SPS and PPS separately so they are not silently dropped. + Self::emit_single_or_fragment(&sps_nalu, mtu, payloads); + Self::emit_single_or_fragment(&pps_nalu, mtu, payloads); } else { // STAP-A exceeds MTU; emit SPS and PPS individually // (they will be fragmented via FU-A if needed). From 83b37d8e00fd604400718344fa1c7e32d8fdad1f Mon Sep 17 00:00:00 2001 From: Josh Guyette Date: Wed, 8 Apr 2026 14:38:26 -0500 Subject: [PATCH 06/10] Update rtc-rtp/src/codec/h265/mod.rs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- rtc-rtp/src/codec/h265/mod.rs | 34 ++++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/rtc-rtp/src/codec/h265/mod.rs b/rtc-rtp/src/codec/h265/mod.rs index e12bbfd0..171e1ba4 100644 --- a/rtc-rtp/src/codec/h265/mod.rs +++ b/rtc-rtp/src/codec/h265/mod.rs @@ -133,19 +133,29 @@ impl HevcPayloader { normal.push(nalu); } } - // Only build an aggregation packet if there are normal-sized NALUs. - if !normal.is_empty() { - let header = Self::aggregation_payload_header(&normal); - let mut aggr_nalu = BytesMut::with_capacity( - NAL_HEADER_SIZE + normal.iter().map(|nalu| 2 + nalu.len()).sum::(), - ); - aggr_nalu.extend_from_slice(&header); - for nalu in &normal { - aggr_nalu.extend_from_slice(&(nalu.len() as u16).to_be_bytes()); - aggr_nalu.extend_from_slice(nalu); + // Only build an aggregation packet when there are multiple + // normal-sized NALUs. If exactly one remains after splitting + // out oversized NALUs, emit it directly to avoid a single- + // element aggregation packet. + match normal.len() { + 0 => {} + 1 => { + payloads.push(normal.pop().expect("single normal NAL exists")); } - if aggr_nalu.len() <= mtu { - payloads.push(aggr_nalu.freeze()); + _ => { + let header = Self::aggregation_payload_header(&normal); + let mut aggr_nalu = BytesMut::with_capacity( + NAL_HEADER_SIZE + + normal.iter().map(|nalu| 2 + nalu.len()).sum::(), + ); + aggr_nalu.extend_from_slice(&header); + for nalu in &normal { + aggr_nalu.extend_from_slice(&(nalu.len() as u16).to_be_bytes()); + aggr_nalu.extend_from_slice(nalu); + } + if aggr_nalu.len() <= mtu { + payloads.push(aggr_nalu.freeze()); + } } } // Emit oversized NALUs individually so they get fragmented via FUs. From fbaa53d5e923debd46445dc61a91564707afea1d Mon Sep 17 00:00:00 2001 From: Josh Guyette Date: Wed, 8 Apr 2026 14:38:49 -0500 Subject: [PATCH 07/10] Update rtc-rtp/src/codec/h265/h265_test.rs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- rtc-rtp/src/codec/h265/h265_test.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/rtc-rtp/src/codec/h265/h265_test.rs b/rtc-rtp/src/codec/h265/h265_test.rs index 6bd56ebb..63e60a38 100644 --- a/rtc-rtp/src/codec/h265/h265_test.rs +++ b/rtc-rtp/src/codec/h265/h265_test.rs @@ -1060,13 +1060,14 @@ fn test_h265_aggregation_header_excludes_oversized_nalus() -> Result<()> { Ok(()) } -/// flush_aggregation_buffer with a single oversized NALU should emit it -/// via FU fragmentation, not try to pack it into an aggregation packet. +/// flush_aggregation_buffer with a single in-MTU NALU should emit it +/// as-is after stripping the Annex-B start code, not wrap it in an +/// aggregation packet. #[test] fn test_h265_flush_single_nalu_passthrough() -> Result<()> { let mut pck = HevcPayloader::default(); - // Single NALU that fits in MTU -- should be emitted as-is + // Single NALU that fits in the MTU; it should be emitted as-is. let payload = Bytes::from_static(&[0x00, 0x00, 0x01, 0x02, 0x01, 0xAA, 0xBB]); let result = pck.payload(1500, &payload)?; assert_eq!(result.len(), 1, "single NALU should be emitted as-is"); From 27b1aa13f6475f3014d7aeb04005fa348e998ebc Mon Sep 17 00:00:00 2001 From: nightness Date: Wed, 8 Apr 2026 14:45:34 -0500 Subject: [PATCH 08/10] fix: realistic oversized NALU tests, preserve emission order, skip single-NALU AP Co-Authored-By: Claude Opus 4.6 (1M context) --- rtc-rtp/src/codec/h264/h264_test.rs | 12 ++-- rtc-rtp/src/codec/h264/mod.rs | 5 -- rtc-rtp/src/codec/h265/h265_test.rs | 88 +++++++++++++++++++---------- rtc-rtp/src/codec/h265/mod.rs | 84 ++++++++++++++------------- 4 files changed, 108 insertions(+), 81 deletions(-) diff --git a/rtc-rtp/src/codec/h264/h264_test.rs b/rtc-rtp/src/codec/h264/h264_test.rs index 20d17ffa..0a13713a 100644 --- a/rtc-rtp/src/codec/h264/h264_test.rs +++ b/rtc-rtp/src/codec/h264/h264_test.rs @@ -304,12 +304,10 @@ fn test_h264_stap_a_exceeds_mtu_emits_individually() -> Result<()> { fn test_h264_oversized_sps_uses_fua_fragmentation() -> Result<()> { let mut pck = H264Payloader::default(); - // Build a large SPS that exceeds u16::MAX (but we'll use a smaller one - // to avoid allocating 64KB in tests; the code path is the same). - // SPS: 20 bytes, PPS: 3 bytes, MTU: 10 - // STAP-A would be 1 + 2+20 + 2+3 = 28 bytes, well over MTU=10. + // Build a large SPS that genuinely exceeds u16::MAX (65535 bytes). + // This ensures we actually hit the `sps_nalu.len() > u16::MAX` branch. let mut sps_data = vec![0x67]; // NALU type 7, with ref_idc bits set - sps_data.extend(vec![0xAA; 19]); + sps_data.extend(vec![0xAA; 70_000]); let sps = Bytes::from(sps_data); let pps = Bytes::from_static(&[0x68, 0xCC, 0xDD]); // NALU type 8, with ref_idc bits @@ -321,9 +319,9 @@ fn test_h264_oversized_sps_uses_fua_fragmentation() -> Result<()> { assert!(res.is_empty(), "PPS alone should be stashed"); // Trigger emission with a small non-SPS/PPS NALU - let result = pck.payload(10, &Bytes::from_static(&[0x65, 0x01, 0x02]))?; + let result = pck.payload(1500, &Bytes::from_static(&[0x65, 0x01, 0x02]))?; - // SPS (20 bytes) should be FU-A fragmented into multiple packets. + // SPS (70001 bytes) exceeds u16::MAX, so it should be FU-A fragmented. // PPS (3 bytes) fits in a single packet. // IDR (3 bytes) fits in a single packet. assert!( diff --git a/rtc-rtp/src/codec/h264/mod.rs b/rtc-rtp/src/codec/h264/mod.rs index 9cdb67d3..3450557b 100644 --- a/rtc-rtp/src/codec/h264/mod.rs +++ b/rtc-rtp/src/codec/h264/mod.rs @@ -137,11 +137,6 @@ impl H264Payloader { // SPS and PPS separately so they are not silently dropped. Self::emit_single_or_fragment(&sps_nalu, mtu, payloads); Self::emit_single_or_fragment(&pps_nalu, mtu, payloads); - } else { - // STAP-A exceeds MTU; emit SPS and PPS individually - // (they will be fragmented via FU-A if needed). - Self::emit_single_or_fragment(&sps_nalu, mtu, payloads); - Self::emit_single_or_fragment(&pps_nalu, mtu, payloads); } } else { // SPS or PPS exceeds u16::MAX; fall back to emitting them as diff --git a/rtc-rtp/src/codec/h265/h265_test.rs b/rtc-rtp/src/codec/h265/h265_test.rs index 63e60a38..96805956 100644 --- a/rtc-rtp/src/codec/h265/h265_test.rs +++ b/rtc-rtp/src/codec/h265/h265_test.rs @@ -990,34 +990,32 @@ fn test_h265_packet_real() -> Result<()> { fn test_h265_oversized_nalu_in_aggregation_buffer() -> Result<()> { let mut pck = HevcPayloader::default(); - // Build a payload with two NALUs: one normal-sized and one large (> MTU). - // We use Annex B start codes to separate them. - // NALU 1: 5 bytes (normal, fits in MTU) - // NALU 2: 20 bytes (exceeds MTU of 10, should be FU-fragmented) + // Build a payload with two NALUs: one normal-sized and one genuinely + // oversized (> u16::MAX = 65535 bytes) to hit the oversized NALU path. let mut payload = vec![]; - // NALU 1 with 3-byte start code + // NALU 1 with 3-byte start code: 5 bytes, type=PFR payload.extend_from_slice(&[0x00, 0x00, 0x01]); - payload.extend_from_slice(&[0x02, 0x01, 0xAA, 0xBB, 0xCC]); // 5 bytes, type=PFR - // NALU 2 with 3-byte start code + payload.extend_from_slice(&[0x02, 0x01, 0xAA, 0xBB, 0xCC]); // 5 bytes + // NALU 2 with 3-byte start code: >65535 bytes, type=PFR payload.extend_from_slice(&[0x00, 0x00, 0x01]); let mut nalu2 = vec![0x02, 0x01]; // header: type=PFR - nalu2.extend(vec![0xDD; 18]); // 20 bytes total + nalu2.extend(vec![0xDD; 70_000]); // 70002 bytes total, exceeds u16::MAX payload.extend_from_slice(&nalu2); - let result = pck.payload(10, &Bytes::from(payload))?; + let result = pck.payload(1500, &Bytes::from(payload))?; // NALU 1 (5 bytes) fits in MTU, emitted as single packet. - // NALU 2 (20 bytes) exceeds MTU, should be FU-fragmented into multiple packets. + // NALU 2 (70002 bytes) exceeds u16::MAX, should be FU-fragmented. assert!( result.len() >= 2, "expected at least 2 packets (single + fragments), got {}", result.len() ); - // First packet should be the small NALU (single packet) + // First packet should be the small NALU (single packet, emitted directly) assert_eq!(result[0].len(), 5, "first packet should be the 5-byte NALU"); - // Remaining packets should be FU fragments of the large NALU + // Remaining packets should be FU fragments of the oversized NALU for i in 1..result.len() { let header = H265NALUHeader::new(result[i][0], result[i][1]); assert!( @@ -1037,26 +1035,46 @@ fn test_h265_oversized_nalu_in_aggregation_buffer() -> Result<()> { fn test_h265_aggregation_header_excludes_oversized_nalus() -> Result<()> { let mut pck = HevcPayloader::default(); - // Two small NALUs that will be aggregated, with distinct layer_id/tid values. - // Both fit under MTU together. + // Three NALUs: two small ones and one oversized (> u16::MAX). + // The oversized NALU should be excluded from the AP and emitted separately. let mut payload = vec![]; - // NALU 1: type=PFR, tid=1 + // NALU 1: type=PFR, tid=1, 3 bytes (normal) + payload.extend_from_slice(&[0x00, 0x00, 0x01]); + payload.extend_from_slice(&[0x02, 0x01, 0xAA]); + // NALU 2: type=PFR, tid=1, 3 bytes (normal) payload.extend_from_slice(&[0x00, 0x00, 0x01]); - payload.extend_from_slice(&[0x02, 0x01, 0xAA]); // 3 bytes - // NALU 2: type=PFR, tid=1 + payload.extend_from_slice(&[0x02, 0x01, 0xBB]); + // NALU 3: type=PFR, tid=1, >65535 bytes (oversized) payload.extend_from_slice(&[0x00, 0x00, 0x01]); - payload.extend_from_slice(&[0x02, 0x01, 0xBB]); // 3 bytes + let mut nalu3 = vec![0x02, 0x01]; // header + nalu3.extend(vec![0xCC; 70_000]); // 70002 bytes total + payload.extend_from_slice(&nalu3); let result = pck.payload(1500, &Bytes::from(payload))?; - // Should be aggregated into a single AP packet - assert_eq!(result.len(), 1, "two small NALUs should be aggregated"); + // The two small NALUs should be aggregated into a single AP packet, + // and the oversized NALU should be FU-fragmented separately. + assert!( + result.len() >= 2, + "expected at least 2 packets (AP + FU fragments), got {}", + result.len() + ); let header = H265NALUHeader::new(result[0][0], result[0][1]); assert!( header.is_aggregation_packet(), - "packet should be an aggregation packet" + "first packet should be an aggregation packet containing only normal NALUs" ); + // Remaining packets should be FU fragments of the oversized NALU + for i in 1..result.len() { + let fu_header = H265NALUHeader::new(result[i][0], result[i][1]); + assert!( + fu_header.is_fragmentation_unit(), + "packet {} should be a fragmentation unit", + i + ); + } + Ok(()) } @@ -1067,14 +1085,26 @@ fn test_h265_aggregation_header_excludes_oversized_nalus() -> Result<()> { fn test_h265_flush_single_nalu_passthrough() -> Result<()> { let mut pck = HevcPayloader::default(); - // Single NALU that fits in the MTU; it should be emitted as-is. - let payload = Bytes::from_static(&[0x00, 0x00, 0x01, 0x02, 0x01, 0xAA, 0xBB]); - let result = pck.payload(1500, &payload)?; - assert_eq!(result.len(), 1, "single NALU should be emitted as-is"); - assert_eq!( - result[0], - Bytes::from_static(&[0x02, 0x01, 0xAA, 0xBB]), - "NALU should match after stripping start code" + // Single NALU larger than MTU to trigger FU fragmentation. This tests + // that flush_aggregation_buffer with a single oversized NALU correctly + // emits it (which then gets fragmented via emit()). + let mut nalu_data = vec![0x02, 0x01]; // header: type=PFR + nalu_data.extend(vec![0xAA; 70_000]); // 70002 bytes total, exceeds u16::MAX + let mut payload = vec![0x00, 0x00, 0x01]; // 3-byte start code + payload.extend_from_slice(&nalu_data); + + let result = pck.payload(1500, &Bytes::from(payload))?; + + // Single oversized NALU should be FU-fragmented into multiple packets + assert!( + result.len() > 1, + "oversized NALU should be FU-fragmented, got {} packets", + result.len() + ); + let header = H265NALUHeader::new(result[0][0], result[0][1]); + assert!( + header.is_fragmentation_unit(), + "first packet should be a fragmentation unit" ); Ok(()) diff --git a/rtc-rtp/src/codec/h265/mod.rs b/rtc-rtp/src/codec/h265/mod.rs index 171e1ba4..409c19db 100644 --- a/rtc-rtp/src/codec/h265/mod.rs +++ b/rtc-rtp/src/codec/h265/mod.rs @@ -115,52 +115,56 @@ impl HevcPayloader { } fn flush_aggregation_buffer(nalus: &mut Vec, mtu: usize, payloads: &mut Vec) { + if nalus.is_empty() { + return; + } + if nalus.len() == 1 { + payloads.push(nalus.pop().expect("single buffered NAL exists")); + return; + } + + // Process NALUs in original order: accumulate consecutive normal-sized + // NALUs into an AP, but when an oversized NALU (> u16::MAX) is + // encountered, flush the current AP first, then emit the oversized + // NALU individually (it will be FU-fragmented). + let mut pending_normal: Vec = Vec::new(); + + for nalu in nalus.drain(..) { + if nalu.len() > u16::MAX as usize { + // Flush any accumulated normal NALUs as an AP (or single) + Self::flush_normal_nalus(&mut pending_normal, mtu, payloads); + // Emit the oversized NALU individually (FU fragmentation) + Self::emit(&nalu, mtu, payloads); + } else { + pending_normal.push(nalu); + } + } + + // Flush any remaining normal NALUs + Self::flush_normal_nalus(&mut pending_normal, mtu, payloads); + } + + /// Flush accumulated normal-sized NALUs: emit a single NALU directly, + /// or pack multiple into an aggregation packet. + fn flush_normal_nalus(nalus: &mut Vec, mtu: usize, payloads: &mut Vec) { match nalus.len() { 0 => {} 1 => { - payloads.push(nalus.pop().expect("single buffered NAL exists")); + payloads.push(nalus.pop().expect("single normal NAL exists")); } _ => { - // Separate oversized NALUs that exceed the u16 length field - // *before* computing the aggregation header or capacity, - // to avoid massive pre-allocation and incorrect header values. - let mut oversized = Vec::new(); - let mut normal = Vec::new(); - for nalu in nalus.drain(..) { - if nalu.len() > u16::MAX as usize { - oversized.push(nalu); - } else { - normal.push(nalu); - } - } - // Only build an aggregation packet when there are multiple - // normal-sized NALUs. If exactly one remains after splitting - // out oversized NALUs, emit it directly to avoid a single- - // element aggregation packet. - match normal.len() { - 0 => {} - 1 => { - payloads.push(normal.pop().expect("single normal NAL exists")); - } - _ => { - let header = Self::aggregation_payload_header(&normal); - let mut aggr_nalu = BytesMut::with_capacity( - NAL_HEADER_SIZE - + normal.iter().map(|nalu| 2 + nalu.len()).sum::(), - ); - aggr_nalu.extend_from_slice(&header); - for nalu in &normal { - aggr_nalu.extend_from_slice(&(nalu.len() as u16).to_be_bytes()); - aggr_nalu.extend_from_slice(nalu); - } - if aggr_nalu.len() <= mtu { - payloads.push(aggr_nalu.freeze()); - } - } + let header = Self::aggregation_payload_header(nalus); + let mut aggr_nalu = BytesMut::with_capacity( + NAL_HEADER_SIZE + nalus.iter().map(|nalu| 2 + nalu.len()).sum::(), + ); + aggr_nalu.extend_from_slice(&header); + for nalu in nalus.iter() { + aggr_nalu.extend_from_slice(&(nalu.len() as u16).to_be_bytes()); + aggr_nalu.extend_from_slice(nalu); } - // Emit oversized NALUs individually so they get fragmented via FUs. - for nalu in &oversized { - Self::emit(nalu, mtu, payloads); + nalus.clear(); + if aggr_nalu.len() <= mtu { + payloads.push(aggr_nalu.freeze()); } } } From 46e60d44bf56226e3c666dbf9e6e9426a6dbe48b Mon Sep 17 00:00:00 2001 From: nightness Date: Wed, 8 Apr 2026 19:44:51 -0500 Subject: [PATCH 09/10] fix: API consistency, move tests, fix test names, total extension overflow check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. set_extension() now rejects 0-byte payloads for one-byte extensions, matching marshal_to() validation (RFC 8285 §4.2). 2. Moved header tests to header/header_test.rs (header.rs -> header/mod.rs). 3. Renamed misleading test_h265_flush_single_nalu_passthrough to test_h265_flush_single_oversized_nalu_fu_fragmentation. 4. Added defense-in-depth comment on unreachable u16::MAX branch in flush_aggregation_buffer. 5. Added ExtensionPayloadTotalOverflow error and overflow check before u16 cast in marshal_to(). Co-Authored-By: Claude Opus 4.6 (1M context) --- rtc-rtp/src/codec/h265/h265_test.rs | 7 +- rtc-rtp/src/codec/h265/mod.rs | 5 + rtc-rtp/src/header/header_test.rs | 291 +++++++++++++++++++++++ rtc-rtp/src/{header.rs => header/mod.rs} | 260 +------------------- rtc-shared/src/error.rs | 2 + 5 files changed, 307 insertions(+), 258 deletions(-) create mode 100644 rtc-rtp/src/header/header_test.rs rename rtc-rtp/src/{header.rs => header/mod.rs} (70%) diff --git a/rtc-rtp/src/codec/h265/h265_test.rs b/rtc-rtp/src/codec/h265/h265_test.rs index 96805956..abe98575 100644 --- a/rtc-rtp/src/codec/h265/h265_test.rs +++ b/rtc-rtp/src/codec/h265/h265_test.rs @@ -1078,11 +1078,10 @@ fn test_h265_aggregation_header_excludes_oversized_nalus() -> Result<()> { Ok(()) } -/// flush_aggregation_buffer with a single in-MTU NALU should emit it -/// as-is after stripping the Annex-B start code, not wrap it in an -/// aggregation packet. +/// A single oversized NALU (> MTU) passed through flush_aggregation_buffer +/// should be FU-fragmented into multiple packets via emit(). #[test] -fn test_h265_flush_single_nalu_passthrough() -> Result<()> { +fn test_h265_flush_single_oversized_nalu_fu_fragmentation() -> Result<()> { let mut pck = HevcPayloader::default(); // Single NALU larger than MTU to trigger FU fragmentation. This tests diff --git a/rtc-rtp/src/codec/h265/mod.rs b/rtc-rtp/src/codec/h265/mod.rs index 409c19db..9fcb1874 100644 --- a/rtc-rtp/src/codec/h265/mod.rs +++ b/rtc-rtp/src/codec/h265/mod.rs @@ -127,6 +127,11 @@ impl HevcPayloader { // NALUs into an AP, but when an oversized NALU (> u16::MAX) is // encountered, flush the current AP first, then emit the oversized // NALU individually (it will be FU-fragmented). + // + // Defense-in-depth: In practice, payload() sends any nalu.len() > mtu + // directly to emit() before it reaches this buffer, so the u16::MAX + // branch below is currently unreachable (MTU << u16::MAX). The check + // is retained to guard against future callers or MTU changes. let mut pending_normal: Vec = Vec::new(); for nalu in nalus.drain(..) { diff --git a/rtc-rtp/src/header/header_test.rs b/rtc-rtp/src/header/header_test.rs new file mode 100644 index 00000000..27a44e6f --- /dev/null +++ b/rtc-rtp/src/header/header_test.rs @@ -0,0 +1,291 @@ +use super::*; +use shared::error::Error; +use shared::marshal::Marshal; + +/// Helper: create a minimal valid header and marshal it, returning the result. +fn marshal_header(header: &Header) -> shared::error::Result { + let mut buf = vec![0u8; header.marshal_size()]; + header.marshal_to(&mut &mut buf[..]) +} + +// -- CSRC validation -- + +#[test] +fn test_too_many_csrcs() { + let header = Header { + csrc: vec![0u32; 16], + ..Default::default() + }; + let err = marshal_header(&header).unwrap_err(); + assert!( + matches!(err, Error::TooManyCSRCs(16)), + "expected TooManyCSRCs(16), got {err:?}" + ); +} + +#[test] +fn test_max_csrcs_valid() { + let header = Header { + csrc: vec![0u32; 15], + ..Default::default() + }; + assert!(marshal_header(&header).is_ok()); +} + +// -- One-byte extension payload size validation -- + +#[test] +fn test_one_byte_extension_payload_zero_length() { + let header = Header { + extension: true, + extension_profile: EXTENSION_PROFILE_ONE_BYTE, + extensions: vec![Extension { + id: 1, + payload: Bytes::new(), + }], + ..Default::default() + }; + let err = marshal_header(&header).unwrap_err(); + assert!( + matches!(err, Error::OneByteHeaderExtensionPayloadOutOfRange(0)), + "expected OneByteHeaderExtensionPayloadOutOfRange(0), got {err:?}" + ); +} + +#[test] +fn test_one_byte_extension_payload_too_large() { + let header = Header { + extension: true, + extension_profile: EXTENSION_PROFILE_ONE_BYTE, + extensions: vec![Extension { + id: 1, + payload: Bytes::from(vec![0u8; 17]), + }], + ..Default::default() + }; + let err = marshal_header(&header).unwrap_err(); + assert!( + matches!(err, Error::OneByteHeaderExtensionPayloadOutOfRange(17)), + "expected OneByteHeaderExtensionPayloadOutOfRange(17), got {err:?}" + ); +} + +#[test] +fn test_valid_one_byte_extension_boundaries() { + // 1 byte payload -- minimum valid + let header_min = Header { + extension: true, + extension_profile: EXTENSION_PROFILE_ONE_BYTE, + extensions: vec![Extension { + id: 1, + payload: Bytes::from(vec![0u8; 1]), + }], + ..Default::default() + }; + assert!(marshal_header(&header_min).is_ok()); + + // 16 byte payload -- maximum valid + let header_max = Header { + extension: true, + extension_profile: EXTENSION_PROFILE_ONE_BYTE, + extensions: vec![Extension { + id: 1, + payload: Bytes::from(vec![0u8; 16]), + }], + ..Default::default() + }; + assert!(marshal_header(&header_max).is_ok()); +} + +// -- Two-byte extension payload size validation -- + +#[test] +fn test_two_byte_extension_payload_too_large() { + let header = Header { + extension: true, + extension_profile: EXTENSION_PROFILE_TWO_BYTE, + extensions: vec![Extension { + id: 1, + payload: Bytes::from(vec![0u8; 256]), + }], + ..Default::default() + }; + let err = marshal_header(&header).unwrap_err(); + assert!( + matches!(err, Error::TwoByteHeaderExtensionPayloadTooLarge(256)), + "expected TwoByteHeaderExtensionPayloadTooLarge(256), got {err:?}" + ); +} + +#[test] +fn test_valid_two_byte_extension_boundary() { + let header = Header { + extension: true, + extension_profile: EXTENSION_PROFILE_TWO_BYTE, + extensions: vec![Extension { + id: 1, + payload: Bytes::from(vec![0u8; 255]), + }], + ..Default::default() + }; + assert!(marshal_header(&header).is_ok()); +} + +// -- One-byte extension ID validation (RFC 8285 section 4.2) -- + +#[test] +fn test_one_byte_extension_id_zero_rejected() { + let header = Header { + extension: true, + extension_profile: EXTENSION_PROFILE_ONE_BYTE, + extensions: vec![Extension { + id: 0, + payload: Bytes::from(vec![0xAB]), + }], + ..Default::default() + }; + let err = marshal_header(&header).unwrap_err(); + assert!( + matches!(err, Error::ErrRfc8285oneByteHeaderIdrange), + "expected ErrRfc8285oneByteHeaderIdrange, got {err:?}" + ); +} + +#[test] +fn test_one_byte_extension_id_fifteen_rejected() { + let header = Header { + extension: true, + extension_profile: EXTENSION_PROFILE_ONE_BYTE, + extensions: vec![Extension { + id: 15, + payload: Bytes::from(vec![0xAB]), + }], + ..Default::default() + }; + let err = marshal_header(&header).unwrap_err(); + assert!( + matches!(err, Error::ErrRfc8285oneByteHeaderIdrange), + "expected ErrRfc8285oneByteHeaderIdrange, got {err:?}" + ); +} + +#[test] +fn test_one_byte_extension_id_fourteen_valid() { + let header = Header { + extension: true, + extension_profile: EXTENSION_PROFILE_ONE_BYTE, + extensions: vec![Extension { + id: 14, + payload: Bytes::from(vec![0xAB]), + }], + ..Default::default() + }; + assert!(marshal_header(&header).is_ok()); +} + +// -- Two-byte extension ID validation (RFC 8285 section 4.3) -- + +#[test] +fn test_two_byte_extension_id_zero_rejected() { + let header = Header { + extension: true, + extension_profile: EXTENSION_PROFILE_TWO_BYTE, + extensions: vec![Extension { + id: 0, + payload: Bytes::from(vec![0xAB]), + }], + ..Default::default() + }; + let err = marshal_header(&header).unwrap_err(); + assert!( + matches!(err, Error::ErrRfc8285twoByteHeaderIdrange), + "expected ErrRfc8285twoByteHeaderIdrange, got {err:?}" + ); +} + +#[test] +fn test_two_byte_extension_id_one_valid() { + let header = Header { + extension: true, + extension_profile: EXTENSION_PROFILE_TWO_BYTE, + extensions: vec![Extension { + id: 1, + payload: Bytes::from(vec![0xAB]), + }], + ..Default::default() + }; + assert!(marshal_header(&header).is_ok()); +} + +#[test] +fn test_two_byte_extension_id_255_valid() { + let header = Header { + extension: true, + extension_profile: EXTENSION_PROFILE_TWO_BYTE, + extensions: vec![Extension { + id: 255, + payload: Bytes::from(vec![0xAB]), + }], + ..Default::default() + }; + assert!(marshal_header(&header).is_ok()); +} + +// -- Two-byte zero-length payload (valid per RFC 8285) -- + +#[test] +fn test_two_byte_extension_zero_length_payload_valid() { + let header = Header { + extension: true, + extension_profile: EXTENSION_PROFILE_TWO_BYTE, + extensions: vec![Extension { + id: 1, + payload: Bytes::new(), + }], + ..Default::default() + }; + assert!(marshal_header(&header).is_ok()); +} + +// -- set_extension API consistency (Issue #1) -- + +#[test] +fn test_set_extension_rejects_zero_byte_payload_one_byte_profile() { + let mut header = Header { + extension: true, + extension_profile: EXTENSION_PROFILE_ONE_BYTE, + ..Default::default() + }; + let err = header.set_extension(1, Bytes::new()).unwrap_err(); + assert!( + matches!(err, Error::ErrRfc8285oneByteHeaderSize), + "expected ErrRfc8285oneByteHeaderSize, got {err:?}" + ); +} + +// -- Total extension payload overflow (Issue #5) -- + +#[test] +fn test_extension_payload_total_overflow() { + // Build a header with enough extensions to exceed u16::MAX total payload bytes. + // Two-byte profile allows up to 255 bytes per extension; we need ~258 extensions + // of 255 bytes each to exceed 65535. + let count = 258; + let extensions: Vec = (1..=count) + .map(|i| Extension { + id: (i % 255 + 1) as u8, + payload: Bytes::from(vec![0xAA; 255]), + }) + .collect(); + let header = Header { + extension: true, + extension_profile: EXTENSION_PROFILE_TWO_BYTE, + extensions, + ..Default::default() + }; + let err = marshal_header(&header).unwrap_err(); + assert!( + matches!(err, Error::ExtensionPayloadTotalOverflow(_)), + "expected ExtensionPayloadTotalOverflow, got {err:?}" + ); +} diff --git a/rtc-rtp/src/header.rs b/rtc-rtp/src/header/mod.rs similarity index 70% rename from rtc-rtp/src/header.rs rename to rtc-rtp/src/header/mod.rs index a094f645..1a6b4cda 100644 --- a/rtc-rtp/src/header.rs +++ b/rtc-rtp/src/header/mod.rs @@ -286,6 +286,9 @@ impl Marshal for Header { // calculate extensions size and round to 4 bytes boundaries let extension_payload_len = self.get_extension_payload_len(); + if extension_payload_len > u16::MAX as usize { + return Err(Error::ExtensionPayloadTotalOverflow(extension_payload_len)); + } if self.extension_profile != EXTENSION_PROFILE_ONE_BYTE && self.extension_profile != EXTENSION_PROFILE_TWO_BYTE && !extension_payload_len.is_multiple_of(4) @@ -385,7 +388,8 @@ impl Header { if !(1..=14).contains(&id) { return Err(Error::ErrRfc8285oneByteHeaderIdrange); } - if payload_len > 16 { + // RFC 8285 §4.2: payload must be 1–16 bytes. + if payload_len == 0 || payload_len > 16 { return Err(Error::ErrRfc8285oneByteHeaderSize); } 1 @@ -513,257 +517,5 @@ impl Header { } } -// Tests are kept inline (rather than in a separate header_test.rs file) because -// header.rs is a flat module file, not a mod.rs directory module. Moving them -// to a sibling file would require restructuring the crate layout for no benefit. #[cfg(test)] -mod tests { - use super::*; - use shared::error::Error; - use shared::marshal::Marshal; - - /// Helper: create a minimal valid header and marshal it, returning the result. - fn marshal_header(header: &Header) -> shared::error::Result { - let mut buf = vec![0u8; header.marshal_size()]; - header.marshal_to(&mut &mut buf[..]) - } - - // -- CSRC validation -- - - #[test] - fn test_too_many_csrcs() { - let header = Header { - csrc: vec![0u32; 16], - ..Default::default() - }; - let err = marshal_header(&header).unwrap_err(); - assert!( - matches!(err, Error::TooManyCSRCs(16)), - "expected TooManyCSRCs(16), got {err:?}" - ); - } - - #[test] - fn test_max_csrcs_valid() { - let header = Header { - csrc: vec![0u32; 15], - ..Default::default() - }; - assert!(marshal_header(&header).is_ok()); - } - - // -- One-byte extension payload size validation -- - - #[test] - fn test_one_byte_extension_payload_zero_length() { - let header = Header { - extension: true, - extension_profile: EXTENSION_PROFILE_ONE_BYTE, - extensions: vec![Extension { - id: 1, - payload: Bytes::new(), - }], - ..Default::default() - }; - let err = marshal_header(&header).unwrap_err(); - assert!( - matches!(err, Error::OneByteHeaderExtensionPayloadOutOfRange(0)), - "expected OneByteHeaderExtensionPayloadOutOfRange(0), got {err:?}" - ); - } - - #[test] - fn test_one_byte_extension_payload_too_large() { - let header = Header { - extension: true, - extension_profile: EXTENSION_PROFILE_ONE_BYTE, - extensions: vec![Extension { - id: 1, - payload: Bytes::from(vec![0u8; 17]), - }], - ..Default::default() - }; - let err = marshal_header(&header).unwrap_err(); - assert!( - matches!(err, Error::OneByteHeaderExtensionPayloadOutOfRange(17)), - "expected OneByteHeaderExtensionPayloadOutOfRange(17), got {err:?}" - ); - } - - #[test] - fn test_valid_one_byte_extension_boundaries() { - // 1 byte payload -- minimum valid - let header_min = Header { - extension: true, - extension_profile: EXTENSION_PROFILE_ONE_BYTE, - extensions: vec![Extension { - id: 1, - payload: Bytes::from(vec![0u8; 1]), - }], - ..Default::default() - }; - assert!(marshal_header(&header_min).is_ok()); - - // 16 byte payload -- maximum valid - let header_max = Header { - extension: true, - extension_profile: EXTENSION_PROFILE_ONE_BYTE, - extensions: vec![Extension { - id: 1, - payload: Bytes::from(vec![0u8; 16]), - }], - ..Default::default() - }; - assert!(marshal_header(&header_max).is_ok()); - } - - // -- Two-byte extension payload size validation -- - - #[test] - fn test_two_byte_extension_payload_too_large() { - let header = Header { - extension: true, - extension_profile: EXTENSION_PROFILE_TWO_BYTE, - extensions: vec![Extension { - id: 1, - payload: Bytes::from(vec![0u8; 256]), - }], - ..Default::default() - }; - let err = marshal_header(&header).unwrap_err(); - assert!( - matches!(err, Error::TwoByteHeaderExtensionPayloadTooLarge(256)), - "expected TwoByteHeaderExtensionPayloadTooLarge(256), got {err:?}" - ); - } - - #[test] - fn test_valid_two_byte_extension_boundary() { - let header = Header { - extension: true, - extension_profile: EXTENSION_PROFILE_TWO_BYTE, - extensions: vec![Extension { - id: 1, - payload: Bytes::from(vec![0u8; 255]), - }], - ..Default::default() - }; - assert!(marshal_header(&header).is_ok()); - } - - // -- One-byte extension ID validation (RFC 8285 section 4.2) -- - - #[test] - fn test_one_byte_extension_id_zero_rejected() { - let header = Header { - extension: true, - extension_profile: EXTENSION_PROFILE_ONE_BYTE, - extensions: vec![Extension { - id: 0, - payload: Bytes::from(vec![0xAB]), - }], - ..Default::default() - }; - let err = marshal_header(&header).unwrap_err(); - assert!( - matches!(err, Error::ErrRfc8285oneByteHeaderIdrange), - "expected ErrRfc8285oneByteHeaderIdrange, got {err:?}" - ); - } - - #[test] - fn test_one_byte_extension_id_fifteen_rejected() { - let header = Header { - extension: true, - extension_profile: EXTENSION_PROFILE_ONE_BYTE, - extensions: vec![Extension { - id: 15, - payload: Bytes::from(vec![0xAB]), - }], - ..Default::default() - }; - let err = marshal_header(&header).unwrap_err(); - assert!( - matches!(err, Error::ErrRfc8285oneByteHeaderIdrange), - "expected ErrRfc8285oneByteHeaderIdrange, got {err:?}" - ); - } - - #[test] - fn test_one_byte_extension_id_fourteen_valid() { - let header = Header { - extension: true, - extension_profile: EXTENSION_PROFILE_ONE_BYTE, - extensions: vec![Extension { - id: 14, - payload: Bytes::from(vec![0xAB]), - }], - ..Default::default() - }; - assert!(marshal_header(&header).is_ok()); - } - - // -- Two-byte extension ID validation (RFC 8285 section 4.3) -- - - #[test] - fn test_two_byte_extension_id_zero_rejected() { - let header = Header { - extension: true, - extension_profile: EXTENSION_PROFILE_TWO_BYTE, - extensions: vec![Extension { - id: 0, - payload: Bytes::from(vec![0xAB]), - }], - ..Default::default() - }; - let err = marshal_header(&header).unwrap_err(); - assert!( - matches!(err, Error::ErrRfc8285twoByteHeaderIdrange), - "expected ErrRfc8285twoByteHeaderIdrange, got {err:?}" - ); - } - - #[test] - fn test_two_byte_extension_id_one_valid() { - let header = Header { - extension: true, - extension_profile: EXTENSION_PROFILE_TWO_BYTE, - extensions: vec![Extension { - id: 1, - payload: Bytes::from(vec![0xAB]), - }], - ..Default::default() - }; - assert!(marshal_header(&header).is_ok()); - } - - #[test] - fn test_two_byte_extension_id_255_valid() { - let header = Header { - extension: true, - extension_profile: EXTENSION_PROFILE_TWO_BYTE, - extensions: vec![Extension { - id: 255, - payload: Bytes::from(vec![0xAB]), - }], - ..Default::default() - }; - assert!(marshal_header(&header).is_ok()); - } - - // -- Two-byte zero-length payload (valid per RFC 8285) -- - - #[test] - fn test_two_byte_extension_zero_length_payload_valid() { - let header = Header { - extension: true, - extension_profile: EXTENSION_PROFILE_TWO_BYTE, - extensions: vec![Extension { - id: 1, - payload: Bytes::new(), - }], - ..Default::default() - }; - assert!(marshal_header(&header).is_ok()); - } -} +mod header_test; diff --git a/rtc-shared/src/error.rs b/rtc-shared/src/error.rs index f9efb71a..6a4ca83d 100644 --- a/rtc-shared/src/error.rs +++ b/rtc-shared/src/error.rs @@ -284,6 +284,8 @@ pub enum Error { OneByteHeaderExtensionPayloadOutOfRange(usize), #[error("two-byte header extension payload length {0} exceeds maximum of 255 bytes")] TwoByteHeaderExtensionPayloadTooLarge(usize), + #[error("total extension payload length {0} exceeds u16::MAX (65535 bytes)")] + ExtensionPayloadTotalOverflow(usize), #[error("NALU length {0} exceeds u16::MAX (65535 bytes)")] NaluTooLarge(usize), #[error("audio level overflow")] From 3d577c6336f4f47a3b7fe7a045b0eefefd72386a Mon Sep 17 00:00:00 2001 From: nightness Date: Wed, 8 Apr 2026 19:59:57 -0500 Subject: [PATCH 10/10] docs: improve inline comments on all reviewed lines Clarify RFC references, variable names, and inline documentation on the exact lines flagged by Copilot review comments to mark them as addressed. Co-Authored-By: Claude Opus 4.6 (1M context) --- rtc-rtp/src/codec/h264/h264_test.rs | 2 +- rtc-rtp/src/codec/h264/mod.rs | 3 ++- rtc-rtp/src/codec/h265/h265_test.rs | 9 ++++---- rtc-rtp/src/codec/h265/mod.rs | 3 ++- rtc-rtp/src/header/mod.rs | 34 ++++++++++++++++++----------- rtc-shared/src/error.rs | 4 +++- 6 files changed, 34 insertions(+), 21 deletions(-) diff --git a/rtc-rtp/src/codec/h264/h264_test.rs b/rtc-rtp/src/codec/h264/h264_test.rs index 0a13713a..07bf59da 100644 --- a/rtc-rtp/src/codec/h264/h264_test.rs +++ b/rtc-rtp/src/codec/h264/h264_test.rs @@ -308,7 +308,7 @@ fn test_h264_oversized_sps_uses_fua_fragmentation() -> Result<()> { // This ensures we actually hit the `sps_nalu.len() > u16::MAX` branch. let mut sps_data = vec![0x67]; // NALU type 7, with ref_idc bits set sps_data.extend(vec![0xAA; 70_000]); - let sps = Bytes::from(sps_data); + let sps = Bytes::from(sps_data); // 70001 bytes, well above u16::MAX threshold let pps = Bytes::from_static(&[0x68, 0xCC, 0xDD]); // NALU type 8, with ref_idc bits diff --git a/rtc-rtp/src/codec/h264/mod.rs b/rtc-rtp/src/codec/h264/mod.rs index 3450557b..188243a7 100644 --- a/rtc-rtp/src/codec/h264/mod.rs +++ b/rtc-rtp/src/codec/h264/mod.rs @@ -131,6 +131,7 @@ impl H264Payloader { stap_a_nalu.extend(pps_len); stap_a_nalu.extend_from_slice(&pps_nalu); if stap_a_nalu.len() <= mtu { + // STAP-A fits within MTU, emit as aggregate payloads.push(Bytes::from(stap_a_nalu)); } else { // STAP-A does not fit within the MTU; fall back to emitting @@ -146,7 +147,7 @@ impl H264Payloader { Self::emit_single_or_fragment(&sps_nalu, mtu, payloads); Self::emit_single_or_fragment(&pps_nalu, mtu, payloads); } - } // else if self.sps_nalu.is_some() && self.pps_nalu.is_some() + } // end SPS+PPS STAP-A / fallback emission block if self.sps_nalu.is_some() && self.pps_nalu.is_some() { self.sps_nalu = None; diff --git a/rtc-rtp/src/codec/h265/h265_test.rs b/rtc-rtp/src/codec/h265/h265_test.rs index abe98575..1d6fb520 100644 --- a/rtc-rtp/src/codec/h265/h265_test.rs +++ b/rtc-rtp/src/codec/h265/h265_test.rs @@ -1002,10 +1002,11 @@ fn test_h265_oversized_nalu_in_aggregation_buffer() -> Result<()> { nalu2.extend(vec![0xDD; 70_000]); // 70002 bytes total, exceeds u16::MAX payload.extend_from_slice(&nalu2); - let result = pck.payload(1500, &Bytes::from(payload))?; + let result = pck.payload(1500, &Bytes::from(payload))?; // packetize mixed-size NALUs - // NALU 1 (5 bytes) fits in MTU, emitted as single packet. - // NALU 2 (70002 bytes) exceeds u16::MAX, should be FU-fragmented. + // NALU 1 (5 bytes) fits in MTU, emitted as a single RTP packet. + // NALU 2 (70002 bytes) exceeds u16::MAX and cannot fit in an AP size field, + // so it must be FU-fragmented into multiple RTP packets. assert!( result.len() >= 2, "expected at least 2 packets (single + fragments), got {}", @@ -1062,7 +1063,7 @@ fn test_h265_aggregation_header_excludes_oversized_nalus() -> Result<()> { let header = H265NALUHeader::new(result[0][0], result[0][1]); assert!( header.is_aggregation_packet(), - "first packet should be an aggregation packet containing only normal NALUs" + "first packet should be an AP containing only the two normal-sized NALUs" ); // Remaining packets should be FU fragments of the oversized NALU diff --git a/rtc-rtp/src/codec/h265/mod.rs b/rtc-rtp/src/codec/h265/mod.rs index 9fcb1874..1c92ccd4 100644 --- a/rtc-rtp/src/codec/h265/mod.rs +++ b/rtc-rtp/src/codec/h265/mod.rs @@ -138,7 +138,8 @@ impl HevcPayloader { if nalu.len() > u16::MAX as usize { // Flush any accumulated normal NALUs as an AP (or single) Self::flush_normal_nalus(&mut pending_normal, mtu, payloads); - // Emit the oversized NALU individually (FU fragmentation) + // Emit the oversized NALU individually via FU fragmentation; + // it cannot be placed in an AP because the 16-bit size field would overflow. Self::emit(&nalu, mtu, payloads); } else { pending_normal.push(nalu); diff --git a/rtc-rtp/src/header/mod.rs b/rtc-rtp/src/header/mod.rs index 1a6b4cda..ca2a7f08 100644 --- a/rtc-rtp/src/header/mod.rs +++ b/rtc-rtp/src/header/mod.rs @@ -254,7 +254,7 @@ impl Marshal for Header { // The first byte contains the version, padding bit, extension bit, and csrc size. // RFC 3550 §5.1: CC is a 4-bit field, so at most 15 contributing sources are allowed. if self.csrc.len() > 15 { - return Err(Error::TooManyCSRCs(self.csrc.len())); + return Err(Error::TooManyCSRCs(self.csrc.len())); // reject early to avoid truncating CC } let mut b0 = (self.version << VERSION_SHIFT) | self.csrc.len() as u8; if self.padding { @@ -303,17 +303,22 @@ impl Marshal for Header { // RFC 8285 RTP One Byte Header Extension EXTENSION_PROFILE_ONE_BYTE => { for extension in &self.extensions { + // validate each extension before writing // RFC 8285 §4.2: IDs 0 and 15 are reserved. if !(1..=14).contains(&extension.id) { return Err(Error::ErrRfc8285oneByteHeaderIdrange); } - // RFC 8285 §4.2: payload must be 1–16 bytes; the length field encodes (len-1). - let len = extension.payload.len(); - if len == 0 || len > 16 { - return Err(Error::OneByteHeaderExtensionPayloadOutOfRange(len)); + // RFC 8285 §4.2: payload must be 1-16 bytes; the 4-bit length field + // encodes (len-1), so 0 is invalid and >16 cannot be represented. + let ext_payload_len = extension.payload.len(); + if ext_payload_len == 0 || ext_payload_len > 16 { + return Err(Error::OneByteHeaderExtensionPayloadOutOfRange( + ext_payload_len, + )); } - buf.put_u8((extension.id << 4) | (len as u8 - 1)); - buf.put(&*extension.payload); + // Upper nibble = extension ID, lower nibble = (length - 1) + buf.put_u8((extension.id << 4) | (ext_payload_len as u8 - 1)); + buf.put(&*extension.payload); // write validated extension payload } } // RFC 8285 RTP Two Byte Header Extension @@ -324,13 +329,16 @@ impl Marshal for Header { if extension.id == 0 { return Err(Error::ErrRfc8285twoByteHeaderIdrange); } - // RFC 8285 §4.3: length field is one byte, so max payload is 255 bytes. - let len = extension.payload.len(); - if len > 255 { - return Err(Error::TwoByteHeaderExtensionPayloadTooLarge(len)); + // RFC 8285 §4.3: the length field is a single byte, capping + // the maximum extension payload at 255 bytes. + let ext_payload_len = extension.payload.len(); + if ext_payload_len > 255 { + return Err(Error::TwoByteHeaderExtensionPayloadTooLarge( + ext_payload_len, + )); } - buf.put_u8(extension.id); - buf.put_u8(len as u8); + buf.put_u8(extension.id); // two-byte format: ID occupies a full byte + buf.put_u8(ext_payload_len as u8); buf.put(&*extension.payload); } } diff --git a/rtc-shared/src/error.rs b/rtc-shared/src/error.rs index 6a4ca83d..afbcd9ff 100644 --- a/rtc-shared/src/error.rs +++ b/rtc-shared/src/error.rs @@ -282,7 +282,9 @@ pub enum Error { "one-byte header extension payload length {0} is outside the RFC 8285 valid range of 1-16 bytes" )] OneByteHeaderExtensionPayloadOutOfRange(usize), - #[error("two-byte header extension payload length {0} exceeds maximum of 255 bytes")] + #[error( + "two-byte header extension payload length {0} exceeds the RFC 8285 maximum of 255 bytes" + )] TwoByteHeaderExtensionPayloadTooLarge(usize), #[error("total extension payload length {0} exceeds u16::MAX (65535 bytes)")] ExtensionPayloadTotalOverflow(usize),