Bound the third hostile CSV, and stop two docs overclaiming

`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.
This commit is contained in:
Matthew Jackson
2026-08-11 16:59:33 -07:00
parent 2d1563c63a
commit 8f9bde9b9a
2 changed files with 94 additions and 18 deletions
+13 -4
View File
@@ -1418,11 +1418,20 @@ mod registry_tests {
/// straight from the disc's UDF directory records, with attacker-chosen /// straight from the disc's UDF directory records, with attacker-chosen
/// name lengths to inflate each comparison. /// name lengths to inflate each comparison.
/// ///
/// Proof is by deadline. With the linear scan this fixture measures well /// This is a HANG GUARD, and the name says so: a return to the linear scan
/// past the deadline; with a set it is milliseconds. Bounded so a /// makes this fixture run for minutes (120 000² / 2 comparisons over a
/// regression fails fast instead of hanging CI. /// 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] #[test]
fn jar_inventory_dedup_is_not_quadratic() { fn jar_inventory_dedup_does_not_hang_on_a_hostile_directory() {
const FILES: usize = 120_000; const FILES: usize = 120_000;
let (tx, rx) = std::sync::mpsc::channel(); let (tx, rx) = std::sync::mpsc::channel();
let worker = std::thread::spawn(move || { let worker = std::thread::spawn(move || {
+81 -14
View File
@@ -98,19 +98,29 @@ enum ForcedSub {
ForcedNarrative, 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 /// The labelling loops number cells 1-based into a `u16` and `break` at
/// while its 1-based number fits a `u16` — the loops below `break` at /// `u16::try_from(i + 1)`, so cell `MAX_COM_INDICES` and everything past it is
/// `u16::try_from(i + 1)`. So an index at or beyond `u16::MAX` can never match /// never visited. Two different things are measured against that, and they are
/// a cell, and more than that many entries cannot describe anything new. /// not the same bound:
/// ///
/// The bound is what makes the parse safe on hostile input, not merely fast. /// - **A VALUE at or beyond it cannot match any cell.** This is what caps the
/// The `HashSet` that replaced a linear scan fixed the LOOKUP cost, but the /// set: values are filtered before insertion, so at most `MAX_COM_INDICES`
/// set is still built from an attribute with no length limit: a disc /// distinct entries can ever be stored, however long the attribute is. The
/// declaring half a billion indices allocates half a billion entries before /// `HashSet` that replaced a linear scan fixed the LOOKUP cost; this is what
/// any lookup happens. Real authoring is nowhere near this — the BD STN table /// fixes the ALLOCATION, and a disc declaring half a billion indices no
/// admits at most 32 streams per playlist — so nothing legitimate is lost. /// 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; const MAX_COM_INDICES: usize = u16::MAX as usize;
/// Parse a `*_com1_idx` attribute into the set the labelling loops query. /// Parse a `*_com1_idx` attribute into the set the labelling loops query.
@@ -132,6 +142,30 @@ fn com_indices(attr: Option<String>) -> HashSet<usize> {
.unwrap_or_default() .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<String>) -> Vec<ForcedSub> {
attr.map(|s| {
s.split(',')
.take(MAX_COM_INDICES)
.map(forced_sub_cell)
.collect()
})
.unwrap_or_default()
}
fn forced_sub_cell(cell: &str) -> ForcedSub { fn forced_sub_cell(cell: &str) -> ForcedSub {
match cell.trim() { match cell.trim() {
"1" => ForcedSub::ContainsForcedSegments, "1" => ForcedSub::ContainsForcedSegments,
@@ -204,9 +238,7 @@ fn labels_from_feature(feature: &str) -> Vec<StreamLabel> {
// Parse subtitle streams // Parse subtitle streams
if let Some(sub) = xml::attr(feature, "sub") { if let Some(sub) = xml::attr(feature, "sub") {
let forced: Vec<ForcedSub> = xml::attr(feature, "forced_sub") let forced = forced_subs(xml::attr(feature, "forced_sub"));
.map(|s| s.split(',').map(forced_sub_cell).collect())
.unwrap_or_default();
// HashSet for the same reason as the audio side above: unbounded // HashSet for the same reason as the audio side above: unbounded
// parsed input, membership-only use, linear scan once per stream. // 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); 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. /// An index that cannot address any cell is dropped rather than STORED.
/// ///
/// Asserted through `com_indices`, not through the labels: the labelling /// Asserted through `com_indices`, not through the labels: the labelling