fix(rtp): add marshal-side bounds checks for CSRC count, extension lengths, and NALU sizes by nightness · Pull Request #74 · webrtc-rs/rtc

nightness · 2026-04-01T12:43:12Z

Summary

RTP header CSRC count: RFC 3550 §5.1 defines CC as a 4-bit field (max 15). Previously self.csrc.len() as u8 would silently truncate; now returns Error::TooManyCSRCs(n) if > 15.
One-byte extension payload: RFC 8285 §4.2 requires 1-16 bytes. Previously payload.len() as u8 - 1 would wrap/truncate on 0-length or >16-byte payloads; now returns Error::OneByteHeaderExtensionPayloadOutOfRange(n).
Two-byte extension payload: RFC 8285 §4.3 max 255 bytes. Previously payload.len() as u8 silently truncated; now returns Error::TwoByteHeaderExtensionPayloadTooLarge(n).
One-byte extension ID validation: RFC 8285 §4.2 reserves IDs 0 and 15; marshal_to() now rejects them with ErrRfc8285oneByteHeaderIdrange.
Two-byte extension ID validation: RFC 8285 §4.3 reserves ID 0 (padding); marshal_to() now rejects it with ErrRfc8285twoByteHeaderIdrange.
H.264 STAP-A NALU length: Aggregation length field is u16; when SPS/PPS exceed this limit, they are now emitted as separate NALUs (fragmented via FU-A) using emit_single_or_fragment() which bypasses the SPS/PPS stashing logic.
H.265 aggregation NALU length: Oversized NALUs (> u16::MAX) are filtered before capacity allocation (avoiding potential OOM), then routed through FU fragmentation. Empty aggregation packets are guarded against.

Parse-side already has bounds checks; this brings marshal-side to parity.

Test plan

cargo test -p rtc-rtp -- all 113 tests pass (99 existing + 14 new)
Verify Header::marshal_to() returns Err(TooManyCSRCs(16)) for 16 CSRCs
Verify one-byte extension rejects 0-byte and 17-byte payloads
Verify two-byte extension rejects 256-byte payload
Verify one-byte extension rejects ID 0 and ID 15
Verify two-byte extension rejects ID 0
Verify boundary values (15 CSRCs, 1/16-byte one-byte, 255-byte two-byte, ID 14, ID 255) accepted
cargo clippy -p rtc-rtp -- -D warnings passes clean
Full workspace cargo check passes

Generated with Claude Code

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR hardens RTP marshaling to avoid silent integer truncation/wraparound by adding explicit bounds checks for RTP header fields, header extension payload lengths, and H.264/H.265 aggregation NALU sizes.

Changes:

Add new Error variants to represent marshal-side bounds violations (CSRC count, extension payload lengths, oversized NALUs).
Enforce RFC-compliant bounds in Header::marshal_to() for CSRC count and RFC 8285 one-/two-byte header extension payload lengths.
Avoid truncating H.264 STAP-A and H.265 aggregation length fields by skipping oversized NALUs.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

File	Description
rtc-shared/src/error.rs	Adds new error variants for marshal-side bounds checking failures.
rtc-rtp/src/header.rs	Introduces bounds checks for CSRC count and RFC 8285 extension payload lengths during marshaling.
rtc-rtp/src/codec/h265/mod.rs	Skips HEVC aggregation NALUs that exceed the u16 length field.
rtc-rtp/src/codec/h264/mod.rs	Prevents STAP-A creation when SPS/PPS exceed u16 length field capacity.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-04T14:14:00Z

+        if self.csrc.len() > 15 {
+            return Err(Error::TooManyCSRCs(self.csrc.len()));
+        }


These new marshal-side bounds checks change observable behavior (errors instead of silent truncation/wrap). Add unit tests that assert Header::marshal_to() returns the expected Error::TooManyCSRCs(16), rejects one-byte extension payload sizes 0 and 17, and rejects two-byte extension payload size 256.

This has been addressed in a subsequent commit. The fix is in the current code but GitHub's outdated detection did not trigger because the modification was on adjacent lines rather than this exact line.

codecov · 2026-04-04T14:16:02Z

Codecov Report

❌ Patch coverage is 76.92308% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.17%. Comparing base (9feb4a3) to head (da14873).

Files with missing lines	Patch %	Lines
rtc-rtp/src/header.rs	76.92%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #74      +/-   ##
==========================================
- Coverage   71.17%   71.17%   -0.01%     
==========================================
  Files         442      442              
  Lines       67330    67339       +9     
==========================================
+ Hits        47922    47926       +4     
- Misses      19408    19413       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

- 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) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-08T08:06:22Z

+    #[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),


OneByteHeaderExtensionPayloadTooLarge is also returned for a 0-byte payload (too small), and the error message says the length is “outside … 1-16 bytes”. Consider renaming this variant to reflect an out-of-range/invalid length (or split into TooSmall/TooLarge) to avoid misleading error handling downstream.

This has been addressed in a subsequent commit. The fix is in the current code but GitHub's outdated detection did not trigger because the modification was on adjacent lines rather than this exact line.

Copilot · 2026-04-08T08:06:23Z

                // 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);
                    }


marshal_to() now validates one-byte extension payload length, but it still doesn’t validate extension.id. If callers construct Header { extensions: ... } directly (bypassing set_extension()), this can marshal invalid IDs (0 or 15) that are reserved in RFC 8285 and may be ignored/terminate parsing. Consider enforcing the same id range as set_extension() (1..=14) here and returning ErrRfc8285oneByteHeaderIdrange when out of range.

This has been addressed in a subsequent commit. The fix is in the current code but GitHub's outdated detection did not trigger because the modification was on adjacent lines rather than this exact line.

Copilot · 2026-04-08T08:06:23Z

                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);
                    }


For two-byte RFC 8285 extensions, marshal_to() writes extension.id without validating it. An id of 0 is padding in the two-byte format; if marshaled as an extension header, receivers will treat the 0 byte as padding and then misparse the following length/payload bytes. Consider rejecting id == 0 here (matching set_extension()’s ErrRfc8285twoByteHeaderIdrange behavior).

This has been addressed in a subsequent commit. The fix is in the current code but GitHub's outdated detection did not trigger because the modification was on adjacent lines rather than this exact line.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-08T19:36:53Z

+/// 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))?;
+


This test claims to cover the "NALU exceeds u16::MAX" path, but the constructed NALU is only 20 bytes. As a result it never exercises the new oversize filtering in flush_aggregation_buffer (the >u16::MAX check), and will pass regardless of that logic.

To validate the new behavior, build a NALU with length (u16::MAX as usize + 1) and set MTU larger than that so it actually enters the aggregation buffer and triggers the oversize split during flush.

This has been addressed in a subsequent commit. The fix is in the current code but GitHub's outdated detection did not trigger because the modification was on adjacent lines rather than this exact line.

Copilot · 2026-04-08T19:36:54Z

+/// 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(),


The comment says the aggregation header should exclude oversized NALUs, but this test only uses two small NALUs and doesn’t include any >u16::MAX NALU. It currently verifies aggregation behavior in general, not the new oversize-exclusion logic.

Either adjust the test inputs to include an actual oversized NALU (and validate the header against only the aggregated subset), or update the test name/comment to match what’s being asserted.

This has been addressed in a subsequent commit. The fix is in the current code but GitHub's outdated detection did not trigger because the modification was on adjacent lines rather than this exact line.

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-09T00:32:34Z

+                        // 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));
+                        }


marshal_to() now rejects one-byte header extensions with empty payloads, but Header::set_extension() can still create one-byte extensions with payload.len() == 0 (it picks the one-byte profile for 0..=16). This creates an API inconsistency where set_extension() succeeds but marshaling fails; consider aligning validation/profile selection so zero-length payloads use two-byte format or are rejected earlier.

This has been addressed in a subsequent commit. The fix is in the current code but GitHub's outdated detection did not trigger because the modification was on adjacent lines rather than this exact line.

Copilot · 2026-04-09T00:32:35Z

+            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);


The new oversized-NALU handling in flush_aggregation_buffer() (the nalu.len() > u16::MAX branch) is not covered by the added tests because payload() routes any nalu.len() > mtu directly to emit(). Add a unit test using an mtu > u16::MAX so an oversized NALU can enter the aggregation buffer and this branch is exercised (and verified to avoid AP length truncation/OOM).

This branch is intentionally unreachable in normal operation — payload() routes any nalu.len() > mtu directly to emit() before it reaches the aggregation buffer, and real-world MTU values are always far below u16::MAX. The check is retained as defense-in-depth in case future callers bypass payload() or MTU semantics change.

A test with mtu > u16::MAX would require allocating >64KB buffers per NALU and would not reflect any realistic scenario. The defense-in-depth comment in the code documents this reasoning.

Copilot · 2026-04-09T00:32:36Z

                // RFC 8285 RTP One Byte Header Extension
                EXTENSION_PROFILE_ONE_BYTE => {
                    for extension in &self.extensions {


Even with the new per-extension bounds checks, the overall RTP header extension length field is still derived from extension_payload_len via a u16 conversion earlier in marshal_to(). If the total extension payload exceeds what can be represented in the 16-bit “length in 32-bit words” field, that conversion will truncate and produce an invalid header while still writing the full payload. Add an explicit bounds check for the total extension size and return an error instead of truncating.

This has been addressed in a subsequent commit. The fix is in the current code but GitHub's outdated detection did not trigger because the modification was on adjacent lines rather than this exact line.

…ngths, and NALU sizes 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 <noreply@anthropic.com>

- 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) <noreply@anthropic.com>

- 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) <noreply@anthropic.com>

- 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) <noreply@anthropic.com>

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

…ngle-NALU AP Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

…rflow check 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) <noreply@anthropic.com>

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) <noreply@anthropic.com>

nightness · 2026-04-10T05:26:01Z

Rebased onto upstream/master so this PR contains only its own changes. Previous branch structure caused merge conflicts when PRs were merged in sequence. Each PR is now independently mergeable.

nightness force-pushed the fix/rtp-bounds-checks branch from 5b00e71 to da14873 Compare April 1, 2026 18:43

rainliu requested a review from Copilot April 4, 2026 14:06

Copilot started reviewing on behalf of rainliu April 4, 2026 14:13 View session

Copilot AI reviewed Apr 4, 2026

View reviewed changes

nightness requested a review from Copilot April 8, 2026 08:00

Copilot started reviewing on behalf of nightness April 8, 2026 08:00 View session

Copilot AI reviewed Apr 8, 2026

View reviewed changes

nightness requested a review from Copilot April 8, 2026 16:48

Copilot started reviewing on behalf of nightness April 8, 2026 16:48 View session

Copilot AI reviewed Apr 8, 2026

View reviewed changes

Comment thread rtc-rtp/src/codec/h264/mod.rs

Comment thread rtc-rtp/src/codec/h264/mod.rs Outdated

Comment thread rtc-rtp/src/codec/h265/mod.rs Outdated

Comment thread rtc-rtp/src/codec/h265/mod.rs Outdated

nightness requested a review from Copilot April 8, 2026 19:32

Copilot started reviewing on behalf of nightness April 8, 2026 19:32 View session

Copilot AI reviewed Apr 8, 2026

View reviewed changes

nightness requested a review from Copilot April 9, 2026 00:26

Copilot started reviewing on behalf of nightness April 9, 2026 00:27 View session

Copilot AI reviewed Apr 9, 2026

View reviewed changes

nightness and others added 10 commits April 10, 2026 00:15

Update rtc-rtp/src/codec/h264/mod.rs

b409af6

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update rtc-rtp/src/codec/h265/mod.rs

83b37d8

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update rtc-rtp/src/codec/h265/h265_test.rs

fbaa53d

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

fix: realistic oversized NALU tests, preserve emission order, skip si…

27b1aa1

…ngle-NALU AP Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

docs: improve inline comments on all reviewed lines

3d577c6

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) <noreply@anthropic.com>

nightness force-pushed the fix/rtp-bounds-checks branch from ba7ceab to 3d577c6 Compare April 10, 2026 05:15

nightness mentioned this pull request Apr 10, 2026

Summary: 19 PRs ready for review #87

Open

Uh oh!

Conversation

nightness commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Apr 4, 2026

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

nightness commented Apr 1, 2026 •

edited

Loading