Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

53 changes: 53 additions & 0 deletions src/rtmp_bridge.rs
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,17 @@ impl DbRtmpBridge {
}
}

/// True once `on_connect` has recorded a remote IP for `conn`. Lets
/// callers that may need to register a connection mid-poll (before the
/// normal `on_connect` pass runs) skip the redundant re-registration and
/// its log line on every subsequent publish/play attempt.
pub(crate) fn is_registered(&self, conn: ConnId) -> bool {
self.conns
.lock()
.get(&conn)
.is_some_and(|cs| !cs.remote_ip.is_empty())
}

fn is_auth_rate_limited(&self, remote_ip: &str) -> bool {
let mut guard = self.auth_failures.lock();
let now = Instant::now();
Expand Down Expand Up @@ -818,6 +829,38 @@ mod tests {
.unwrap();
}

#[test]
fn publish_failures_count_toward_auth_rate_limit_when_remote_ip_known() {
let db = Arc::new(Db::open(":memory:").unwrap());
let bridge = test_bridge(db);
let ip = "203.0.113.7:1935";

for conn in 0..RTMP_AUTH_MAX_FAILURES as u64 {
bridge.on_connect(conn, ip);
assert!(bridge.authorize_publish(conn, "live", "bogus").is_err());
}

bridge.on_connect(RTMP_AUTH_MAX_FAILURES as u64, ip);
assert!(bridge.is_auth_rate_limited(&remote_ip_of(ip)));
}

#[test]
fn publish_before_on_connect_skips_auth_failure_tracking() {
let db = Arc::new(Db::open(":memory:").unwrap());
let bridge = test_bridge(db);
let ip = "203.0.113.7:1935";

for conn in 0..(RTMP_AUTH_MAX_FAILURES as u64 + 2) {
assert!(bridge.authorize_publish(conn, "live", "bogus").is_err());
bridge.on_connect(conn, ip);
}

assert!(
!bridge.is_auth_rate_limited(&remote_ip_of(ip)),
"failed publish attempts before on_connect must not consume the per-IP auth budget"
);
}

#[test]
fn auth_rate_limit_survives_reconnect_from_same_ip() {
let db = Arc::new(Db::open(":memory:").unwrap());
Expand Down Expand Up @@ -965,6 +1008,16 @@ mod tests {
assert_eq!(db.publisher_list(Some("s1")).len(), 1);
}

#[test]
fn is_registered_reflects_on_connect_state() {
let db = Arc::new(Db::open(":memory:").unwrap());
let bridge = test_bridge(db);

assert!(!bridge.is_registered(7));
bridge.on_connect(7, "127.0.0.1:1000");
assert!(bridge.is_registered(7));
}

#[test]
fn authorize_publish_switching_streams_deactivates_prior_publisher() {
let db = Arc::new(Db::open(":memory:").unwrap());
Expand Down
59 changes: 58 additions & 1 deletion src/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
//! and the RTMP listener(s), then runs until a shutdown signal arrives.

use parking_lot::Mutex;
use std::cell::Cell;
use std::collections::{HashMap, HashSet};
use std::sync::atomic::{AtomicBool, Ordering};
use std::sync::{Arc, Mutex as StdMutex};
Expand All @@ -17,6 +18,22 @@ use crate::rtmp_bridge::{DbRtmpBridge, FrameInfo, FrameKind, RtmpEventHandler};
/// registered on the RTMP thread before the poll loop starts.
pub(crate) static RTMP_BRIDGE: StdMutex<Option<Arc<DbRtmpBridge>>> = StdMutex::new(None);

thread_local! {
static RTMP_POLL_SERVER: Cell<Option<*mut librtmp2::server::Server>> = const {
Cell::new(None)
};
}

/// Pin the active RTMP server for the duration of `poll()` so publish/play
/// callbacks can resolve `remote_addr` before auth rate limiting.
pub(crate) fn set_rtmp_poll_server(server: *mut librtmp2::server::Server) {
RTMP_POLL_SERVER.with(|cell| cell.set(Some(server)));
}

pub(crate) fn clear_rtmp_poll_server() {
RTMP_POLL_SERVER.with(|cell| cell.set(None));
}

/// How often the poll loop wakes up to service the RTMP/RTMPS listener(s).
pub(crate) const POLL_INTERVAL_MS: u64 = 50;

Expand Down Expand Up @@ -64,11 +81,48 @@ where
}
}

/// Register the client IP on the bridge before publish/play auth runs. During
/// `server.poll()` the publish/play callbacks can fire before
/// `process_server_connections` reaches `on_connect`, which would otherwise
/// skip per-IP auth-failure tracking and rate limiting.
fn ensure_conn_registered_for_auth(conn_id: u64) {
// Skip the pointer walk entirely once the normal `on_connect` pass (or an
// earlier call from this same function) has already registered the
// remote IP. Without this check every publish/play attempt on an
// already-registered connection re-ran `on_connect` and its "new
// connection" log line, which was both misleading and needless lock
// contention on the hot path.
if with_rtmp_bridge(|bridge| bridge.is_registered(conn_id)).unwrap_or(true) {
return;
}
RTMP_POLL_SERVER.with(|cell| {
let Some(server_ptr) = cell.get() else {
return;
};
if server_ptr.is_null() {
return;
}
// 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)
Comment on lines +107 to +111

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Avoid aliasing the RTMP server during callbacks

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));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Clean up pre-bookkeeping connection state

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 👍 / 👎.

Comment on lines +105 to +115

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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/null

Repository: 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.

});
}

pub(crate) fn rtmp_publish_cb(conn_id: u64, app: &str, stream_key: &str) -> bool {
ensure_conn_registered_for_auth(conn_id);
with_rtmp_bridge(|b| b.authorize_publish(conn_id, app, stream_key).is_ok()).unwrap_or(false)
}

pub(crate) fn rtmp_play_cb(conn_id: u64, app: &str, play_key: &str) -> bool {
ensure_conn_registered_for_auth(conn_id);
with_rtmp_bridge(|b| b.authorize_play(conn_id, app, play_key).is_ok()).unwrap_or(false)
}

Expand Down Expand Up @@ -543,7 +597,10 @@ impl ServerApp {
break;
}

if let Err(e) = server.poll(0) {
set_rtmp_poll_server(&mut server);
let poll_result = server.poll(0);
clear_rtmp_poll_server();
if let Err(e) = poll_result {
crate::log_warn!("RTMP polling stopped: {e}");
break;
}
Expand Down
9 changes: 6 additions & 3 deletions src/test_support.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ use crate::http::{self, AppState};
use crate::logger;
use crate::rtmp_bridge::{DbRtmpBridge, RtmpEventHandler};
use crate::server::{
POLL_INTERVAL_MS, RTMP_BRIDGE, TrackedConn, process_server_connections, rtmp_media_cb,
rtmp_play_cb, rtmp_publish_cb,
POLL_INTERVAL_MS, RTMP_BRIDGE, TrackedConn, clear_rtmp_poll_server, process_server_connections,
rtmp_media_cb, rtmp_play_cb, rtmp_publish_cb, set_rtmp_poll_server,
};

static TEST_RUNTIME: OnceLock<Runtime> = OnceLock::new();
Expand Down Expand Up @@ -140,7 +140,10 @@ impl TestServer {
break;
}

if server.poll(0).is_err() {
set_rtmp_poll_server(&mut server);
let poll_result = server.poll(0);
clear_rtmp_poll_server();
if poll_result.is_err() {
break;
}

Expand Down