diff --git a/crates/rutster-media/src/loop_driver.rs b/crates/rutster-media/src/loop_driver.rs index 69989e1..d647db8 100644 --- a/crates/rutster-media/src/loop_driver.rs +++ b/crates/rutster-media/src/loop_driver.rs @@ -83,7 +83,14 @@ pub fn drive(session: &mut RtcSession, now: Instant) -> Option { // Hot-path policy: drop + observe, don't crash. tracing::warn!("str0m rejected input packet; dropping"); } - session.last_rx = now; + // F4 (adversarial review): the previous code bumped + // `last_rx` here on every datagram `Receive::new` + // classified — including STUN consent checks and DTLS + // keepalives. Spec §4.5 specifies "no **RTP** packets"; + // only `Event::MediaData` (depacketized audio) advances + // the timer. The bump now lives in `handle_event`'s + // `Event::MediaData` arm, so a peer that keeps ICE + // alive but sends no audio is reaped on schedule. } // WouldBlock (unix) / TimedOut (windows) — no packets this cycle. Err(e) if matches!(e.kind(), ErrorKind::WouldBlock | ErrorKind::TimedOut) => break, @@ -225,7 +232,7 @@ pub fn drive(session: &mut RtcSession, now: Instant) -> Option { } /// Dispatch a str0m `Event` to the audio pipe or to state bookkeeping. -fn handle_event(session: &mut RtcSession, event: str0m::Event, _now: Instant) { +fn handle_event(session: &mut RtcSession, event: str0m::Event, now: Instant) { use str0m::Event; match event { Event::MediaData(media) => { @@ -237,6 +244,9 @@ fn handle_event(session: &mut RtcSession, event: str0m::Event, _now: Instant) { session.pipe.on_pcm_frame(pcm); } // Decode failed → drop + observe (per §3.8). Don't kill the peer. + // F4: idle timer keys off this — only depacketized audio resets + // the 60 s window per spec §4.5. STUN/DTLS keepalives don't. + session.last_rx = now; } Event::IceConnectionStateChange(state) => { tracing::info!( diff --git a/crates/rutster-media/src/packet_io.rs b/crates/rutster-media/src/packet_io.rs index e7742c4..db5f241 100644 --- a/crates/rutster-media/src/packet_io.rs +++ b/crates/rutster-media/src/packet_io.rs @@ -62,3 +62,75 @@ impl PacketIo for std::net::UdpSocket { std::net::UdpSocket::send_to(self, buf, dst) } } + +#[cfg(test)] +pub(crate) mod test_support { + //! Test fakes + fixtures for `loop_driver::drive()` regression tests + //! (slice-1 review "test-coverage gap" finding). Lives here so any + //! `#[cfg(test)] mod tests` in the crate can drive `drive()` with + //! synthetic packets without spinning up a real UDP loopback pair. + + use super::*; + use std::collections::VecDeque; + + /// In-memory fake `PacketIo`. Enqueue inbound datagrams via `enqueue`; + /// `recv_from` pops them in FIFO order and returns `WouldBlock` when + /// empty (mirroring `UdpSocket::set_nonblocking(true)`). Sends are + /// discarded — F4/F1's regression tests care only about the inbound + /// path. A test that needs to assert on outbound sends can extend + /// this fake to record them. + pub(crate) struct FakePackets { + incoming: VecDeque<(Vec, SocketAddr)>, + } + + impl FakePackets { + pub(crate) fn new() -> Self { + Self { + incoming: VecDeque::new(), + } + } + pub(crate) fn enqueue(&mut self, bytes: &[u8], src: SocketAddr) { + self.incoming.push_back((bytes.to_vec(), src)); + } + } + + impl PacketIo for FakePackets { + fn recv_from(&mut self, buf: &mut [u8]) -> io::Result<(usize, SocketAddr)> { + match self.incoming.pop_front() { + Some((bytes, src)) => { + let n = bytes.len().min(buf.len()); + buf[..n].copy_from_slice(&bytes[..n]); + Ok((n, src)) + } + None => Err(io::Error::from(io::ErrorKind::WouldBlock)), + } + } + fn send_to(&mut self, buf: &[u8], _dst: SocketAddr) -> io::Result { + // Discard outbound packets — str0m sends its own STUN/DTLS + // handshake packets on the first poll; the test doesn't care. + Ok(buf.len()) + } + } + + /// A 12-byte RTP-header-shaped datagram (`V=2, PT=8 PCMA, seq=1, + /// ts=0, ssrc=1`). str0m's `Receive::new` (`io/mod.rs:114-151`) + /// classifies any datagram with `byte0 ∈ [128, 192)` and `len > 2` + /// as RTP *without further validation* — that's the bug-surface for + /// F4: a non-RTP-payload datagram (or a stray RTP that str0m has no + /// configured PT for) reaches `handle_input` and bumps `last_rx` + /// regardless of whether `handle_input` succeeds (which is the bug, + /// since the spec says only **depacketized audio** — `Event::MediaData` + /// — should reset the timer). + /// + /// STUN bytes don't work here because `StunMessage::parse` requires + /// a USERNAME attribute that doesn't exist without a real handshake; + /// `Receive::new` returns Err and the bump is skipped (false-pass + /// on the test). RTP-shaped bytes are classifiable-but-non-MediaData — + /// exactly the bug shape the idle-timeout spec §4.5 warns against. + pub(crate) const RTP_HEADER_NO_PAYLOAD: [u8; 12] = [ + 0x80, 0x08, // V=2, P=0, X=0, CC=0; M=0; PT=8 (PCMA) + 0x00, 0x01, // Sequence number: 1 + 0x00, 0x00, 0x00, 0x00, // Timestamp: 0 + 0x00, 0x00, 0x00, 0x01, // SSRC: 1 + ]; +} diff --git a/crates/rutster-media/src/rtc_session.rs b/crates/rutster-media/src/rtc_session.rs index f7c3769..7cbd5da 100644 --- a/crates/rutster-media/src/rtc_session.rs +++ b/crates/rutster-media/src/rtc_session.rs @@ -389,6 +389,46 @@ a=candidate:1 1 UDP 2113667327 192.168.1.42 50000 typ host\r /// The old `assert!(audio_mid.is_none())` panicked on a path fed by /// unauthenticated external input; the typed error makes the contract /// explicit. `debug_assert!` documents the invariant for tests. + /// Adversarial review F4: the idle timeout (spec §4.5) is "60 s of no + /// **RTP packets**" — STUN consent checks and DTLS keepalives must NOT + /// reset the timer. Pre-fix `drive()` bumped `last_rx` on every + /// datagram `Receive::new` successfully classified (including STUN), + /// so a peer that kept ICE alive but sent no audio was never reaped + /// within 60 s. Post-fix the bump lives only in + /// `handle_event::Event::MediaData`. + #[test] + fn f4_non_rtp_datagram_does_not_reset_idle_timer() { + use crate::packet_io::test_support::{FakePackets, RTP_HEADER_NO_PAYLOAD}; + use std::net::SocketAddr; + use std::time::Duration; + + let mut fake = FakePackets::new(); + let stun_src: SocketAddr = "127.0.0.1:9999".parse().unwrap(); + fake.enqueue(&RTP_HEADER_NO_PAYLOAD, stun_src); + + let local_addr: SocketAddr = "127.0.0.1:0".parse().unwrap(); + let mut session = RtcSession::new_with_socket(Box::new(fake), local_addr); + let _ = session.accept_offer(BROWSER_SDP_OFFER).expect("offer"); + let initial_last_rx = session.last_rx; + + // Drive once at a later `now`; the RTP-shaped datagram is queued + // so Step 1's `recv_from` returns Ok, `Receive::new` classifies + // it as RTP, and `handle_input` rejects (no configured PT for + // ssrc=1 / no handshake). Pre-fix that still bumps `last_rx` + // (the bump happens regardless of handle_input's outcome, which + // is the bug). Post-fix the bump lives only in + // `handle_event::Event::MediaData` which never fires for an + // unconfigured PT → last_rx stays at initial. + let later = initial_last_rx + Duration::from_secs(10); + let _ = crate::loop_driver::drive(&mut session, later); + + assert_eq!( + session.last_rx, initial_last_rx, + "F4: non-RTP datagrams (STUN/DTLS) must NOT reset the idle timer; \ + only Event::MediaData should bump last_rx (spec §4.5)" + ); + } + #[test] fn second_accept_offer_returns_already_negotiated_not_panic() { let mut session = RtcSession::new().expect("session");