Skip to content

Commit ddc9eb7

Browse files
kixelatedclaude
andauthored
fix(moq-net): bound frame size in create_frame/append_frame (#1882)
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 parent a0f0f4a commit ddc9eb7

2 files changed

Lines changed: 30 additions & 4 deletions

File tree

rs/moq-net/src/model/frame.rs

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,11 @@ use crate::{Error, Result};
1717
/// a group. 16 MiB was too tight for a high-bitrate CMAF fragment carried as one
1818
/// frame; 32 MiB covers that while keeping the per-frame preallocation bounded.
1919
///
20-
// TODO enforce this in [Frame::produce] / [FrameProducer::new] so the limit is
21-
// guaranteed for every caller, not just the wire decode paths. Blocked on
22-
// making the constructor fallible (returning [Result]), which is an API break.
20+
/// Enforced on the wire decode (above) and in
21+
/// [`GroupProducer::create_frame`](super::group::GroupProducer::create_frame), which rejects an
22+
/// oversized frame *before* `produce()` allocates its buffer. A direct `FrameProducer::new` still
23+
/// allocates ahead of the [`append_frame`](super::group::GroupProducer::append_frame) backstop;
24+
/// closing that gap means a fallible constructor, which is a breaking change left to a separate `dev` PR.
2325
pub(crate) const MAX_FRAME_SIZE: u64 = 32 * 1024 * 1024;
2426

2527
/// A chunk of data with an upfront size.

rs/moq-net/src/model/group.rs

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ use std::task::{Poll, ready};
1212

1313
use bytes::Bytes;
1414

15-
use crate::{Error, Result};
15+
use crate::{Error, MAX_FRAME_SIZE, Result};
1616

1717
use super::{Frame, FrameConsumer, FrameProducer};
1818

@@ -179,13 +179,23 @@ impl GroupProducer {
179179

180180
/// Create a frame with an upfront size
181181
pub fn create_frame(&mut self, info: Frame) -> Result<FrameProducer> {
182+
// Reject before `produce()`: `FrameProducer::new` preallocates `size` bytes, so an oversized
183+
// frame must be caught here or it triggers the very allocation the limit exists to prevent.
184+
if info.size > MAX_FRAME_SIZE {
185+
return Err(Error::FrameTooLarge);
186+
}
182187
let frame = info.produce();
183188
self.append_frame(frame.clone())?;
184189
Ok(frame)
185190
}
186191

187192
/// Append a frame producer to the group.
188193
pub fn append_frame(&mut self, frame: FrameProducer) -> Result<()> {
194+
// Backstop for direct callers (the buffer is already allocated by the time we hold a
195+
// FrameProducer); `create_frame` is the path that rejects before allocating.
196+
if frame.size > MAX_FRAME_SIZE {
197+
return Err(Error::FrameTooLarge);
198+
}
189199
let mut state = modify(&self.state)?;
190200
if state.fin {
191201
return Err(Error::Closed);
@@ -441,6 +451,20 @@ mod test {
441451
assert_eq!(chunks[0], Bytes::from_static(b"helloworld"));
442452
}
443453

454+
#[test]
455+
fn append_rejects_oversized_frame() {
456+
let mut producer = Group { sequence: 0 }.produce();
457+
let err = producer.create_frame(Frame {
458+
size: MAX_FRAME_SIZE + 1,
459+
});
460+
assert!(
461+
matches!(err, Err(Error::FrameTooLarge)),
462+
"a frame over the limit is rejected"
463+
);
464+
// A frame at the limit is still accepted.
465+
assert!(producer.create_frame(Frame { size: MAX_FRAME_SIZE }).is_ok());
466+
}
467+
444468
#[test]
445469
fn get_frame_by_index() {
446470
let mut producer = Group { sequence: 0 }.produce();

0 commit comments

Comments
 (0)