Skip to content

DataChannel: reject oversized data messages before they break the channel - #1031

Merged
pblazej merged 2 commits into
mainfrom
blaze/dc-limits
Jun 9, 2026
Merged

DataChannel: reject oversized data messages before they break the channel#1031
pblazej merged 2 commits into
mainfrom
blaze/dc-limits

Conversation

@pblazej

@pblazej pblazej commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Summary

Sending more than the negotiated SCTP max-message-size makes libwebrtc return success from sendData and then abruptly tear down the publisher data channel — every subsequent publish fails until a full reconnect.

  • Parse a=max-message-size (RFC 8841) from the publisher answer SDP, clamp to a 64000-byte default.
  • Reject in DataChannelPair.makeRequest after serialization (encoded byte count = what goes on the wire). 0 means "no limit".
  • Log an error when the publisher channel transitions to .closed without an SDK-initiated reset.

Ports the fix from livekit/rust-sdks#1137. Android does the simpler hardcoded-constant variant at LocalParticipant.kt:984-986.

Test plan

  • Unit tests: parameterized SDP parser (6 cases) + size-guard tests (oversizedSendRejectedWithInvalidParameter, zeroLimitDisablesTheSizeGuard)
  • xcodebuild build macOS clean
  • SwiftLint clean
  • CI matrix

@xianshijing-lk xianshijing-lk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some questions / nits

Comment thread Sources/LiveKit/Core/DataChannelPair.swift
Comment thread Sources/LiveKit/Core/Room+SignalClientDelegate.swift
Comment thread Sources/LiveKit/Core/DataChannelPair.swift
Base automatically changed from blaze/concurrent-dc-sends to main June 8, 2026 08:40
…nnel

Sending more than the negotiated SCTP max-message-size makes libwebrtc
return success from `sendData` and then abruptly tear down the publisher
data channel; every subsequent publish then fails until the next full
reconnect. Mirrors the fix from livekit/rust-sdks#1137.

Parse `a=max-message-size` (RFC 8841) from the publisher answer SDP in
`Room+SignalClientDelegate.didReceiveAnswer`, clamp to a 64000-byte
default, and enforce the cap in `DataChannelPair.makeRequest` after
serialization — the encoded byte count is what actually goes on the
wire. A value of 0 means "no limit". Also logs an error when the
publisher channel transitions to `.closed` without an SDK-initiated
reset, surfacing the remaining failure modes for diagnosis.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@pblazej
pblazej force-pushed the blaze/dc-limits branch from d2991e2 to cc51fbf Compare June 8, 2026 08:58
@pblazej

pblazej commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

@xianshijing-lk the questions refer to the contract defined in livekit/rust-sdks#1137 rather than implementation details, so I'm delaying this PR until the above is merged.

@pblazej
pblazej marked this pull request as draft June 8, 2026 09:08
@pblazej
pblazej marked this pull request as ready for review June 9, 2026 08:08
@pblazej

pblazej commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

Confirmed parity with merged livekit/rust-sdks#1137 — 64000-byte default, clamped to that ceiling when the answer SDP advertises more (e.g. libwebrtc's 256 KiB), 0 honored as RFC 8841 "no limit", and unexpected-close logged at error level. The only diff is N/A for Swift: the Rust SDK now also wires the close listener for its third _data_track channel, which doesn't exist here (yet).

@pblazej
pblazej requested a review from 1egoman June 9, 2026 08:10

@xianshijing-lk xianshijing-lk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@pblazej
pblazej merged commit 90f4424 into main Jun 9, 2026
38 of 39 checks passed
@pblazej
pblazej deleted the blaze/dc-limits branch June 9, 2026 09:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants