Skip to content

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

Merged
Goldziher merged 1 commit into
harfbuzz:mainfrom
scadastrangelove:fix/colr-paint-visit-budget
Aug 5, 2026
Merged

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

Conversation

@scadastrangelove

Copy link
Copy Markdown
Contributor

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/).
tobocop2 added a commit to tobocop2/kreuzberg that referenced this pull request Aug 5, 2026
… (upstream xberg-io#225)

Upstream pull request harfbuzz/ttf-parser#225, commit 99aa5e3, applied
verbatim to the vendored v0.25.1 tree.

RecursionStack detects a cycle only on the current root-to-leaf path, since
an entry is popped as soon as its call returns. A paint graph shaped as a
DAG, where the same offset is reachable through several sibling branches and
no single branch cycles, slips past it entirely: a chain of PaintColrLayers
records whose layer slots all point at one shared next-level record forces
branching^depth calls while never re-entering the active path. A total-visit
budget now bounds the work for one top-level paint call regardless of
branching factor.

The comment on the new constant refers to glyf::MAX_COMPONENT_VISITS, which
upstream xberg-io#224 introduces; that pick lands later in this branch.
Goldziher added a commit to xberg-io/xberg that referenced this pull request Aug 5, 2026
…nreleased correctness and DoS fixes (#1384)

* chore(ttf-parser): vendor upstream ttf-parser v0.25.1 verbatim

Upstream harfbuzz/ttf-parser is currently unmaintained: at the time of
vendoring the most recent commit was 2025-11-22 and correctness fixes were
sitting unreviewed, so waiting on a release is not a plan.

This commit is the upstream v0.25.1 tag, unmodified, and nothing else. Every
upstream fix we carry lands as its own commit on top, so `git log` shows
exactly which upstream pull request was taken and when, and each one can be
reviewed and reverted independently. All xberg-side modifications land in a
final separate commit.

Not vendored: benches/, examples/, c-api/, testing-tools/, meson.build.

* fix(ttf-parser): apply upstream harfbuzz/ttf-parser#228 (dotsection)

Upstream pull request harfbuzz/ttf-parser#228, commit 023f8163, applied
verbatim to the vendored v0.25.1 tree.

ttf-parser rejects any CFF charstring containing the deprecated `dotsection`
operator (escape `12 0`) with UnsupportedOperator, which aborts the whole
charstring and leaves the glyph with no outline. Adobe's Type 1 to Type 2
conversion preserves the operator, so real-world fonts still carry it, and
the glyphs that carry it are exactly the dot-bearing ones: i, j, ! and .
A page rendered from such a PDF silently loses those characters, and OCR run
over the page transcribes the gaps.

The operator takes no arguments and is a no-op for outlining, matching
read-fonts and FreeType. It is now skipped rather than rejected. Upstream's
own two regression cases come with it and pin the behavior, including that
operands already on the argument stack survive it untouched.

* docs(ttf-parser): document Face::style() OS/2 fallback (upstream #203)

Upstream pull request harfbuzz/ttf-parser#203, commit b422ac0, applied
verbatim to the vendored v0.25.1 tree.

Face::style() reads the style from the OS/2 table and falls back to the
head table's mac style bits when OS/2 is absent. The rustdoc did not say
so, leaving callers to read the implementation to find out which table
actually answers the query. Documentation only; no behavior change.

* fix(ttf-parser): apply upstream harfbuzz/ttf-parser#222 (CFF2 BLEND stack guard)

Upstream pull request harfbuzz/ttf-parser#222, commit 3a585f1, applied
verbatim to the vendored v0.25.1 tree.

The CFF2 `blend` operator pops its operand count off the argument stack
without first checking that the stack holds anything. `ArgumentsStack::pop`
is unchecked, so a charstring that reaches `blend` with an empty stack reads
a stale slot and carries on with a bogus operand count. Every other pop in
the CFF2 interpreter already guards its stack depth; this was the last one
that did not. It now returns InvalidArgumentsStackLength like the rest.

* fix(ttf-parser): apply upstream harfbuzz/ttf-parser#223 (avar i16 overflow)

Upstream pull request harfbuzz/ttf-parser#223, commit 32439d2, applied
verbatim to the vendored v0.25.1 tree.

avar::map_value computed `value - from + to` in i16 at three sites. A
segment map whose from and to coordinates sit at opposite extremes pushes
that expression outside i16, and with overflow checks off in release it
wraps silently and yields a normalized coordinate pointing at the wrong
place on the axis. The arithmetic is promoted to i32 behind a shared
shift_coordinate helper, which rejects out-of-range results rather than
wrapping them.

One i32 multiply in the interpolation branch of the same function can
still overflow. That is a separate pre-existing defect, untouched by this
patch, and is being reported upstream rather than diverged here.

* fix(ttf-parser): cap total paint-graph node visits in COLRv1 painting (upstream #225)

Upstream pull request harfbuzz/ttf-parser#225, commit 99aa5e3, applied
verbatim to the vendored v0.25.1 tree.

RecursionStack detects a cycle only on the current root-to-leaf path, since
an entry is popped as soon as its call returns. A paint graph shaped as a
DAG, where the same offset is reachable through several sibling branches and
no single branch cycles, slips past it entirely: a chain of PaintColrLayers
records whose layer slots all point at one shared next-level record forces
branching^depth calls while never re-entering the active path. A total-visit
budget now bounds the work for one top-level paint call regardless of
branching factor.

The comment on the new constant refers to glyf::MAX_COMPONENT_VISITS, which
upstream #224 introduces; that pick lands later in this branch.

* fix(ttf-parser): parse loca for fonts with the maximum 65535 glyphs (upstream #226)

Upstream pull request harfbuzz/ttf-parser#226, commit 86daf57, applied
verbatim to the vendored v0.25.1 tree.

A loca table holds numGlyphs + 1 offsets, so a font at the spec maximum of
65535 glyphs has 65536 of them. The offset count was held in a u16, and
u16::try_from(65536) fails, which made Table::parse return None for the whole
table. Face::parse then leaves glyf as None, so the face yields no TrueType
outlines at all, and the gvar path dies with it through the `glyf?` in
Face::outline_glyph. A second, narrower instance of the same overflow made
the last glyph unreachable even when the table did parse.

The counters and the offset array widen to u32, which also makes good on the
existing comment about ignoring a longer-than-expected loca. The array read
stays bounded at numGlyphs + 1 entries, so nothing new is allocated or read.

This is a latent fix rather than a live one: a scan of 452 system fonts plus
the fixtures in tree found no glyf-flavored face large enough to trigger it.
It also widens the public loca::Table signature, which no crate in this
workspace's dependency graph calls.

* fix(ttf-parser): read fvar HIDDEN_AXIS from bit 0, not reserved bit 3 (upstream #216)

Upstream pull request harfbuzz/ttf-parser#216, commit 9b9e55f, applied
verbatim to the vendored v0.25.1 tree.

The fvar axis record's HIDDEN_AXIS flag is bit 0. VariationAxis::parse read
bit 3, which the spec reserves and requires to be clear, so hidden was
initialized from a bit that carries no meaning: every hidden axis was
reported as visible, and any font that set the reserved bit was reported as
hidden. Confirmed against the spec and against skrifa, which defines
AXIS_HIDDEN_FLAG as 0x1.

A regression test follows in the next commit; the one-line change is
silently reversible and reverting it breaks nothing in the existing suite.

* test(ttf-parser): pin fvar HIDDEN_AXIS bit-0 decoding

xberg-authored follow-up to the previous commit. Upstream #216 ships no test,
and nothing in the existing suite reads VariationAxis::hidden, so reverting
the one-line fix leaves every test green.

Three cases over a synthetic single-axis fvar table: bit 0 set means hidden,
no flags means visible, and reserved bit 3 set means visible. The last one is
what pins the change rather than smoke-testing around it, since it fails in
the opposite direction if the old `flags >> 3` reading comes back.

The test lives at the top level of tests/ rather than in tests/tables/,
because adding a module there would mean editing tests/tables/main.rs, which
is kept byte-identical to upstream so future cherry-picks stay conflict-free.
No feature gate: the dev-dependency on compat/ pulls default features, which
unifies variable-fonts onto the crate in every test build.

* fix(ttf-parser): return None from set_variation for an unknown axis tag (upstream #207)

Upstream pull request harfbuzz/ttf-parser#207, commit 52a9811, applied
verbatim to the vendored v0.25.1 tree.

Face::set_variation documents that it returns None when the face has no such
variation axis, but it returned Some(()) unconditionally: the loop simply
found no matching tag, changed nothing, and reported success. Callers could
not tell a typo'd or unsupported axis from an applied one.

The maintainer's open question on the PR was whether the docs were merely
outdated. They are not: pre-0.20 the method carried an explicit else branch
returning None, and the rewrite that turned it into a loop dropped it. This
restores the documented contract rather than inventing one.

A regression test follows in the next commit; nothing in the existing suite
asserts this return value.

* test(ttf-parser): pin set_variation unknown-axis contract

xberg-authored follow-up to the previous commit. Upstream #207 ships no test
and nothing in the existing suite asserts what set_variation returns, so the
restored contract would silently rot.

Three arms over fixtures already in tree. Only the first exercises the fix:
an unknown tag on a variable face returned Some(()) before #207 and returns
None after, verified by reverting the change on a pod and watching this test
go red. The known-tag case guards against over-correcting into always-None.
The non-variable case is already handled by the `!self.is_variable()` guard
ahead of the loop, so it passes either way; it is kept for the other half of
the documented contract and labelled as such rather than being left to look
like coverage it does not provide.

The test lives at the top level of tests/ rather than in tests/tables/, so
tests/tables/main.rs stays byte-identical to upstream.

* fix(ttf-parser): cap total component visits in glyf/gvar composite outlining (upstream #224)

Upstream pull request harfbuzz/ttf-parser#224, commit dd2337b, applied
verbatim to the vendored v0.25.1 tree.

MAX_COMPONENTS bounds the depth of a single component chain, not how often a
component subtree is revisited through sibling components. A composite glyph
whose components all point at one shared child forces branching^depth calls
while staying under the depth cap and needing only depth+1 distinct glyphs.
Both outline_impl and outline_var_impl now carry a shared visit budget from
the top-level entry point.

The budget as upstream wrote it charges one unit per call, which bounds the
number of calls but not the work inside them, since a single glyph can carry
up to 65535 component records. The next commit charges per record.

* fix(ttf-parser): charge the glyf/gvar visit budget per component record

xberg-authored follow-up to the previous commit. Upstream #224's budget is
charged once on entry to outline_impl / outline_var_impl, which bounds the
number of calls but not the work inside them: a single composite glyph can
carry up to 65535 component records, and components whose glyph range does
not resolve are iterated and skipped without ever recursing. A glyph padded
with unresolvable components therefore costs one budget unit no matter how
wide it is, and can be re-walked until the depth cap stops it. The budget is
now charged per component record in both loops, so total work is bounded by
MAX_COMPONENT_VISITS rather than only total call count.

Charging components_count up front in gvar would not do: it is computed as
`.count() as u16`, so a glyph with an exact multiple of 65536 components
truncates to zero and is charged nothing.

MAX_COMPONENT_VISITS stays at 100_000, still around four orders of magnitude
above any real font, and the per-record cost is O(1).

No test: the observable difference is a timing bound, and a timing assertion
would be flaky in CI. This is the first behavior-affecting xberg divergence
inside the vendored src/ tree, so both sites carry a comment saying so.

* test(ttf-parser): pin a total-visit budget on the CFF charstring interpreter

xberg-authored regression test, committed before the fix. A single glyph whose
charstring calls one global subroutine 150,000 times, each call flat (depth 1)
so it stays under the nesting limit, drives one interpreter invocation per call.
Without a total-visit budget the glyph outlines successfully after doing all the
work; the test asserts outlining fails instead.

Deterministic, not timing-based: the assertion is on the return value. Applied
on its own on a pod, the test fails (the glyph outlines) and the following
commit makes it pass. Lives at the top level of tests/ so tests/tables/main.rs
stays byte-identical to upstream.

This commit alone is red; the interpreter has no budget yet. The fix is next.

* fix(ttf-parser): cap total charstring visits in the CFF interpreter

xberg-authored, not from an upstream PR. Upstream #224 and #225 cap the total
work of glyf/gvar composite outlining and COLRv1 painting, but the CFF
charstring interpreter had no equivalent, and upstream (unmaintained) does not
address it.

STACK_LIMIT bounds how deeply subroutine calls nest on the current path, not how
many are made in total. A charstring that calls a subroutine over and over
without nesting stays under STACK_LIMIT forever and drives one
_parse_char_string invocation per call with no ceiling, exponential in the worst
case. This is the path a PDF renderer exercises most, since CFF is the dominant
embedded-font flavor.

Both the CFF1 and CFF2 interpreters now carry a MAX_CHARSTRING_VISITS budget of
100_000, charged once per invocation through the shared parser context and
returning the existing NestingLimitReached error when exhausted. A real glyph
needs a few hundred. Reusing the existing error keeps the public API unchanged.

Mirrors the shape of upstream #224. This is the second behavior-affecting xberg
change inside the vendored src/ tree, alongside the #224 per-record budget.
The previous commit's regression test now passes.

* feat(ttf-parser): route the transitive ttf-parser onto the vendored copy

Nothing in xberg depends on ttf-parser directly; it arrives through
pdf_oxide, fontdb, lopdf and rustybuzz, which request it by its crates.io
name. Cargo matches [patch.crates-io] entries on package name alone, so a
crate named xberg-ttf-parser cannot redirect them. The compat/ subcrate
carries the upstream name, re-exports the vendored parser, and is what the
workspace patch entry points at. One parser is compiled, so types stay
identical across every consumer. It is never published.

The only edits to vendored source are 17 lines of crate-level lint allows.
The workspace lint set is stricter than upstream's CI, and these are allowed
rather than fixed because div_ceil and is_multiple_of postdate upstream's
1.63 MSRV, so rewriting them would break upstream's own build. For the same
reason the vendored tree is excluded from poly formatting.

The vendored tests keep referring to ttf_parser via a path-only dev-dependency
on the shim, which cargo strips when packaging. tiny-skia-path is bumped to the
0.12 already in the workspace so the lockfile does not carry two copies.

xberg-ttf-parser is published alongside the other vendored crates. Note that
[patch] does not propagate to downstream crates.io consumers: this fixes every
artifact we build and ship, not people who depend on xberg from the registry.

* test(pdf): render a dotsection CFF font end to end through the vendored parser

Carries the regression test from the superseded PR #1363, which proves the
user-visible fix: that xberg actually paints the dot-bearing glyphs, not merely
that the parser accepts the operator. The vendored crate's own tests cover the
charstring interpreter; nothing here proved a glyph reaches the page.

Renders a single-page PDF embedding a synthetic Type 1C font (fontTools-built,
no third-party font data) whose i, j, period, colon, semicolon, exclam and
question glyphs carry the deprecated dotsection operator the way Adobe's Type 1
to Type 2 converter emits it. It rasterizes through the production render path
and asserts per-glyph ink; three control glyphs without dotsection guard the
fixture itself. Stock ttf-parser 0.25.1 drops all seven dotsection glyphs, so
this passes only because the workspace routes onto the vendored parser.

Gated on the `pdf` feature, which CI reaches through `full` -> `formats` -> `pdf`,
so `cargo test -p xberg --features full` runs it. Verified locally as well
(Apple M1 Pro, macOS 14 arm64): `cargo test -p xberg --features pdf` green.

---------

Co-authored-by: Na'aman Hirschfeld <nhirschfeld@gmail.com>
@Goldziher

Copy link
Copy Markdown
Collaborator

Merging, with two follow-ups I'll apply on top rather than asking you to revise.

The gap is real and the choke-point analysis is right. I verified all 26 recursion sites rather than taking the description on trust — every parse_paint call in parse_paint_impl (formats 1, 4-10, 12-31, and both arms of PaintComposite) plus the PaintColrGlyph path through paint_implpaint_v1 funnels through parse_paint. No site is missed, and it was correct to change nothing else.

The existing RecursionStack is a path structure, not a visited set: pop() runs as soon as the call returns, so stack[..len] is the active root-to-leaf path. It bounds height at 64 and forbids a repeat along one path, and says nothing about how many paths exist. With layers_count being a u8, fan-out reaches 255 per node, so a ~1 KB font gets arbitrarily deep re-walks with no offset ever repeating on any single path. Both existing guards are defeated exactly as you describe.

No state leakRecursionStack has exactly one construction site, inside pub fn paint, which is the sole entry point and is never called internally. Fresh budget per top-level call, so painting many glyphs in sequence cannot start failing spuriously. That was my main worry and it is clean.

Follow-up 1 (I'll apply): move the charge above the free bail-outs. consume_visit() currently sits after Stream::new_at, the format read::<u8>, and the contains(offset) cycle check — all three return without charging. An attacker sets 254 of each node's 255 slots to an on-path ancestor (free contains() bail) and one slot to the real successor, which makes the effective bound about 100,000 × 256 ≈ 25.5M entries rather than 100,000. Charging first makes the bound exact and matches harfbuzz, which decrements edge_count before dispatch.

Follow-up 2: the constant's comment. It cites glyf::MAX_COMPONENT_VISITS — that symbol didn't exist when you wrote this, but #224 landed a few minutes ago and introduced it at glyf.rs:437, so the reference is now valid rather than dangling. I'll still reword it to cite harfbuzz's HB_MAX_GRAPH_EDGE_COUNT = 2048 as the empirical anchor, since "we picked a number" is weaker than "the production reference ships 2048 and renders Noto Color Emoji." 100,000 stays — being ~48x looser than harfbuzz means zero false-rejection risk.

One thing outside this diff that I'm filing separately: paint_v0 is charged a single visit for a loop of up to num_layers = 65,535 iterations. Aim the budgeted nodes at such a record and you still get billions of layer paints. Same vulnerability class, pre-existing, not yours to fix here.

Silent partial paint on exhaustion matches the existing cycle/depth behaviour, so it's consistent rather than a regression. Rebases clean.

Thanks — the fan-out-versus-depth distinction was correctly identified and the single-choke-point claim held up under a full audit.

@Goldziher
Goldziher merged commit f03027c into harfbuzz:main Aug 5, 2026
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

2 participants