From 5c6a6d0785628e3b80c7f2687e49943db19d0ee1 Mon Sep 17 00:00:00 2001 From: Matthew Jackson <1085847+MattJackson@users.noreply.github.com> Date: Wed, 29 Jul 2026 22:28:43 -0700 Subject: [PATCH] Round 5: reject a degenerate fixed lace, bound the pending buffer by bytes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five fixes. Three are real defects with regression tests; two are bounds that were expressible but not expressed. A fixed-size lace (RFC 9559 §10.3.4) whose body is empty declared n frames and carried none. The divisibility check passed, because 0 % n is 0, and `chunks` yields nothing on an empty slice whatever width it is given — so the clamp that existed to avoid chunks(0) returned zero frames where the Lacing Head said n. The whole lace vanished with no error raised and the caller saw a clean short block. A zero-size frame cannot be a valid frame, so it is now malformed. A disc read failure while fetching a directory entry's ICB became a file size of zero rather than an error. Zero is indistinguishable from a genuinely empty file, so an unreadable ICB on a damaged disc silently changed which titles a caller saw as present — read_directory already fails hard on its entry-budget guard, so propagating is also what the surrounding code does. read_file_size still returns Ok(0) for an ICB whose tag is neither File Entry nor Extended File Entry, which is a real zero and not a failure. The pending-frame buffer was capped at 4096 frames, which does not bound memory: frames are arbitrarily large and a UHD video frame runs to a few hundred KB, so the existing cap permitted over a gigabyte. Now bounded by bytes as well, at 64 MiB. round_up_grain overflowed for inputs within one grain of u64::MAX — div_ceil then multiply — and the wrapped product is small, turning the largest possible estimate into a negligible reserve. It saturates, and the reserve is clamped to what a `free` box's 32-bit size field can actually hold, since writing a larger one truncated the size and left mdat beyond a box claiming to be far shorter. No real title comes close; a 90 GB UHD title estimates a few MiB. The AC-3 resync guard now advances the PTS cadence like both of its sibling branches, so the three paths out of that block cannot disagree. This one is defensive and has NO test: reaching it needs input that both parses frames and leaves a megabyte of residue, and the parser's own carry rules drop pre-sync junk and cap a partial frame at 8192 bytes, so no such input was found. Stated here rather than covered by a test that would pass either way. Two findings from this round were rejected on inspection. A reported panic in the .mpls suffix check does not exist: the `.get(..)` on the line above returns None off a char boundary and `filter` never runs its closure, so the byte index is unreachable. A test written for it passed against the unfixed code, which is what surfaced the error. --- src/mux/codec/ac3.rs | 6 ++++ src/mux/mkvstream.rs | 56 +++++++++++++++++++++++++++--- src/mux/mp4/mod.rs | 30 ++++++++++++++-- src/udf.rs | 81 ++++++++++++++++++++++++++++++++++++++++++-- 4 files changed, 165 insertions(+), 8 deletions(-) diff --git a/src/mux/codec/ac3.rs b/src/mux/codec/ac3.rs index 3c56593..4ac4da5 100644 --- a/src/mux/codec/ac3.rs +++ b/src/mux/codec/ac3.rs @@ -507,6 +507,12 @@ impl CodecParser for Ac3Parser { MAX_AC3_BUF ); self.buf.clear(); + // Advance the cadence, as both sibling branches below do, so the + // three paths out of this block cannot disagree. Defensive: no + // input reaching this parser was found that both parses frames and + // leaves a residue this large, so the stale-cadence bug this + // prevents is not currently reachable and has no regression test. + self.flush_pts_ns = frame_pts_ns; } else { self.buf = tail.to_vec(); // The carried bytes, when later completed and emitted (next call diff --git a/src/mux/mkvstream.rs b/src/mux/mkvstream.rs index a6d8493..18a8643 100644 --- a/src/mux/mkvstream.rs +++ b/src/mux/mkvstream.rs @@ -98,6 +98,11 @@ struct ReadState { /// stream — past it we build with no measured field order (logged) rather than /// buffer unbounded. const MAX_PENDING_FRAMES: usize = 4096; +/// Companion byte cap on the same pending buffer. The frame count alone does not +/// bound memory: frames are arbitrarily large, and a UHD video frame runs to a +/// few hundred KB, so 4096 of them is over a gigabyte. 64 MiB is far more than +/// the handful of frames a real audio-only prefix produces, and finite. +const MAX_PENDING_BYTES: usize = 64 << 20; enum Mode { Write(WriteMode), @@ -140,6 +145,10 @@ struct PendingMux { /// carries an optional MVC dependent-view `BlockAdditional` (present only /// for a 3D base-view frame that was already paired before activation). buffered: Vec<(crate::pes::PesFrame, Option>)>, + /// Running payload total of `buffered`, so the cap can bound bytes and not + /// only frame count. Maintained on push; `buffered` is drained exactly once, + /// at activation, after which neither field is consulted again. + buffered_bytes: usize, } /// Matroska container stream. @@ -439,6 +448,7 @@ impl MkvStream { video_track, opening_capture_path: output_path.map(|p| p.to_path_buf()), buffered: Vec::new(), + buffered_bytes: 0, }))), }) } @@ -535,7 +545,9 @@ impl MkvStream { // No video track: nothing to wait for — build on frame one. None => true, }; - (is_video || p.buffered.len() >= MAX_PENDING_FRAMES, is_video) + let capped = + p.buffered.len() >= MAX_PENDING_FRAMES || p.buffered_bytes >= MAX_PENDING_BYTES; + (is_video || capped, is_video) } _ => unreachable!("guarded above"), }; @@ -550,8 +562,11 @@ impl MkvStream { Ok(()) } else { if let Mode::Write(WriteMode::Pending(p)) = &mut self.mode { - p.buffered - .push((frame.clone(), additional.map(|a| a.to_vec()))); + let add = additional.map(|a| a.to_vec()); + p.buffered_bytes = p + .buffered_bytes + .saturating_add(frame.data.len() + add.as_ref().map_or(0, |a| a.len())); + p.buffered.push((frame.clone(), add)); } Ok(()) } @@ -1393,7 +1408,17 @@ fn split_lacing(lacing: u8, body: &[u8]) -> Option> { return None; } let each = rest.len() / n; - return Some(rest.chunks(each.max(1)).take(n).collect()); + // A zero-size frame carries no data and cannot be a valid frame, so + // `each == 0` is malformed rather than "n empty frames". It has to be + // rejected explicitly: 0 % n == 0 passes the divisibility check above, + // and `chunks` on an empty slice yields nothing whatever its argument, + // so clamping the chunk width instead returned Some(vec![]) — zero + // frames where the Lacing Head declared n, dropping the whole lace + // silently with no error raised. + if each == 0 { + return None; + } + return Some(rest.chunks(each).take(n).collect()); } LACING_XIPH => { // §10.3.2: each size is a run of 0xFF octets (255 each) terminated @@ -1610,6 +1635,29 @@ fn block_vint(d: &[u8]) -> (u64, usize) { #[cfg(test)] mod tests { + + /// A fixed lace whose body is empty declares n frames and carries none. It is + /// malformed, and must be rejected rather than silently yielding zero frames: + /// 0 % n == 0 passes the divisibility check, and `chunks` on an empty slice + /// yields nothing whatever width it is given, so the whole lace vanished with + /// no error raised and the caller saw a clean short block. + #[test] + fn fixed_lacing_with_an_empty_body_is_malformed_not_zero_frames() { + // Lacing Head only: count_minus_one = 2, i.e. three frames declared, + // followed by no payload at all. + assert_eq!(super::split_lacing(super::LACING_FIXED, &[2u8]), None); + // Same shape for a single declared frame. + assert_eq!(super::split_lacing(super::LACING_FIXED, &[0u8]), None); + } + + /// The non-degenerate fixed lace still splits evenly, so the guard above did + /// not tighten the valid case. + #[test] + fn fixed_lacing_splits_an_evenly_divisible_body() { + let laced = super::split_lacing(super::LACING_FIXED, &[2u8, 1, 2, 3, 4, 5, 6]) + .expect("three 2-byte frames is a well-formed fixed lace"); + assert_eq!(laced, vec![&[1u8, 2][..], &[3, 4][..], &[5, 6][..]]); + } use super::*; use crate::pes::Stream as _; use std::io::Cursor; diff --git a/src/mux/mp4/mod.rs b/src/mux/mp4/mod.rs index b8bafac..4efdd58 100644 --- a/src/mux/mp4/mod.rs +++ b/src/mux/mp4/mod.rs @@ -59,9 +59,15 @@ const RESERVE_BUFFER: u64 = 4 << 20; // 4 MiB const RESERVE_FLOOR: u64 = 8 << 20; // 8 MiB /// Rounding granularity for the reserve. const RESERVE_GRAIN: u64 = 4 << 20; // 4 MiB +/// Largest reserve expressible in a `free` box's 32-bit size field, rounded down +/// to a whole grain. +const RESERVE_CAP: u64 = (u32::MAX as u64 / RESERVE_GRAIN) * RESERVE_GRAIN; +/// Round up to `RESERVE_GRAIN`, saturating rather than wrapping. `div_ceil` then +/// multiply overflows for inputs within one grain of `u64::MAX`, which would turn +/// an enormous estimate into a tiny reserve — the opposite of the intent. fn round_up_grain(x: u64) -> u64 { - x.div_ceil(RESERVE_GRAIN) * RESERVE_GRAIN + x.div_ceil(RESERVE_GRAIN).saturating_mul(RESERVE_GRAIN) } /// Estimate the faststart hole: `round_up_4MB(bytes_per_sample × est_samples)` @@ -98,7 +104,16 @@ fn estimate_reserve(title: &DiscTitle, included: &[usize]) -> u64 { } } let est = (est_samples as u64).saturating_mul(BYTES_PER_SAMPLE); - round_up_grain(est).max(RESERVE_FLOOR) + RESERVE_BUFFER + let reserve = round_up_grain(est) + .max(RESERVE_FLOOR) + .saturating_add(RESERVE_BUFFER); + // The hole is a `free` box with a 32-bit size field, so a reserve at or above + // u32::MAX cannot be expressed: writing it truncated the size and left mdat + // beyond a box that claimed to be far shorter. Clamp to the largest + // grain-aligned value the field can hold. No real title comes near this — + // a 90 GB UHD title estimates a few MiB — but truncating silently produces an + // unreadable file, so it is bounded rather than trusted. + reserve.min(RESERVE_CAP) } /// One accumulated sample's bookkeeping (the mdat bytes are already on disk). @@ -1420,6 +1435,17 @@ mod tests { #[test] fn reserve_rounds_to_4mb_plus_buffer() { // round_up_4MB(x) + 4 MiB, floored at 8 MiB. + // Saturates rather than wrapping. div_ceil(GRAIN) * GRAIN overflows within + // one grain of u64::MAX, and the wrapped product is SMALL — which would + // turn the largest possible estimate into a negligible reserve, the exact + // opposite of the intent. Only the no-wrap property matters here. + assert!( + round_up_grain(u64::MAX) >= u64::MAX - (4 << 20), + "round_up_grain must saturate near u64::MAX, not wrap to a small value" + ); + // The reserve the writer emits must fit the `free` box's 32-bit size field. + assert!(RESERVE_CAP <= u32::MAX as u64); + assert_eq!(RESERVE_CAP % RESERVE_GRAIN, 0); assert_eq!(round_up_grain(1), 4 << 20); assert_eq!(round_up_grain(4 << 20), 4 << 20); assert_eq!(round_up_grain((4 << 20) + 1), 8 << 20); diff --git a/src/udf.rs b/src/udf.rs index 9418a8a..7d3f842 100644 --- a/src/udf.rs +++ b/src/udf.rs @@ -1098,8 +1098,16 @@ fn read_directory( }); } - // Read the ICB to get file size - let file_size = read_file_size(reader, meta_start, icb_lba).unwrap_or(0); + // Read the ICB to get file size. A read failure here must + // propagate, not become a size of zero: this function already + // fails hard on the budget guard above, and a file reported as + // zero bytes is indistinguishable from a genuinely empty one, so + // an unreadable ICB on a damaged disc would silently change which + // titles a caller considers present. `read_file_size` still + // returns Ok(0) for an ICB whose tag is neither File Entry (261) + // nor Extended File Entry (266), which is a real zero, not a + // failure. + let file_size = read_file_size(reader, meta_start, icb_lba)?; if is_dir && depth < MAX_DIR_DEPTH { // Cycle guard: skip any ICB LBA we have already opened as @@ -2221,6 +2229,75 @@ mod tests { assert_eq!(parse_udf_name(&raw), "BDMV"); } + /// A disc read failure while fetching an entry's ICB must propagate, not + /// become a file size of zero. A zero size is indistinguishable from a + /// genuinely empty file, so an unreadable ICB on a damaged disc silently + /// changed which titles a caller saw as present. + #[test] + fn read_directory_propagates_an_icb_read_failure_instead_of_size_zero() { + /// Serves every sector from an inner MemReader except one, which fails + /// the way a bad sector does. + struct FailingAt { + inner: MemReader, + fail_lba: u32, + } + impl SectorSource for FailingAt { + fn read_sectors( + &mut self, + lba: u32, + count: u16, + buf: &mut [u8], + recovery: bool, + ) -> Result { + if lba == self.fail_lba { + return Err(Error::DiscRead { + sector: lba as u64, + status: None, + sense: None, + }); + } + self.inner.read_sectors(lba, count, buf, recovery) + } + } + + // One directory holding one file entry whose ICB lives at LBA 7. + let mut dir = [0u8; 2048]; + let mut name_bytes = vec![8u8]; + name_bytes.extend_from_slice(b"CLPI"); + dir[0..2].copy_from_slice(&257u16.to_le_bytes()); + dir[18] = 0x00; // a file, not a parent, not a dir + dir[19] = name_bytes.len() as u8; + dir[24..28].copy_from_slice(&7u32.to_le_bytes()); + dir[38..38 + name_bytes.len()].copy_from_slice(&name_bytes); + + let mut inner = MemReader::new(); + inner.put(5, build_efe_icb(2048, 2048, 60)); + inner.put(60, dir); + inner.put(7, build_efe_icb(123, 2048, 0)); + + // Sanity: with every sector readable the entry parses and carries its + // real size, so the failure below is the ICB read and nothing else. + let ok = read_directory(&mut inner, 0, 0, 5, "ROOT", 0, &mut 0, &mut HashSet::new()) + .expect("dir parses when every sector reads"); + assert_eq!(ok.entries.len(), 1); + assert_eq!(ok.entries[0].size, 123); + + let mut reader = FailingAt { + inner: MemReader::new(), + fail_lba: 7, + }; + reader.inner.put(5, build_efe_icb(2048, 2048, 60)); + reader.inner.put(60, dir); + reader.inner.put(7, build_efe_icb(123, 2048, 0)); + + let err = read_directory(&mut reader, 0, 0, 5, "ROOT", 0, &mut 0, &mut HashSet::new()) + .expect_err("an unreadable entry ICB must fail the scan, not report size 0"); + assert!( + matches!(err, Error::DiscRead { sector: 7, .. }), + "the propagated error must name the sector that failed, got {err:?}" + ); + } + #[test] fn read_directory_honors_l_iu_offset_for_fid_name() { // ECMA-167 §14.4 File Identifier Descriptor: the File Identifier