RTMP: Fix chunk timestamp/basic-header decoding and harden packet unmarshal. v8.0.3#4680
Merged
Conversation
…oxy. Port the C++ srs_protocol_rtmp_stack.cpp fix (ossrs#4356) to the Go proxy's RTMP parser in internal/rtmp. For chunk fmt=1/2 the extended timestamp encodes a timestamp delta, not an absolute timestamp. The parser previously assigned the extended value to the message timestamp unconditionally, so once a delta reached 0xffffff the DTS was miscomputed, and since audio and video deltas differ this could cause A/V desync. Changes: - Split chunkStream's single ext-ts bool into hasExtendedTimestamp (presence) plus a raw extendedTimestamp uint32, mirroring the C++ has_extended_timestamp_ / extended_timestamp_ fields. - Compute the message timestamp once: extended value when present, else the 3-byte header delta; assign it as absolute for fmt=0 and accumulate it as a delta for fmt=1/2 (and a fmt=3 first chunk). - Resolve the 'detect the extended timestamp' TODO: peek the 4 bytes and leave them as payload when a librtmp/ffmpeg-style sender omits the ext-ts on a Type-3 chunk (Go equivalent of the C++ skip(-4)). - Add unit tests for the fmt=1 delta-crossing-0xffffff case and the Type-3 omitted-ext-ts case. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
readBasicHeader overwrote cid with the 2-byte form (64 + byte2) before testing whether the 3-byte form was in use, so the `cid == 1` check could never be true and the 3-byte branch was dead code. Chunk basic headers with marker == 1 (chunk stream IDs 320-65599) consumed only one of the two trailing bytes, leaving the high-order byte in the stream and desyncing the chunk parser. Keep the original marker before cid is overwritten and branch on it, matching the C++ reference (srs_protocol_rtmp_stack.cpp, read_basic_header). The arithmetic inside the branch was already correct. Also correct the unit test, which had encoded the buggy result (expected cid=65 instead of 577, leaving a byte unread); it now guards the 3-byte path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Implements the Go RTMP stack unit-test plan (internal/rtmp), cross-referenced to the C++ srs_utest_manual_protocol/protocol2/rtmp suites, and fixes the one panic class the fuzz targets exposed. Tests added (rtmp_test.go): - TestReadMessageInterleavedMultiStream interleaved multi-cid reassembly - TestReadMessageLargeChunkStreamID 2-/3-byte cid full-message read - TestProtocolWritePacketReadMessageRoundTrip encoder<->decoder, 14 pkts x 4 sizes - TestReadMessageTimestampDiscontinuity backward/forward jump, 31-bit wrap - TestReadWriteLargePayloadChunkBoundaries chunk-boundary stress - TestGoldenWireBytes golden bytes for headers + controls - FuzzReadMessage / FuzzDecodeMessage / FuzzPacketUnmarshal untrusted-input fuzz - TestPacketUnmarshalAdversarialInputs resource-safety / truncation - TestProtocolTransactionMapConcurrency -race on the transaction map Hardening (rtmp.go): the fuzz targets found that the variantCallPacket family counted a stale optional-field default (CommandObject/Args pre-set to Null by a New*Packet constructor) when the wire data was exhausted, so Size() overran the caller's p = p[Size():] advance and panicked on truncated, untrusted input. Reset those fields to nil before the presence check, and route the embedded advances in CallPacket/CreateStreamResPacket/PublishPacket/PlayPacket through a bounds-checked advanceBytes helper so any future Size()/consumed mismatch becomes a clean error instead of a slice-out-of-range panic. Verified: full proxy unit suite passes; TestProtocolTransactionMapConcurrency clean under go test -race -count=3; all proxy E2E scripts pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The "Run the proxy E2E tests:" wording read as if running the first script was enough. Make explicit that every E2E script must run, one at a time (fixed ports prevent parallelism), and not to stop early since a later test can fail even when earlier ones pass. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
winlinvip
changed the title
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.
Fixes three RTMP chunk-stream decoding bugs in the proxy and hardens AMF0 command-packet unmarshalling against malformed input, backed by a new protocol unit-test suite.
All changes are confined to the
internal/rtmppackage. No public API, log format, or emitted wire format changes — these are decode-correctness and robustness fixes only.**3-byte chunk basic header decode (
readBasicHeader) **The 3-byte basic-header form (cid 64–65599) was selected by testing
cid == 1aftercidhad already been overwritten with64 + t, so it was never detected. Capture the original marker before overwriting and test that instead.Extended-timestamp handling (
chunkStream,readMessageHeader)0xffffff. Timestamp computation is unified into a single post-step: extended timestamp when present, otherwise the 3-byte header delta; fmt=0 absolute, fmt=1/2 accumulated.Peek+ conditionalDiscard: if the peeked value differs from the stored one on a non-first chunk, those 4 bytes are payload and are left in the reader.extendedTimestampbool intohasExtendedTimestamp(bool) andextendedTimestamp(the last raw value, used for the detection above).Packet unmarshal hardening
advanceBytes(p, n)helper that bounds-checks eachp = p[field.Size():]advance, turning a slice-out-of-range panic into a clean error on truncated/untrusted input. Applied inCallPacket,CreateStreamResPacket,PublishPacket, andPlayPacket.CommandObject/Argsto nil before probing for their presence, so a stale constructor default (e.g. Null) isn't counted bySize()and can't overflow a later advance.