From a00343334c96e400dba9d21f567e90320071c835 Mon Sep 17 00:00:00 2001 From: nightness Date: Wed, 1 Apr 2026 07:13:32 -0500 Subject: [PATCH 1/3] fix(interceptor): clamp total_lost to signed 24-bit max per RFC 3550 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RFC 3550 §6.4.1 defines total_lost as a 24-bit signed integer. The previous clamp used 0xFFFFFF (unsigned 24-bit max = 16,777,215) which allowed values that overflow the signed 24-bit range. Fix the clamp to 0x7FFFFF (signed 24-bit positive max = 8,388,607) so values are always representable in the reception report field. --- rtc-interceptor/src/report/receiver_stream.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/rtc-interceptor/src/report/receiver_stream.rs b/rtc-interceptor/src/report/receiver_stream.rs index 578a1cd7..ca8f9728 100644 --- a/rtc-interceptor/src/report/receiver_stream.rs +++ b/rtc-interceptor/src/report/receiver_stream.rs @@ -139,12 +139,12 @@ impl ReceiverStream { self.total_lost += total_lost_since_report; - // allow up to 24 bits - if total_lost_since_report > 0xFFFFFF { - total_lost_since_report = 0xFFFFFF; + // allow up to signed 24 bits (RFC 3550 §6.4.1: total_lost is i32) + if total_lost_since_report > 0x7FFFFF { + total_lost_since_report = 0x7FFFFF; } - if self.total_lost > 0xFFFFFF { - self.total_lost = 0xFFFFFF + if self.total_lost > 0x7FFFFF { + self.total_lost = 0x7FFFFF } // Calculate DLSR (Delay Since Last SR) - RFC 3550 From 7f862555ae1d160728516919c515eb503dfe6964 Mon Sep 17 00:00:00 2001 From: nightness Date: Wed, 8 Apr 2026 01:41:53 -0500 Subject: [PATCH 2/3] address review feedback for PR #77 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Introduce MAX_TOTAL_LOST named constant (0x7F_FFFF) for the signed 24-bit ceiling per RFC 3550 §6.4.1 - Clamp total_lost_since_report before adding to self.total_lost, and use saturating_add to prevent overflow/wrap - Fix test assertion from 0xFFFFFF to MAX_TOTAL_LOST (0x7F_FFFF) Co-Authored-By: Claude Opus 4.6 (1M context) --- rtc-interceptor/src/report/receiver_stream.rs | 27 ++++++++++--------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/rtc-interceptor/src/report/receiver_stream.rs b/rtc-interceptor/src/report/receiver_stream.rs index ca8f9728..f46cd329 100644 --- a/rtc-interceptor/src/report/receiver_stream.rs +++ b/rtc-interceptor/src/report/receiver_stream.rs @@ -3,6 +3,10 @@ use std::time::Instant; /// Number of packets tracked per u64 entry in the bitmap. const PACKETS_PER_ENTRY: usize = 64; +/// Maximum value for `total_lost` per RFC 3550 §6.4.1. +/// The field is a signed 24-bit integer, so the positive maximum is 0x7F_FFFF. +const MAX_TOTAL_LOST: u32 = 0x7F_FFFF; + pub(crate) struct ReceiverStream { ssrc: u32, receiver_ssrc: u32, @@ -137,15 +141,12 @@ impl ReceiverStream { } }; - self.total_lost += total_lost_since_report; - - // allow up to signed 24 bits (RFC 3550 §6.4.1: total_lost is i32) - if total_lost_since_report > 0x7FFFFF { - total_lost_since_report = 0x7FFFFF; - } - if self.total_lost > 0x7FFFFF { - self.total_lost = 0x7FFFFF - } + // Clamp before adding to prevent overflow (RFC 3550 §6.4.1: signed 24-bit max) + total_lost_since_report = total_lost_since_report.min(MAX_TOTAL_LOST); + self.total_lost = self + .total_lost + .saturating_add(total_lost_since_report) + .min(MAX_TOTAL_LOST); // Calculate DLSR (Delay Since Last SR) - RFC 3550 // Return 0 if no SR has been received yet @@ -461,9 +462,9 @@ mod tests { #[test] fn test_receiver_stream_24bit_loss_clamping() { - // Test that total_lost is clamped to 24 bits (0xFFFFFF) + // Test that total_lost is clamped to signed 24-bit max (MAX_TOTAL_LOST) let mut stream = ReceiverStream::new(123456, 90000); - stream.total_lost = 0xFFFFFE; // Almost at max + stream.total_lost = MAX_TOTAL_LOST - 1; // Almost at max let now = Instant::now(); @@ -473,7 +474,7 @@ mod tests { let rr = stream.generate_report(now); - // Should be clamped to 0xFFFFFF - assert_eq!(rr.reports[0].total_lost, 0xFFFFFF); + // Should be clamped to MAX_TOTAL_LOST (0x7F_FFFF per RFC 3550 §6.4.1) + assert_eq!(rr.reports[0].total_lost, MAX_TOTAL_LOST); } } From 6a8d1647b6d5d18fe01aee74d069f85820fa2532 Mon Sep 17 00:00:00 2001 From: nightness Date: Wed, 8 Apr 2026 09:24:33 -0500 Subject: [PATCH 3/3] fix: address Copilot review comments on PR #77 - Reword MAX_TOTAL_LOST doc to explain u32 storage clamped to signed 24-bit positive range for RFC 3550 wire format - Add matching doc on ReceptionReport.total_lost field - Fix marshal bounds check from (1 << 25) to 0xFF_FFFF (correct 24-bit limit) - Add boundary tests for 24-bit marshal limit (0xFF_FFFF ok, 0x100_0000 rejected) Co-Authored-By: Claude Opus 4.6 (1M context) --- rtc-interceptor/src/report/receiver_stream.rs | 5 +++- .../receiver_report/receiver_report_test.rs | 26 ++++++++++++++++++- rtc-rtcp/src/reception_report.rs | 11 ++++++-- 3 files changed, 38 insertions(+), 4 deletions(-) diff --git a/rtc-interceptor/src/report/receiver_stream.rs b/rtc-interceptor/src/report/receiver_stream.rs index f46cd329..ec1c538a 100644 --- a/rtc-interceptor/src/report/receiver_stream.rs +++ b/rtc-interceptor/src/report/receiver_stream.rs @@ -4,7 +4,10 @@ use std::time::Instant; const PACKETS_PER_ENTRY: usize = 64; /// Maximum value for `total_lost` per RFC 3550 §6.4.1. -/// The field is a signed 24-bit integer, so the positive maximum is 0x7F_FFFF. +/// +/// The wire format is a signed 24-bit integer. We store it as `u32` but +/// clamp to the signed-positive maximum (`0x7F_FFFF`) so the sign bit +/// stays clear when the value is serialised into the 24-bit RTCP field. const MAX_TOTAL_LOST: u32 = 0x7F_FFFF; pub(crate) struct ReceiverStream { diff --git a/rtc-rtcp/src/receiver_report/receiver_report_test.rs b/rtc-rtcp/src/receiver_report/receiver_report_test.rs index 22450a54..9524bd6a 100644 --- a/rtc-rtcp/src/receiver_report/receiver_report_test.rs +++ b/rtc-rtcp/src/receiver_report/receiver_report_test.rs @@ -191,7 +191,31 @@ fn test_receiver_report_roundtrip() { None, ), ( - "totallost overflow", + "totallost at 24-bit max", + ReceiverReport { + ssrc: 1, + reports: vec![ReceptionReport { + total_lost: 0xFF_FFFF, + ..Default::default() + }], + ..Default::default() + }, + None, + ), + ( + "totallost overflow at 24-bit boundary", + ReceiverReport { + ssrc: 1, + reports: vec![ReceptionReport { + total_lost: 0x100_0000, + ..Default::default() + }], + ..Default::default() + }, + Some(Error::InvalidTotalLost), + ), + ( + "totallost overflow large", ReceiverReport { ssrc: 1, reports: vec![ReceptionReport { diff --git a/rtc-rtcp/src/reception_report.rs b/rtc-rtcp/src/reception_report.rs index a873dafa..300f1b69 100644 --- a/rtc-rtcp/src/reception_report.rs +++ b/rtc-rtcp/src/reception_report.rs @@ -29,6 +29,12 @@ pub struct ReceptionReport { pub fraction_lost: u8, /// The total number of RTP data packets from source SSRC that have /// been lost since the beginning of reception. + /// + /// RFC 3550 §6.4.1 defines this as a signed 24-bit integer on the wire + /// (negative values can occur when duplicates arrive). We store it as + /// `u32` and clamp to `0x7F_FFFF` so the sign bit stays clear during + /// serialisation. A future revision may switch to `i32` to fully + /// represent the negative-loss (duplicate) case. pub total_lost: u32, /// The least significant 16 bits contain the highest sequence number received /// in an RTP data packet from source SSRC, and the most significant 16 bits extend @@ -116,8 +122,9 @@ impl Marshal for ReceptionReport { buf.put_u8(self.fraction_lost); - // pack TotalLost into 24 bits - if self.total_lost >= (1 << 25) { + // Pack TotalLost into 24 bits (RFC 3550 §6.4.1). + // Values above 0xFF_FFFF cannot be represented in 24 bits. + if self.total_lost > 0xFF_FFFF { return Err(Error::InvalidTotalLost); }