Skip to content

Cap total paint-graph node visits in COLRv1 painting - #225

Open
scadastrangelove wants to merge 1 commit into
harfbuzz:mainfrom
scadastrangelove:fix/colr-paint-visit-budget
Open

Cap total paint-graph node visits in COLRv1 painting#225
scadastrangelove wants to merge 1 commit into
harfbuzz:mainfrom
scadastrangelove:fix/colr-paint-visit-budget

Conversation

@scadastrangelove

Copy link
Copy Markdown

Closes #221.

Adds a visits_left: u32 field to RecursionStack (initialized to a new
MAX_PAINT_VISITS: u32 = 100_000 constant in Table::paint, its only construction site) and a
consume_visit() method, checked in parse_paint alongside the existing cycle check:

// Cycle detected
if recursion_stack.contains(offset) {
    return None;
}

// Total-visit budget exceeded (a DAG can revisit the same offset via different sibling
// branches without ever cycling on the active path -- see RecursionStack::visits_left)
recursion_stack.consume_visit().ok()?;

recursion_stack.push(offset).ok()?;

Because parse_paint is the single choke point all 25 recursive call sites go through, this
needed no changes to any of them, or to any of the 5 function signatures in the call chain
(paintpaint_implpaint_v1parse_paintparse_paint_impl).

Testing

  • Full suite green: cargo test --all-features --release — 148/149 (pre-existing unrelated
    failure, see Guard CFF2 BLEND operator against empty argument stack #222 for detail).
  • depth=18/branching=3 (16–20+s before) now completes in ~3.6ms, paint() called 66,658
    times (bounded by the new budget, as expected); depth=20/branching=3 (didn't complete in
    20s before) now completes in ~4ms.

Discovered by the rust-in-peace security pipeline.

RecursionStack only detects a cycle on the current active call path (an
entry is popped once its call returns), so the same paint offset reachable
through different sibling branches of a DAG -- never actually cycling on
any one path -- is not caught. PaintColrLayers's handler loop also discards
each recursive parse_paint() result with no ?, so a failed/cyclic branch
never short-circuits exploration of the rest, which is exactly what lets a
shared-subtree paint graph force branching^depth calls.

Adds a visits_left budget directly to RecursionStack (100_000, mirroring
glyf/gvar's fix), checked in parse_paint alongside the existing cycle
check. Since parse_paint is the single choke point all 25 recursive call
sites already go through, this needed no signature changes beyond
RecursionStack itself.

Discovered by the rust-in-peace security pipeline
(https://github.com/scadastrangelove/rust-in-peace/).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

COLRv1 PaintColrLayers DAG with shared layer subtrees causes unbounded CPU cost — RecursionStack only detects same-path cycles, not cross-branch reuse

1 participant