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