fix(slice-1): F4 — idle timer keys on MediaData not every datagram
Adversarial review F4 (Med, confirmed): spec §4.5 specifies '60 s of no
RTP packets.' The implementation bumped `last_rx` on every datagram
`Receive::new` classified — including STUN consent checks and DTLS
keepalives. A peer that kept ICE alive but sent no audio (muted mic,
revoked permission) was never reaped within 60 s.
Move the bump to `handle_event::Event::MediaData` only — only
depacketized audio resets the timer. Spec §4.5 wording was already
correct (clarified in 8680f2b); the code now matches.
TDD via the new PacketIo harness:
- FakePackets (VecDeque-backed) injects an RTP-header-shaped datagram
(classifiable but not MediaData-bearing) without a network round-trip.
- RED: `f4_non_rtp_datagram_does_not_reset_idle_timer` asserted
`last_rx == initial`; pre-fix the bump in Step 1 on every classified
datagram made the test fail.
- GREEN: removed the Step-1 bump; added the bump in
`handle_event::Event::MediaData`. Test passes.
(Fixture rationale: STUN bytes don't work — `StunMessage::parse` needs
a USERNAME attribute that doesn't exist without a real handshake, so
`Receive::new` returns Err and the bump is skipped → false-pass.
RTP-header bytes multiplex as RTP without further validation, so they
reach `handle_input` — exactly the bug shape.)
This commit is contained in:
@@ -83,7 +83,14 @@ pub fn drive(session: &mut RtcSession, now: Instant) -> Option<Duration> {
|
|||||||
// Hot-path policy: drop + observe, don't crash.
|
// Hot-path policy: drop + observe, don't crash.
|
||||||
tracing::warn!("str0m rejected input packet; dropping");
|
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.
|
// WouldBlock (unix) / TimedOut (windows) — no packets this cycle.
|
||||||
Err(e) if matches!(e.kind(), ErrorKind::WouldBlock | ErrorKind::TimedOut) => break,
|
Err(e) if matches!(e.kind(), ErrorKind::WouldBlock | ErrorKind::TimedOut) => break,
|
||||||
@@ -225,7 +232,7 @@ pub fn drive(session: &mut RtcSession, now: Instant) -> Option<Duration> {
|
|||||||
}
|
}
|
||||||
|
|
||||||
/// Dispatch a str0m `Event` to the audio pipe or to state bookkeeping.
|
/// 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;
|
use str0m::Event;
|
||||||
match event {
|
match event {
|
||||||
Event::MediaData(media) => {
|
Event::MediaData(media) => {
|
||||||
@@ -237,6 +244,9 @@ fn handle_event(session: &mut RtcSession, event: str0m::Event, _now: Instant) {
|
|||||||
session.pipe.on_pcm_frame(pcm);
|
session.pipe.on_pcm_frame(pcm);
|
||||||
}
|
}
|
||||||
// Decode failed → drop + observe (per §3.8). Don't kill the peer.
|
// 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) => {
|
Event::IceConnectionStateChange(state) => {
|
||||||
tracing::info!(
|
tracing::info!(
|
||||||
|
|||||||
@@ -62,3 +62,75 @@ impl PacketIo for std::net::UdpSocket {
|
|||||||
std::net::UdpSocket::send_to(self, buf, dst)
|
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<u8>, 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<usize> {
|
||||||
|
// 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
|
||||||
|
];
|
||||||
|
}
|
||||||
|
|||||||
@@ -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
|
/// The old `assert!(audio_mid.is_none())` panicked on a path fed by
|
||||||
/// unauthenticated external input; the typed error makes the contract
|
/// unauthenticated external input; the typed error makes the contract
|
||||||
/// explicit. `debug_assert!` documents the invariant for tests.
|
/// 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]
|
#[test]
|
||||||
fn second_accept_offer_returns_already_negotiated_not_panic() {
|
fn second_accept_offer_returns_already_negotiated_not_panic() {
|
||||||
let mut session = RtcSession::new().expect("session");
|
let mut session = RtcSession::new().expect("session");
|
||||||
|
|||||||
Reference in New Issue
Block a user