From 8f9bde9b9a6d425f74b5235a6e647966ba53e8dd Mon Sep 17 00:00:00 2001 From: Matthew Jackson <1085847+MattJackson@users.noreply.github.com> Date: Tue, 11 Aug 2026 16:59:33 -0700 Subject: [PATCH] Bound the third hostile CSV, and stop two docs overclaiming MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `forced_sub` was the last unbounded attacker-controlled list in paramount.rs. `MAX_COM_INDICES` capped the two `*_com1_idx` attributes; this one had nothing capping it at all, and unlike them it has no value to filter — a cell is a classification of the position it sits at, so its bound has to be positional. Extracted as `forced_subs` for the same reason `com_indices` was extracted: through `labels_from_feature` the bound is unobservable, because the subtitle loop cannot reach those cells either, so the assertion could not fail. The `MAX_COM_INDICES` doc claimed the entry-allocation argument for the whole constant. It is the VALUE filter that caps the set (values below the bound, so at most that many distinct entries, however long the attribute); the `take` caps the WORK. Both are real and they are not the same bound; the doc now says which is which. `jar_inventory_dedup_is_not_quadratic` called itself proof by deadline. It is a hang guard — a return to the linear scan runs for minutes and would wedge CI rather than fail it — and no assertion in it can tell a BTreeSet from any other sub-quadratic dedup. Renamed and documented for what it does. Its margin was measured before keeping it: 0.14s debug / 0.07s release against 10s, ~70x, unlike the 6x that made paramount.rs's wall-clock test flake. --- src/labels/mod.rs | 17 ++++++-- src/labels/paramount.rs | 95 +++++++++++++++++++++++++++++++++++------ 2 files changed, 94 insertions(+), 18 deletions(-) diff --git a/src/labels/mod.rs b/src/labels/mod.rs index c57ce36..5f661dc 100644 --- a/src/labels/mod.rs +++ b/src/labels/mod.rs @@ -1418,11 +1418,20 @@ mod registry_tests { /// 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. + /// This is a HANG GUARD, and the name says so: a return to the linear scan + /// makes this fixture run for minutes (120 000² / 2 comparisons over a + /// 180-byte shared prefix), which without the deadline would wedge CI + /// rather than fail it. It is not a complexity proof — no assertion here + /// can distinguish `BTreeSet` from any other sub-quadratic dedup, and the + /// clock-free half of the claim (dedup, sort, directory exclusion) belongs + /// to `jar_inventory_dedups_sorts_and_skips_dirs` below. + /// + /// The deadline is a real margin, unlike the 6x one that made + /// `paramount.rs`'s wall-clock test flake under a loaded CI box: measured + /// at 0.14 s debug / 0.07 s release against 10 s, so ~70x. A shared CPU + /// does not close that; a quadratic dedup does not survive it. #[test] - fn jar_inventory_dedup_is_not_quadratic() { + fn jar_inventory_dedup_does_not_hang_on_a_hostile_directory() { const FILES: usize = 120_000; let (tx, rx) = std::sync::mpsc::channel(); let worker = std::thread::spawn(move || { diff --git a/src/labels/paramount.rs b/src/labels/paramount.rs index 4ccfff3..7973bd4 100644 --- a/src/labels/paramount.rs +++ b/src/labels/paramount.rs @@ -98,19 +98,29 @@ enum ForcedSub { ForcedNarrative, } -/// The most `*_com1_idx` entries worth parsing from one playlist. +/// The one number behind every cap in this parser: the highest CSV cell +/// position that can ever be addressed. /// -/// These indices address CSV cell positions, and a cell is only addressable -/// while its 1-based number fits a `u16` — the loops below `break` at -/// `u16::try_from(i + 1)`. So an index at or beyond `u16::MAX` can never match -/// a cell, and more than that many entries cannot describe anything new. +/// The labelling loops number cells 1-based into a `u16` and `break` at +/// `u16::try_from(i + 1)`, so cell `MAX_COM_INDICES` and everything past it is +/// never visited. Two different things are measured against that, and they are +/// not the same bound: /// -/// The bound is what makes the parse safe on hostile input, not merely fast. -/// The `HashSet` that replaced a linear scan fixed the LOOKUP cost, but the -/// set is still built from an attribute with no length limit: a disc -/// declaring half a billion indices allocates half a billion entries before -/// any lookup happens. Real authoring is nowhere near this — the BD STN table -/// admits at most 32 streams per playlist — so nothing legitimate is lost. +/// - **A VALUE at or beyond it cannot match any cell.** This is what caps the +/// set: values are filtered before insertion, so at most `MAX_COM_INDICES` +/// distinct entries can ever be stored, however long the attribute is. The +/// `HashSet` that replaced a linear scan fixed the LOOKUP cost; this is what +/// fixes the ALLOCATION, and a disc declaring half a billion indices no +/// longer costs half a billion entries. +/// - **A POSITION at or beyond it describes nothing new.** This caps the WORK, +/// not the memory — the value filter already made the set small, but without +/// it every one of those half a billion cells is still split and parsed. It +/// is an early exit at the first position whose contents provably cannot +/// matter, and it is why `forced_sub` — which holds no set at all, and so +/// gets no protection from the value rule — is bounded too. +/// +/// Real authoring is nowhere near either limit: the BD STN table admits at +/// most 32 streams per playlist, so nothing legitimate is lost. const MAX_COM_INDICES: usize = u16::MAX as usize; /// Parse a `*_com1_idx` attribute into the set the labelling loops query. @@ -132,6 +142,30 @@ fn com_indices(attr: Option) -> HashSet { .unwrap_or_default() } +/// Parse the `forced_sub` attribute into the cell list the subtitle loop +/// queries — the third attacker-controlled CSV in this file, and the last one +/// that was still unbounded. +/// +/// Bounded by POSITION, and it has no other choice: a `*_com1_idx` list holds +/// values that can be filtered, and that filter is what caps its set, but a +/// `forced_sub` cell is a classification of the position it sits at, so there +/// is nothing to filter and nothing else would ever cap this. The Vec is read +/// only as `forced.get(i)` from a loop that stops at `MAX_COM_INDICES`, so +/// every cell past that is unreachable by construction. +/// +/// Extracted, like [`com_indices`], so the bound is OBSERVABLE. Through +/// `labels_from_feature` it is not: the subtitle loop cannot reach those cells +/// either, so a label-level assertion passes whether or not the cap exists. +fn forced_subs(attr: Option) -> Vec { + attr.map(|s| { + s.split(',') + .take(MAX_COM_INDICES) + .map(forced_sub_cell) + .collect() + }) + .unwrap_or_default() +} + fn forced_sub_cell(cell: &str) -> ForcedSub { match cell.trim() { "1" => ForcedSub::ContainsForcedSegments, @@ -204,9 +238,7 @@ fn labels_from_feature(feature: &str) -> Vec { // Parse subtitle streams if let Some(sub) = xml::attr(feature, "sub") { - let forced: Vec = xml::attr(feature, "forced_sub") - .map(|s| s.split(',').map(forced_sub_cell).collect()) - .unwrap_or_default(); + let forced = forced_subs(xml::attr(feature, "forced_sub")); // HashSet for the same reason as the audio side above: unbounded // parsed input, membership-only use, linear scan once per stream. @@ -418,6 +450,41 @@ mod tests { assert_eq!(com_indices(Some("0,2,4".to_string())).len(), 3); } + /// `forced_sub` is bounded too — the third CSV in the same function, and + /// the one that had no value filter to hide behind. + /// + /// Read through `forced_subs` rather than through the labels for the same + /// reason the two tests above read the set: the subtitle loop stops at + /// `MAX_COM_INDICES`, so a label-level assertion cannot tell a bounded + /// parse from an unbounded one. + #[test] + fn forced_sub_cells_past_the_last_addressable_one_are_not_parsed() { + let hostile = "0,".repeat(MAX_COM_INDICES + 50_000); + let cells = forced_subs(Some(hostile)); + assert_eq!( + cells.len(), + MAX_COM_INDICES, + "parsed {} cells — the forced_sub parse is still unbounded", + cells.len() + ); + } + + /// Bounding it must not change what a legitimate playlist means: the + /// cells that CAN address a stream still classify exactly as before. + #[test] + fn bounding_forced_sub_leaves_the_addressable_cells_alone() { + let cells = forced_subs(Some("0,1,3,2".to_string())); + assert_eq!( + cells, + vec![ + ForcedSub::None, + ForcedSub::ContainsForcedSegments, + ForcedSub::ForcedNarrative, + ForcedSub::ForcedNarrative, + ] + ); + } + /// An index that cannot address any cell is dropped rather than STORED. /// /// Asserted through `com_indices`, not through the labels: the labelling