security: gate RTMP relay until publish/play keys validated (bug scan: server)#31
Conversation
librtmp2 accepted publish/play commands and relayed audio/video before librtmp2-server could validate keys in its poll loop. A holder of a viewer play_key could publish under that name and inject frames to other players until the connection was evicted. Vendor a patched librtmp2 with Conn::relay_enabled (default false) and enable relay only after DbRtmpBridge accepts the session. Co-authored-by: Alexander Wagner <info@alexanderwagnerdev.com>
…07-01) Co-authored-by: Alexander Wagner <info@alexanderwagnerdev.com>
📝 WalkthroughWalkthroughThis PR gates RTMP relaying on publish/play authorization in the host server and vendors a new ChangesApplication relay-state integration
Vendored librtmp2 protocol library
Estimated code review effort: 5 (Critical) | ~150 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ServerApp
participant Conn
participant DbRtmpBridge
Client->>ServerApp: publish/play request
ServerApp->>DbRtmpBridge: on_publish / on_play
DbRtmpBridge-->>ServerApp: authorization result
alt authorized
ServerApp->>Conn: relay_enabled = true
else rejected or closed
ServerApp->>Conn: relay_enabled = false
ServerApp->>Conn: pending_relay.clear()
ServerApp->>DbRtmpBridge: on_close
end
sequenceDiagram
participant ClientApp
participant Client
participant Server
participant Conn
ClientApp->>Client: connect / publish / play
Client->>Server: TCP handshake + AMF commands
Server->>Conn: recv / handle_command / send responses
Client->>Server: send_frame or poll
Server->>Conn: chunk_read / decode
Conn->>Server: queued relay frame
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
This PR addresses a security flaw in the RTMP relay path where media could be relayed before publish/play keys were validated by librtmp2-server. The fix introduces an explicit “relay gate” (Conn::relay_enabled) in the vendored librtmp2 server so relaying (including init-frame replay) only occurs after the host application authorizes the session.
Changes:
- Vendored a patched
librtmp2and switched the server crate to use the vendored path dependency. - Added/used
Conn::relay_enabledto gate both live relay and cached init-frame replay until authorization succeeds; disabled relay and cleared queued relay frames on unauthorized eviction. - Added loopback + interop test tooling in the vendored
librtmp2tree.
Reviewed changes
Copilot reviewed 4 out of 77 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| vendor/librtmp2/tests/server_client_loopback.rs | New end-to-end loopback publish smoke test over real TCP sockets. |
| vendor/librtmp2/tests/interop/play_interop.sh | New play/pull interop script (mediamtx + ffmpeg publisher + librtmp2 client). |
| vendor/librtmp2/tests/interop/ffmpeg_interop.sh | New ffmpeg publish interop script against librtmp2 ingest example. |
| vendor/librtmp2/tests/interop/enhanced_rtmp_interop.sh | New Enhanced-RTMP ingest interop script for HEVC/AV1 FourCC paths. |
| vendor/librtmp2/src/transport.rs | Adds plaintext/TLS transport abstraction used by server/client I/O paths. |
| vendor/librtmp2/src/session/stream.rs | Introduces Stream state container (but currently includes conflicting helper fns). |
| vendor/librtmp2/src/session/state_machine.rs | State transition helper and state-name mapping. |
| vendor/librtmp2/src/session/publish.rs | Publish-flow helper (currently incomplete regarding stream name). |
| vendor/librtmp2/src/session/play.rs | Play-flow helper (currently a no-op). |
| vendor/librtmp2/src/session/mod.rs | Session module wiring + re-exports. |
| vendor/librtmp2/src/server/mod.rs | Implements server poll loop, frame relay, init-frame replay, and relay gating via relay_enabled. |
| vendor/librtmp2/src/net.rs | Networking helper for parsing host/port authorities. |
| vendor/librtmp2/src/message/mod.rs | Message module wiring + re-exports. |
| vendor/librtmp2/src/message/message.rs | Message dispatch including aggregate decode and command dispatch. |
| vendor/librtmp2/src/message/control.rs | Control message encode/decode + tests. |
| vendor/librtmp2/src/message/command.rs | RTMP command encode/decode + bounds checks. |
| vendor/librtmp2/src/log.rs | Logging subsystem with optional callback. |
| vendor/librtmp2/src/lib.rs | Crate root + extern "C" FFI surface for server/client. |
| vendor/librtmp2/src/handshake.rs | RTMP handshake implementation + tests (contains a 24-bit time parse bug). |
| vendor/librtmp2/src/flv/video_tag.rs | FLV video tag parsing (rejects ExHeader so enhanced path can handle it). |
| vendor/librtmp2/src/flv/script_tag.rs | FLV script tag (metadata) parsing with type-marker handling. |
| vendor/librtmp2/src/flv/mod.rs | FLV module wiring + re-exports. |
| vendor/librtmp2/src/flv/audio_tag.rs | FLV audio tag parsing. |
| vendor/librtmp2/src/ertmp/reconnect.rs | Enhanced RTMP v2 reconnect parse/write. |
| vendor/librtmp2/src/ertmp/multitrack.rs | Enhanced RTMP v2 multitrack parse/write. |
| vendor/librtmp2/src/ertmp/modex.rs | Enhanced RTMP v2 ModEx parse/write with graceful degradation. |
| vendor/librtmp2/src/ertmp/mod.rs | E-RTMP module wiring + re-exports. |
| vendor/librtmp2/src/ertmp/metadata.rs | HDR/colorInfo metadata helpers + FourCC→videocodecid conversion. |
| vendor/librtmp2/src/ertmp/fourcc.rs | FourCC registry and codec mapping helpers. |
| vendor/librtmp2/src/ertmp/exvideo.rs | Enhanced RTMP v1 ExVideoTagHeader parsing. |
| vendor/librtmp2/src/ertmp/exaudio.rs | Enhanced RTMP v1 ExAudioTagHeader parsing. |
| vendor/librtmp2/src/ertmp/connect_caps.rs | Enhanced RTMP v1/v2 capability structures parse/write helpers. |
| vendor/librtmp2/src/chunk/writer.rs | RTMP chunk writer + tests. |
| vendor/librtmp2/src/chunk/state.rs | Per-connection chunk stream registry/state with memory bounds. |
| vendor/librtmp2/src/chunk/reader.rs | RTMP chunk reader with peek-first strategy + reassembly limits. |
| vendor/librtmp2/src/chunk/mod.rs | Chunk module wiring + re-exports. |
| vendor/librtmp2/src/bytes.rs | Endian/byte helpers + tests. |
| vendor/librtmp2/src/buffer.rs | Growable buffer implementation + tests. |
| vendor/librtmp2/src/amf/mod.rs | AMF module wiring + re-exports. |
| vendor/librtmp2/src/amf/amf3.rs | Minimal AMF3 encode/decode + tests. |
| vendor/librtmp2/src/alloc.rs | Custom allocator hook plumbing. |
| vendor/librtmp2/scripts/abi-baseline.sh | ABI dump/compare helper script for releases. |
| vendor/librtmp2/README.md | Vendored librtmp2 documentation. |
| vendor/librtmp2/LICENSE | Vendored librtmp2 license. |
| vendor/librtmp2/examples/play_pull.rs | Interop example: pull/play from real RTMP server. |
| vendor/librtmp2/examples/minimal_server.rs | Minimal server example wiring callbacks and polling. |
| vendor/librtmp2/examples/minimal_client.rs | Minimal client example: connect + publish + send frame. |
| vendor/librtmp2/examples/ffmpeg_ingest.rs | Interop ingest example: accept ffmpeg publisher and count frames. |
| vendor/librtmp2/docs/roadmap.md | Vendored roadmap/status docs. |
| vendor/librtmp2/docs/protocol-mapping-legacy.md | Protocol mapping documentation (legacy RTMP). |
| vendor/librtmp2/docs/protocol-mapping-ertmp-v2.md | Protocol mapping documentation (Enhanced RTMP v2). |
| vendor/librtmp2/docs/protocol-mapping-ertmp-v1.md | Protocol mapping documentation (Enhanced RTMP v1). |
| vendor/librtmp2/docs/architecture.md | Vendored architecture documentation. |
| vendor/librtmp2/docs/abi-policy.md | ABI policy documentation. |
| vendor/librtmp2/CONTRIBUTING.md | Contribution guidelines for vendored librtmp2. |
| vendor/librtmp2/concept/README.md | Vendored concept docs index. |
| vendor/librtmp2/CLAUDE.md | Vendored build/test guidance doc. |
| vendor/librtmp2/CHANGELOG.md | Vendored changelog. |
| vendor/librtmp2/Cargo.toml | Vendored crate manifest. |
| vendor/librtmp2/build.rs | Vendored build script. |
| vendor/librtmp2/.gitignore | Vendored gitignore. |
| vendor/librtmp2/.github/workflows/tests.yml | Vendored CI: cargo build/test + clippy. |
| vendor/librtmp2/.github/workflows/release.yml | Vendored release packaging workflow. |
| vendor/librtmp2/.github/workflows/interop-play.yml | Vendored CI workflow for play/pull interop test. |
| vendor/librtmp2/.github/workflows/interop-ffmpeg.yml | Vendored CI workflow for ffmpeg publish + enhanced ingest interop. |
| vendor/librtmp2/.github/workflows/abi-check.yml | Vendored semver/ABI check workflow (cargo-semver-checks). |
| vendor/librtmp2/.github/dependabot.yml | Vendored dependabot config. |
| src/server.rs | Enables relay only after on_publish/on_play succeeds; disables relay and clears queued frames on eviction. |
| Cargo.toml | Switches librtmp2 dependency from git to local vendored path. |
| Cargo.lock | Removes git source entry for librtmp2 due to vendoring. |
| .cursor/bug-scan-progress.md | Bug scan status update describing the relay-gating fix. |
| .cursor/bug-scan-config.md | Bug scan configuration file added. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: fc8d9f3f53
ℹ️ 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".
| if entry.stream_id.is_empty() { | ||
| entry.stream_id = rtmp_bridge.stream_id_for_conn(conn_id); | ||
| } | ||
| conn.relay_enabled = true; |
There was a problem hiding this comment.
When a client has only a valid play_key, on_play succeeds and this line sets the same relay_enabled bit that Conn::handle_message uses to accept inbound publisher audio/video. That client can then send a publish command plus media on the same RTMP connection; server.poll() queues and relays those frames while relay_enabled && current_stream.is_publishing before this outer loop gets a chance to call on_publish and reject the publish, so the play-key injection hole remains for clients that pipeline commands in one poll cycle. Use separate flags for receiving relayed frames vs. accepting publisher frames, or otherwise disable publishing on a play-authorized connection until publish validation succeeds.
Useful? React with 👍 / 👎.
| && conn | ||
| .current_stream | ||
| .as_ref() | ||
| .map(|s| s.is_playing && s.name == frame.stream_name) |
There was a problem hiding this comment.
When streams are used as documented, publishers send the publish_key as the RTMP stream name while players send the distinct play_key, but this relay match compares those raw command names. As a result an authorized player for the same DB stream never matches the publisher's frames, so normal publish/play with separate keys produces no media; after DbRtmpBridge resolves both keys, relay matching should use the canonical DB stream id or rewrite both connections to a shared stream identifier.
Useful? React with 👍 / 👎.
| match msg.msg_type_id { | ||
| msg_dispatch::RTMP_MSG_AMF0_COMMAND => self.handle_command(payload), |
There was a problem hiding this comment.
This dispatcher ignores RTMP control messages such as Set Chunk Size, even though message::decode has the code to update the connection's chunk registry. Common RTMP publishers send Set Chunk Size and then fragment later command/media messages at the larger size; this server keeps assuming 128-byte inbound chunks, so it starts interpreting payload bytes as continuation headers and corrupts or drops the stream. Handle RTMP_MSG_SET_CHUNK_SIZE here (or route through the existing decoder) before accepting publisher traffic.
Useful? React with 👍 / 👎.
| .unwrap_or(false); | ||
| if !is_player || conn.app != frame.app { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
For an authorized publisher on a stream with restrictive allowed_codecs, process_connections() drains pending_relay and sends each new frame to players here before the outer src/server.rs poll loop reads detected_*_codec and calls DbRtmpBridge::on_frame. That means the first frame or batch for a disallowed codec is already delivered and cached before the connection is kicked, so codec blocking is not actually enforced at the relay boundary; run the codec check before queuing/forwarding publisher frames or let the application veto frames synchronously.
Useful? React with 👍 / 👎.
| } | ||
| // Legacy audio codec ID in high nibble. | ||
| Some(match (payload[0] >> 4) & 0x0F { | ||
| 10 => "mp4a".to_string(), |
There was a problem hiding this comment.
Detecting normal AAC audio as mp4a interacts badly with the API default allowed_codecs (avc1,hvc1,av01 in src/http.rs): a default stream published from OBS/FFmpeg with audio will be accepted, then kicked as soon as this first AAC packet is classified because the server enforces the same allowlist for audio and video. Add mp4a/expected audio tokens to the default, or only enforce audio when the configured allowlist includes audio codecs.
Useful? React with 👍 / 👎.
| self.recv_buffer | ||
| .write(data) | ||
| .map_err(|_| ErrorCode::Internal)?; | ||
| self.bytes_received = self.bytes_received.wrapping_add(data.len() as u32); |
There was a problem hiding this comment.
For live publishers that send more than 4 GiB over one connection, this wrapping 32-bit counter rolls back to a small value before src/server.rs casts it to u64 for update_publisher_stats. The stats row then reports a decreasing/reset bytes_in and the bitrate calculation sees a zero delta after wrap, corrupting long-running stream metrics; keep a separate non-wrapping u64 total for application stats while preserving any 32-bit RTMP acknowledgement sequence separately.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/server.rs (2)
187-193: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUse a per-connection identity instead of raw
client_fd. If the OS reuses an fd before the oldtrackedentry is cleared, a new connection can inheritconnected/publishing/playing/stream_idstate and skip auth or close handling.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server.rs` around lines 187 - 193, The connection tracking in server.connections currently keys tracked state off raw client_fd, which can be reused by the OS and accidentally carry over connected/publishing/playing/stream_id state between different peers. Update the connection identity logic in the server loop to use a per-connection unique identity instead of conn.client_fd, and make sure the tracked map and current_ids cleanup/lookup use that stable identity wherever conn_id is derived.
203-228: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRe-authorize on stream changes.
TrackedConnonly latchespublishing/playingonce, butvendor/librtmp2/src/session/conn.rsoverwritescurrent_stream.nameon laterpublish/playcommands, so a client can switch streams without hittingon_publish/on_playagain.src/server.rs:203-228🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server.rs` around lines 203 - 228, The current relay authorization in server.rs only checks TrackedConn.publishing and TrackedConn.playing once, so later stream name changes can bypass rtmp_bridge.on_publish and rtmp_bridge.on_play. Update the logic around the stream state handling in the connection update path to re-run authorization when current_stream.name changes, and refresh entry.publishing/entry.playing and entry.stream_id based on the new stream rather than latching forever. Use the existing symbols rtmp_bridge.on_publish, rtmp_bridge.on_play, TrackedConn, and stream.current_stream.name to locate and adjust the check.
🟠 Major comments (19)
vendor/librtmp2/src/chunk/writer.rs-11-29 (1)
11-29: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate the chunk invariants before serializing.
chunk_writecurrently trusts inputs that can disagree:payload_len > payload.len()will panic at Lines 75-76,msg.msg_length != payload_lenwrites a malformed RTMP header, andcsid < 2falls into the reserved basic-header encodings inbasic_header. Reject those cases up front instead of emitting corrupt chunks or crashing.Also applies to: 73-76, 106-119
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/src/chunk/writer.rs` around lines 11 - 29, Validate the chunk invariants at the start of chunk_write before any serialization happens. Add upfront checks for payload_len exceeding payload.len(), msg.msg_length not matching payload_len, and msg.csid being below the reserved range used by basic_header. Reject these cases with an error from chunk_write rather than proceeding to write a malformed header or panicking later in the function.vendor/librtmp2/src/buffer.rs-54-72 (1)
54-72: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
from_staticis not actually a borrowed view.Lines 64-72 clone the caller’s slice into a new
Vec, then mark the buffer asowned = false. That means callers will not see writes in the original slice, and later growth now fails even though this instance already owns heap storage. Either make this API truly borrowed or treat it as an owned copy and name/document it that way.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/src/buffer.rs` around lines 54 - 72, The Buffer::from_static constructor is mislabeled: it clones the input slice into a new Vec but still marks owned as false. Update from_static so its behavior matches its name by either creating a real borrowed view over the provided data or, if it must copy, set owned consistently and rename/document it as an owned copy. Make sure the Buffer fields in from_slice/from_static reflect the actual ownership semantics.vendor/librtmp2/src/chunk/reader.rs-253-275 (1)
253-275: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftCompressed chunk headers are using timestamp deltas as absolute timestamps.
Lines 261-270 overwrite
type0_timestampwith the rawfmt=1/2header value, and Line 274 later reports that as the message timestamp. In RTMP those fields are deltas from the previous chunk-stream timestamp, andfmt=3new messages must advance by the previous delta too. After the firstfmt=0message on a CSID, later messages can get the wrong timestamp and arrive out of order downstream.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/src/chunk/reader.rs` around lines 253 - 275, The chunk timestamp handling in reader::read_chunk is treating fmt=1/2 values as absolute timestamps instead of deltas. Update the logic around stream.type0_timestamp, stream.type0_ext_ts, and the effective_ts calculation so that fmt=1/2 add their delta to the previous chunk-stream timestamp, and ensure fmt=3 reuses and advances the prior delta correctly. Keep the fix localized to the timestamp resolution path in chunk::reader::reader and preserve the existing state fields for subsequent chunks.vendor/librtmp2/src/amf/amf3.rs-129-135 (1)
129-135: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFail closed for unsupported AMF3 string references.
Returning
Ok(0)silently turns a valid string reference into an empty string, which can corrupt decoded command/app/stream fields. ReturnUnsupporteduntil reference-table decoding exists.Proposed fix
if inline_bit == 0 { - // String reference — not fully implemented - if !out.is_empty() { - out[0] = 0; - } - return Ok(0); + // String reference — not fully implemented. + return Err(ErrorCode::Unsupported); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/src/amf/amf3.rs` around lines 129 - 135, The AMF3 string reference branch in the string decoding logic is currently failing open by returning an empty string, which can corrupt decoded fields. Update the AMF3 string handling in the relevant decode function to fail closed when inline_bit indicates a string reference instead of an inline string. In the string decoder path, return an Unsupported error from the string reference case until reference-table decoding is implemented, and keep the existing inline string handling unchanged.vendor/librtmp2/src/handshake.rs-142-145 (1)
142-145: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPopulate
time2in S2/C2 instead of leaving it zero.These packets clone C1/S1 and only replace bytes 0-3. Because bytes 4-7 in C1/S1 are the reserved zero field, S2/C2 go out with
time2 == 0instead of echoing the peer timestamp. That breaks the simple-handshake echo format and can fail against stricter RTMP implementations.Proposed fix
// S2 echoes C1 with time2 replaced let mut s2 = c1.clone(); s2[..4].copy_from_slice(&server_time.to_be_bytes()); + s2[4..8].copy_from_slice(&c1[..4]); @@ // C2 echoes S1 with time2 replaced let mut c2 = s1.clone(); c2[..4].copy_from_slice(&get_time().to_be_bytes()); + c2[4..8].copy_from_slice(&s1[..4]);Also applies to: 218-220
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/src/handshake.rs` around lines 142 - 145, Update the S2/C2 construction in handshake.rs so `time2` is echoed instead of left as zero. In the `S2` and `C2` generation paths, after cloning `c1`/`s1`, populate bytes 4-7 with the peer timestamp (`time2`) while still writing the server timestamp into bytes 0-3. Use the existing `S2`/`C2` handshake logic and the relevant clone/write points to ensure the simple-handshake echo format matches RTMP expectations.vendor/librtmp2/src/transport.rs-267-295 (1)
267-295: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftKeep TLS handshakes out of the poll loop.
Server::poll()callsaccept_new_connections()inline, andTlsCtx::accept()switches the socket to blocking mode beforeSslAcceptor::accept(). A slow or stalled TLS client can therefore pause every other connection until the handshake finishes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/src/transport.rs` around lines 267 - 295, `TlsCtx::accept()` is performing the TLS handshake synchronously on the same path used by `Server::poll()`/`accept_new_connections()`, which can block the entire poll loop on slow clients. Update the accept flow so `SslAcceptor::accept()` is not run inline with polling; either defer handshake to a separate worker or make the handshake non-blocking and retryable, while keeping the steady-state socket setup in `Transport::accept()` and preserving the existing timeout/nonblocking restore behavior.Source: Learnings
vendor/librtmp2/src/message/command.rs-242-255 (1)
242-255: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReject oversized publish/play stream names instead of truncating them.
read_string_truncsilently clips AMF strings to the destination buffer, andread_publish/read_playuse that result as the stream key. Invendor/librtmp2/src/session/conn.rs:360-560, that truncated name is then stored on the active stream and used for publish/play routing. Two distinct long keys that share the same 255-byte prefix will therefore collapse to the same(app, stream_name)and can be authorized or relayed as the wrong stream. Return an error for oversize stream identifiers here instead of accepting a truncated prefix.Also applies to: 258-264, 297-323
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/src/message/command.rs` around lines 242 - 255, The publish/play command parsing in read_publish and the related read_play path currently accepts oversized AMF stream identifiers by truncating them with read_string_trunc, which can collapse distinct keys into the same app/stream_name pair. Update these parsers to validate the decoded string length against the destination buffers and return an error when the stream name or app field exceeds the allowed size, rather than silently clipping it. Use the existing helpers in command.rs around read_publish/read_play and keep conn.rs routing logic expecting only exact, non-truncated identifiers.vendor/librtmp2/src/message/message.rs-45-86 (1)
45-86: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winEnhanced RTMP frames are being decoded with the legacy FLV header layout.
When bit 7 is set, the low nibble is the E-RTMP packet type, not the legacy codec id. These helpers still interpret ExAudio / ExVideo payloads as legacy tags, so callbacks can receive bogus codec metadata (for example, ExVideo packet types mapping to
Jpeg/Sorenson). The crate already addsvendor/librtmp2/src/ertmp/exaudio.rsandvendor/librtmp2/src/ertmp/exvideo.rs; use those parsers here before populatingFrame, otherwise the advertised E-RTMP support is broken on both direct and aggregate message paths.Also applies to: 88-127
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/src/message/message.rs` around lines 45 - 86, The audio/video frame helpers are still parsing ExAudio/ExVideo payloads with the legacy FLV header layout, so enhanced RTMP packet types are being misread as codec IDs. Update the frame-building path in deliver_audio_frame (and the matching video helper in the same message module) to detect the E-RTMP bit-7 case and delegate parsing to the existing exaudio/exvideo parsers before filling Frame fields. Make sure the callback receives metadata derived from the enhanced packet type, not the legacy tag, so both direct and aggregate message paths use the new E-RTMP format correctly.vendor/librtmp2/src/ertmp/fourcc.rs-31-35 (1)
31-35: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDon't alias unrelated codecs in the FourCC tables.
vp09is mapped toVideoCodec::Vp6, andec-3toAudioCodec::G711A. That makes the forward lookup misclassify incoming media and also breaks the reverse helpers later in this file, e.g. serializing VP6 asvp09and G.711 A-law asec-3.vendor/librtmp2/src/ertmp/exaudio.rs:10-18also usesfourcc_to_audio_codec()for classification, so this leaks into live ingest too. If the enums do not have VP9/EC-3 variants yet, please add them instead of reusing unrelated legacy codecs.Also applies to: 62-66, 100-116
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/src/ertmp/fourcc.rs` around lines 31 - 35, The FourCC tables are aliasing unrelated codecs, so update the mappings in fourcc.rs to use the correct codec variants for vp09 and ec-3 instead of reusing VideoCodec::Vp6 and AudioCodec::G711A. If the needed variants do not exist yet, add proper VP9 and EC-3 entries to the VideoCodec and AudioCodec enums, then ensure the forward lookup helpers and the reverse serialization helpers in the same file stay consistent. Also verify fourcc_to_audio_codec in exaudio.rs continues to classify incoming media correctly with the updated audio mapping.vendor/librtmp2/src/ertmp/multitrack.rs-17-25 (1)
17-25: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftThe multitrack descriptor is not AMF0-encoded as written.
Lines 54-65 label this as AMF0, but the code writes raw big-endian integers and skips the string type marker. The parser mirrors that same custom layout, so compliant peers will not decode descriptors emitted here, and AMF0-encoded inputs will be misclassified on read. Please encode/decode the actual AMF0 types, or route this through the existing AMF helpers.
Also applies to: 54-69
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/src/ertmp/multitrack.rs` around lines 17 - 25, The multitrack descriptor handling in multitrack.rs is using a custom binary layout instead of real AMF0, so both the writer and parser need to be updated together. Fix the descriptor serialization/deserialization in the multitrack encode/decode path by using the existing AMF helpers or by emitting/parsing the proper AMF0 type markers and values instead of raw big-endian integers. Make sure the logic around the multitrack type field in the relevant writer and parser functions stays symmetric and compliant with AMF0.vendor/librtmp2/src/client/mod.rs-109-111 (1)
109-111: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate the
onStatusresult before changing client state.
wait_for_command("onStatus")accepts both success and error statuses. A rejected publish/play can still move the client toPublishingorPlaying, allowing later calls likesend_frame()to proceed under the wrong state.Also applies to: 144-146
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/src/client/mod.rs` around lines 109 - 111, The publish/play flow in the client state transition logic currently advances to Publishing/Playing immediately after wait_for_command("onStatus"), even when the server reported an error. Update the relevant paths in the client state machine around the send_command_msg + wait_for_command("onStatus") sequence (and the matching play branch) to inspect the returned onStatus result first, only setting self.state when the status indicates success and leaving the client out of Publishing/Playing on rejection.vendor/librtmp2/src/server/mod.rs-135-140 (1)
135-140: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftAdd an explicit relay-enable path for the public server API.
Accepted connections start with
relay_enabled = false, but thisServernever exposes publish/play authorization callbacks or otherwise enables it. Because both init replay and live forwarding requirerelay_enabled, the standalone server will never relay media without callers mutatingconnectionsinternals.Expose publish/play events or an explicit authorization hook that sets
conn.relay_enabled = trueafter the host approves the stream; avoid embedding auth policy in the protocol library. Based on learnings,librtmp2must remain a pure protocol library without authentication policy.Also applies to: 199-200, 248-257
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/src/server/mod.rs` around lines 135 - 140, The Server connection setup currently pushes new Conn values with relay_enabled left disabled, so the public API never enters a relaying state. Update Server::accept/new-connection handling to expose an explicit approval hook or publish/play callback path (for example alongside on_connect_cb/on_frame_cb) that lets the host authorize the stream and set conn.relay_enabled = true before media forwarding begins. Keep the protocol layer policy-free by only providing the hook in Server and Conn initialization logic, and ensure the relay-enabled path is applied wherever accepted connections are created and later used for init replay/live forwarding.Source: Learnings
vendor/librtmp2/src/server/mod.rs-122-129 (1)
122-129: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAvoid blocking TLS handshakes inside the accept loop.
The comment says
TlsCtx::accept()switches the socket to blocking for the TLS handshake. A client that connects and stalls the handshake can blockServer::poll()and prevent other connections from being processed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/src/server/mod.rs` around lines 122 - 129, The TLS handshake in Server::poll via TlsCtx::accept is currently performed inline in the accept loop, which can stall the whole server when a client blocks mid-handshake. Move the accept/handshake work out of the main accept path by deferring it to a non-blocking/background step or by reworking the connection setup so the loop only registers the accepted socket and does not wait on the handshake; keep the existing stream ownership/error handling around ctx.accept and the transport setup in Server::poll.vendor/librtmp2/src/session/play.rs-8-10 (1)
8-10: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake
play_beginupdate stream state or keep it private.This public API reports success without marking the stream as playing or recording
stream_name, so callers can observe a successful play transition that never actually happened.Proposed fix
-pub fn play_begin(_stream: &mut Stream, _stream_name: &str) -> Result<()> { +pub fn play_begin(stream: &mut Stream, stream_name: &str) -> Result<()> { + stream.is_playing = true; + stream.name = stream_name.to_string(); Ok(()) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/src/session/play.rs` around lines 8 - 10, The public play_begin function currently returns success without changing any Stream state, so update it to record the provided stream_name and mark the stream as playing, or make play_begin private if it is only a stub. Use the play_begin function and the Stream type in session/play.rs to locate the fix, and ensure callers cannot observe a successful transition unless the stream state is actually updated.vendor/librtmp2/src/lib.rs-220-224 (1)
220-224: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReturn the actual connection fd.
lrtmp2_conn_get_fdcurrently ignores a valid connection and always returns-1, so FFI callers cannot poll or track the underlying socket.Proposed fix
-pub unsafe extern "C" fn lrtmp2_conn_get_fd(_conn: *const session::Conn) -> i32 { - -1 +pub unsafe extern "C" fn lrtmp2_conn_get_fd(conn: *const session::Conn) -> i32 { + if conn.is_null() { + return -1; + } + (*conn).get_fd() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/src/lib.rs` around lines 220 - 224, lrtmp2_conn_get_fd currently ignores the passed session::Conn and always returns -1, so update this FFI helper to read the actual file descriptor from the connection object. Use the existing session::Conn type and its underlying socket/stream accessors in lrtmp2_conn_get_fd, handling null or unavailable state safely, and return the real fd instead of the hardcoded fallback.vendor/librtmp2/src/flv/video_tag.rs-34-38 (1)
34-38: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject malformed AVC tags and sign-extend composition time.
For
VideoCodec::H264, a legacy FLV video tag needs the 1-byte header plus AVC packet type and 24-bit composition offset. Lines 34-38 currently accept 1-4 byte AVC payloads and zero-extend negative composition offsets, while the enhanced parser sign-extends the same 24-bit field.Proposed fix
- if data.len() >= 5 && tag.codec == VideoCodec::H264 { + if tag.codec == VideoCodec::H264 { + if data.len() < 5 { + return Err(ErrorCode::Io); + } tag.avc_packet_type = data[1]; - tag.composition_time = - ((data[2] as u32) << 16) | ((data[3] as u32) << 8) | (data[4] as u32); + let ct = ((data[2] as i32) << 16) | ((data[3] as i32) << 8) | data[4] as i32; + let ct = if ct & 0x0080_0000 != 0 { + ct | 0xFF00_0000u32 as i32 + } else { + ct + }; + tag.composition_time = ct as u32; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/src/flv/video_tag.rs` around lines 34 - 38, In `video_tag.rs`, tighten the `VideoTag::parse` handling for `VideoCodec::H264` so it only accepts a valid AVC payload with the expected header and 4 additional bytes, instead of allowing malformed 1-4 byte payloads. Also update the composition time decoding to sign-extend the 24-bit offset before storing it in `tag.composition_time`, matching the behavior of the enhanced parser and preserving negative offsets correctly.vendor/librtmp2/scripts/abi-baseline.sh-19-35 (1)
19-35: 🎯 Functional Correctness | 🟠 Major | ⚖️ Poor tradeoffThis script still assumes a Makefile build that isn’t present.
vendor/librtmp2/scripts/abi-baseline.shcallsmake clean,make DEBUG=1 all, andmake install PREFIX=..., then expectsliblibrtmp2.sounder that prefix. Switch it to the Cargo build path or add the missing Makefile, otherwise this fails immediately.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/scripts/abi-baseline.sh` around lines 19 - 35, The build_and_dump helper still hardcodes a Makefile-based flow that does not exist, so it will fail before generating the ABI dump. Update build_and_dump in abi-baseline.sh to use the Cargo build/install path expected by the project, or add the missing Makefile targets that provide clean, all, and install behavior plus the liblibrtmp2.so output under the install prefix. Ensure the ABI dump step still points at the correct built library location after the build path change.vendor/librtmp2/Cargo.toml-15-21 (1)
15-21: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDefault
tlsmakes the prebuilt shared library distro-specific
opensslis enabled by default here, and the release workflow packages thecdylibbuilt onubuntu-latestaslibrtmp2-<version>-linux-x86_64.tar.gz. That.sowill depend on the runner’s OpenSSL/glibc ABI and can fail to load on other Linux distros.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/Cargo.toml` around lines 15 - 21, The default feature set currently enables tls via openssl, which makes the built cdylib depend on the runner’s system OpenSSL and glibc ABI. Update the Cargo.toml feature configuration so tls is not enabled by default, and only opt into openssl when explicitly requested through the tls feature. Keep the feature wiring around default, tls, and the openssl optional dependency consistent so the prebuilt librtmp2 shared library stays portable across Linux distros.vendor/librtmp2/.github/workflows/release.yml-24-26 (1)
24-26: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPin manual dispatches to the requested tag.
Both checkout steps still build the selected workflow ref, not thetaginput, so a manual run from a branch can package the wrong commit under the requested version. The release gate should follow the same manual path, or branch-dispatched runs will skip release creation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/.github/workflows/release.yml` around lines 24 - 26, Manual dispatches are still checking out the workflow ref instead of the requested tag, so release creation can run against the wrong commit. Update the checkout logic in the release workflow so the manual path uses the tag input for the release-gating step and the packaging step, keeping both checkout actions aligned through the relevant checkout configuration and tag-handling logic.
🟡 Minor comments (21)
vendor/librtmp2/tests/interop/play_interop.sh-52-63 (1)
52-63: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winFail explicitly when the RTMP port never becomes ready.
If all 50 probes miss while mediamtx is still alive, the script continues into publish/play steps against an endpoint that never came up.
Suggested fix
+ready=0 for _ in $(seq 1 50); do if ! kill -0 "$MTX" 2>/dev/null; then echo "mediamtx exited during startup" cat "$MTX_LOG" || true exit 1 @@ if (exec 3<>"/dev/tcp/127.0.0.1/$PORT") 2>/dev/null; then exec 3>&- + ready=1 break fi sleep 0.2 done + +if [ "$ready" -ne 1 ]; then + echo "mediamtx did not open RTMP on :$PORT" + cat "$MTX_LOG" || true + exit 1 +fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/tests/interop/play_interop.sh` around lines 52 - 63, The startup probe in play_interop.sh only handles the mediamtx-exited case and then falls through even when all 50 port checks fail, so add an explicit failure path after the loop. Use the existing MTX, MTX_LOG, and PORT checks around the readiness loop to detect when no successful /dev/tcp connection was made, then print a clear readiness error, dump the log, and exit nonzero before any publish/play steps run.vendor/librtmp2/tests/interop/ffmpeg_interop.sh-59-69 (1)
59-69: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDon't let
wait "$SRV"bypass the failure-reporting path.With
set -eactive again, a failing ingest process exits the script atwait "$SRV"beforeSRV_RCis captured, so the server log is lost on the path where it's most useful.Suggested fix
-# Wait for the ingest server to finish (it exits 0 on success). -wait "$SRV" -SRV_RC=$? +# Wait for the ingest server to finish (it exits 0 on success). +SRV_RC=0 +wait "$SRV" || SRV_RC=$? trap - EXIT🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/tests/interop/ffmpeg_interop.sh` around lines 59 - 69, The ingest server wait path in ffmpeg_interop.sh is being exited early by set -e, so the failure-reporting block never runs and the log is skipped. Update the shutdown sequence around wait "$SRV" and SRV_RC so the wait failure is captured without aborting immediately, then always clear the EXIT trap and print "$LOG" before checking SRV_RC and exiting with the ingest server status.vendor/librtmp2/tests/interop/enhanced_rtmp_interop.sh-67-71 (1)
67-71: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCapture the ingest exit code without tripping
errexit.Once
set -eis back on, a failingwait "$srv"aborts the function beforercis assigned, so the ingest log and explicit failure message never run.Suggested fix
- wait "$srv"; local rc=$? + local rc=0 + wait "$srv" || rc=$? echo "== [$label] ingest log =="; cat "$log" if [ "$rc" -ne 0 ]; then echo "[$label] ENHANCED-RTMP INTEROP FAILED (ingest exit=$rc)"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/tests/interop/enhanced_rtmp_interop.sh` around lines 67 - 71, The ingest failure handling in enhanced_rtmp_interop.sh still lets `set -e` terminate the function before `rc` is captured. Update the `wait "$srv"` flow in the interop test helper so the exit status is stored safely even when `wait` fails, then use that saved status for the ingest log and the explicit failure branch in the same function.vendor/librtmp2/CLAUDE.md-53-53 (1)
53-53: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd language tags to the fenced examples.
Both unlabeled code fences are already tripping markdownlint; marking them as
textwill clear the warning and make the docs easier to render consistently.Suggested tweak
- ``` + ```text- ``` + ```textAlso applies to: 69-69
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/CLAUDE.md` at line 53, The fenced examples in CLAUDE.md are missing language tags, causing markdownlint warnings. Update the unlabeled code fences in the documented examples to use a text tag, and apply the same fix to the other affected fence referenced in the comment so the markdown renders consistently. Use the existing fenced-example blocks in the document as the targets.Source: Linters/SAST tools
vendor/librtmp2/CHANGELOG.md-53-59 (1)
53-59: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDrop
CONTRIBUTING.mdfrom Planned.This PR adds the guide already, so leaving it here makes the changelog stale as soon as it lands.
Suggested tweak
- - `CONTRIBUTING.md` guidelines🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/CHANGELOG.md` around lines 53 - 59, Remove the stale CONTRIBUTING.md item from the Planned section in CHANGELOG.md so the roadmap matches the PR state. Update the Planned bullet list to reflect only remaining work, keeping the existing changelog entries in sync with the actual repository contents and the release plan.vendor/librtmp2/docs/roadmap.md-64-67 (1)
64-67: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove docs that are already being added.
CHANGELOG.mdandCONTRIBUTING.mdare part of this PR, so listing them as missing will make the roadmap stale on merge. Keep only gaps that still remain.Suggested tweak
-- **Project docs** — `CHANGELOG.md` and `CONTRIBUTING.md` are not yet present.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/docs/roadmap.md` around lines 64 - 67, The roadmap entry in roadmap.md still says CHANGELOG.md and CONTRIBUTING.md are missing even though this PR adds them, so update the roadmap text to remove those items and leave only the remaining open gaps. Make the edit in the docs list near the dump_frames/minimal_server/minimal_client section, keeping the wording aligned with the current state of the project and preserving the other roadmap items unchanged.vendor/librtmp2/CONTRIBUTING.md-8-8 (1)
8-8: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign the internal-header path with the rest of the docs.
CLAUDE.mdsays internal headers live undersrc/**/*.h, but this guide sends contributors toinclude/librtmp2-server/. That split will point people at the wrong tree.Suggested tweak
- - **ABI Stability**: Only change public headers (located in `include/librtmp2/`) with major version bumps. Internal headers (in `include/librtmp2-server/`) can change freely. + - **ABI Stability**: Only change public headers (located in `include/librtmp2/`) with major version bumps. Internal headers (in `src/**/*.h`) can change freely.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/CONTRIBUTING.md` at line 8, The CONTRIBUTING guide points contributors to the wrong internal-header location, conflicting with CLAUDE.md. Update the ABI Stability guidance in the CONTRIBUTING.md text so the internal headers are described using the same `src/**/*.h` path convention as the rest of the docs, and keep the public-header rule unchanged.vendor/librtmp2/docs/protocol-mapping-ertmp-v2.md-19-20 (1)
19-20: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMake the
CAPS_NEGOTIATEDplacement consistent.This doc puts the state between
CONNECTEDandSTREAM_CREATED, butCLAUDE.mddocuments it beforeAPP_CONNECTED. Keeping both versions will confuse anyone wiring the negotiation flow.Suggested tweak
-State machine adds a `CAPS_NEGOTIATED` state between `CONNECTED` and -`STREAM_CREATED` (`src/session/state_machine.c`). +State machine adds a `CAPS_NEGOTIATED` state between `CONNECTED` and +`APP_CONNECTED` (`src/session/state_machine.c`).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/docs/protocol-mapping-ertmp-v2.md` around lines 19 - 20, The state ordering for CAPS_NEGOTIATED is inconsistent between this protocol mapping doc and the state machine flow. Update the documentation in protocol-mapping-ertmp-v2.md so CAPS_NEGOTIATED matches the canonical order used by state_machine.c and CLAUDE.md, and make sure the transition description reflects the same negotiation sequence around CONNECTED, APP_CONNECTED, and STREAM_CREATED.vendor/librtmp2/docs/abi-policy.md-11-37 (1)
11-37: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRetarget the ABI policy to the Rust FFI surface.
The policy is still framed around a C-header distribution (
include/librtmp2/*.h, hidden visibility scripts,liblrtmp2.so.0), but the vendored crate exposesextern "C"symbols directly fromsrc/lib.rsand doesn't have the documentedinclude/tree. Please update the policy so release reviewers are checking the real export boundary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/docs/abi-policy.md` around lines 11 - 37, The ABI policy is still describing a C-header-based distribution, but the actual export boundary is the Rust FFI surface in src/lib.rs. Update the policy in abi-policy.md to refer to the Rust-exported extern "C" symbols and any Rust-side public ABI types/functions instead of include/librtmp2/*.h, hidden-visibility scripts, or liblrtmp2.so.0, so reviewers validate the real crate-facing ABI surface.vendor/librtmp2/docs/architecture.md-46-58 (1)
46-58: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the module map to the crate’s actual Rust layout.
The table still uses the old C directory structure (
core/alloc.c,server/server.c, etc.), while the repo structure inREADME.mdis the flat Rust module set undersrc/*.rs. Keeping both layouts here will send contributors to files that do not exist.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/docs/architecture.md` around lines 46 - 58, The module map in the architecture doc still points to the old C-style directories and files, so update the table to match the current flat Rust layout under src/*.rs. Adjust each entry to reference the actual Rust modules and their responsibilities, using the existing section labels like core, handshake, chunk, message, amf, flv, ertmp, session, server, and client so contributors can navigate to the correct code.vendor/librtmp2/docs/protocol-mapping-ertmp-v1.md-11-64 (1)
11-64: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winPoint the mapping at the Rust files.
Every implementation reference here still uses
*.c/*.h, but the actual code in this vendor tree lives undersrc/ertmp/*.rs. Please rename the paths so the spec-to-code map remains navigable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/docs/protocol-mapping-ertmp-v1.md` around lines 11 - 64, The protocol mapping table still points to C/H files, but the implementation now lives in the Rust modules under src/ertmp. Update every reference in this document so the spec-to-code map uses the corresponding .rs files for the same symbols, including lrtmp2_video_header_t, lrtmp2_ertmp_exaudio_parse(), videocodecid_from_fourcc(), and lrtmp2_fourcc_list_t, so readers can navigate directly to the actual Rust implementations.vendor/librtmp2/concept/librtmp2-core.md-5-7 (1)
5-7: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRewrite the concept in Rust terms.
This still describes
librtmp2as a C library and says the core shall be written in C, which conflicts with the Rust crate inREADME.md/src/lib.rs. The C-centric repository tree below will also mislead contributors unless it is explicitly marked as legacy context.Also applies to: 41-43, 63-160
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/concept/librtmp2-core.md` around lines 5 - 7, Rewrite the concept text in Rust terms so it matches the crate described by src/lib.rs and README.md rather than a C library. Update the overview in librtmp2-core.md to describe librtmp2 as a Rust crate and keep any C references only as legacy/background context; also align the repository/project wording under AlexanderWagnerDev accordingly. Use the existing concept sections in this document to remove C-centric language and make the tree/context clearly secondary to the Rust implementation.vendor/librtmp2/README.md-133-142 (1)
133-142: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign the client constructor type across docs and FFI.
README.mddocumentslrtmp2_client_create(const lrtmp2_server_config_t *config), butconcept/librtmp2-core.mduseslrtmp2_client_config_tandsrc/lib.rsexposeslrtmp2_client_create(_config: *const ServerConfig). Keep these in sync so downstream bindings don’t copy the wrong signature.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/README.md` around lines 133 - 142, The client constructor type is inconsistent across the docs and FFI surface, so update the referenced client creation API to use one shared config type everywhere. Align the signature in the README client section, the concept document, and the Rust binding around lrtmp2_client_create and ServerConfig/lrtmp2_client_config_t so all exported docs and bindings describe the same constructor shape.vendor/librtmp2/src/amf/amf3.rs-48-50 (1)
48-50: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject out-of-range U29 values instead of truncating.
val & 0x1FFFFFFFsilently converts invalid inputs like0x20000000to0, producing corrupted AMF3 output.Proposed fix
+const AMF3_U29_MAX: u32 = 0x1FFF_FFFF; + fn write_u29(buf: &mut Buffer, val: u32) -> Result<()> { - let v = val & 0x1FFFFFFF; + if val > AMF3_U29_MAX { + return Err(ErrorCode::Amf); + } + let v = val;Also applies to: 153-156
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/src/amf/amf3.rs` around lines 48 - 50, The AMF3 U29 encoding logic in write_u29 is masking invalid inputs with 0x1FFFFFFF, which silently truncates out-of-range values. Update write_u29 (and the related handling near the other referenced block) to validate val before encoding, return an error for any value above the U29 range, and only proceed with byte emission for valid inputs so invalid AMF3 data is rejected instead of corrupted.vendor/librtmp2/src/amf/mod.rs-8-9 (1)
8-9: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAvoid the glob re-exports here.
amf0andamf3both exportread_type,read_string,write_string,write_null, andread_boolean, sopub use amf0::*; pub use amf3::*;makescrate::amf::*ambiguous and triggers theambiguous_glob_reexportslint. Prefer exposing the modules directly or re-exporting only explicit, uniquely named items.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/src/amf/mod.rs` around lines 8 - 9, The AMF module is using glob re-exports from amf0 and amf3, which creates ambiguous items in crate::amf::* because both modules export the same function names. Update the amf::mod re-exports to avoid pub use amf0::* and pub use amf3::*; instead expose the modules directly or re-export only explicitly chosen unique symbols from amf0 and amf3 so the ambiguous_glob_reexports lint is not triggered.vendor/librtmp2/src/handshake.rs-133-133 (1)
133-133: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winParse C1's timestamp as 32-bit.
Line 133 reads a 4-byte handshake field with
ntoh24, sohs.peer_timeloses the high byte once the peer timestamp exceeds0x00FF_FFFF. The client-side path already usesntoh32for the analogous field.Proposed fix
- hs.peer_time = ntoh24(&c1[..4]); + hs.peer_time = ntoh32(&c1[..4]); @@ -// Fix the s1 building in server_read_c1 -fn ntoh24(buf: &[u8]) -> u32 { - ((buf[0] as u32) << 16) | ((buf[1] as u32) << 8) | (buf[2] as u32) -}Also applies to: 241-244
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/src/handshake.rs` at line 133, The C1 handshake timestamp is being decoded with the 24-bit helper, so the peer timestamp truncates for values above 0x00FF_FFFF. Update the C1 parsing in the handshake receive path so hs.peer_time uses the 32-bit network-to-host conversion, matching the client-side handling and any other analogous timestamp reads in handshake.rs.vendor/librtmp2/src/message/control.rs-137-151 (1)
137-151: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReturn
Erron short control payloads instead of panicking.These decoders index raw offsets without validating
data.len(). Because they are public helpers re-exported byvendor/librtmp2/src/message/mod.rs, a short slice will abort the caller instead of producing anErrorCode. Add explicit minimum-length checks inside each decoder so malformed control frames fail closed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/src/message/control.rs` around lines 137 - 151, The public control decoders in read_set_peer_bandwidth and read_user_control currently index into raw slices without validating length, which can panic on malformed frames. Add explicit minimum-length checks at the start of each helper and return an Err via the existing Result/ErrorCode path when the payload is too short, keeping the decoding logic in control.rs fail-closed for callers re-exported through message::mod.vendor/librtmp2/src/ertmp/connect_caps.rs-15-20 (1)
15-20: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winValidate the FourCC slice length before copying.
cc[..4]will panic if a caller passes fewer than 4 bytes. This helper is public API for capability construction, so malformed input should returnErr(ErrorCode::Io)instead of taking down the process.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/src/ertmp/connect_caps.rs` around lines 15 - 20, The public helper fourcc_list_add currently copies from cc[..4] without validating the input length, which can panic on short slices. Update fourcc_list_add in connect_caps.rs to check that cc has at least 4 bytes before the copy, and return Err(ErrorCode::Io) for malformed input just like the MAX_FOURCCS guard. Keep the existing behavior of writing into FourCcList.entries and incrementing count only after the slice length check passes.vendor/librtmp2/src/flv/audio_tag.rs-13-34 (1)
13-34: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject unknown SoundFormat values instead of rewriting them to AAC.
The default branch maps every unsupported codec nibble to
AudioCodec::Aac, so reserved/invalid FLV audio tags are silently routed down the AAC path and may pick up an arbitraryaac_packet_type. Returning an error here is safer than fabricating AAC metadata for a different payload.Suggested fix
tag.codec = match (data[0] >> 4) & 0x0F { 0 => AudioCodec::Pcm, 1 => AudioCodec::Adpcm, 2 => AudioCodec::Mp3, 3 => AudioCodec::PcmLe, 4 => AudioCodec::Nelly16k, 5 => AudioCodec::Nelly8k, 6 => AudioCodec::Nelly, 7 => AudioCodec::G711A, 8 => AudioCodec::G711U, 10 => AudioCodec::Aac, 11 => AudioCodec::Speex, 14 => AudioCodec::Opus, - _ => AudioCodec::Aac, + _ => return Err(ErrorCode::Protocol), };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/src/flv/audio_tag.rs` around lines 13 - 34, The SoundFormat parsing in `audio_tag::parse` currently rewrites every unknown codec nibble to `AudioCodec::Aac`, which can misclassify reserved or invalid FLV audio tags. Update the `match` on `data[0] >> 4` to return an error for unsupported values instead of falling back to AAC, and keep the AAC-specific `aac_packet_type` handling only for genuine `AudioCodec::Aac` tags.vendor/librtmp2/.github/workflows/interop-play.yml-13-13 (1)
13-13: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winDisable persisted checkout credentials in this PR workflow. The job runs PR-controlled code after checkout, so leaving the checkout token available to later steps is unnecessary;
vendor/librtmp2/.github/workflows/tests.ymlalready setspersist-credentials: false.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/.github/workflows/interop-play.yml` at line 13, The checkout step in the interop-play workflow is leaving GitHub credentials persisted for later steps, which is unnecessary for PR-controlled code. Update the existing actions/checkout usage in this workflow to disable persisted credentials, matching the safer configuration already used in tests.yml; use the checkout step in the workflow as the target and add the persist-credentials setting there.vendor/librtmp2/.github/workflows/tests.yml-37-38 (1)
37-38: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMake Clippy actually gate regressions
cargo clippyneeds-- -D warningshere, and the crate root’s#![allow(clippy::all)]invendor/librtmp2/src/lib.rsshould be narrowed or removed; otherwise this job still won’t catch Clippy regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/.github/workflows/tests.yml` around lines 37 - 38, The Clippy workflow job is not actually failing on new lint regressions because it only runs cargo clippy without denying warnings. Update the Clippy step in the tests workflow to pass -- -D warnings, and review vendor/librtmp2/src/lib.rs to narrow or remove the crate-level #![allow(clippy::all)] so the lint gate can catch regressions as intended.
🧹 Nitpick comments (2)
vendor/librtmp2/build.rs (1)
1-3: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused
ccbuild-dependency.This
build.rsonly emits a rerun-if-changed directive and never invokes thecccrate declared inCargo.toml's[build-dependencies]. If no native compilation is actually needed, drop theccdependency to avoid unnecessary build overhead and confusion about the crate's build requirements.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/build.rs` around lines 1 - 3, The build script in main only emits a rerun-if-changed directive and does not use the cc build-dependency at all, so remove the unused cc entry from Cargo.toml’s build-dependencies unless native compilation is actually needed. If the build script is meant to stay minimal, keep fn main as-is and ensure no other build logic references cc.vendor/librtmp2/scripts/abi-baseline.sh (1)
53-58: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueFragile stash/checkout error handling.
git stash || trueandgit stash pop || truesilently swallow failures, and thegit checkout - || git checkout mainfallback can leave the working tree on an unexpected branch if the initial checkout of$BASELINE_TAGfailed for another reason. Failed stash pops could also silently drop the developer's local changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/scripts/abi-baseline.sh` around lines 53 - 58, The stash/checkout flow in abi-baseline.sh is swallowing real failures and can leave the repo in an unexpected state. Update the sequence around git stash, git checkout "$BASELINE_TAG", and git stash pop so failures are detected and handled explicitly instead of using fallback-or-true behavior. Use the existing script flow in build_and_dump and the checkout restore logic to ensure the original branch is restored only when the baseline checkout succeeds, and avoid silently discarding local changes if stash pop fails.
| } | ||
| cmsg.fmt = 0; | ||
|
|
||
| let payload = unsafe { std::slice::from_raw_parts(frame.data, frame.size as usize) }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Validate frame.data before constructing a slice.
Client::send_frame is a safe Rust API but can trigger UB when frame.size > 0 and frame.data is null or invalid. At minimum, reject null data for non-empty frames before from_raw_parts.
Proposed fix
- let payload = unsafe { std::slice::from_raw_parts(frame.data, frame.size as usize) };
+ if frame.size > 0 && frame.data.is_null() {
+ return Err(ErrorCode::Internal);
+ }
+ let payload = if frame.size == 0 {
+ &[][..]
+ } else {
+ unsafe { std::slice::from_raw_parts(frame.data, frame.size as usize) }
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let payload = unsafe { std::slice::from_raw_parts(frame.data, frame.size as usize) }; | |
| if frame.size > 0 && frame.data.is_null() { | |
| return Err(ErrorCode::Internal); | |
| } | |
| let payload = if frame.size == 0 { | |
| &[][..] | |
| } else { | |
| unsafe { std::slice::from_raw_parts(frame.data, frame.size as usize) } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@vendor/librtmp2/src/client/mod.rs` at line 170, `Client::send_frame` is
constructing a slice from `frame.data` with `from_raw_parts` without checking
validity first, which can lead to UB for non-empty frames. In `send_frame`,
validate the `frame.data` pointer before creating `payload`: if `frame.size >
0`, reject null `frame.data` (and any other obviously invalid pointer case your
API can detect) before calling `std::slice::from_raw_parts`. Keep the check near
the existing `payload` construction so the unsafe block only runs on verified
input.
| #[derive(Debug, Clone, Copy, PartialEq, Eq, thiserror::Error)] | ||
| #[repr(C)] | ||
| pub enum ErrorCode { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file around the reported lines.
sed -n '1,120p' vendor/librtmp2/src/types.rs
printf '\n---\n'
# Find all uses of thiserror in the vendored crate.
rg -n "thiserror|impl Display for ErrorCode|enum ErrorCode" vendor/librtmp2 -SRepository: OpenRTMP/librtmp2-server
Length of output: 3042
Drop thiserror::Error or add per-variant #[error(...)] attributes and remove the manual Display impl.
thiserror::Error here will clash with the manual Display impl below; if the derive stays, each variant also needs an explicit #[error(...)].
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@vendor/librtmp2/src/types.rs` around lines 40 - 42, The ErrorCode definition
currently mixes thiserror::Error with a manual Display implementation, so choose
one approach and make it consistent. Either remove the thiserror::Error derive
from ErrorCode or add explicit #[error(...)] attributes for every variant and
then حذف the manual Display impl; use the ErrorCode enum and its Display
implementation as the symbols to update.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
vendor/librtmp2/src/alloc.rs (1)
23-28: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse a read-mostly lock for allocator hooks
lrtmp2_malloc/lrtmp2_realloc/lrtmp2_freeall take the same globalMutex, so every allocation is serialized across threads even though the hooks are read-mostly after init.RwLockor a set-once atomic design would remove that hot-path contention.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendor/librtmp2/src/alloc.rs` around lines 23 - 28, The global allocator hook state in ALLOCATOR_HOOKS is guarded by a Mutex, which serializes the hot-path read access in lrtmp2_malloc, lrtmp2_realloc, and lrtmp2_free. Replace the Mutex-based design with a read-mostly synchronization strategy such as RwLock or a set-once atomic initialization pattern, and update the allocator hook accessors to take shared reads while preserving safe mutation during setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@vendor/librtmp2/src/alloc.rs`:
- Around line 23-28: The global allocator hook state in ALLOCATOR_HOOKS is
guarded by a Mutex, which serializes the hot-path read access in lrtmp2_malloc,
lrtmp2_realloc, and lrtmp2_free. Replace the Mutex-based design with a
read-mostly synchronization strategy such as RwLock or a set-once atomic
initialization pattern, and update the allocator hook accessors to take shared
reads while preserving safe mutation during setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 867b2baa-e927-4727-9054-1e823b1d2df5
📒 Files selected for processing (1)
vendor/librtmp2/src/alloc.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2d3fb2823
ℹ️ 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".
| stream.type0_ext_ts = ext_ts; | ||
| } | ||
| 1 => { | ||
| stream.type0_timestamp = final_timestamp; |
There was a problem hiding this comment.
When publishers use RTMP compressed chunk headers (fmt=1, and the same assignment is repeated for fmt=2 just below), the timestamp field is a delta from the previous message, not an absolute timestamp. Assigning it directly means relayed media after the first frame gets timestamps like 33/40ms over and over instead of a monotonic timeline, which breaks playback/cached keyframe timing for common FFmpeg/OBS streams; accumulate the delta onto the prior stream timestamp and preserve the delta for fmt=3 messages.
Useful? React with 👍 / 👎.
| match msg.msg_type_id { | ||
| msg_dispatch::RTMP_MSG_AMF0_COMMAND => self.handle_command(payload), | ||
| msg_dispatch::RTMP_MSG_AUDIO => { | ||
| if self.relay_enabled |
There was a problem hiding this comment.
When the crate is used as a standalone librtmp2::server::Server (as in the added ffmpeg_ingest example and server_client_loopback test), no application code ever sets Conn::relay_enabled after publish. Because this branch wraps both queuing and on_frame_cb, those users never receive audio/video frames at all; keep the authorization gate only on relay queuing or expose a way for direct server mode to enable it after publish.
Useful? React with 👍 / 👎.
| } | ||
| Ok(()) | ||
| } | ||
| _ => Ok(()), |
There was a problem hiding this comment.
When a publisher sends media in RTMP aggregate messages (RTMP_MSG_AGGREGATE, type 0x16), this default arm ignores the message entirely; the added decode_aggregate path in message::message is never called by Conn::read_messages, so all subtag audio/video frames are lost and neither relay nor callbacks see them. Handle aggregates here by splitting them into the same pending relay/callback path instead of falling through.
Useful? React with 👍 / 👎.
| return Err(ErrorCode::Protocol); | ||
| } | ||
| let mut amf = Buffer::with_capacity(256); | ||
| command::build_publish(&mut amf, &self.stream_key, &self.app)?; |
There was a problem hiding this comment.
live as the publish type, not the app name
For RTMP URLs whose app is anything other than live (for example rtmp://host/camera/stream), this passes self.app as the fifth AMF argument to publish, but command::build_publish writes that position as the publish type. Servers that validate the type (live/record/append) can reject an otherwise valid publish; send a valid publish type or make it a separate parameter.
Useful? React with 👍 / 👎.
| let mut amf = Buffer::with_capacity(256); | ||
| command::build_publish(&mut amf, &self.stream_key, &self.app)?; | ||
| self.send_command_msg(self.stream_id, amf.as_slice())?; | ||
| self.wait_for_command("onStatus")?; |
There was a problem hiding this comment.
When a server rejects a publish with an error onStatus (for example NetStream.Publish.BadName or NetStream.Publish.BadConnection), wait_for_command("onStatus") still returns Ok and this path moves the client into Publishing, so later send_frame calls can appear to succeed even though the peer rejected or closed the stream. Parse the returned status object and only transition on the expected success code.
Useful? React with 👍 / 👎.
| /// Handle a reassembled message. | ||
| fn handle_message(&mut self, msg: &ChunkMessage, payload: &[u8]) -> Result<()> { | ||
| match msg.msg_type_id { | ||
| msg_dispatch::RTMP_MSG_AMF0_COMMAND => self.handle_command(payload), |
There was a problem hiding this comment.
When a client sends legal AMF3 command messages (RTMP_MSG_AMF3_COMMAND, type 0x11) for connect, createStream, publish, or play, this match only dispatches AMF0 commands and the AMF3 message falls through to the default ignore path. The message::decode module already strips the AMF3 marker, but this connection path bypasses it, so AMF3-speaking clients never reach the RTMP state machine.
Useful? React with 👍 / 👎.
Summary
Deep bug scan of the server module (
src/server.rs) found a critical security issue: librtmp2 relayed audio/video before librtmp2-server could validate publish/play keys in its poll loop.Bug and impact
A holder of a viewer
play_keycould send an RTMPpublishcommand using that key as the stream name. librtmp2 accepted the publish at the protocol layer and relayed frames to legitimate players watching the same(app, stream_name)pair — beforeDbRtmpBridge::on_publishran on the next poll iteration. This allowed short-lived media injection to viewers.Root cause
Authorization is asynchronous:
server.poll()drives librtmp2's protocol state machine (including relay) in a background thread, while key validation happens afterward in the same poll loop viartmp_bridge.on_publish/on_play. There was no gate preventing relay for unauthorized sessions.Fix
Conn::relay_enabled(defaultfalse)relay_enabled = trueonly afteron_publish/on_playsucceedspending_relayand disable relay when evicting unauthorized connectionsrelay_enabledValidation
cargo test— 28/28 passingdeleted_streamseviction, RTMP thread shutdown, API token concurrent insertBug scan progress
Added
.cursor/bug-scan-config.mdand.cursor/bug-scan-progress.md; server module marked complete. Next module:rtmp_bridge.Summary by CodeRabbit