Close the review's protocol-parity and input-correctness findings - #28
Merged
Conversation
The Rust reassembler is a hand-written mirror of the Swift one and names that file as its specification, but it had drifted in three ways that all made the host-side tests validate a receiver nobody runs. A fragment whose count disagreed with the live frame's rebuilt that frame, discarding everything already buffered for it; Swift keeps the first-seen count and ignores the odd fragment, so one late duplicate could throw away nine tenths of a keyframe. A replayed fragment overwrote good bytes and counted as another receipt. And frags_lost counted the fragments that DID arrive rather than the ones that did not, so a frame missing one of ten reported nine lost — the end-to-end test fed the ABR controller roughly nine times the real loss and passed, while a real iPad barely moved it. All three now match Swift, with tests for each; the test that locked in the rebuild behavior is replaced by one that proves progress survives. Control messages were authorized by source IP alone: the parsed header, which carries the session id, was discarded. An IP is shared by every process on the device and by everything behind the same NAT, and is trivially spoofed on the local link, so a stray INPUT_EVENT clicked the mouse, a stray RECEIVER_REPORT steered the bitrate, and a late BYE from a superseded session tore down the one that replaced it. Every message except HELLO2 now has to carry the id the handshake minted. The client already did this in the other direction. The client's BYE never went out. sendBye enqueued three copies on the control queue and the caller cancelled the socket on the next line, so even the zero-delay copy was still waiting its turn. The host saw nothing, streamed at a dead peer for the full liveness timeout, held the virtual display up for the same three seconds, and answered an immediate reconnect with "busy". The first copy is now sent synchronously. A drag that ended on a letterbox bar released at the middle of the desktop, dropping whatever was being dragged there, because an out-of-video release has no coordinate and fell back to screen centre. It now releases at the last point that was inside the video. Finally, the input deduper remembered exactly one edge id, but the client sends each press and release twice on purpose and Wi-Fi reorders freely: began(N), ended(N+1), began(N) walked the single slot and let the replayed press through as a second click. It remembers the last eight.
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.
The last batch from the
/code-reviewpass.The Rust reassembler had drifted from its own specification
proto/src/reassembly.rsis a hand-written mirror ofFrameAssembler.swiftand says so in its header — "if behavior here diverges, the Swift side is the specification". It had diverged three ways, and since the E2E fake receiver uses the Rust copy, the tests were validating a receiver nobody ships:frags_lostwas inverted — it counted the fragments that did arrive. A frame missing one of ten reported nine lost, so the E2E fed the ABR controller ~9× the real loss and passed, while a real iPad barely moved it.All three now match Swift, each with a test. The test that locked in the rebuild behavior is replaced by one proving progress survives.
Control messages were authorized by source IP alone
The parsed header — which carries the session id — was discarded (
Ok((_, message))). An IP is shared by every process on the device and everything behind the same NAT, and is trivially spoofed on the local link. So a strayINPUT_EVENTclicked the mouse, a strayRECEIVER_REPORTsteered the bitrate, and a lateBYEfrom a superseded session tore down the one that replaced it. Every message except HELLO2 now has to carry the id the handshake minted. (The Swift client already gated this way in the other direction.)The client's BYE never actually went out
sendByeenqueued three copies on the control queue; the caller cancels the socket on the next line, so even the zero-delay copy was still waiting its turn. The host saw no goodbye, streamed at a dead peer for the full 3 s liveness timeout, held the virtual display up just as long, and answered an immediate reconnect with "busy". The first copy is now synchronous.Two input fixes
A drag ending on a letterbox bar released at the middle of the desktop — dropping the dragged window there — because an out-of-video release has no coordinate and fell back to screen centre. It now releases at the last point inside the video.
The edge deduper remembered exactly one id, but the client sends each press/release twice on purpose and Wi-Fi reorders freely:
began(N), ended(N+1), began(N)walked the single slot and let the replayed press through as a second click. It now remembers the last eight.Gates
107 Rust tests (6 new), 68 iOS tests (1 new), zero clippy warnings, simulator E2E green at 180 frames / 60 fps.