sctp: don't discard received-but-undelivered data on incoming stream reset - #1
Open
lann wants to merge 43 commits into
Open
sctp: don't discard received-but-undelivered data on incoming stream reset#1lann wants to merge 43 commits into
lann wants to merge 43 commits into
Conversation
…address (webrtc-rs#136) Connectivity checks and data writes for a server-reflexive local candidate were tagged with the candidate's NAT-mapped address, which no local socket is bound to; drivers routing outbound transmits by transport.local_addr had to drop them, so ICE never connected when the srflx path was the only viable one. Per RFC 8445 sec 6.1.2, checks for a reflexive candidate must be sent from its base. Add Candidate::base_addr() (related address for srflx/prflx, addr() otherwise) and use it in Agent::send_stun and the peer connection ICE handler write path.
…s#140) `RTCDataChannelInit` derived `Default`, so `ordered` came out `false` — contradicting both the field's own doc comment ("The default value of `true` guarantees that data will be delivered in order") and the W3C dictionary, where `ordered` is defined as `= true`. `create_data_channel(label, None)` did not consult that default at all: it left `DataChannelParameters` on its own derived default, which is `false` too. Route `None` through `RTCDataChannelInit::default()` so the documented defaults have a single definition. An unordered channel is not merely out-of-order. Unordered chunks bypass SCTP's ordered-delivery queue, so a first message can overtake the `DATA_CHANNEL_OPEN` sent on the same stream; the peer receives user data on a stream it has not accepted yet, `RTCDataChannelInternal::accept` rejects it as a non-DCEP PPID, and that error is logged and discarded on the pipeline's read pass. The message is lost with no error reaching either side. Closes webrtc-rs#139
…tc-rs#138) `RTCDataChannel::send` checked only that the channel was registered, which it is from `create_data_channel` onwards. The condition that actually matters -- whether its SCTP stream exists -- was checked later, in `DataChannelHandler::handle_write`, and that runs on the pipeline's write pass where an `Err` is logged and discarded: if let Err(err) = handler.handle_write(msg) { warn!("{}.handle_write got error: {}", handler.name(), err); } So the caller was handed `Ok(())` for a message that was dropped on the floor, with only a stray warning to show for it. Sending on an already closed channel had the same shape. Check the real condition at the send boundary instead, where the error can still reach the caller: `ErrDataChannelNotOpen` (new variant -- `Error` is `#[non_exhaustive]`) while the channel is `connecting`, `ErrDataChannelClosed` once it is gone. Keeping them distinct matters: the first is worth retrying after the channel opens, the second never is. This also stops a rejected send from charging `outstanding_bytes`. Those bytes never entered the SCTP pipeline, so nothing would ever release them, and the leaked counter would permanently shrink the channel's send window. Note this errors where W3C `send()` prescribes buffering for a `connecting` channel. Buffering is the better contract and a much larger change; erroring is the smallest step that stops the silent data loss, and it is what the write path already decided -- it just could not say so.
…-rs#137) `SettingEngine::disable_certificate_fingerprint_verification` had a field and a setter but no reader: the flag was never passed to `RTCDtlsTransport`, so the handshake always installed the fingerprint-matching `verify_peer_certificate` callback and enabling the option had no observable effect. The neighbouring `allow_insecure_verification_algorithm` is plumbed through the exact same path, which makes the omission easy to spot side by side. Pass the flag down and build the callback only when the comparison is wanted. `with_verify_peer_certificate` takes an `Option`, and leaving it out is what disables the check — `insecure_skip_verify` is already true, so this callback is the only thing standing between the peer's certificate and acceptance. It does not weaken the "a certificate must be presented" requirement: `client_auth` is `RequireAnyClientCert`, which the DTLS layer enforces on its own (flight4 rejects an empty `peer_certificates` with `ErrClientCertificateRequired` before the callback would run). This is required by protocols where the answerer cannot know the offerer's fingerprint ahead of time. libp2p's WebRTC-Direct is the canonical case: the server synthesizes the client's offer locally with a placeholder fingerprint and authenticates the peer afterwards with a Noise handshake over the data channel. Without this, the DTLS handshake fails with ErrNoMatchingCertificateFingerprint. Adds an integration test covering both directions — a mismatched fingerprint connects with the option enabled, and still fails with it left at the default.
… the peer connection can have one concrete type
…tcp-processing-boxed to demonstrate how to use RTCPeerConnection<BoxedInterceptor> directly
…default set offers ECDHE_RSA suites incompatible with an ECDSA certificate (handshake stalls)
…-tuple → 437 (Allocation Mismatch) #835
…alidate` (webrtc-rs#141) * Allow RSA key for DTLS * Cleanup * Move related peer connection functionality out of existing DTLS test * Add integration test for RSA dtls keys * Prevent unused warning
[Pre-1.0] G3 — Crypto provider unification webrtc-rs#128
P8 — Integrate with the async webrtc repository
… MessageIntegrity<'a> (webrtc-rs#145) * remove-public-API-crypto_provider-from-RTCPeerConnection * refactor MessageIntegrity<'a> * fix compiler errors * refactor rtc-srtp * fix cargo build --workspace --all-targets --features crypto-ring,crypto-aws-lc-rs
optimize poll_timeout
…reset When a peer sends messages and then immediately resets the stream (close-after-send), the DATA chunks and the outgoing-stream-reset RECONFIG can be processed in the same input batch. reset_streams_if_any would unregister the stream outright, dropping the reassembly queue with received-but-unread messages still inside, so the application saw the channel close with only a prefix of the messages delivered. Per RFC 6525, data received before the reset must still be delivered to the upper layer. Defer the stream teardown when the reassembly queue is still readable: keep the stream registered in read-only state (marked reset_pending), refuse new inbound DATA for it, and complete unregister_stream (emitting AssociationLost/Reset) once the consumer drains the queue via read_sctp. The reset is still answered "Success - Performed" as before.
lann
force-pushed
the
sctp-reset-undelivered-data
branch
from
August 6, 2026 11:41
89f204f to
89f518e
Compare
This was referenced Aug 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When a peer sends N messages on a stream and then immediately resets it (the RFC 8831 §6.7 close-after-send pattern), the tail DATA chunk(s) and the outgoing-stream-reset RECONFIG are often processed by the receiver in the same input batch.
reset_streams_if_anycalledunregister_streamoutright, removing the stream and its reassembly queue with received-but-unread messages still inside. The application intermittently saw the channel close with only a prefix of the messages delivered (receive-side silent data loss, timing-dependent).Per RFC 6525, data received before the reset must still be delivered to the upper layer.
Full analysis:
SCTP-RESET-DISCARDS-UNDELIVERED-DATA.md(observed ~1/3 failure rate on thechannel-close-flushconformance test against a libwebrtc sender). Confirmed still present at upstream webrtc-rs/rtc HEAD (a10cd2c).Fix
In
reset_streams_if_any, when the in-order condition holds but the stream's reassembly queue is still readable:reset_pending, keep it registered in read-only state (RecvSendState::Readable), and queue aStreamEvent::Readableso the consumer drains it;handle_data;unregister_stream(emittingAssociationLost { reason: Reset }) fromread_sctponce the queue is drained.The reset is still answered "Success - Performed" as before — only local delivery is deferred.
Testing
New regression test
test_assoc_reset_defers_teardown_until_undelivered_data_readreproduces the exact failure deterministically (4 writes +stop()in one batch; pre-fix it fails withErrStreamNotExistedbefore reading message 0) and verifies all messages are delivered, the stream is torn down after draining, andAssociationLost(Reset)is emitted.cargo test -p rtc-sctp: 118 passedcargo clippy -p rtc-sctp --all-targets: no new warnings2026-08-06: rebased onto upstream
webrtc-rs/rtcmaster (afcc19f, post-0.21 crypto migration); still applies cleanly — upstream v0.20.0 and master both retain the unconditionalunregister_streamon incoming reset.cargo test -p rtc-sctp(121 tests + the new regression test), fmt, and clippy pass on the rebased branch.