fix(sctp): slice each chunk to its own length in Packet::unmarshal by mikekap · Pull Request #89 · webrtc-rs/rtc

mikekap · 2026-04-17T23:08:49Z

Summary

Several chunk unmarshals (FORWARD-TSN, ABORT, ERROR, SHUTDOWN) bound their inner parse loops by buf.len(), which is only correct when the chunk is alone in the packet. Any trailing chunk pushes the loop past the chunk boundary, typically producing ErrChunkTooShort / ErrErrorCauseTooSmall / ErrInvalidChunkSize and dropping the whole packet. RFC 4960 §6.10 allows bundling every chunk except INIT, INIT ACK, and SHUTDOWN COMPLETE, so this is legal traffic.

The case that prompted this is PR-SCTP FORWARD-TSN advancing over purely-unordered abandoned DATA. RFC 3758 §3.2 says stream/ssn pairs MUST NOT be reported for unordered TSNs, so the chunk is the minimum 8 bytes with no pairs. If it's coalesced with the retransmitted DATA that follows it (common), the receiver silently drops every such packet and the peer retransmits until it happens to send one without a leading FORWARD-TSN.

Every SCTP chunk shares the same 4-byte header (Type | Flags | Length; RFC 4960 §3.2), so this change has Packet::unmarshal and PartialDecode::finish read the Length field themselves and hand each chunk-specific unmarshal a buffer sized exactly to that chunk. No per-chunk source changes — each parser now receives the precondition (buf.len() == chunk_length) it already assumed.

Test plan

Added test_unmarshal_variable_length_chunks_followed_by_other_chunks as a regression test.

Several chunk unmarshals (FORWARD-TSN, ABORT, ERROR, SHUTDOWN) bounded their inner parse loops by `buf.len()`, which was only correct when the chunk was alone in the packet. Any trailing chunk pushed the loop past the chunk boundary, typically producing ErrChunkTooShort / ErrErrorCauseTooSmall / ErrInvalidChunkSize and dropping the whole packet. RFC 4960 §6.10 allows bundling every chunk except INIT, INIT ACK, and SHUTDOWN COMPLETE, so this is legal traffic. This shows up in the wild with PR-SCTP FORWARD-TSN advancing over purely-unordered abandoned DATA: RFC 3758 §3.2 requires stream/ssn pairs not be reported for unordered TSNs, so the chunk is the minimum 8 bytes with no pairs. If it's coalesced with the retransmitted DATA that follows, the receiver silently drops every such packet and the peer retransmits until it happens to send one without a leading FORWARD-TSN. This also adds a regression test bundling FORWARD-TSN (no streams) + ABORT + ERROR + SHUTDOWN + DATA in one packet and asserting all five chunks round-trip.

Several chunk unmarshals (FORWARD-TSN, ABORT, ERROR, SHUTDOWN) bounded their inner parse loops by `buf.len()`, which was only correct when the chunk was alone in the packet. Any trailing chunk pushed the loop past the chunk boundary, typically producing ErrChunkTooShort / ErrErrorCauseTooSmall / ErrInvalidChunkSize and dropping the whole packet. RFC 4960 §6.10 allows bundling every chunk except INIT, INIT ACK, and SHUTDOWN COMPLETE, so this is legal traffic. This shows up in the wild with PR-SCTP FORWARD-TSN advancing over purely-unordered abandoned DATA: RFC 3758 §3.2 requires stream/ssn pairs not be reported for unordered TSNs, so the chunk is the minimum 8 bytes with no pairs. If it's coalesced with the retransmitted DATA that follows, the receiver silently drops every such packet and the peer retransmits until it happens to send one without a leading FORWARD-TSN. This also adds a regression test bundling FORWARD-TSN (no streams) + ABORT + ERROR + SHUTDOWN + DATA in one packet and asserting all five chunks round-trip. Backport of webrtc-rs/rtc#89.

rainliu

Thank you for this fix

codecov · 2026-04-18T17:36:53Z

Codecov Report

❌ Patch coverage is 72.50000% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.22%. Comparing base (38578a8) to head (49e63ae).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
rtc-sctp/src/packet.rs	72.50%	11 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #89      +/-   ##
==========================================
+ Coverage   71.21%   71.22%   +0.01%     
==========================================
  Files         442      442              
  Lines       67352    67356       +4     
==========================================
+ Hits        47963    47976      +13     
+ Misses      19389    19380       -9

☔ 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.

Copilot

Pull request overview

Fixes SCTP packet/chunk parsing when multiple chunks are bundled by ensuring each chunk-specific unmarshal only sees bytes up to that chunk’s declared Length (RFC 4960 §3.2), preventing variable-length chunk parsers from accidentally reading into subsequent chunks and failing the whole packet.

Changes:

Added slice_chunk() helper to compute and slice each chunk’s buffer to its header-declared length.
Updated PartialDecode::finish and Packet::unmarshal to pass per-chunk slices into chunk-specific unmarshal implementations.
Added a regression test covering variable-length chunks followed by additional chunks.

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

Copilot · 2026-04-18T17:37:03Z

+    if offset + chunk_length > raw.len() {
+        return Err(Error::ErrChunkHeaderNotEnoughSpace);
+    }
+    Ok(raw.slice(offset..offset + chunk_length))


slice_chunk reads the length field directly and slices out the chunk, but it no longer validates the 0–3 bytes of SCTP chunk padding that may follow the chunk on the wire (previously, ChunkHeader::unmarshal could validate non-zero padding when the buffer included chunk+padding). Consider explicitly checking that the padding bytes between offset + chunk_length and the next 4-byte boundary are zero (when present), or reuse ChunkHeader::unmarshal for this validation.

Suggested change

if offset + chunk_length > raw.len() {

return Err(Error::ErrChunkHeaderNotEnoughSpace);

}

Ok(raw.slice(offset..offset + chunk_length))

let chunk_end = offset + chunk_length;

if chunk_end > raw.len() {

return Err(Error::ErrChunkHeaderNotEnoughSpace);

}

let padded_end = (chunk_end + 3) & !0x03;

if padded_end > raw.len() {

return Err(Error::ErrChunkHeaderNotEnoughSpace);

}

if raw[chunk_end..padded_end].iter().any(|&b| b != 0) {

return Err(Error::ErrChunkHeaderInvalidLength);

}

Ok(raw.slice(offset..chunk_end))

Copilot · 2026-04-18T17:37:03Z

+                Box::new(ChunkAbort {
+                    error_causes: vec![ErrorCause {
+                        code: PROTOCOL_VIOLATION,
+                        ..Default::default()
+                    }],
+                }),
+                Box::new(ChunkError {
+                    error_causes: vec![ErrorCause {
+                        code: UNRECOGNIZED_CHUNK_TYPE,
+                        raw: Bytes::from_static(&[0xc0, 0x00, 0x00, 0x08, 0x00, 0x00, 0x00, 0x03]),
+                    }],
+                }),
+                Box::new(ChunkShutdown {
+                    cumulative_tsn_ack: 0x12345678,
+                }),
+                Box::new(ChunkPayloadData {
+                    unordered: true,
+                    beginning_fragment: true,
+                    ending_fragment: true,
+                    tsn: 4,
+                    stream_identifier: 1,
+                    payload_type: PayloadProtocolIdentifier::Binary,
+                    user_data: Bytes::from_static(&[0xaa, 0xbb, 0xcc, 0xdd]),
+                    ..Default::default()
+                }),
+            ],


The regression test constructs a packet that bundles DATA with ABORT and also places chunks after ABORT. Per the ABORT chunk rules (and this crate’s ABORT docs), DATA must not be bundled with ABORT and ABORT is expected to be last (later chunks may be ignored). To keep the test representative of legal traffic and future-proof against stricter validation, consider using a valid bundle (e.g., FORWARD-TSN/ERROR/SHUTDOWN followed by DATA, and either omit ABORT or place ABORT last without DATA).

Copilot · 2026-04-18T17:37:04Z

+            let chunk_buf = slice_chunk(&self.remaining, offset)?;
            let ct = ChunkType(self.remaining[offset]);
            let c: Box<dyn Chunk> = match ct {
-                CT_INIT => Box::new(ChunkInit::unmarshal(&self.remaining.slice(offset..))?),
-                CT_INIT_ACK => Box::new(ChunkInit::unmarshal(&self.remaining.slice(offset..))?),
-                CT_ABORT => Box::new(ChunkAbort::unmarshal(&self.remaining.slice(offset..))?),
-                CT_COOKIE_ECHO => {
-                    Box::new(ChunkCookieEcho::unmarshal(&self.remaining.slice(offset..))?)
-                }
-                CT_COOKIE_ACK => {
-                    Box::new(ChunkCookieAck::unmarshal(&self.remaining.slice(offset..))?)
-                }
-                CT_HEARTBEAT => {
-                    Box::new(ChunkHeartbeat::unmarshal(&self.remaining.slice(offset..))?)
-                }
-                CT_PAYLOAD_DATA => Box::new(ChunkPayloadData::unmarshal(
-                    &self.remaining.slice(offset..),
-                )?),
-                CT_SACK => Box::new(ChunkSelectiveAck::unmarshal(
-                    &self.remaining.slice(offset..),
-                )?),
-                CT_RECONFIG => Box::new(ChunkReconfig::unmarshal(&self.remaining.slice(offset..))?),
-                CT_FORWARD_TSN => {
-                    Box::new(ChunkForwardTsn::unmarshal(&self.remaining.slice(offset..))?)
-                }
-                CT_ERROR => Box::new(ChunkError::unmarshal(&self.remaining.slice(offset..))?),
-                CT_SHUTDOWN => Box::new(ChunkShutdown::unmarshal(&self.remaining.slice(offset..))?),
-                CT_SHUTDOWN_ACK => Box::new(ChunkShutdownAck::unmarshal(
-                    &self.remaining.slice(offset..),
-                )?),
-                CT_SHUTDOWN_COMPLETE => Box::new(ChunkShutdownComplete::unmarshal(
-                    &self.remaining.slice(offset..),
-                )?),
+                CT_INIT => Box::new(ChunkInit::unmarshal(&chunk_buf)?),
+                CT_INIT_ACK => Box::new(ChunkInit::unmarshal(&chunk_buf)?),


Now that chunk_buf is sliced to the chunk header’s declared length, it’s safer to advance the loop using that same on-the-wire length (chunk_buf.len() + padding) rather than c.value_length(). Some chunk unmarshals can successfully return while leaving 1–3 trailing bytes unaccounted for, which would make value_length() smaller than the declared length and desynchronize parsing of subsequent chunks.

Copilot · 2026-04-18T17:37:04Z

+            let chunk_buf = slice_chunk(raw, offset)?;
            let ct = ChunkType(raw[offset]);
            let c: Box<dyn Chunk> = match ct {
-                CT_INIT => Box::new(ChunkInit::unmarshal(&raw.slice(offset..))?),
-                CT_INIT_ACK => Box::new(ChunkInit::unmarshal(&raw.slice(offset..))?),
-                CT_ABORT => Box::new(ChunkAbort::unmarshal(&raw.slice(offset..))?),
-                CT_COOKIE_ECHO => Box::new(ChunkCookieEcho::unmarshal(&raw.slice(offset..))?),
-                CT_COOKIE_ACK => Box::new(ChunkCookieAck::unmarshal(&raw.slice(offset..))?),
-                CT_HEARTBEAT => Box::new(ChunkHeartbeat::unmarshal(&raw.slice(offset..))?),
-                CT_PAYLOAD_DATA => Box::new(ChunkPayloadData::unmarshal(&raw.slice(offset..))?),
-                CT_SACK => Box::new(ChunkSelectiveAck::unmarshal(&raw.slice(offset..))?),
-                CT_RECONFIG => Box::new(ChunkReconfig::unmarshal(&raw.slice(offset..))?),
-                CT_FORWARD_TSN => Box::new(ChunkForwardTsn::unmarshal(&raw.slice(offset..))?),
-                CT_ERROR => Box::new(ChunkError::unmarshal(&raw.slice(offset..))?),
-                CT_SHUTDOWN => Box::new(ChunkShutdown::unmarshal(&raw.slice(offset..))?),
-                CT_SHUTDOWN_ACK => Box::new(ChunkShutdownAck::unmarshal(&raw.slice(offset..))?),
-                CT_SHUTDOWN_COMPLETE => {
-                    Box::new(ChunkShutdownComplete::unmarshal(&raw.slice(offset..))?)
-                }
+                CT_INIT => Box::new(ChunkInit::unmarshal(&chunk_buf)?),
+                CT_INIT_ACK => Box::new(ChunkInit::unmarshal(&chunk_buf)?),
+                CT_ABORT => Box::new(ChunkAbort::unmarshal(&chunk_buf)?),


Consider advancing offset based on chunk_buf.len() (plus padding) instead of c.value_length(). If a chunk unmarshal returns successfully without accounting for every byte within the declared chunk length, using value_length() here can leave bytes behind and cause the next iteration to start mid-chunk.

mikekap force-pushed the fix/forward-tsn-trailing-chunks branch 2 times, most recently from 53d6ef6 to cf0cf34 Compare April 17, 2026 23:51

mikekap changed the title ~~fix(sctp): parse FORWARD-TSN followed by other chunks in same packet~~ fix(sctp): slice each chunk to its own length in Packet::unmarshal Apr 17, 2026

mikekap marked this pull request as ready for review April 17, 2026 23:51

mikekap force-pushed the fix/forward-tsn-trailing-chunks branch from cf0cf34 to 49e63ae Compare April 17, 2026 23:53

rainliu requested a review from Copilot April 18, 2026 17:31

Copilot started reviewing on behalf of rainliu April 18, 2026 17:31 View session

rainliu approved these changes Apr 18, 2026

View reviewed changes

Copilot AI reviewed Apr 18, 2026

View reviewed changes

rainliu merged commit ca0d153 into webrtc-rs:master Apr 18, 2026
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(sctp): slice each chunk to its own length in Packet::unmarshal#89

fix(sctp): slice each chunk to its own length in Packet::unmarshal#89
rainliu merged 1 commit into
webrtc-rs:masterfrom
mikekap:fix/forward-tsn-trailing-chunks

mikekap commented Apr 17, 2026 •

edited

Loading

Uh oh!

rainliu left a comment

Uh oh!

codecov Bot commented Apr 18, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 18, 2026

Uh oh!

Copilot AI Apr 18, 2026

Uh oh!

Copilot AI Apr 18, 2026

Uh oh!

Copilot AI Apr 18, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

-    if offset + chunk_length > raw.len() {
-        return Err(Error::ErrChunkHeaderNotEnoughSpace);
-    }
-    Ok(raw.slice(offset..offset + chunk_length))
+    let chunk_end = offset + chunk_length;
+    if chunk_end > raw.len() {
+        return Err(Error::ErrChunkHeaderNotEnoughSpace);
+    }
+    let padded_end = (chunk_end + 3) & !0x03;
+    if padded_end > raw.len() {
+        return Err(Error::ErrChunkHeaderNotEnoughSpace);
+    }
+    if raw[chunk_end..padded_end].iter().any(|&b| b != 0) {
+        return Err(Error::ErrChunkHeaderInvalidLength);
+    }
+    Ok(raw.slice(offset..chunk_end))

Uh oh!

Conversation

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

Uh oh!

Summary

Test plan

Uh oh!

rainliu left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 18, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mikekap commented Apr 17, 2026 •

edited

Loading

codecov Bot commented Apr 18, 2026 •

edited

Loading