Fix fifteen defects across perf, resource, panics and key hygiene
All 21 findings held up under verification; 15 fixed here, 6 deferred to files another agent held this round, 0 rejected. **A defect in my own round-2 probe fix.** CHUNK_SECTORS was 1024, and 1024 % 3 == 1 — verified — so every chunk after the first was misaligned against the 6144-byte AACS aligned unit and would be REJECTED by DecryptingSectorSource's alignment gate. On an encrypted disc the forced-subtitle probe I added last round would have read almost nothing past its first chunk. Now 1023 sectors (341 aligned units) with a const assertion that fails the build if it stops dividing, plus set_unit_base per extent so the source's gate is anchored where the extent actually starts. **The same probe skipped sectors on a short read**, advancing by the REQUESTED count rather than the bytes actually returned, so a partial read silently left a gap in the middle of the evidence. It now advances by n/SECTOR_BYTES and clamps n to the buffer. **Its cache key omitted the PGS PID set**, so a playlist declaring an extra subtitle PID got another playlist's verdict for a track that had never been probed. And the key was the whole extent list, so partial clip sharing missed entirely. Both fixed by keying (start_lba, sector_count, pid) — and per-extent keying was shown SOUND rather than assumed: ForcedTracker is two monotone booleans, so per-extent evidence composes by field-wise OR, order- and grouping-independently. Making that honest required per-extent demux state, so an extent's evidence comes only from its own bytes, and memoising only extents whose read reached a designed stop. **A reachable panic in the timeline.** mkvstream::parse_block accepts a TimestampScale up to i64::MAX, so a video frame can set high_ns = i64::MAX and the next passive frame panicked adding the backstep. In release it wrapped negative instead, firing the straggler clamp for essentially every passive frame — audio and subtitles rewritten onto the wrong point of the output timeline. All four sites saturate. **A public constructor divided by zero**: PrefetchedSectorSource::new_with_events with unit_align == 0. Now InvalidInput, matching its batch_sectors sibling. **Two Debug impls printed key material.** DiscInputs (volume_id, mkb, unit_key_ro, samples) and UnitKeyFile both derived Debug. Nothing logs them today — fixed as prevention, because the next tracing::debug! someone adds is the leak. A doc claim that DiscInputs "contains no secrets" was false and is corrected. **An env-var multiply could overflow** in file_sector_source; now bounded at 64 GiB like its writeback sibling, with the parse split out so the bound is testable without touching process env. **The mp4 demuxer allowed one sample per file byte** — ~64x RAM amplification. Now file_len/16, since only vide/soun tracks are indexed and the shortest legal AC-3 frame is 128 bytes. **Two pipeline concurrency defects**: a consumer apply() error was invisible to the producer, and abandon/finalise had a TOCTOU where a caller could report an unfinalised output. Both fixed with compare-exchange state rather than a bool. **Two per-frame copies removed**, both MEASURED rather than reasoned: the AU assembler now hands its allocation to the frame (same pointer, unchanged capacity, proven by asserting the pointer) and tsmux reuses one Annex-B buffer across frames. Both keep capacity deliberately — a naive split_off would have cost more than it saved. **A comment pointed at the wrong file** for a mirrored constant; the mirror is now compiler-enforced with a const assertion converting 90 kHz ticks to ns, so drift fails the build. Deferred to another agent's files, all confirmed: detect_rate's fractional-twin snap, the mp4 reserve's u32 truncation, round_up_grain's overflow, the quadratic base-key gap fill, and MkvStream's frame cap counting frames rather than bytes. Every fix verified red by reverting it. Also noted for later: DecodeSampleSet still derives Debug over multi-MB of on-disc ciphertext.
This commit is contained in:
Matthew Jackson
+70
-15
@@ -35,6 +35,17 @@ const MAX_TRACKS: usize = 512;
|
||||
/// hostile file's allocation without truncating any legitimate track.
|
||||
const MAX_SAMPLE_COUNT: usize = 1 << 24;
|
||||
|
||||
/// Smallest number of FILE bytes one indexed sample is assumed to occupy — the
|
||||
/// divisor that turns `file_len` into a sample-count budget (see `from_reader`).
|
||||
///
|
||||
/// Only `vide` and `soun` tracks are indexed here, and no real coded video or
|
||||
/// audio access unit is anywhere near this small: the shortest legal AC-3 frame is
|
||||
/// 128 bytes, an AAC frame is hundreds, and a video sample carries at least a NAL
|
||||
/// header plus slice data. A 2-hour title runs to thousands of file bytes per
|
||||
/// sample, so this cannot truncate a genuine track — it only stops a crafted
|
||||
/// sample table from claiming more samples than the file could possibly hold.
|
||||
const MIN_FILE_BYTES_PER_SAMPLE: u64 = 16;
|
||||
|
||||
/// Absolute ceiling on a single allocation sized from an untrusted MP4 field (a
|
||||
/// per-sample buffer or the `moov` payload). The EOF check alone is not enough:
|
||||
/// `file_len` is cheaply inflatable with a sparse file (`truncate -s 8G`), so a
|
||||
@@ -98,13 +109,16 @@ impl<R: Read + Seek> Mp4Reader<R> {
|
||||
// crafted file with a fixed-size `stsz` claiming count=0xFFFFFFFF can't
|
||||
// inflate the `sizes`/`Vec<SampleRef>` allocations past the file's own size
|
||||
// (a genuine large title has file_len ≫ sample count, so it is unaffected).
|
||||
// NOTE on the bound this actually gives: each indexed sample costs about
|
||||
// 52 bytes of RAM (SampleRef 40 + u32 size 4 + u64 offset 8, plus 4 each
|
||||
// for the expanded stts/ctts), so the ceiling is ~52x file_len, not 1x —
|
||||
// capped by MAX_SAMPLE_COUNT. That is still a real bound (a 1 MiB crafted
|
||||
// file cannot reach the 16M-sample ceiling), just not the "past the file's
|
||||
// own size" the previous comment implied.
|
||||
let mut sample_budget = MAX_SAMPLE_COUNT.min(file_len.min(usize::MAX as u64) as usize);
|
||||
// The bound is in file BYTES PER SAMPLE, not in samples: each indexed
|
||||
// sample costs ~60 bytes of RAM (SampleRef 40 + u32 size 4 + u64 offset 8,
|
||||
// plus 4 each for the expanded stts/ctts), so a budget of one sample per
|
||||
// file byte still let a 16 MiB crafted file (a fixed-size `stsz` declaring
|
||||
// 16M one-byte samples, a one-entry stsc/stco/stts) force a ~1 GiB eager
|
||||
// allocation and a 16M-element sort before a single frame was read — a 64x
|
||||
// amplification. Dividing by MIN_FILE_BYTES_PER_SAMPLE caps the
|
||||
// amplification at ~4x instead.
|
||||
let mut sample_budget = MAX_SAMPLE_COUNT
|
||||
.min((file_len / MIN_FILE_BYTES_PER_SAMPLE).min(usize::MAX as u64) as usize);
|
||||
|
||||
// Bound the scan at MAX_TRACKS *matches* so a crafted moov packed with tiny
|
||||
// (8-byte) trak headers can't force the scan to materialize a Vec far
|
||||
@@ -1341,10 +1355,10 @@ mod tests {
|
||||
/// NOT the 16M `MAX_SAMPLE_COUNT` ceiling. Mutation check: revert the budget
|
||||
/// to a bare `MAX_SAMPLE_COUNT` and this file yields ~16M samples, failing the
|
||||
/// `<= file_len` (and `< MAX_SAMPLE_COUNT`) assertions below.
|
||||
#[test]
|
||||
fn stsz_sample_count_bounded_by_file_len() {
|
||||
use std::io::Cursor;
|
||||
// A minimal audio trak, but with a fixed-size stsz lying about its count.
|
||||
/// A minimal-but-complete audio `trak` whose fixed-size `stsz` LIES about its
|
||||
/// sample count (`u32::MAX`), with an stsc/stts wide enough to place whatever
|
||||
/// count survives the budget. The shape the sample-table budget bounds.
|
||||
fn audio_trak_hostile_count() -> Vec<u8> {
|
||||
let mdhd = {
|
||||
let mut p = vec![0u8; 24];
|
||||
p[12..16].copy_from_slice(&48_000u32.to_be_bytes()); // timescale
|
||||
@@ -1410,6 +1424,13 @@ mod tests {
|
||||
mdia.extend_from_slice(&hdlr);
|
||||
mdia.extend_from_slice(&minf);
|
||||
let trak = mp4_box(b"trak", &mp4_box(b"mdia", &mdia));
|
||||
trak
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn stsz_sample_count_bounded_by_file_len() {
|
||||
use std::io::Cursor;
|
||||
let trak = audio_trak_hostile_count();
|
||||
let moov = mp4_box(b"moov", &trak);
|
||||
|
||||
let file_len = moov.len() as u64;
|
||||
@@ -1418,11 +1439,16 @@ mod tests {
|
||||
"fixture stays a few hundred bytes ({file_len})"
|
||||
);
|
||||
let rd = Mp4Reader::from_reader(Cursor::new(moov), "hostile".into()).unwrap();
|
||||
// The index must be bounded by the file's byte length, NOT the 16M ceiling.
|
||||
// The index must be bounded by the file's byte length in FILE BYTES PER
|
||||
// SAMPLE, not one sample per byte: each indexed sample costs ~60 bytes of
|
||||
// RAM, so a one-sample-per-byte budget still let a 16 MiB crafted file
|
||||
// force a ~1 GiB allocation (64x amplification) before a frame was read.
|
||||
assert!(
|
||||
(rd.samples.len() as u64) <= file_len,
|
||||
"sample count {} must be bounded by file_len {file_len}, not the count lie",
|
||||
rd.samples.len()
|
||||
(rd.samples.len() as u64) <= file_len / MIN_FILE_BYTES_PER_SAMPLE,
|
||||
"sample count {} must be bounded by file_len/{MIN_FILE_BYTES_PER_SAMPLE} \
|
||||
({}), not by the count lie",
|
||||
rd.samples.len(),
|
||||
file_len / MIN_FILE_BYTES_PER_SAMPLE
|
||||
);
|
||||
assert!(
|
||||
rd.samples.len() < MAX_SAMPLE_COUNT,
|
||||
@@ -1430,6 +1456,35 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
/// The RAM amplification the byte-per-sample budget actually bounds: a small
|
||||
/// crafted file whose `stsz` claims `u32::MAX` samples must not force an eager
|
||||
/// multi-hundred-MB index. At ~60 bytes of RAM per indexed sample, a budget of
|
||||
/// one sample per file byte gave ~60x the input size; the assertion below pins
|
||||
/// the amplification factor rather than a raw count, so it fails if the budget
|
||||
/// ever goes back to counting samples per byte.
|
||||
#[test]
|
||||
fn sample_index_ram_is_bounded_by_a_multiple_of_the_file() {
|
||||
use std::io::Cursor;
|
||||
// ~64 KiB of `free` padding so file_len is large enough for the ratio to
|
||||
// be meaningful, with the same lying stsz as above.
|
||||
let mut traks = Vec::new();
|
||||
traks.extend_from_slice(&audio_trak_hostile_count());
|
||||
let mut file = mp4_box(b"moov", &traks);
|
||||
file.extend_from_slice(&mp4_box(b"free", &vec![0u8; 64 * 1024]));
|
||||
let file_len = file.len() as u64;
|
||||
|
||||
let rd = Mp4Reader::from_reader(Cursor::new(file), "amp".into()).unwrap();
|
||||
// ~60 bytes of RAM per sample; assert the index cannot exceed ~4x the file.
|
||||
const RAM_PER_SAMPLE: u64 = 60;
|
||||
let ram = rd.samples.len() as u64 * RAM_PER_SAMPLE;
|
||||
assert!(
|
||||
ram <= file_len * 4,
|
||||
"sample index RAM {ram} B from a {file_len} B file is more than 4x \
|
||||
amplification ({} samples)",
|
||||
rd.samples.len()
|
||||
);
|
||||
}
|
||||
|
||||
/// Build an audio `trak` identical to `audio_trak(48_000)` but with the
|
||||
/// named stbl child box omitted. Used to reach the untrusted-input guards
|
||||
/// that drop a track whose `stsz` says samples exist yet whose `stco`/`co64`
|
||||
|
||||
Reference in New Issue
Block a user