Skip to content

Commit 600462f

Browse files
nightnessclaude
andcommitted
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) <noreply@anthropic.com>
1 parent 2f6261c commit 600462f

3 files changed

Lines changed: 38 additions & 4 deletions

File tree

rtc-interceptor/src/report/receiver_stream.rs

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,10 @@ use std::time::Instant;
44
const PACKETS_PER_ENTRY: usize = 64;
55

66
/// Maximum value for `total_lost` per RFC 3550 §6.4.1.
7-
/// The field is a signed 24-bit integer, so the positive maximum is 0x7F_FFFF.
7+
///
8+
/// The wire format is a signed 24-bit integer. We store it as `u32` but
9+
/// clamp to the signed-positive maximum (`0x7F_FFFF`) so the sign bit
10+
/// stays clear when the value is serialised into the 24-bit RTCP field.
811
const MAX_TOTAL_LOST: u32 = 0x7F_FFFF;
912

1013
pub(crate) struct ReceiverStream {

rtc-rtcp/src/receiver_report/receiver_report_test.rs

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -191,7 +191,31 @@ fn test_receiver_report_roundtrip() {
191191
None,
192192
),
193193
(
194-
"totallost overflow",
194+
"totallost at 24-bit max",
195+
ReceiverReport {
196+
ssrc: 1,
197+
reports: vec![ReceptionReport {
198+
total_lost: 0xFF_FFFF,
199+
..Default::default()
200+
}],
201+
..Default::default()
202+
},
203+
None,
204+
),
205+
(
206+
"totallost overflow at 24-bit boundary",
207+
ReceiverReport {
208+
ssrc: 1,
209+
reports: vec![ReceptionReport {
210+
total_lost: 0x100_0000,
211+
..Default::default()
212+
}],
213+
..Default::default()
214+
},
215+
Some(Error::InvalidTotalLost),
216+
),
217+
(
218+
"totallost overflow large",
195219
ReceiverReport {
196220
ssrc: 1,
197221
reports: vec![ReceptionReport {

rtc-rtcp/src/reception_report.rs

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,12 @@ pub struct ReceptionReport {
2929
pub fraction_lost: u8,
3030
/// The total number of RTP data packets from source SSRC that have
3131
/// been lost since the beginning of reception.
32+
///
33+
/// RFC 3550 §6.4.1 defines this as a signed 24-bit integer on the wire
34+
/// (negative values can occur when duplicates arrive). We store it as
35+
/// `u32` and clamp to `0x7F_FFFF` so the sign bit stays clear during
36+
/// serialisation. A future revision may switch to `i32` to fully
37+
/// represent the negative-loss (duplicate) case.
3238
pub total_lost: u32,
3339
/// The least significant 16 bits contain the highest sequence number received
3440
/// in an RTP data packet from source SSRC, and the most significant 16 bits extend
@@ -116,8 +122,9 @@ impl Marshal for ReceptionReport {
116122

117123
buf.put_u8(self.fraction_lost);
118124

119-
// pack TotalLost into 24 bits
120-
if self.total_lost >= (1 << 25) {
125+
// Pack TotalLost into 24 bits (RFC 3550 §6.4.1).
126+
// Values above 0xFF_FFFF cannot be represented in 24 bits.
127+
if self.total_lost > 0xFF_FFFF {
121128
return Err(Error::InvalidTotalLost);
122129
}
123130

0 commit comments

Comments
 (0)