Repository navigation
fix(mux,quic): clear the fmp4 and connection fuzz findings - #7961
Conversation
Quest-Claim: b2043dcd63eb88d1
fmp4: a `trun` without per-sample fields stores only a count, so a 104-byte moof planned 65,792 samples (7 MB) before any payload arrived, breaking the fuzz target's allocation bound. A fragment now declares at most one sample per byte of its metadata past 4,096, besides MAX_FRAGMENT_SAMPLES. connection: the harness stopped tainting a tampered datagram at the first packet whose tag held the first changed byte, so a later coalesced packet's corrupted DATAGRAM frame tripped the check. Later packets are now decoded. Commits the nightly inputs for both, and the 2026-11-04 cmaf OOM input that #7843 fixed. Updates the fuzz findings epic with the triage. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
changed the title
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 1 minute. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed head 6da6455b093f9237eeb39a6828417298fea7befe against base d2a7eadc2b218089fa3b59177d2003d9afbfcc97.
No actionable findings. The direction looks sound: the size-relative sample cap runs before metadata expansion and sample planning, retains the absolute cap, and has boundary and mixed-run coverage. Continuing past a packet whose tag was modified correctly lets the taint scan inspect subsequent coalesced packets. The committed regression paths are picked up by the existing replay tests.
Verification limits: GitHub-only static review of the diff and surrounding callers/tests. I did not run tests or fuzzers, or inspect/replay the binary crash inputs. The Check workflow for this head was queued at review time; the validation results in the PR description were not independently reproduced.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The per-byte bound also refuses valid, previously accepted audio imports, and the user-facing refusal list still names only the 131,072 cap.
Reviewed changes
- fMP4 sample budget —
metadata::Scan::finishnow refuses amoofdeclaring more thanmin(MAX_FRAGMENT_SAMPLES, 4096 + metadata bytes)samples, closing the field-lesstrunamplification before any sample is planned. Tests cover both sides of the new boundary, and a cap-sized fragment whose field-less samples are paid for by 4-byte entries. - QUIC fuzz harness —
Peer::taintnow continues past a coalesced packet whose tag holds the first changed byte instead of returning, so later packets' frames are marked forged. The change is monotone (it only adds markings), the inequality selects exactly the tag region, and the skipped packet's frame bytes are all beforefromhence unchanged, so it cannot cause a new false positive. - Regression inputs and records —
fmp4/fieldless-trun-count,cmaf/metadata-preflight, andconnection/crash-a0ef11af…, all replayed byfuzz::tests::regressions; the CHANGELOG entry and the epic triage.
I found no hole in either fix: every sample-plan site is reachable only after a checked moof, and the absolute cap keeps a fragment at this bound within MAX_HELD_BYTES. The inline comment is about the class of input the new ratio bound refuses.
ℹ️ The documented refusal list is now stale
doc/lib/rs/moq-mux.md's fMP4 row itemizes when an import is refused and still says only a moof of more than 131,072 samples is refused, so a reader can believe a much larger fragment is accepted. Add the per-byte rule there, and, if it stays, the practical minimum fragment length.
Technical details
# Stale fMP4 limit list in the user docs
## Affected sites
- `doc/lib/rs/moq-mux.md:20` — "a `moof` of more than 131,072 samples ... is refused" omits the ratio bound added at `rs/moq-mux/src/container/fmp4/metadata.rs:48`.
- `rs/moq-mux/CHANGELOG.md:25` — carries the new rule, but the library page is where the refusal conditions are listed together.
## Required outcome
A reader of the library page knows a short `moof` declaring thousands of samples is refused before parsing, not only one above 131,072.deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏
| if self.samples > MAX_FRAGMENT_SAMPLES { | ||
| /// Check the counts gathered from `len` bytes of metadata. | ||
| fn finish(self, len: usize) -> Result<()> { | ||
| if self.samples > MAX_FRAGMENT_SAMPLES.min(FRAGMENT_SAMPLE_BASE + len as u64) { |
There was a problem hiding this comment.
FRAGMENT_SAMPLE_BASE assumes a fragment is live and short, but this check also governs moq import fmp4 from a file or stdin, where a producer can put the whole stream in one moof. ffmpeg always writes tfhd default size and duration and adds per-sample fields only when a sample differs, while FF_MOV_FLAG_FRAG_KEYFRAME flushes only on video, so an audio-only AC-3 or CBR Opus file arrives as one field-less trun; at ~4,180 samples (≈2 min of AC-3) that file is now refused, where the 131,072 cap accepted ≈70 min.
Technical details
# A long field-less audio fragment is now refused
## Affected sites
- `rs/moq-mux/src/container/fmp4/metadata.rs:48` — `len` is the `moof` body on the streaming path (`import.rs:508`), so the budget is 4096 + body bytes.
- `rs/moq-mux/src/container/fmp4/mod.rs:53-59` — `FRAGMENT_SAMPLE_BASE` and its "A live fragment is a handful of samples" doc.
- `rs/moq-mux/src/container/fmp4/sample.rs:147` — a sample costs at least one `mdat` byte and the claimed ranges are disjoint, so the plan grows with delivered payload; only the reservation happens eagerly at `begin_mdat`.
## Concrete input
`ffmpeg -f lavfi -i sine=frequency=440:duration=180 -c:a ac3 -f mp4 -movflags empty_moov - | moq import fmp4` produces one ~90-byte `moof` whose `trun` carries only a count: `mov_write_tfhd_tag` sets `MOV_TFHD_DEFAULT_SIZE|MOV_TFHD_DEFAULT_DURATION`, `mov_write_trun_tag` adds `MOV_TRUN_SAMPLE_SIZE`/`_DURATION` only if a sample differs, and the `FF_MOV_FLAG_FRAG_KEYFRAME` flush requires `par->codec_type == AVMEDIA_TYPE_VIDEO` (movenc.c ~5734, ~5806, ~7630). AC-3 frames are fixed size and duration, so 5,760 samples > ~4,180.
## Required outcome
Either this file imports, or the limitation is recorded where users read the other refusal conditions (`doc/lib/rs/moq-mux.md`). The fuzz invariant itself is not at risk: one sample needs a byte of payload, so a plan grown with delivered bytes stays proportional.
## Open questions for the human
- Is a single-fragment audio file an intended import, or is the importer only for live/short fragments?
- If it is intended, could the plan be built as `mdat` bytes arrive (`begin_mdat` already has the declared length at `import.rs:1095`, and `write_mdat` walks chunks) instead of reserving it from the declared count?
Advances quest/m1/fuzz-findings/README.md ("Media fuzz findings", an
[S]watcher). Triages every Fuzz job that failed in the scheduled Nightly runs of 2026-11-03 to 2026-11-05, fixes the two that still fail on main, and updates the epic. The epic stays: it is deleted once the scheduled runs show every target green (decided 2026-11-05, maintainer).Triage per target
Each job's crash artifact was downloaded and replayed through the target's
fuzz::tests::regressionson current main.ts/step.rs:431ts/step-scan)annexb/scuffle-h265-transform-depth)None was intermittent: every failing input reproduced deterministically or was already fixed at a known commit, so nothing goes to m0's test flakes.
For ingest, the 11-04 (16-byte) and 11-05 (15-byte) stall inputs are newer than the one #7808 was written for. Both pass
fuzz::tests::regressionson its head63fdf85642, so they need no new quest.fmp4: field-less
truncountsThe input's last
moofis 104 bytes with onetrunwhose flags carry only a data offset and whose count is 65,792. Such a run stores just a count, andbegin_mdatplans every sample (about 113 bytes each) when themdatheader arrives, before any payload.MAX_FRAGMENT_SAMPLES(131,072) bounds that at about 26 MB but not in proportion to the input, which is what the fuzz target checks.Fix:
metadata::Scan::finishnow refuses a fragment declaring more thanmin(MAX_FRAGMENT_SAMPLES, 4096 + metadata bytes)samples with the existingError::TooManySamples. Atrunentry is at least 4 bytes, so only field-less runs can reach the per-byte limit; a fragment that stores entries is unaffected. One crate-private constant (FRAGMENT_SAMPLE_BASE), no new error variant.connection: harness taint stopped too early
Two
Corruptops hit one 106-byte datagram holding two coalesced packets (0..54 and 54..106). The harness tracks only the first changed byte (52), which lies in packet one's tag, andPeer::taintreturned there as if nothing later changed. The second change fell in packet two's DATAGRAM frame, which the connection delivered, and the harness counted it as a never-sent datagram.taintnow skips only that packet and goes on decoding later ones. Not a moq-quic bug: the connection delivered exactly the frame it received.Changes
rs/moq-mux/src/container/fmp4/{mod,metadata,import}.rs: the per-byte sample limit, docs, and tests.huge_trun_sample_counts_are_refusedcovers the limit and the value just past it.fragment_plans_within_the_held_boundnow expects a field-less run at the cap to be refused, and adds a cap-sized fragment whose field-less samples are paid for by 4-byte entries.counts_are_checked_across_runs_and_tracks_before_parsingpads themoofto reach the cap.rs/moq-quic/src/fuzz/connection.rs:return→continueintaint, with the doc corrected.rs/moq-mux/fuzz/regressions/fmp4/fieldless-trun-count(the nightly crash),rs/moq-mux/fuzz/regressions/cmaf/metadata-preflight(the 11-04 OOM), andrs/moq-quic/fuzz/regressions/connection/crash-a0ef11af….rs/moq-mux/CHANGELOG.md: Fixed entry.quest/m1/fuzz-findings/README.md: the triage, and what remains.Public API: no signature change.
fmp4::Importand the CMAF metadata checks now refuse amoofdeclaring more than 4,096 samples beyond one per metadata byte, withError::TooManySamples, whose message no longer namesMAX_FRAGMENT_SAMPLES. Wire: none.Choices made without asking
t3-cced8ddfabout three hours before. I replaced its approach (a separate field-less count, a newTooManyInheritedSamplesvariant, and three constants) with one limit on the existing sample count. The earlier worktree is untouched.1b68a5983c) replaying it ran out the test's time while allocating. On main it passes.Validation
fuzz::tests::regressionsfailed with "2036 input bytes allocated 7461897 bytes in 670 allocations" (fmp4) and "datagram 5373952 was never sent" (connection). Both pass on this branch.just rs fuzz fmp4 -- -max_total_time=300(569,306 runs) andjust rs fuzz connection -- -max_total_time=300(120,106 runs) on the final code: no findings.quest gate just check: passes.Review
quest review --base origin/main, Codex gpt-6.1-sol at medium effort, head6da6455b093f9237eeb39a6828417298fea7befe. 0 findings.Follow-ups
None.
(Written by Claude Opus 5.5)