Fix: one bad datagram could permanently freeze the video - #25
Merged
Conversation
A media fragment carrying a stream_epoch higher than the host will ever reach latched that epoch on the receiver. Every real fragment afterwards compared as stale and was dropped, forever — the seq-restart path re-saved the poisoned epoch, and nothing can exceed u32::MAX, so the only escape was a full reconnect. Nothing noticed it happen. Stale-epoch drops return before the loss counters, so the HUD showed 0% loss, and the liveness watchdog keys on control-plane heartbeats, which keep flowing from a perfectly healthy host. The user sees a frozen picture with every indicator green. Reachable from a single spoofed datagram by anyone on the same L2 (the session id rides in cleartext in every media header), or non-adversarially from a middlebox mangling the four epoch bytes of a legitimate packet. Both receivers now re-sync: after 512 CONSECUTIVE stale-epoch drops the held epoch is treated as bogus and the assembler adopts whatever is actually arriving. Genuine stragglers from a previous run can't trip it, because the new run's fragments interleave and are accepted, which resets the streak — the count only climbs when nothing at all is getting through, which is exactly the stuck state. Recovery costs well under a second at streaming rates instead of never. Regression tests on both sides cover the poison case and prove interleaved stragglers stay dropped. Also: the iPad parsed STREAM_CONFIG and threw it away (`case .streamConfig: break`), so the host's immediate notify on an ABR step or codec switch was dead traffic and the Settings readout lagged up to a heartbeat behind. Wired through to the published config. Found by /code-review over the merged revamp; both findings verified against the tree before fixing.
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
1 participant
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.
Two findings from the
/code-reviewpass over the merged revamp, both verified against the tree before fixing.The bug: epoch latch (real, and quiet)
A media fragment carrying a
stream_epochhigher than the host will ever reach latched that epoch on the receiver. Every real fragment afterwards compared as stale and was dropped forever — the seq-restart path re-saves the poisoned epoch, and nothing can exceedu32::MAX, so the only escape was a full reconnect.The nasty part is that nothing notices:
returnbefore the loss counters, so the HUD reports 0% loss.signalLostnever fires and auto-reconnect never triggers.The user sees a frozen picture with every indicator green. Reachable from a single spoofed datagram by anyone on the same L2 (the session id rides in cleartext in every media header, and the connected UDP flow only requires spoofing the host's IP:port), or non-adversarially from a middlebox mangling the four epoch bytes of a legitimate packet.
The fix
Both receivers (
proto/src/reassembly.rsand the SwiftFrameAssemblerit mirrors) now re-sync: after 512 consecutive stale-epoch drops, the held epoch is treated as bogus and the assembler adopts whatever is actually arriving.Why that threshold is safe: genuine stragglers from a previous run can't trip it, because the new run's fragments interleave and are accepted, which resets the streak. The count only climbs when nothing at all is getting through — exactly the stuck state. Recovery costs well under a second at streaming rates, instead of never.
Regression tests on both sides cover the poison case (which fails on the old code) and prove interleaved stragglers stay dropped across 2× the threshold.
Also: STREAM_CONFIG was parsed and discarded
case .streamConfig: break— so the host's immediate notify on an ABR step or codec switch was dead traffic, and the Settings readout lagged up to a heartbeat behind. Now wired through to the published config.Gates
103 Rust tests (2 new) with zero clippy warnings, 65 iOS tests (2 new), simulator E2E green at 180 frames / 61 fps.