Cap total paint-graph node visits in COLRv1 painting - #225
Open
scadastrangelove wants to merge 1 commit into
Open
Cap total paint-graph node visits in COLRv1 painting#225scadastrangelove wants to merge 1 commit into
scadastrangelove wants to merge 1 commit into
Conversation
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/).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #221.
Adds a
visits_left: u32field toRecursionStack(initialized to a newMAX_PAINT_VISITS: u32 = 100_000constant inTable::paint, its only construction site) and aconsume_visit()method, checked inparse_paintalongside the existing cycle check:Because
parse_paintis the single choke point all 25 recursive call sites go through, thisneeded no changes to any of them, or to any of the 5 function signatures in the call chain
(
paint→paint_impl→paint_v1→parse_paint→parse_paint_impl).Testing
cargo test --all-features --release— 148/149 (pre-existing unrelatedfailure, see Guard CFF2 BLEND operator against empty argument stack #222 for detail).
paint()called 66,658times (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.