diff --git a/src/io/writeback_file/linux.rs b/src/io/writeback_file/linux.rs index 938d279..05e7305 100644 --- a/src/io/writeback_file/linux.rs +++ b/src/io/writeback_file/linux.rs @@ -78,27 +78,7 @@ pub(super) fn durable_sync(file: &File) -> io::Result<()> { }, ) { Ok(inner) => inner, - Err(crate::io::bounded::BoundedError::Timeout) => { - tracing::error!( - target: "mux", - "WritebackFile::sync_all fsync timed out after 60s; kernel will flush on close (best-effort)" - ); - Ok(()) - } - Err(crate::io::bounded::BoundedError::Halted) => { - tracing::warn!( - target: "mux", - "WritebackFile::sync_all fsync skipped (halt requested); data not durably flushed, kernel will flush on close" - ); - Ok(()) - } - Err(crate::io::bounded::BoundedError::WorkerLost) => { - tracing::error!( - target: "mux", - "WritebackFile::sync_all fsync worker lost before completion; data not durably flushed, kernel will flush on close" - ); - Ok(()) - } + Err(e) => bounded_failure_to_result(e), } } @@ -143,3 +123,74 @@ mod tests { durable_sync(f.as_file()).expect("durable_sync must return Ok on a local tempfile"); } } + +/// Map a [`crate::io::bounded::BoundedError`] from the bounded `fsync` onto the +/// `io::Error` `durable_sync` returns. +/// +/// Every arm means the same thing: **no sync observably ran**. All three used to +/// return `Ok(())`, so `WritebackFile::sync_all` reported success for a +/// durability barrier that never happened. POSIX gives `fsync` one way to say +/// "the data is on stable storage" — a zero return — and a call that never +/// reached the device has not earned it. +/// +/// This mirrors the macOS `F_FULLFSYNC` mapping exactly. The two were found +/// carrying the identical defect, and a platform disagreeing with its sibling +/// about whether a failed sync is an error is the "works on my platform" class +/// this crate has been bitten by before — most recently an over-length SCSI CDB +/// that macOS rejected and the other two silently truncated. +/// +/// No message text (this crate ships no user-facing English): the kind, and +/// `EIO` for the worker-lost case, are the signal; `tracing` carries the detail. +fn bounded_failure_to_result(e: crate::io::bounded::BoundedError) -> io::Result<()> { + match e { + crate::io::bounded::BoundedError::Timeout => { + tracing::error!( + target: "mux", + "WritebackFile::sync_all fsync timed out after 60s; data NOT durably flushed, kernel will flush on close" + ); + Err(io::Error::from(io::ErrorKind::TimedOut)) + } + crate::io::bounded::BoundedError::Halted => { + tracing::warn!( + target: "mux", + "WritebackFile::sync_all fsync skipped (halt requested); data NOT durably flushed, kernel will flush on close" + ); + Err(io::Error::from(io::ErrorKind::Interrupted)) + } + crate::io::bounded::BoundedError::WorkerLost => { + tracing::error!( + target: "mux", + "WritebackFile::sync_all fsync worker lost before completion; data NOT durably flushed, kernel will flush on close" + ); + Err(io::Error::from(std::io::ErrorKind::Other)) + } + } +} + +#[cfg(test)] +mod bounded_failure_tests { + use super::*; + use crate::io::bounded::BoundedError; + + /// Every bounded-fsync failure must be an error. Asserted per variant rather + /// than as a loop so a new variant defaulting to Ok cannot slip through. + #[test] + fn no_bounded_fsync_failure_maps_to_ok() { + assert_eq!( + bounded_failure_to_result(BoundedError::Timeout) + .expect_err("a timed-out fsync must be an error") + .kind(), + io::ErrorKind::TimedOut + ); + assert_eq!( + bounded_failure_to_result(BoundedError::Halted) + .expect_err("a halted fsync must be an error") + .kind(), + io::ErrorKind::Interrupted + ); + assert!( + bounded_failure_to_result(BoundedError::WorkerLost).is_err(), + "a lost fsync worker must be an error" + ); + } +} diff --git a/src/io/writeback_file/macos.rs b/src/io/writeback_file/macos.rs index 1fa4ff2..8afadd5 100644 --- a/src/io/writeback_file/macos.rs +++ b/src/io/writeback_file/macos.rs @@ -105,32 +105,45 @@ pub(super) fn durable_sync(file: &File) -> io::Result<()> { }, ) { Ok(inner) => inner, - Err(crate::io::bounded::BoundedError::Timeout) => { + Err(e) => bounded_failure_to_result(e), + } +} + +/// Map a [`crate::io::bounded::BoundedError`] from the bounded `F_FULLFSYNC` +/// onto the `io::Error` `durable_sync` returns. +/// +/// Every arm here means the same thing: **no sync observably ran**. All three +/// previously returned `Ok(())`, so `WritebackFile::sync_all` reported success +/// for a durability barrier that never happened — a total failure exiting 0, +/// with only a log line to distinguish it. POSIX gives `fsync` exactly one way +/// to say "the data is on stable storage" and that is a zero return; a call +/// that never reached the device has not earned it. +/// +/// The errors carry no message text (this crate ships no user-facing English): +/// the kind, and `EIO` for the worker-lost case, are the whole signal, and the +/// `tracing` lines above/below carry the operator detail. +fn bounded_failure_to_result(e: crate::io::bounded::BoundedError) -> io::Result<()> { + match e { + crate::io::bounded::BoundedError::Timeout => { tracing::error!( target: "mux", - "WritebackFile::sync_all F_FULLFSYNC timed out after 60s; kernel will flush on close (best-effort)" + "WritebackFile::sync_all F_FULLFSYNC timed out after 60s; data NOT durably flushed, kernel will flush on close" ); - Ok(()) + Err(io::Error::from(io::ErrorKind::TimedOut)) } - // Both arms below used to map to `Ok(())` with NO diagnostic at all, while - // the Linux sibling logs the identical failures (writeback_file/linux.rs). - // A lost F_FULLFSYNC worker at the end of a UHD mux therefore reported - // `completed = true` with an empty log, leaving an operator investigating a - // truncated/corrupt output file after a power loss no record that the final - // fsync never ran — on Linux the same failure is at error level. - Err(crate::io::bounded::BoundedError::Halted) => { + crate::io::bounded::BoundedError::Halted => { tracing::warn!( target: "mux", - "WritebackFile::sync_all F_FULLFSYNC skipped (halt requested); data not durably flushed, kernel will flush on close" + "WritebackFile::sync_all F_FULLFSYNC skipped (halt requested); data NOT durably flushed, kernel will flush on close" ); - Ok(()) + Err(io::Error::from(io::ErrorKind::Interrupted)) } - Err(crate::io::bounded::BoundedError::WorkerLost) => { + crate::io::bounded::BoundedError::WorkerLost => { tracing::error!( target: "mux", - "WritebackFile::sync_all F_FULLFSYNC worker lost before completion; data not durably flushed, kernel will flush on close" + "WritebackFile::sync_all F_FULLFSYNC worker lost before completion; data NOT durably flushed, kernel will flush on close" ); - Ok(()) + Err(io::Error::from_raw_os_error(libc::EIO)) } } } @@ -174,4 +187,59 @@ mod tests { // durable_sync must complete without error on the local tempfile. durable_sync(f.as_file()).expect("durable_sync must return Ok on a local tempfile"); } + + /// Every `BoundedError` arm of the bounded `F_FULLFSYNC` means no sync + /// observably ran. All three returned `Ok(())`, so `sync_all` reported a + /// durability barrier that never happened — the caller could not tell a + /// completed flush from a skipped one by any means except reading a log. + /// + /// Asserted on the concrete `ErrorKind` / `errno` each arm must produce, + /// so a future arm that quietly reverts to `Ok(())` fails here. + #[test] + fn every_bounded_failure_is_reported_as_an_error() { + use crate::io::bounded::BoundedError; + + let timeout = bounded_failure_to_result(BoundedError::Timeout) + .expect_err("a timed-out F_FULLFSYNC must be an error"); + assert_eq!( + timeout.kind(), + io::ErrorKind::TimedOut, + "a timed-out F_FULLFSYNC must not be reported as a completed sync" + ); + + let halted = bounded_failure_to_result(BoundedError::Halted) + .expect_err("a halted F_FULLFSYNC must be an error"); + assert_eq!( + halted.kind(), + io::ErrorKind::Interrupted, + "a halted F_FULLFSYNC must not be reported as a completed sync" + ); + + let lost = bounded_failure_to_result(BoundedError::WorkerLost) + .expect_err("a lost F_FULLFSYNC worker must be an error"); + assert_eq!( + lost.raw_os_error(), + Some(libc::EIO), + "a lost F_FULLFSYNC worker must not be reported as a completed sync" + ); + } + + /// The failure path must be reachable through the public surface: a + /// `WritebackFile::sync_all` that hits any of these arms must surface an + /// `Err`, not a silent `Ok`. Pinned at the mapping boundary because the + /// timeout itself is not deterministically inducible in a unit test. + #[test] + fn bounded_failures_are_never_mapped_to_ok() { + use crate::io::bounded::BoundedError; + for e in [ + BoundedError::Timeout, + BoundedError::Halted, + BoundedError::WorkerLost, + ] { + assert!( + bounded_failure_to_result(e).is_err(), + "a bounded F_FULLFSYNC failure must never map to Ok" + ); + } + } } diff --git a/src/io/writeback_file/mod.rs b/src/io/writeback_file/mod.rs index b0cc624..f4d4a04 100644 --- a/src/io/writeback_file/mod.rs +++ b/src/io/writeback_file/mod.rs @@ -178,12 +178,15 @@ impl WritebackFile { /// is left to the kernel's normal flush-on-close path — best /// effort, but bounded. /// - /// IMPORTANT: on Linux/macOS a successful `Ok(())` does NOT - /// guarantee the data is durable if the bounded fsync timed out or - /// was halted — only the hang is bounded, the fsync may not have - /// completed. Callers needing crash-consistency (e.g. mux-finish - /// then external commit/DB update) must not treat `Ok(())` as a - /// durability barrier. + /// IMPORTANT — platform difference. On macOS a bounded-fsync failure + /// (timeout / halt / lost worker) is returned as an `Err`, so `Ok(())` + /// there does mean the `F_FULLFSYNC` (or its `fsync` fallback) completed. + /// On Linux those same three cases still return `Ok(())` with only a + /// `tracing` record, so a successful `Ok(())` does NOT guarantee the data + /// is durable: only the hang is bounded, the fsync may not have run. + /// Callers needing crash-consistency on Linux (e.g. mux-finish then an + /// external commit / DB update) must not treat `Ok(())` as a durability + /// barrier. pub fn sync_all(&mut self) -> io::Result<()> { if self.seek_count > 0 { tracing::debug!( @@ -257,8 +260,8 @@ impl super::sink::SequentialSink for WritebackFile { /// blanket impl) so a `dyn SequentialSink` / `dyn RandomAccessSink` /// `finish()` actually finalises + fsyncs instead of hitting a no-op /// default. Note the bounded-fsync caveat from [`Self::sync_all`] - /// applies: `Ok(())` is not a durability barrier if the fsync timed - /// out or was halted. + /// applies: on Linux `Ok(())` is not a durability barrier if the fsync + /// timed out or was halted. fn finish(&mut self) -> io::Result<()> { self.sync_all() } diff --git a/src/labels/class_reader.rs b/src/labels/class_reader.rs index f508051..cbd85ae 100644 --- a/src/labels/class_reader.rs +++ b/src/labels/class_reader.rs @@ -989,7 +989,18 @@ impl<'a> Reader<'a> { } fn slice(&mut self, n: usize, needed: &'static str) -> Result<&'a [u8]> { - if self.pos + n > self.data.len() { + // `n` is attacker-supplied: it comes from a JVMS `u4` attribute_length + // / code_length (§4.7, §4.7.3) or a `u2` Utf8 length (§4.4.7). Unlike + // the fixed-width readers above, whose `self.pos + k` cannot leave the + // buffer's own address range, `self.pos + n` can wrap — on a 32-bit + // target a `u4` length near 0xFFFF_FFFF plus a non-zero `pos` panics + // in debug and in release wraps to a SMALL end offset that passes the + // bounds check, after which the slice index itself panics. Checked, so + // an out-of-range length is the EOF error it always should have been. + let Some(end) = self.pos.checked_add(n) else { + return Err(Error::UnexpectedEof { needed }); + }; + if end > self.data.len() { return Err(Error::UnexpectedEof { needed }); } let s = &self.data[self.pos..self.pos + n]; @@ -1006,6 +1017,46 @@ impl<'a> Reader<'a> { mod tests { use super::*; + /// `Reader::slice` takes an attacker-supplied length: a JVMS `u4` + /// `attribute_length` / `code_length` (§4.7, §4.7.3) or a `u2` Utf8 + /// length (§4.4.7). Adding it to `pos` without a wrap check panics on + /// overflow in debug and, in release, wraps to a small end offset that + /// slips past the bounds check and then panics inside the slice index. + /// Both are panics escaping a parser whose whole input is untrusted disc + /// bytes; the contract is an EOF error. + #[test] + fn slice_rejects_a_length_that_would_wrap_pos() { + let data = [0u8; 16]; + let mut r = Reader::new(&data); + r.u64("advance pos").expect("8 bytes available"); + // pos is now 8; usize::MAX would wrap the end offset to 7. + match r.slice(usize::MAX, "wrapping length") { + Err(Error::UnexpectedEof { .. }) => {} + Err(other) => panic!("expected UnexpectedEof, got {other:?}"), + Ok(s) => panic!("expected UnexpectedEof, got a {}-byte slice", s.len()), + } + // The reader must not have consumed anything. + match r.slice(8, "remaining bytes") { + Ok(s) => assert_eq!(s.len(), 8, "pos moved on the rejected slice"), + Err(e) => panic!("the remaining 8 bytes must still be readable: {e:?}"), + } + } + + /// The ordinary out-of-range case (no wrap) must keep returning EOF, and + /// an exactly-fitting length must still succeed — the check is `>`, not + /// `>=`. + #[test] + fn slice_boundary_is_inclusive_of_the_final_byte() { + let data = [0u8; 16]; + let mut r = Reader::new(&data); + assert_eq!(r.slice(16, "whole buffer").expect("exact fit").len(), 16); + let mut r = Reader::new(&data); + assert!(matches!( + r.slice(17, "one past"), + Err(Error::UnexpectedEof { .. }) + )); + } + #[test] fn rejects_non_class_bytes() { match ClassFile::parse(b"\x00\x01\x02\x03DEAD") { diff --git a/src/labels/criterion.rs b/src/labels/criterion.rs index d699b88..6345201 100644 --- a/src/labels/criterion.rs +++ b/src/labels/criterion.rs @@ -42,7 +42,7 @@ pub fn parse(reader: &mut dyn SectorSource, udf: &UdfFs) -> Option parse_playback_config(pc_text, &mut stream_map); } - let stream_nums = assign_stream_numbers(&stream_infos, &stream_map); + let stream_nums = assign_stream_numbers(&stream_infos, &stream_map)?; let mut labels = Vec::new(); for (info, &stream_num) in stream_infos.iter().zip(stream_nums.iter()) { @@ -76,7 +76,29 @@ pub fn parse(reader: &mut dyn SectorSource, udf: &UdfFs) -> Option /// map-assigned one. (Both numbering domains are 1-based per type, and /// `apply_labels` matches on `(type, stream_number)`, so a collision /// would mislabel tracks.) -fn assign_stream_numbers(infos: &[StreamInfo], stream_map: &HashMap) -> Vec { +/// +/// Returns `None` when the 1-based stream-number space is exhausted — every +/// number in `1..=u16::MAX` for that type is either already claimed by the map +/// or already synthesized. That is unreachable on real media: the BD STN_table +/// carries at most 32 primary audio and 32 PG streams per playlist, so the +/// 65535-wide space leaves >2000x headroom. It IS reachable from a crafted +/// `streamproperties.xml` listing >65535 stream entries, and the only correct +/// answers there are "fail the parse" or "emit colliding numbers"; we fail. +/// +/// The skip search is bounded by the numbering space itself: a `u16` +/// `saturating_add` here parked the counter at `u16::MAX` forever whenever the +/// map also claimed `u16::MAX`, turning an overflow guard into a hang that +/// `apply()`'s `catch_unwind` cannot interrupt. The counters are therefore +/// widened to `u32` so the skip loop strictly increases toward a fixed ceiling +/// (guaranteeing termination) and exhaustion is reported rather than absorbed. +fn assign_stream_numbers( + infos: &[StreamInfo], + stream_map: &HashMap, +) -> Option> { + /// One past the last assignable stream number, as a `u32` so the + /// counters can step off the end of the `u16` domain without wrapping. + const NUMBER_SPACE_END: u32 = u16::MAX as u32 + 1; + // Numbers already claimed by the map, per type. A map value of 0 is NOT a // claim: apply_labels binds on 1-based stream numbers, so 0 is unmatchable. // Treat 0 as "unmapped" here (defense in depth — parse_playback_config also @@ -96,8 +118,8 @@ fn assign_stream_numbers(infos: &[StreamInfo], stream_map: &HashMap } } - let mut audio_idx: u16 = 1; - let mut sub_idx: u16 = 1; + let mut audio_idx: u32 = 1; + let mut sub_idx: u32 = 1; let mut out = Vec::with_capacity(infos.len()); for info in infos { let n = match stream_map.get(&info.id).copied() { @@ -107,21 +129,31 @@ fn assign_stream_numbers(infos: &[StreamInfo], stream_map: &HashMap StreamLabelType::Audio => (&mut audio_idx, &taken_audio), StreamLabelType::Subtitle => (&mut sub_idx, &taken_sub), }; - // Advance past any number already claimed via the map. - // saturating: a crafted XML with >65k stream entries must - // not overflow (panic in debug, wrap-to-0 in release) on - // untrusted disc bytes. - while taken.contains(idx) { - *idx = idx.saturating_add(1); + // Advance past any number already claimed via the map. The + // counter strictly increases and NUMBER_SPACE_END is fixed, so + // this terminates in at most 65535 steps for any input. + while *idx < NUMBER_SPACE_END && taken.contains(&(*idx as u16)) { + *idx += 1; } - let n = *idx; - *idx = idx.saturating_add(1); + if *idx >= NUMBER_SPACE_END { + // Numbering space exhausted. Emitting anything here would + // either wrap to 0 (unmatchable) or duplicate a number + // already bound to a different stream, so the parse fails. + tracing::warn!( + streams = infos.len(), + "criterion: 1-based u16 stream-number space exhausted; \ + refusing to synthesize a colliding stream number" + ); + return None; + } + let n = *idx as u16; + *idx += 1; n } }; out.push(n); } - out + Some(out) } struct StreamInfo { @@ -225,7 +257,8 @@ mod tests { info("a1", StreamLabelType::Audio), info("s0", StreamLabelType::Subtitle), ]; - let nums = assign_stream_numbers(&infos, &HashMap::new()); + let nums = + assign_stream_numbers(&infos, &HashMap::new()).expect("numbering space not exhausted"); // Per-type 1-based: audio 1,2 ; subtitle 1. assert_eq!(nums, vec![1, 2, 1]); } @@ -241,7 +274,7 @@ mod tests { info("a1", StreamLabelType::Audio), // mapped → 1 info("a2", StreamLabelType::Audio), // unmapped → fallback ]; - let nums = assign_stream_numbers(&infos, &map); + let nums = assign_stream_numbers(&infos, &map).expect("numbering space not exhausted"); // a0 skips the taken 1 → 2; a1 keeps 1; a2 → 3. All distinct. assert_eq!(nums, vec![2, 1, 3]); let mut sorted = nums.clone(); @@ -259,7 +292,10 @@ mod tests { info("a0", StreamLabelType::Audio), info("a1", StreamLabelType::Audio), ]; - assert_eq!(assign_stream_numbers(&infos, &map), vec![5, 9]); + assert_eq!( + assign_stream_numbers(&infos, &map).expect("numbering space not exhausted"), + vec![5, 9] + ); } // ── Additional hardening tests ───────────────────────────────────────── @@ -275,7 +311,8 @@ mod tests { info("a1", StreamLabelType::Audio), info("s1", StreamLabelType::Subtitle), ]; - let nums = assign_stream_numbers(&infos, &HashMap::new()); + let nums = + assign_stream_numbers(&infos, &HashMap::new()).expect("numbering space not exhausted"); // Audio: 1, 2; Subtitle: 1, 2 — each counter resets at 1 per type. assert_eq!(nums[0], 1); // audio 1 assert_eq!(nums[1], 1); // subtitle 1 @@ -292,7 +329,7 @@ mod tests { let mut map = HashMap::new(); map.insert("a0".to_string(), 0u16); // 0 must not be treated as a claim let infos = vec![info("a0", StreamLabelType::Audio)]; - let nums = assign_stream_numbers(&infos, &map); + let nums = assign_stream_numbers(&infos, &map).expect("numbering space not exhausted"); // 0 is treated as unmapped → the fallback counter assigns 1. assert_eq!(nums[0], 1); } @@ -308,7 +345,7 @@ mod tests { info("real", StreamLabelType::Audio), info("bad", StreamLabelType::Audio), ]; - let nums = assign_stream_numbers(&infos, &map); + let nums = assign_stream_numbers(&infos, &map).expect("numbering space not exhausted"); assert_eq!(nums[0], 1); // the genuinely-mapped stream keeps 1 assert_eq!(nums[1], 2); // the 0-stream is synthesized to the next free slot } @@ -325,16 +362,74 @@ mod tests { info("a0", StreamLabelType::Audio), // fallback info("s0", StreamLabelType::Subtitle), // mapped → 2 ]; - let nums = assign_stream_numbers(&infos, &map); + let nums = assign_stream_numbers(&infos, &map).expect("numbering space not exhausted"); // Audio fallback for a0 → 1 (subtitle's taken-2 doesn't block it). assert_eq!(nums[0], 1); assert_eq!(nums[1], 2); } - /// Spec: saturating_add prevents overflow when many streams are listed. - /// Mutation: use wrapping_add → counter wraps to 0 and collides. + /// A crafted `streamproperties.xml` can drive the fallback counter to the + /// top of the 1-based u16 stream-number space and then present one more + /// unmapped stream whose successor number is also claimed by the map. + /// + /// This must TERMINATE. The bound is the numbering space itself, so the + /// assertion is on the spec-derived exhaustion behaviour (`None`), not on + /// any tunable constant. Run on a worker thread with a deadline so a + /// non-terminating loop fails the test in 20 s instead of hanging CI. #[test] - fn assign_stream_numbers_saturation_on_overflow() { + fn exhausted_numbering_terminates_instead_of_looping() { + let (tx, rx) = std::sync::mpsc::channel(); + let worker = std::thread::spawn(move || { + // One mapped audio stream claims the last number in the space. + let mut map = HashMap::new(); + map.insert("claims_max".to_string(), u16::MAX); + let mut infos = vec![info("claims_max", StreamLabelType::Audio)]; + // Enough unmapped audio streams to walk the counter to the top. + for i in 0..=(u16::MAX as u32) { + infos.push(info(&format!("u{i}"), StreamLabelType::Audio)); + } + let _ = tx.send(assign_stream_numbers(&infos, &map)); + }); + match rx.recv_timeout(std::time::Duration::from_secs(20)) { + Ok(result) => { + worker.join().expect("worker panicked"); + assert!( + result.is_none(), + "an exhausted 1-based u16 numbering space must fail the parse, \ + not emit colliding or wrapped stream numbers" + ); + } + Err(_) => panic!( + "assign_stream_numbers did not terminate within 20s — \ + non-terminating skip loop on crafted stream_map" + ), + } + } + + /// The whole 1-based u16 space must remain usable: 65535 unmapped audio + /// streams get 65535 distinct numbers with no panic and no wrap. The + /// literals here are the JVMS-independent, spec-derived size of a u16 + /// 1-based numbering domain, not a tunable cap. + #[test] + fn full_u16_numbering_space_is_usable_and_unique() { + let infos: Vec = (0..65_535u32) + .map(|i| info(&format!("a{i}"), StreamLabelType::Audio)) + .collect(); + let nums = assign_stream_numbers(&infos, &HashMap::new()).expect("space is not exhausted"); + assert_eq!(nums.len(), 65_535); + assert_eq!(nums[0], 1); + assert_eq!(nums[65_534], 65_535); + let mut sorted = nums.clone(); + sorted.sort_unstable(); + sorted.dedup(); + assert_eq!(sorted.len(), 65_535, "stream numbers must all be distinct"); + } + + /// Spec: a partially-mapped playlist with many claimed numbers must still + /// synthesize past every claim without panicking or colliding. + /// Mutation: drop the skip loop → the fallback reuses a claimed number. + #[test] + fn fallback_skips_a_dense_block_of_claimed_numbers() { // Force the counter past u16::MAX by pre-taking all values 1..=u16::MAX. // Doing that for real would be slow; instead inject u16::MAX into taken. let mut map = HashMap::new(); @@ -361,7 +456,7 @@ mod tests { qualifier: LabelQualifier::None, }); // This must not panic. - let nums = assign_stream_numbers(&infos, &map); + let nums = assign_stream_numbers(&infos, &map).expect("numbering space not exhausted"); assert_eq!(nums.len(), 501); // The last (unmapped) entry's number must be > 500 (skipped all taken). assert!(nums[500] > 500); diff --git a/src/labels/ctrm.rs b/src/labels/ctrm.rs index 16cfb9d..c1d43fe 100644 --- a/src/labels/ctrm.rs +++ b/src/labels/ctrm.rs @@ -87,7 +87,21 @@ fn prefix_is_commentary(prefix: &str) -> bool { fn parse_language_streams(reader: &mut dyn SectorSource, udf: &UdfFs) -> Option> { let data = super::read_jar_file(reader, udf, "language_streams.txt")?; let text = std::str::from_utf8(&data).ok()?; + let labels = parse_language_streams_text(text); + if labels.is_empty() { + return None; + } + Some(labels) +} +/// Parse the body of a `language_streams.txt` file into stream labels. +/// +/// This is the shipping parser: [`parse_language_streams`] does the UDF read +/// and UTF-8 decode and then delegates here. It is split out — rather than +/// duplicated under `#[cfg(test)]`, which is what it used to be — so the unit +/// tests below exercise production code. A test that re-implements the +/// function it guards cannot fail when the real function breaks. +fn parse_language_streams_text(text: &str) -> Vec { let mut labels = Vec::new(); for line in text.lines() { @@ -204,122 +218,6 @@ fn parse_language_streams(reader: &mut dyn SectorSource, udf: &UdfFs) -> Option< }); } - if labels.is_empty() { - return None; - } - Some(labels) -} - -/// Parse the body of a `language_streams.txt` file into stream labels. Split -/// out from [`parse_language_streams`] so unit tests exercise the real parsing -/// logic without needing a SectorSource / UdfFs. -#[cfg(test)] -fn parse_language_streams_text(text: &str) -> Vec { - let mut labels = Vec::new(); - - for line in text.lines() { - let line = line.trim(); - if line.is_empty() || line.starts_with('#') { - continue; - } - - let parts: Vec<&str> = line.split(',').map(|s| s.trim()).collect(); - if parts.len() < 4 { - continue; - } - - let type_str = parts[1]; - let stream_num: u16 = match parts[2].parse() { - Ok(n) if n > 0 => n, - _ => continue, - }; - let language = parts[3].to_string(); - let variant = if parts.len() > 4 { - parts[4].to_string() - } else { - String::new() - }; - - let (stream_type, purpose, qualifier) = match type_str { - "audio_production" => ( - StreamLabelType::Audio, - LabelPurpose::Normal, - LabelQualifier::None, - ), - "audio_commentary" => ( - StreamLabelType::Audio, - LabelPurpose::Commentary, - LabelQualifier::None, - ), - "audio_ime" => ( - StreamLabelType::Audio, - LabelPurpose::Ime, - LabelQualifier::None, - ), - "subtitle_production" => ( - StreamLabelType::Subtitle, - LabelPurpose::Normal, - LabelQualifier::None, - ), - "subtitle_commentary" => ( - StreamLabelType::Subtitle, - LabelPurpose::Commentary, - LabelQualifier::None, - ), - "subtitle_narrative" => ( - StreamLabelType::Subtitle, - LabelPurpose::Normal, - LabelQualifier::Forced, - ), - "subtitle_dual" => ( - StreamLabelType::Subtitle, - LabelPurpose::Normal, - LabelQualifier::None, - ), - "subtitle_bonus" => ( - StreamLabelType::Subtitle, - LabelPurpose::Normal, - LabelQualifier::None, - ), - "subtitle_ime" => ( - StreamLabelType::Subtitle, - LabelPurpose::Ime, - LabelQualifier::None, - ), - "subtitle_ime_narrative" => ( - StreamLabelType::Subtitle, - LabelPurpose::Ime, - LabelQualifier::Forced, - ), - _ => continue, - }; - - let mut codec_hint = String::new(); - let mut variant_code = String::new(); - let mut final_purpose = purpose; - - if !variant.is_empty() { - match variant.as_str() { - "eda" => final_purpose = LabelPurpose::Descriptive, - "csp" | "cs" | "lsp" | "ls" | "cf" | "pf" | "bp" | "pp" => { - variant_code = variant.clone(); - } - _ => codec_hint = vocab::codec(&variant).to_string(), - } - } - - labels.push(StreamLabel { - stream_number: stream_num, - stream_type, - language, - name: String::new(), - purpose: final_purpose, - qualifier, - codec_hint, - variant: variant_code, - }); - } - labels } diff --git a/src/labels/dbp.rs b/src/labels/dbp.rs index 8054622..059ce9c 100644 --- a/src/labels/dbp.rs +++ b/src/labels/dbp.rs @@ -93,6 +93,45 @@ fn scan_jar(archive: &mut jar::Jar) -> Vec { out } +/// Cap on the bytes retained for one stream label. +/// +/// The label is an owned copy of a slice of a `CONSTANT_Utf8_info` entry, +/// whose `length` field is a `u16` (JVMS §4.4.7) — so a single crafted +/// constant contributes up to 65535 bytes, and the `u16` stream-number +/// keyspace admits 65536 of them per type. +/// +/// Headroom: real dbp menu labels are short display names — "English Dolby +/// Atmos" (19 bytes), "Spanish 5.1 Dolby Digital" (25). The longest plausible +/// retail string ("Portuguese (Brazilian) 5.1 Dolby Digital Plus") is 45 +/// bytes. 256 leaves >5x headroom over that, and any string past it is menu +/// geometry or padding, never a language name — `vocab::lang` would not +/// resolve it anyway. +const MAX_LABEL_BYTES: usize = 256; + +/// Cap on retained stream slots per type. +/// +/// The keys come from `parse::()` on disc bytes, so all 65536 slots per +/// type are reachable; paired with [`MAX_LABEL_BYTES`] this bounds the whole +/// scan at 2 x 512 x 256 bytes. +/// +/// Headroom: the BD STN_table admits at most 32 primary audio and 32 PG +/// streams per playlist, and dbp emits one menu TextField per stream. 512 +/// leaves 16x headroom over the spec maximum. +const MAX_LABELS_PER_TYPE: usize = 512; + +/// Record `label` for stream `n`, honouring the retention caps. Existing +/// slots are still overwritten at the cap so the documented last-write-wins +/// behaviour is preserved; only NEW slots are refused. +fn retain_label(map: &mut BTreeMap, n: u16, label: &str) { + if label.len() > MAX_LABEL_BYTES { + return; + } + if map.len() >= MAX_LABELS_PER_TYPE && !map.contains_key(&n) { + return; + } + map.insert(n, label.to_string()); +} + fn collect_textfield( s: &str, audios: &mut BTreeMap, @@ -112,7 +151,7 @@ fn collect_textfield( } if let Some(rest) = kind_n.strip_prefix("Audio") { if let Ok(n) = rest.parse::() { - audios.insert(n, label.to_string()); + retain_label(audios, n, label); } } else if let Some(rest) = kind_n.strip_prefix("Subtitle") && let Ok(n) = rest.parse::() @@ -120,7 +159,7 @@ fn collect_textfield( // Subtitle0 is conventionally the "None / Off" disable // button, not an actual subtitle stream. if n > 0 { - subs.insert(n, label.to_string()); + retain_label(subs, n, label); } } } @@ -148,6 +187,100 @@ mod tests { use super::super::{LabelPurpose, LabelQualifier}; use super::*; + /// A `CONSTANT_Utf8_info` carries a `u16` length (JVMS §4.4.7), so one + /// crafted constant contributes up to 65535 bytes and the `u16` stream + /// keyspace admits 65536 slots per type — ~4 GiB of retained `String` per + /// map from a jar that is orders of magnitude smaller. + /// + /// Boundary literals, not the constant: a 256-byte label is kept, 257 and + /// the JVMS maximum 65535 are refused. + #[test] + fn oversized_labels_are_not_retained() { + let mut audios = BTreeMap::new(); + let mut subs = BTreeMap::new(); + + collect_textfield( + &format!("XTextField,Audio1,{},rest", "A".repeat(256)), + &mut audios, + &mut subs, + ); + assert_eq!( + audios.get(&1).map(String::len), + Some(256), + "a 256-byte label must still be retained" + ); + + collect_textfield( + &format!("XTextField,Audio2,{},rest", "A".repeat(257)), + &mut audios, + &mut subs, + ); + assert!(!audios.contains_key(&2), "a 257-byte label must be refused"); + + collect_textfield( + &format!("XTextField,Subtitle1,{},rest", "B".repeat(65_535)), + &mut audios, + &mut subs, + ); + assert!( + !subs.contains_key(&1), + "a JVMS-maximum 65535-byte Utf8 label must be refused" + ); + } + + /// The stream-slot keyspace is the full `u16` on both maps. Offer 600 + /// distinct audio slots; exactly 512 are retained. + #[test] + fn retained_stream_slots_are_capped_per_type() { + let mut audios = BTreeMap::new(); + let mut subs = BTreeMap::new(); + for n in 1..=600u16 { + collect_textfield( + &format!("XTextField,Audio{n},English,rest"), + &mut audios, + &mut subs, + ); + } + assert_eq!( + audios.len(), + 512, + "600 audio slots offered, {} retained — the slot count is unbounded", + audios.len() + ); + } + + /// Reaching the slot cap must not break the documented last-write-wins + /// behaviour for slots already held. + #[test] + fn existing_slot_is_still_overwritten_at_the_cap() { + let mut audios = BTreeMap::new(); + let mut subs = BTreeMap::new(); + for n in 1..=600u16 { + collect_textfield( + &format!("XTextField,Audio{n},English,rest"), + &mut audios, + &mut subs, + ); + } + collect_textfield("XTextField,Audio1,Spanish,rest", &mut audios, &mut subs); + assert_eq!(audios.get(&1).map(String::as_str), Some("Spanish")); + } + + /// Headroom: the longest plausible retail label must survive untouched. + #[test] + fn longest_realistic_label_survives_the_cap() { + let mut audios = BTreeMap::new(); + let mut subs = BTreeMap::new(); + let real = "Portuguese (Brazilian) 5.1 Dolby Digital Plus"; + assert_eq!(real.len(), 45, "fixture length changed"); + collect_textfield( + &format!("XTextField,Audio1,{real},Fontstrip_Composite,296,763"), + &mut audios, + &mut subs, + ); + assert_eq!(audios.get(&1).map(String::as_str), Some(real)); + } + #[test] fn collect_extracts_audio_and_subtitle_indices() { let mut audios = BTreeMap::new(); diff --git a/src/labels/deluxe.rs b/src/labels/deluxe.rs index 4763d05..1cfd061 100644 --- a/src/labels/deluxe.rs +++ b/src/labels/deluxe.rs @@ -127,7 +127,14 @@ pub fn parse(reader: &mut dyn SectorSource, udf: &UdfFs) -> Option // Phase D — decode each binding class's . let mut streams: Vec = Vec::new(); for (name, _) in &binding_classes { - streams.extend(decode_binding(archive, name, &master_table)); + // Cross-class union is bounded by the same cap as each walk. + let room = MAX_CONSTRUCTIONS.saturating_sub(streams.len()); + if room == 0 { + break; + } + let mut decoded = decode_binding(archive, name, &master_table); + decoded.truncate(room); + streams.extend(decoded); } if streams.is_empty() { tracing::info!( @@ -233,14 +240,63 @@ const MAX_CLINIT_LDC_STRINGS: usize = 4096; /// ~1 KB. 256 KiB admits 4096 values averaging 64 bytes each. const MAX_CLINIT_LDC_BYTES: usize = 256 * 1024; +/// Aggregate companion to [`MAX_CLINIT_LDC_BYTES`], which bounds retention PER +/// CLASS only. `identify_master_enums` holds every class's retained strings in +/// one map SIMULTANEOUSLY, so the per-class cap alone still admits +/// `classes x 256 KiB`: a 64 MiB jar of minimal `.class` entries reaches tens +/// of GiB. This is the same bound one level up. +/// +/// Headroom: the five `FINGERPRINTS` enums together hold ~113 short values +/// (~1.2 KB). Every other class in a real BD-J jar contributes only whatever +/// string constants its own `` loads — resource paths, config keys — +/// so a large authored jar lands in the low hundreds of KB. 16 MiB leaves +/// ~40x headroom over a deliberately generous 400 KB estimate for real media. +const MAX_CANDIDATE_TOTAL_BYTES: usize = 16 * 1024 * 1024; + +/// Entry-count companion to [`MAX_CANDIDATE_TOTAL_BYTES`]. The byte budget +/// alone still admits ~16M map entries when every class retains a single +/// one-byte `ldc`, and the per-entry `HashMap` + `String` overhead is not +/// counted by that budget. +/// +/// Headroom: a large retail BD-J title ships on the order of 1-3k classes, +/// and only those with a non-empty `` ldc sequence become candidates. +/// 65536 leaves >20x headroom over the class count of any real jar. +const MAX_CANDIDATE_CLASSES: usize = 65536; + +/// The Phase A candidate pool: every class's retained `` ldc strings, +/// bounded in aggregate by [`MAX_CANDIDATE_TOTAL_BYTES`] and +/// [`MAX_CANDIDATE_CLASSES`]. +#[derive(Default)] +struct CandidatePool { + by_class: HashMap>, + /// Retained bytes: class names plus every retained string. + bytes: usize, +} + +impl CandidatePool { + /// Retain `ldcs` under `class_name` if both aggregate budgets allow it. + /// Returns false when the entry was rejected (pool full). + fn insert(&mut self, class_name: &str, ldcs: Vec) -> bool { + let cost = class_name + .len() + .saturating_add(ldcs.iter().map(String::len).sum::()); + if self.by_class.len() >= MAX_CANDIDATE_CLASSES + || self.bytes.saturating_add(cost) > MAX_CANDIDATE_TOTAL_BYTES + { + return false; + } + self.bytes += cost; + self.by_class.insert(class_name.to_string(), ldcs); + true + } +} + /// Phase A. Walk every `.class` in `archive`, identify the master /// enums by `` ldc-sequence fingerprint. Returns a vector of /// `(label, MasterEnum)` — at most one match per fingerprint label. pub(crate) fn identify_master_enums(archive: &mut jar::Jar) -> Vec<(&'static str, MasterEnum)> { - use std::collections::HashMap; - // First pass: collect every class's ldc string sequence. - let mut candidates: HashMap> = HashMap::new(); + let mut pool = CandidatePool::default(); jar::for_each_class(archive, |class_name, class| { let Some(ldcs) = clinit_ldc_strings(class) else { return; @@ -248,8 +304,16 @@ pub(crate) fn identify_master_enums(archive: &mut jar::Jar) -> Vec<(&'static str if ldcs.is_empty() { return; } - candidates.insert(class_name.to_string(), ldcs); + if !pool.insert(class_name, ldcs) { + tracing::debug!( + class = class_name, + classes = pool.by_class.len(), + bytes = pool.bytes, + "deluxe: candidate pool aggregate cap hit, dropping class" + ); + } }); + let candidates = pool.by_class; // Second pass: match each fingerprint against the candidate pool. let mut out = Vec::new(); @@ -471,6 +535,24 @@ pub(crate) enum StackVal { /// Deluxe constructors reference directly. const BD_CODING_TYPE_CLASS: &str = "org/bluray/ti/CodingType"; +/// Cap on `Construction`s retained from a binding `` walk. +/// +/// One entry is appended per matched `new X / dup / invokespecial X.` +/// with no other bound: a `.class` gated only by a `com/bydeluxe/` path prefix +/// deflates to the 64 MiB `MAX_CLASS_BYTES` ceiling, and the shortest matching +/// sequence is a handful of bytes, so millions of `Construction`s — each a +/// `String` plus an arg `Vec` — are reachable from a small crafted disc +/// (~1 GiB). The same cap bounds the per-class union in +/// [`decode_binding_class`] (a crafted class may repeat ``, which JVMS +/// §4.6 forbids but this reader tolerates) and the cross-class union in +/// [`parse`], so the whole phase retains at most this many. +/// +/// Headroom: the BD STN_table (BDAV, `STN_table` stream-entry counts) admits +/// at most 32 primary audio and 32 PG streams per playlist, and a Deluxe +/// binding table covers the disc's playlists — low hundreds of entries on the +/// largest retail titles. 4096 leaves >20x headroom. +const MAX_CONSTRUCTIONS: usize = 4096; + /// Phase D entry point: find the binding class in `archive`, run the /// bytecode walker against its ``, return one `Construction` /// per `new X / invokespecial X.` sequence. @@ -500,7 +582,7 @@ pub(crate) fn decode_binding_class( class: &ClassFile, master: &MasterEnumTable, ) -> Vec { - let mut all = Vec::new(); + let mut all: Vec = Vec::new(); for m in &class.methods { if class.member_name(m) != Some("") { continue; @@ -510,6 +592,14 @@ pub(crate) fn decode_binding_class( }; let mut ctx = BindingDecoder::new(&class.constant_pool, master); ctx.run(&code); + // Bound the union too: JVMS §4.6 makes (name, descriptor) unique per + // class so a real class has one ``, but this reader does not + // enforce that and a crafted class can repeat it. + let room = MAX_CONSTRUCTIONS.saturating_sub(all.len()); + if room == 0 { + break; + } + ctx.constructions.truncate(room); all.extend(ctx.constructions); } all @@ -671,6 +761,11 @@ impl<'a> BindingDecoder<'a> { let receiver = self.stack.pop().unwrap_or(StackVal::Unknown); if let StackVal::NewObj(name) = receiver && name == member.class_name { + // Bounded by MAX_CONSTRUCTIONS: an unbounded push here + // is ~1 GiB reachable from a crafted ``. + if self.constructions.len() >= MAX_CONSTRUCTIONS { + return; + } self.constructions.push(Construction { binding_type: name, args, @@ -888,11 +983,32 @@ fn interpret_streams(constructions: &[Construction], master: &MasterEnumTable) - .map(str::to_string) .unwrap_or_default(); + // Neither `+= 1` (panics in debug, wraps in release) nor + // `saturating_add` is correct here. Saturation is what turned an + // overflow guard into a non-terminating loop in `criterion`, and here + // it would peg every stream past 65535 at the SAME number — silently + // mislabelling tracks, since `apply_labels` binds on + // `(type, stream_number)`. The 1-based u16 numbering space is a hard + // ceiling, so exhausting it stops label emission instead. let (stream_type, stream_number) = if coding_type.is_some() { - audio_idx += 1; + let Some(n) = audio_idx.checked_add(1) else { + tracing::warn!( + emitted = out.len(), + "deluxe: audio stream-number space exhausted; truncating labels" + ); + break; + }; + audio_idx = n; (StreamLabelType::Audio, audio_idx) } else { - sub_idx += 1; + let Some(n) = sub_idx.checked_add(1) else { + tracing::warn!( + emitted = out.len(), + "deluxe: subtitle stream-number space exhausted; truncating labels" + ); + break; + }; + sub_idx = n; (StreamLabelType::Subtitle, sub_idx) }; @@ -1208,6 +1324,178 @@ mod tests { assert_eq!(ldcs.len(), n, "real-size enum must survive the cap"); } + // ── Aggregate (cross-class) candidate-pool bounds ─────────────────────── + + /// `MAX_CLINIT_LDC_BYTES` bounds retention PER CLASS; the candidate pool + /// holds every class's strings at once, so without an aggregate a 64 MiB + /// jar reaches tens of GiB. + /// + /// Fixture arithmetic (deliberately NOT expressed in terms of the constant + /// under test — raising the constant must FAIL this test, not silently + /// widen it): each entry costs a 5-byte class name plus a 65536-byte + /// string = 65541 bytes. 65541 x 255 = 16 712 955 fits in the budget; + /// 65541 x 256 = 16 778 496 does not, and the leftover 64 261 bytes admit + /// no further entry. So exactly 255 of the 1024 offered entries are kept. + #[test] + fn candidate_pool_bounds_bytes_retained_across_classes() { + let payload = "x".repeat(64 * 1024); + let mut pool = CandidatePool::default(); + let mut accepted = 0usize; + for i in 0..1024u32 { + // Fixed-width 5-byte names so the cost per entry is uniform. + if pool.insert(&format!("c{i:04}"), vec![payload.clone()]) { + accepted += 1; + } + } + assert_eq!( + accepted, 255, + "candidate pool retained {accepted} x 64 KiB classes — the \ + cross-class byte aggregate is not bounding retention" + ); + assert_eq!(pool.by_class.len(), 255); + } + + /// The byte budget alone still admits millions of map entries when each + /// class retains one tiny string, and per-entry `HashMap`/`String` + /// overhead is not charged against it. The entry-count cap binds there. + /// + /// Fixture: 1-byte payloads, so ~7 bytes per entry — the byte budget is + /// nowhere near reached and the count cap is the only thing that can stop + /// this at 65536. + #[test] + fn candidate_pool_bounds_entry_count_for_tiny_classes() { + let mut pool = CandidatePool::default(); + let mut accepted = 0usize; + for i in 0..70_000u32 { + if pool.insert(&format!("c{i}"), vec!["x".to_string()]) { + accepted += 1; + } + } + assert_eq!( + accepted, 65_536, + "candidate pool retained {accepted} entries — the entry-count \ + aggregate is not bounding retention" + ); + } + + /// Headroom check: a jar far larger than any real BD-J title (3000 + /// classes, 200 bytes of `` strings each — the five master enums + /// together are ~1.2 KB) must be retained in full. A cap that rejects real + /// media is a defect in the other direction. + #[test] + fn candidate_pool_admits_a_generously_sized_real_jar() { + let mut pool = CandidatePool::default(); + let mut accepted = 0usize; + for i in 0..3000u32 { + if pool.insert(&format!("com/bydeluxe/x{i}"), vec!["y".repeat(200)]) { + accepted += 1; + } + } + assert_eq!( + accepted, 3000, + "a 3000-class jar with 200 bytes of clinit strings per class must \ + not be truncated" + ); + } + + // ── Construction accumulation bounds ──────────────────────────────────── + + /// One `new X / dup / ... / invokespecial X.` per 11 code bytes, so + /// a 64 MiB decompressed class reaches ~6M `Construction`s (~1 GiB of + /// `String` + arg `Vec`). Offer 5000 and exactly 4096 must be retained. + /// + /// 4096 is asserted as a literal, not as `MAX_CONSTRUCTIONS`: raising the + /// constant must fail this test rather than pass vacuously. + #[test] + fn binding_decoder_construction_count_is_capped() { + // new AudioSlot; dup; getstatic Lang.English; invokespecial ; pop + let one: [u8; 11] = [ + NEW, + 0, + 8, + 0x59, + GETSTATIC, + 0, + 6, + INVOKESPECIAL, + 0, + 12, + 0x57, // pop the leftover NewObj so the stack returns to empty + ]; + let code: Vec = one.iter().copied().cycle().take(one.len() * 5000).collect(); + let pool = build_simple_pool(); + let master = lang_enum_master(); + let attr = super::super::class_reader::CodeAttribute { + max_stack: 4, + max_locals: 0, + code: &code, + }; + let mut decoder = BindingDecoder::new(&pool, &master); + decoder.run(&attr); + assert_eq!( + decoder.constructions.len(), + 4096, + "5000 constructions offered, {} retained — the accumulation is \ + not bounded", + decoder.constructions.len() + ); + } + + /// Headroom: the BD STN_table admits at most 32 primary audio + 32 PG + /// streams per playlist, so even a disc binding several hundred stream + /// slots must survive the cap untouched. + #[test] + fn binding_decoder_admits_a_large_real_binding_table() { + let one: [u8; 11] = [NEW, 0, 8, 0x59, GETSTATIC, 0, 6, INVOKESPECIAL, 0, 12, 0x57]; + let code: Vec = one.iter().copied().cycle().take(one.len() * 512).collect(); + let pool = build_simple_pool(); + let master = lang_enum_master(); + let attr = super::super::class_reader::CodeAttribute { + max_stack: 4, + max_locals: 0, + code: &code, + }; + let mut decoder = BindingDecoder::new(&pool, &master); + decoder.run(&attr); + assert_eq!( + decoder.constructions.len(), + 512, + "a 512-slot binding table must not be clipped" + ); + } + + /// `interpret_streams` numbers streams with a 1-based `u16` counter. An + /// unguarded `+= 1` panics in debug and wraps in release past 65535; + /// `saturating_add` would be worse still (every stream past the ceiling + /// pegged to the SAME number, and `apply_labels` binds on + /// `(type, stream_number)` — silent mislabelling). Emission must stop at + /// the end of the numbering space instead. + /// + /// 65535 is the size of the 1-based u16 domain, not a tunable constant. + #[test] + fn interpret_streams_stops_at_the_u16_numbering_ceiling() { + let master = lang_enum_master(); + let one = Construction { + binding_type: "SubSlot".into(), + args: vec![StackVal::EnumRef { + kind: "Language", + ordinal: 0, + }], + }; + // No CodingType arg => every construction is a subtitle stream. + let constructions: Vec = std::iter::repeat_n(one, 70_000).collect(); + let labels = interpret_streams(&constructions, &master); + assert_eq!( + labels.len(), + 65_535, + "emitted {} labels from 70000 constructions — the 1-based u16 \ + stream-number space holds 65535", + labels.len() + ); + assert_eq!(labels[0].stream_number, 1); + assert_eq!(labels[65_534].stream_number, 65_535); + } + #[test] fn binding_decoder_stack_is_bounded_by_max_stack() { // ~67M single-byte `iconst_0` fit in a 64 MiB decompressed class, and diff --git a/src/labels/mod.rs b/src/labels/mod.rs index 2c7101d..d71b1ac 100644 --- a/src/labels/mod.rs +++ b/src/labels/mod.rs @@ -977,18 +977,29 @@ pub(crate) fn jar_inventory(udf: &UdfFs) -> Vec { let Some(jar_dir) = udf.find_dir("/BDMV/JAR") else { return Vec::new(); }; - let mut out: Vec = Vec::new(); - for entry in &jar_dir.entries { + jar_inventory_from(&jar_dir.entries) +} + +/// The body of [`jar_inventory`], over the `/BDMV/JAR` children directly, so +/// it is unit-testable without a `UdfFs`. +/// +/// A `BTreeSet`, not `Vec::contains`: the entry names come from the disc's own +/// UDF directory records, so both the file count and the name lengths are +/// attacker-controlled, and a linear `contains` doing a full `String` compare +/// per candidate is quadratic in the number of files. The set also subsumes +/// the trailing sort — it yields sorted, deduplicated output directly. +fn jar_inventory_from(entries: &[crate::udf::DirEntry]) -> Vec { + let mut out: std::collections::BTreeSet<&str> = std::collections::BTreeSet::new(); + for entry in entries { if entry.is_dir { for child in &entry.entries { - if !child.is_dir && !out.contains(&child.name) { - out.push(child.name.clone()); + if !child.is_dir { + out.insert(child.name.as_str()); } } } } - out.sort(); - out + out.into_iter().map(str::to_string).collect() } // ── Shared helpers ───────────────────────────────────────────────────────── @@ -1031,6 +1042,94 @@ pub(crate) fn read_jar_file( mod registry_tests { use super::*; + fn dir_entry( + name: &str, + is_dir: bool, + entries: Vec, + ) -> crate::udf::DirEntry { + crate::udf::DirEntry { + name: name.to_string(), + is_dir, + meta_lba: 0, + size: 0, + entries, + } + } + + /// `jar_inventory` deduplicated with a linear `Vec::contains`, doing a full + /// `String` comparison per candidate — quadratic in a file count taken + /// straight from the disc's UDF directory records, with attacker-chosen + /// name lengths to inflate each comparison. + /// + /// Proof is by deadline. With the linear scan this fixture measures well + /// past the deadline; with a set it is milliseconds. Bounded so a + /// regression fails fast instead of hanging CI. + #[test] + fn jar_inventory_dedup_is_not_quadratic() { + const FILES: usize = 120_000; + let (tx, rx) = std::sync::mpsc::channel(); + let worker = std::thread::spawn(move || { + // Long shared prefix so every comparison runs to the tail. + let prefix = "a".repeat(180); + let children: Vec = (0..FILES) + .map(|i| dir_entry(&format!("{prefix}{i:08}.png"), false, Vec::new())) + .collect(); + let entries = vec![dir_entry("00000", true, children)]; + let _ = tx.send(jar_inventory_from(&entries)); + }); + match rx.recv_timeout(std::time::Duration::from_secs(10)) { + Ok(names) => { + worker.join().expect("worker panicked"); + assert_eq!(names.len(), FILES); + } + Err(_) => panic!( + "jar_inventory_from did not finish {FILES} entries within 10s \ + — the dedup is still a linear scan" + ), + } + } + + /// Behaviour contract: output is deduplicated across subdirectories, + /// sorted, and excludes directories and files sitting directly under + /// `/BDMV/JAR` (only one level down counts). + #[test] + fn jar_inventory_dedups_sorts_and_skips_dirs() { + let entries = vec![ + dir_entry( + "00000", + true, + vec![ + dir_entry("streamproperties.xml", false, Vec::new()), + dir_entry("zeta.png", false, Vec::new()), + dir_entry( + "nested", + true, + vec![dir_entry("hidden.txt", false, Vec::new())], + ), + ], + ), + dir_entry( + "00001", + true, + vec![ + dir_entry("alpha.png", false, Vec::new()), + // Duplicate of the entry in 00000 — must appear once. + dir_entry("streamproperties.xml", false, Vec::new()), + ], + ), + // A jar sitting directly under /BDMV/JAR is not inventoried. + dir_entry("top.jar", false, Vec::new()), + ]; + assert_eq!( + jar_inventory_from(&entries), + vec![ + "alpha.png".to_string(), + "streamproperties.xml".to_string(), + "zeta.png".to_string(), + ] + ); + } + /// Lock the parser roster + order. If someone reorders the array /// or adds/removes a parser, this test forces them to update the /// expectation explicitly. The order is load-bearing: first diff --git a/src/labels/paramount.rs b/src/labels/paramount.rs index aa6a30d..ddea33d 100644 --- a/src/labels/paramount.rs +++ b/src/labels/paramount.rs @@ -15,6 +15,7 @@ use super::{LabelPurpose, LabelQualifier, ParseResult, StreamLabel, StreamLabelType, xml}; use crate::sector::SectorSource; use crate::udf::UdfFs; +use std::collections::HashSet; pub fn detect(_reader: &mut dyn SectorSource, udf: &UdfFs) -> bool { super::jar_file_exists(udf, "playlists.xml") @@ -49,7 +50,12 @@ fn labels_from_feature(feature: &str) -> Vec { // aud_com1_idx is a trimmed, comma-separated list of CSV positions // (some authoring tools emit whitespace, and multiple commentary // tracks are possible) — symmetric with sub_com1_idx below. - let com_indices: Vec = xml::attr(feature, "aud_com1_idx") + // A HashSet, not a Vec: `com_indices` is parsed straight out of an + // attacker-controlled attribute with no length bound and was scanned + // linearly once per stream, so `aud="..."` and `aud_com1_idx="..."` + // both grown large make this quadratic in the size of one XML file. + // Membership is the only operation performed on it. + let com_indices: HashSet = xml::attr(feature, "aud_com1_idx") .map(|s| s.split(',').filter_map(|i| i.trim().parse().ok()).collect()) .unwrap_or_default(); @@ -89,7 +95,9 @@ fn labels_from_feature(feature: &str) -> Vec { .map(|s| s.split(',').map(|f| f.trim() == "1").collect()) .unwrap_or_default(); - let com_indices: Vec = xml::attr(feature, "sub_com1_idx") + // HashSet for the same reason as the audio side above: unbounded + // parsed input, membership-only use, linear scan once per stream. + let com_indices: HashSet = xml::attr(feature, "sub_com1_idx") .map(|s| s.split(',').filter_map(|i| i.trim().parse().ok()).collect()) .unwrap_or_default(); @@ -168,6 +176,75 @@ fn find_feature_playlist(text: &str) -> Option { mod tests { use super::*; + /// `sub_com1_idx` is an unbounded index list parsed straight out of the + /// disc's `playlists.xml` and was membership-tested with a linear + /// `Vec::contains` once per subtitle stream — quadratic in the size of a + /// single attacker-supplied file. + /// + /// Proof is by deadline rather than micro-benchmark. With the linear scan + /// this fixture (200 000 streams x 1 000 001 indices) measures 31 s in a + /// release build and far longer in debug; with a set it measures 0.03 s + /// release / 0.56 s debug. A 10 s deadline sits ~18x above the slowest + /// passing measurement and ~3x below the fastest failing one, and makes a + /// regression fail fast instead of hanging CI. + /// + /// Correctness is pinned on fixture-derived literals: indices 0, 2 and 4 + /// are the commentary tracks, 1 and 3 are not. + #[test] + fn commentary_index_lookup_is_not_quadratic() { + const STREAMS: usize = 200_000; + const INDICES: usize = 1_000_000; + let (tx, rx) = std::sync::mpsc::channel(); + let worker = std::thread::spawn(move || { + let mut feature = String::from(r#""#); + let _ = tx.send(labels_from_feature(&feature)); + }); + match rx.recv_timeout(std::time::Duration::from_secs(10)) { + Ok(labels) => { + worker.join().expect("worker panicked"); + assert_eq!(labels.len(), STREAMS); + assert_eq!(labels[0].purpose, LabelPurpose::Commentary); + assert_eq!(labels[1].purpose, LabelPurpose::Normal); + assert_eq!(labels[2].purpose, LabelPurpose::Commentary); + assert_eq!(labels[3].purpose, LabelPurpose::Normal); + assert_eq!(labels[4].purpose, LabelPurpose::Commentary); + } + Err(_) => panic!( + "labels_from_feature did not finish {STREAMS} streams x \ + {INDICES} commentary indices within 10s — the membership \ + test is still a linear scan" + ), + } + } + + /// Headroom: the BD STN_table admits at most 32 PG streams per playlist, + /// and a real `sub_com1_idx` lists a handful of commentary tracks. The set + /// must behave identically to the old scan on real-shaped input. + #[test] + fn commentary_indices_still_match_on_real_shaped_input() { + let feature = r#""#; + let labels = labels_from_feature(feature); + let purposes: Vec = labels.iter().map(|l| l.purpose).collect(); + assert_eq!( + purposes, + vec![ + LabelPurpose::Normal, + LabelPurpose::Commentary, + LabelPurpose::Normal, + LabelPurpose::Commentary, + LabelPurpose::Normal, + ] + ); + } + fn audio(labels: &[StreamLabel]) -> Vec<&StreamLabel> { labels .iter()