fix(ci): rustfmt test_support imports for PR #59#61
Conversation
Register client remote_addr inside publish/play callbacks during poll() so per-IP auth failure tracking applies before the first publish/play attempt. Adds regression tests for the pre-on_connect race. Co-authored-by: Alexander Wagner <info@alexanderwagnerdev.com>
PR #59 added clear_rtmp_poll_server and set_rtmp_poll_server imports that rustfmt wants on two lines instead of three. Co-authored-by: Alexander Wagner <info@alexanderwagnerdev.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds thread-local pinning of the active RTMP server during polling so publish/play callbacks can register connections via ChangesRTMP auth registration fix
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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: 4e1c882466
ℹ️ 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".
| let server = unsafe { &*server_ptr }; | ||
| let Some(conn) = server | ||
| .connections | ||
| .iter() | ||
| .find(|c| c.conn_id == conn_id && c.client_fd >= 0) |
There was a problem hiding this comment.
When a publish/play callback fires from server.poll(0), librtmp2::Server::poll(&mut self) is still inside its connection-processing loop, so self.connections is already mutably borrowed while a connection is being parsed. Dereferencing the thread-local raw pointer here creates a shared &Server and iterates the same connections vector reentrantly, which is undefined behavior in Rust even on the same thread; under live RTMP publish/play traffic this can miscompile or crash instead of just looking up the remote address. The callback path needs to avoid reading Server through a raw alias while poll owns &mut self (for example by having the library pass the remote address or by maintaining a separate pre-populated conn-id map).
Useful? React with 👍 / 👎.
| else { | ||
| return; | ||
| }; | ||
| with_rtmp_bridge(|bridge| bridge.on_connect(conn_id, &conn.remote_addr)); |
There was a problem hiding this comment.
When a client sends an unauthorized publish/play and then disconnects before this poll() returns, this early on_connect creates a bridge ConnState but process_server_connections never sees the connection and therefore never adds it to tracked; the existing close-detection loop only calls on_close for tracked IDs, so these remote-IP-only entries remain in DbRtmpBridge::conns indefinitely. A burst of failed auth attempts that closes immediately after the command can grow that map without bound, bypassing the auth-failure bucket cap this change is trying to preserve.
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: 1
🤖 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.
Inline comments:
In `@src/server.rs`:
- Around line 96-106: The `on_connect` callback in `src/server.rs` is creating a
shared `&Server` from `server_ptr` while `server.poll()` still holds a mutable
borrow, which violates Rust aliasing rules. Update this path to avoid forming
`&Server` from the raw pointer during polling; instead, read only the needed
`remote_addr` via raw pointer access or pass it through the callback from the
polling code, then call `with_rtmp_bridge(|bridge| bridge.on_connect(...))`
using that value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a58f6d6d-7b42-4594-9346-ab77058f3c64
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
src/rtmp_bridge.rssrc/server.rssrc/test_support.rs
| // SAFETY: `RTMP_POLL_SERVER` is set only on the RTMP thread for the | ||
| // duration of `server.poll()`, which exclusively owns `server`. | ||
| let server = unsafe { &*server_ptr }; | ||
| let Some(conn) = server | ||
| .connections | ||
| .iter() | ||
| .find(|c| c.conn_id == conn_id && c.client_fd >= 0) | ||
| else { | ||
| return; | ||
| }; | ||
| with_rtmp_bridge(|bridge| bridge.on_connect(conn_id, &conn.remote_addr)); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm poll takes &mut self and inspect the publish/play callback signatures
fd -t f 'server.rs' | rg -l 'fn poll' | xargs -r rg -nP -C2 'fn poll\s*\('
rg -nP -C3 'on_publish_cb|on_play_cb|type .*PublishCb|remote_addr' $(fd -t f '.rs' | rg -i 'librtmp2|server|types' | head -50) 2>/dev/nullRepository: OpenRTMP/librtmp2-server
Length of output: 2555
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- src/server.rs around the relevant code ---'
sed -n '1,140p' src/server.rs | cat -n
echo
echo '--- search for poll / thread-local server pointer usage ---'
rg -n -C 3 'set_rtmp_poll_server|RTMP_POLL_SERVER|poll\(0\)|server\.poll|on_publish_cb|on_play_cb|remote_addr' src/server.rs
echo
echo '--- all librtmp2 server method signatures available in repo ---'
rg -n -C 2 'pub fn poll|fn poll|on_publish_cb|on_play_cb|remote_addr' -g '*.rs' .Repository: OpenRTMP/librtmp2-server
Length of output: 15077
Avoid creating &Server from server_ptr while poll() is active
server.poll(0) still holds &mut server when this callback runs, so let server = unsafe { &*server_ptr }; creates an aliased shared reference to the same object. That is UB under Rust’s aliasing rules; keep this path on raw pointers or pass remote_addr through the callback instead.
🤖 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 96 - 106, The `on_connect` callback in
`src/server.rs` is creating a shared `&Server` from `server_ptr` while
`server.poll()` still holds a mutable borrow, which violates Rust aliasing
rules. Update this path to avoid forming `&Server` from the raw pointer during
polling; instead, read only the needed `remote_addr` via raw pointer access or
pass it through the callback from the polling code, then call
`with_rtmp_bridge(|bridge| bridge.on_connect(...))` using that value.
ensure_conn_registered_for_auth previously called bridge.on_connect (and its "new connection" debug log line) on every single publish/play callback for an already-registered connection, not just the first time it needed to catch up before the poll-loop's normal on_connect pass. Gate it behind DbRtmpBridge::is_registered so the reentrant Server lookup and re-registration only happen once per connection.
3 participants
Fixes failing
cargo fmt --checkon PR #59.PR #59 added
clear_rtmp_poll_serverandset_rtmp_poll_serverimports insrc/test_support.rswith a layout that rustfmt rejects. This applies the expected two-line import grouping.Failure: https://github.com/OpenRTMP/librtmp2-server/actions/runs/28912243143
Targets: cursor/application-security-review-bc92
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by CodeRabbit