fix(stream): drop stray datagrams instead of splicing them into the TS output - #704
Conversation
…S output A non-RTP datagram arriving on a media socket was forwarded to the client verbatim. The media sockets are unconnected, so anything can land in the same recv() as the media -- in practice a ZTE ZXV10STB punch reply, which rtp2httpd solicits on every 30s keepalive since #701. The 84-byte packet shifts the rest of the HTTP body off the 188-byte TS grid. ffmpeg-based players resync on the next sync byte and show nothing; the embedded web player (mpegts.js) only probes for TS alignment at the start of a stream, so it stalls on "loading" and re-breaks every 30s. Only bare MPEG-TS is legitimate on the non-RTP path (servers negotiating plain MP2T, raw TS multicast), so gate the pass-through on the existing stream_payload_is_mpegts() check and drop everything else. For comparison, rtsproxy discards the same packets in RtpPipeline::process() and additionally sends the punch only once, after SETUP. Keeping our periodic punch is deliberate: it maintains the media-path NAT mapping that issue #700's line needs.
Documentation previewThe documentation preview has been deployed for this pull request. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24680e91da
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * unconnected and accept from anyone. Splicing such a datagram into the | ||
| * output breaks the 188-byte TS alignment for the rest of the stream, | ||
| * which strict demuxers (mpegts.js) never recover from. */ | ||
| if (!stream_payload_is_mpegts(payload, payload_len)) { |
There was a problem hiding this comment.
When a stray datagram is 189–375 bytes long and happens to start with 0x47, stream_payload_is_mpegts() returns true because it checks only offset 0 and never verifies that payload_len is divisible by 188. This branch then forwards the entire datagram, so an unrelated sender can still shift every subsequent TS packet off the 188-byte grid that this change intends to preserve. Use a stricter validation here, including an integral TS-packet length, before passing the buffer through.
Useful? React with 👍 / 👎.
1 participant
Summary
A non-RTP datagram arriving on a media socket was forwarded to the client verbatim. The media sockets are unconnected, so anything can land in the same
recv()as the media — in practice a ZTEZXV10STBpunch reply, which rtp2httpd solicits on every 30s keepalive since #701.The 84-byte packet shifts the rest of the HTTP body off the 188-byte TS grid. ffmpeg-based players (aptv, VLC) resync on the next sync byte and show nothing; the embedded web player (mpegts.js) only probes for TS alignment at the start of a stream, so it stalls on "loading" and re-breaks every 30 seconds.
Only bare MPEG-TS is legitimate on the non-RTP path (servers negotiating plain MP2T, raw TS multicast), so the pass-through is now gated on the existing
stream_payload_is_mpegts()check and everything else is dropped with a debug log.Why the reporter saw what they saw
Feedback on #701: the web player stalls ~30s in while other clients are fine, and routing through rtsproxy fixes it. All three observations fall out of this bug:
RTSP_KEEPALIVE_INTERVAL_MS, when the punch is re-sent.RtpPipeline::process()(src/protocol/rtp_pipeline.cpp:19), and it only punches once after SETUP — the per-keepalive resend is commented out in both of its clients (src/clients/rtsp_to_rtsp_client.cpp:977,src/clients/rtsp_to_http_client.cpp:177). Its 20s vs our 30s keepalive period is not a factor.The periodic punch is deliberately kept: it maintains the media-path NAT mapping that #700's line needs. rtsproxy dropping it looks like a workaround for this same corruption rather than a protocol requirement.
One caveat worth stating plainly: the origin of the stray datagram is inferred from code on both sides, not from a capture. This fix closes the injection path, so it holds regardless of the source — but if the reporter still sees a 30s cadence afterwards, the new
-v 4log line (Stream: Dropped N-byte datagram that is neither RTP nor MPEG-TS) plus atcpdump -i any -n udp port <client_rtp_port> -Xwill show what is really arriving.Test plan
New
test_probe_echo_never_reaches_the_clientdrives the real failure:MockRTSPServerZTEgains anecho_probe_afteroption that bounces the received 84-byte punch packet back onto the media port mid-stream, the way a ZTE server acks it. The test asserts the body contains noZXV10STBbytes, that every 188-byte boundary still holds a0x47sync byte, and that the drop is logged.Verified non-vacuous: with
src/stream.creverted and the binary rebuilt, it fails onassert b'ZXV10STB' not in body— the punch bytes really are relayed into the TS stream today.uv run ruff check e2e: cleanRefs #700.
🤖 Generated with Claude Code