Repository navigation
feat: scene font bundles with pre-filled TMP and UI Toolkit atlases (0.20.0) - #126
Conversation
Scene .ttf/.otf files (the explorer's font_src, unity-explorer#10118) now convert to a font bundle: the source Font, a dynamic TMP_FontAsset and a dynamic UI Toolkit FontAsset, each with an SDFAA atlas pre-filled with ASCII, smart punctuation, the euro sign and the common Latin-1 letters. Common text renders on the first frame instead of waiting for FreeType on the main thread; any other character is still added at runtime through the bundled font. - fontgen: FreeType-exact face and glyph metrics, TextCore's packing convention, and the exact distance field, fitted against atlases Unity 6000.5 baked itself - builder::font: assembles the bundle from template/font-types.mac.bundle, a Unity-built type donor (generator in template/src/FontTypesTemplate.cs) - SerializedFile format 23 (Unity 6000.5) read and write support, needed to read the donor - live: fonts convert for scenes only, digest-named under a new Font recipe; a font that does not parse gets no bundle and does not fail the entity - sfdump and fontcal examples for inspecting bundles and re-fitting against Unity Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- fontgen::sdf used f64::hypot, which links glibc's hypot@GLIBC_2.35 past the 2.34 floor the node addon is held to; the curve deviation now takes the square root directly - crate/abgen-wasm and crate/abgen-node keep their own lockfiles and did not know ttf-parser - templates_missing_reports_every_absent_required_template lists the font template Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
a1a5986 to
426315c
Compare
`pub mod fontgen;` had been inserted under the `#[cfg(not(target_arch = "wasm32"))]` that gates glbscan, which dropped fontgen from the wasm build and compiled glbscan into it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
popuz
left a comment
There was a problem hiding this comment.
🤖 Review by Claude Code, posted on behalf of Vitaly.
Security pass on the font lane (perf side looks great — measured on the client: pre-fill cuts Latin-text rasterization from ~40–50 ms to ~4 ms per burst).
As it stands the conversion is not a security boundary yet:
- Original bytes ship to the client —
builder/font.rs(font.insert("m_FontData", Value::Bytes(bytes.to_vec()))). Any character outside the pre-filled set makes the client's FreeType parse the untrusted original file, and the scene controls its own text, so it can always trigger that. Suggestion: sanitize (OTS or fontTools re-serialization, drop hinting/unknown tables) and embed the sanitized bytes instead. - Validation is minimal —
fontgen/mod.rsis_font_file/is_supported: magic +Face::parse+ upem + one mapped character. No file-size cap, no glyph/point/segment limits. Suggest matching the client's 16 MB cap (or lower) and capping points per glyph. - Magic accepts
OTTOandtrue, but the protocol and the client's raw path are TrueType-only (00010000). Suggest restricting to TrueType so both paths accept the same set. - Converter DoS —
fontgen/sdf.rssigned_distanceis brute force O(pixels × segments) with no spatial culling, run on all cores, with every pre-filled outline held in memory before packing. A font with heavy outlines could burn a lot of CPU/RAM per conversion. Suggest bbox culling plus a timeout / memory cap on the font lane so a slow font is skipped rather than stalling the pipeline.
Review on #126 (security pass): - Only sfnt version 0x00010000 and .ttf paths take the font lane, matching the protocol's font_src ("a ttf font file") and the explorer's header check; OTF/CFF, `true`, collections and web fonts get no bundle. - Inputs are capped: the explorer's 16 MB file limit, 8192 outline points per glyph with composites expanded, 1M flattened segments across the pre-filled glyphs. - The distance field no longer evaluates every segment at every texel. A texel a gradient scale from the outline saturates, so each segment visits only the texels within that spread, and winding comes from per-row crossings; the bytes are unchanged (pinned by a test against the exhaustive field). The bake refuses a font whose pre-fill would exceed a fixed work budget instead of stalling the conversion. - The pre-fill gains åæøÅÆØýÝÿ, the Latin-1 letters it was missing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rning Review on #126: the bundle embedded the uploaded file, which the client's FreeType parses whenever text needs a glyph outside the pre-fill, and a scene controls its own text. - fontgen::rebuild writes a new TrueType file from what ttf-parser parsed and the limits accepted: glyf/loca re-encoded without hinting instructions (composites flattened, glyph ids kept, fractional midpoints left implied), cmap formats 4 and 12, head/hhea/hmtx/maxp/OS/2/post with the source's metrics, a family/style name table, and a GPOS of one kern lookup. Hinting programs, variation, colour/bitmap and every other layout table are dropped. Capped at 8192 points per glyph, 4M per font, coordinates within ±16383 and 16 MB of output. - fontgen::kerning reads the pairs among the pre-fill glyphs from the GPOS kern feature (or the legacy kern table), visiting at most 256 subtables. - The bake runs on the rebuilt font, so the atlas and the font FreeType reads agree, and both font assets carry the pairs as m_GlyphPairAdjustmentRecords, scaled the way TextCore stores what it reads from GPOS. - A font the lane refuses is logged and skipped instead of failing the entity. - sfdump: SFDUMP_NODES=<field> prints a field's type tree. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A font the lane turns away (not TrueType, over a limit, not rebuildable) still gets no bundle and never reaches the failed list, but it now raises the manifest's exit code the way an undecodable image does, so the refusal is visible to whoever reads the conversion result. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@popuz thanks for the security pass. All four points are addressed:
Also from your client-side note: the pre-fill now includes Still open: a Unity check of a bundle carrying the rebuilt font. That means FreeType loading it, characters added at runtime, and kerned pairs rendering. The earlier Unity runs predate the rebuild. |
decentraland-bot
left a comment
There was a problem hiding this comment.
Review: follow-up on the security pass (HEAD 98efcae)
Thanks for the thorough turnaround. I checked each of the four points from @popuz's review against the code and against ttf-parser 0.25.1 (the version in Cargo.lock).
| Point | Status |
|---|---|
| 1. Original bytes ship to the client | ✅ Fixed. m_FontData is baked.font_data, the rebuilt font. No source table is copied whole. name is re-encoded, post is v3 and OS/2 is rebuilt field by field. |
| 2. Validation is minimal | |
| 3. TrueType only | 0x00010000 magic is enforced. The .ttf path is not enforced in build_bundle (P2 below). |
| 4. Converter DoS | render. But the budget does not cover rebuild or kerning, which run first and can exhaust memory or stall from a file under 1 KB. |
regen::guard catches panics but not running out of memory, and bake has no timeout. So any of the issues below can take down the shared JIT server or lambda, and it then fails again on the same font when it is retried.
P0 — Blocker
- Unbounded cmap enumeration (
fontgen/rebuild.rs:266).subtable.codepoints(|cp| codepoints.push(cp))runs before any limit. ttf-parser's format 12codepointswalksstart..=endfor each group without checking the range. A single 12-byte group0..=0xFFFFFFFFpushes about 4.3e9u32values (about 17 GB) beforechar::from_u32filters anything. Repeated encoding records and large format 4 segments make it worse.- Fix: walk the groups yourself, clamping each to
0..=0x10FFFF(or use a 0x110000-bit set), and dedup the subtables.
- Fix: walk the groups yourself, clamping each to
P1 — Major
- Point caps are checked after composite expansion (
fontgen/rebuild.rs:233, alsofontgen/mod.rs:376).outline_glyphruns to completion beforec.points > MAX_GLYPH_POINTSis tested. ttf-parser limits composite depth to 32 (MAX_COMPONENTS) but not fan-out.- Example: a chain of about 30 composites, each referencing the next twice, expands to around 2^30 points (several GB).
MAX_FONT_POINTSis also checked only between glyphs. - Fix: before outlining, do a memoized walk of the
glyfcomponent graph with saturating point counts. Alternatively, haveContoursstop recording at the cap, though that bounds memory only and not CPU.
- Example: a chain of about 30 composites, each referencing the next twice, expands to around 2^30 points (several GB).
- The GPOS
kernfeature walk is unbounded (fontgen/kerning.rs:40-46).features.filter(kern).flat_map(lookup_indices).collect()runs beforededupand before the 256-subtable cap. Feature records are 6 bytes and can all point at one Feature table with 65535 lookup indices, giving about 4e9u16values from under 1 MB of input.- Fix: collect the indices into a 65536-bit set.
- Every font build error is treated as a tolerated refusal (
live.rs:1431,live.rs:601-612).Err(e) if it.is_fontcatches template, cache I/O and fetch errors along with real refusals.font_supportedreturnsfalsewhenfetch_mmapfails.image_decode_okreturnstruein that case, so the build runs and the error surfaces.- Either way the manifest records
fontand the name never reaches the failed list. A passing network or infra error therefore drops the scene's font until it is reconverted by hand. - Fix: tolerate only a typed refusal from
fontgen(an error enum or a marker), and send everything else tofailed_m.
P2 — Minor
- Font detection by content, not path (
builder/mod.rs:657,:840). Any bytes starting with00 01 00 00take the font lane. Atex.pngholding TTF bytes skips thefont_supportedgate, then either ships a font bundle under an image name or lands on the failed list.build()(live.rs:~723) also matches.ttfwithout the scene-only gate. Gate onis_font_path(file)as well. - The coordinate bound can be bypassed (
fontgen/rebuild.rs:124-125). Nested near-2.0 F2Dot14 scales push a coordinate pasti32::MIN.x.round() as i32saturates, and.abs()then panics in a debug build or wraps in a release build, so the ±16,383 guarantee doesn't hold. Check thef32value (!x.is_finite() || x.abs() > 16383.0) before casting. - The kerning loop counts subtables, not lookups (
fontgen/kerning.rs:57-84). Lookups with no subtables never advancevisited, so the pair loop can run about 40k × 65535 times. - The kerning fallback contradicts the PR text. Going over 256 subtables, or a single bad lookup index (
?atkerning.rs:58), returnsNoneand falls back to the legacykerntable. It does not "ship without kerning" as the description says. - The pairs in the assets can disagree with the GPOS.
bake_facegets the originalpairs, butgpos_tablecan trim them to fit the 64 KB offset limit (rebuild.rs:~447). Thenm_GlyphPairAdjustmentRecordsand the embedded GPOS differ. - The
outline_glyphresult is ignored (rebuild.rs:233). A malformed glyph can leave partial contours, which get re-encoded instead of becoming an empty glyph. - Bitmap-only fonts are accepted. A
0x00010000font with noglyfpassesis_supportedand bakes a blank atlas. Refuse it when no pre-fill glyph has an outline. - Refusals aren't cached. Each platform and JIT request reruns
rebuildandbakeon the same hostile font. - Composite fidelity. ttf-parser flattening ignores USE_MY_METRICS, and possibly point-matched anchors. The round-trip test compares ttf-parser with ttf-parser, so it can't catch this.
- Tests.
- Add assertions that
checksum(font) == 0xB1B0AFBAand that each table checksum is right. - Ideally, load the font through FreeType or OTS once in CI. A malicious-font fixture for each cap (cmap range, composite bomb, feature overlay) would also prevent regressions.
- Add assertions that
- Nits.
sdf.rs:228says "TrueType and CFF".".ttf"is defined three times (live.rs:17,fontgen/mod.rs:146,naming.rs:418).- The format-23 SerializedFile writer is unused and untested.
MAX_RENDER_WORK(400M evaluations) is probably seconds per platform, not "a few hundred ms". Worth measuring.
Rebuilt TrueType: spec check
I checked these by hand and they look correct:
- The table directory: sorting, search fields, alignment, checksums and
checkSumAdjustment. headwith longloca,hhea/hmtxconsistency, andmaxpv1.0 with the hinting fields zeroed.glyfflag and delta encoding.cmap4 (terminator, idDelta wrap) and 12.name(3/1/0x409).- GPOS offsets, and PairPos format 1 with sorted Coverage and PairSets.
For the open Unity check, worth confirming:
- That glyphs are rendered unhinted. With no bytecode, a hinted load may go through the autohinter and differ from the pre-fill.
- That TextCore's runtime GPOS reader accepts the minimal GPOS without duplicating the pre-filled pair records.
- That
x_advance * 90 / upemmatches TextCore's 26.6 rounding. - A composite-heavy font, for accents.
Other checks
- CI: all checks pass. Address sanitizer was skipped.
- Consumers: no public API break. This is a new lane gated on
is_fontand the scene entity type, and format-22 output is unchanged. - Git conventions: the title, branch and commits follow ADR-6. The first commit body mentions
.otfand #10118, so use the PR text in the squash message. - Rollout: scenes converted before this PR, and any later
Fontgeneration bump, won't pick up fonts without an explicit reconversion sweep. Worth planning.
Verdict: changes requested. Points 1 and 3 are resolved, and the SDF work is solid. But the bounds are still enforced after the expensive expansion, so a crafted font under 1 KB can still exhaust the converter's memory. Fix P0 #1 and P1 #2–#4 and this is close.
Reviewed by Jarvis 🤖 · Requested by Juan Ignacio Molteni [Dalkia] (<@U03JSUQ5Z7U>) via Slack
Follow-up review on #126: the caps were enforced after the expensive expansion, so a crafted font under 1 KB could still exhaust the converter. - cmap: each subtable is walked over its own declared ranges, read raw, clamped to Unicode and charged against a budget for the whole font; encoding records are capped at 32 (also gating is_supported, since ttf-parser tries each record per glyph_index). No more codepoints(). - Composites: the glyf component graph is sized first with memoised, saturating counts; a glyph over the point cap, a font over the total, a cycle, over 64 direct components or nesting past 32 is refused before any outline is expanded. Contours stop recording past the cap as well, and a glyph ttf-parser cannot outline whole is written empty. - GPOS: kern lookup indices go into a fixed bit set under a visit budget; lookups count toward the subtable cap; a GPOS past a cap or with an unresolvable index ships without kerning (the legacy kern fallback applies only to fonts without a kern feature). - fontgen::Refused is a typed error: live.rs tolerates only that, so a template, cache or fetch error fails the bundle and a retry builds the font; font_supported treats unreadable content as "build and see", and a refusal is cached per hash. - Also: the font lane needs a .ttf path as well as TrueType bytes, build() applies the scene-only gate, coordinates are range-checked as floats, the assets record exactly the pairs the GPOS kept, bitmap-only fonts are refused, ".ttf" is defined once. - Tests: a TrueType fixture assembler; a composite bomb, a composite cycle, a cmap group over all code points, cmap ranges past the budget, 40 encoding records, a 2000-record kern feature overlay, sfnt checksums, and the format-23 SerializedFile writer round-tripping the donor. - fontcal prints a font's render work; Azeret Mono and Inter use about 2% of the budget. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Follow-up addressed in 619b300. Every P0/P1 held up against the code; thanks for checking P0 — cmap. No more P1 — composites. P1 — GPOS feature walk. Indices go into a fixed 65,536-bit set; the walk stops after 65,536 indices named, 256 lookups + subtables visited (lookups now count), or an unresolvable lookup index. Each of those means "no kerning", which the PR text had claimed; the legacy P1 — tolerated errors. P2s done: the Measured: Not done: a FreeType/OTS load in CI (new toolchain dependency); |
decentraland-bot
left a comment
There was a problem hiding this comment.
Re-review: 619b300
Thanks, this is a solid round. Most of the previous findings are now closed, and I checked each one against the code and ttf-parser 0.25.1. Three things still block merge: Windows CI fails on a test this commit adds, and the new composite and cmap pre-walks can each still be pushed into unbounded work.
Previous findings
| Finding | Status |
|---|---|
| P0 cmap enumeration | ✅ Fixed for formats 0/4/6/10/12. Ranges are clamped, records are capped and deduped, and each range is charged before it is walked. Format 13 has a new issue (#2 below). |
| P1 composite point caps | |
| P1 GPOS feature walk | ✅ Fixed. Indices go into a bit set, and the named, lookup and subtable counts are capped. The pair loop is bounded. |
| P1 every build error tolerated | ✅ Fixed. Only Refused is tolerated, the downcast works through anyhow context, refusals are cached by hash, and a fetch failure no longer counts as a refusal. |
| P2s | ✅ All fixed except FreeType/OTS in CI and composite fidelity, which you listed as follow-ups: path gating, the scene-only gate in build(), the float coordinate check, lookups counted, kerning fallback, kept pairs plumbed through, None outlines, bitmap-only fonts refused, refusal cache, checksum tests, format-23 round trip, .ttf constant, nits. |
P1 — Major
- The composite pre-walk doesn't bound ttf-parser's work (
fontgen/rebuild.rs:156-185,:201-250). It has two holes:- It counts points, not visits. A leaf with
numberOfContours = 0scores 0 points. A chain where each glyph references the previous one 64 times passesexpanded_points, and ttf-parser'soutline_implthen visits about 64^31 nodes. Fan-out 2 is already about 2e9 visits. No point is pushed, so theContours::pushcap never trips. The converter hangs on a font of a few KB. - It reads composites differently from ttf-parser.
components()always skips 2 or 4 bytes of arguments. ttf-parser'sCompositeGlyphIterreads them only whenARGS_ARE_XY_VALUES(0x0002) is set. With that flag clear, ttf-parser reads those bytes as the next component's flags and glyph id, so a font can hide components from the pre-walk. That brings back the point bomb (CPU-bound now, because the push cap limits memory). - Fix: memoise a saturating count of nodes (1 + children), and cap both the per-glyph and per-font totals. Parse components exactly the way ttf-parser does, including the args quirk. Even better, flatten composites from raw
glyfyourself rather than callingoutline_glyphon the source font, so the walk you bound is the walk that runs.
- It counts points, not visits. A leaf with
- cmap format 13 lookups are linear (
rebuild.rs:543,fontgen/mod.rs:193,:229). ttf-parser'sSubtable13::glyph_indexscans every group.- Attack: a format 13 subtable with about 1.3M groups that clamp to empty (they cost no budget), plus one group covering
0..=0x10FFFF(which costs 1.1M of it). That is about 1e12 comparisons per platform build, from a 16 MB file. - Separately,
face.glyph_indexinis_supportedand the pre-fill tries every unicode record. 32 records pointing at the same large format 13 subtable add up to about 8e9 comparisons on the gating path. - Fix: compute the glyph straight from the raw group while walking it (
start_glyphfor 13,start_glyph + cp - startfor 12). For the gate and the pre-fill, use the map you built (deduped by offset) rather thanFace::glyph_index, or refuse format 13 above a small group count.
- Attack: a format 13 subtable with about 1.3M groups that clamp to empty (they cost no budget), plus one group covering
- Windows CI fails (
windows tests + sanity).fontgen::kerning::tests::a_feature_overlay_ships_without_kerning_instead_of_being_walkedpanics atttf-parser-0.25.1/src/parser.rs:415with attempt to add with overflow.- Cause:
LazyArrayIter16keeps its index as au16. Walking a full 65,535-entrylookup_indicesto the end overflows it on the call that would returnNone. Release builds wrap and stop, but debug and overflow-checked builds panic. - Fix: stop before exhausting the iterator (iterate
0..lenwith.get(i), or break at the cap before the lastnext()). - The test is also weak: its lookup has no subtables, so
pairsis empty with or without the cap. Give it a real PairPos subtable so it shows the cap is what drops the kerning.
- Cause:
P2 — Minor
- An oversized rebuild fails with an untyped error (
rebuild.rs:1073). This error usesanyhow!, notrefuse!. A font under 100 KB can stay under 4M expanded points and still rebuild past 16 MB, sinceencode_glyphwrites no flag repeats. It then lands on the failed list, isn't cached, and every retry rebuilds it. Make itrefuse!. - A
.ttfthat isn't TrueType falls through to the texture builder (builder/mod.rs:639-642). This covers OTF or WOFF bytes, or a file over 16 MB. Usuallyfont_supportedcatches it first, but it now returnstruewhen the fetch fails, so a texture bundle can ship under the font's name. Refuse any.ttfpath whose bytes are not a font file. - The memo bypasses the depth check (
rebuild.rs:224). A memoised result returns beforedepth > MAX_COMPOSITE_DEPTHis checked, so chains walked from the leaf up pass at depth 1. ttf-parser still stops at 32, so it does no harm today, but the refusal doesn't happen. Store each glyph's subtree height in the memo and checkdepth + height. - The point sizing and
Contours::pushcount different things. Implied midpoints and closing points mean a real glyph with about 4,100 mostly off-curve points passes the sizing, then hits the push cap and refuses the whole font. Rare in practice. - The cmap range pre-pass isn't charged (
subtable_ranges). It reads every group's record before the budget applies. That is bounded (about 45M reads) but uncharged. - Missing fixtures: a zero-point fan-out composite, the xy-args desync, a large format 13 subtable, a typed refusal through
build_bundle, and an oversized rebuild.
Other checks
- Real fonts: no regressions from the new caps that I could find. Format 14 yields no ranges, symbol (3,0) behaves as before, and format 4 with
idRangeOffsetgoes through ttf-parser. ttf-parser's own format 4as i16drops glyph ids of 32768 and above, but that predates this PR. - CI:
windows tests + sanityfails as described in #3. arm/x86 nextest, the lambda, node addon and wasm jobs pass.arm lintsandx86 build + sanitywere still pending when I checked. Address sanitizer was skipped.
Verdict: changes requested. The design is right, and this is close. Fix #1–#3 and add fixtures for them.
Reviewed by Jarvis 🤖 · Requested by Juan Ignacio Molteni [Dalkia] (<@U03JSUQ5Z7U>) via Slack
…that runs Re-review on #126: the composite pre-walk counted points while ttf-parser's work is per node, it parsed components differently from ttf-parser (arguments are only present with ARGS_ARE_XY_VALUES), format 13 lookups were linear, and walking a 65535-entry lookup index list to its end overflowed ttf-parser's u16 iterator on Windows CI. - glyf: composites are flattened by this crate from the raw table, mirroring ttf-parser 0.25 (argument quirk, Transform::combine, depth 32, one-point glyphs empty); ttf-parser no longer outlines the source. Every glyph is sized first from the component graph with memoised, saturating node, point and height counts: over 1,024 visits or 8,192 points per glyph, 1M visits or 4M points per font, 64 direct components, nesting past 32 or a cycle is refused before anything expands. The fidelity test still compares ttf-parser's outline of the original with ttf-parser's outline of the rebuild. - cmap: resolved per format from the raw subtables (0/4/6/10/12/13), never through Face::glyph_index; one unit of work per record read and per code point visited, under one budget for the font. The pre-fill and is_supported use this map too. - kerning: lookup indices are read by position, not iterated; an unresolvable index means no kerning; the overlay fixture now kerns a real pair when under the cap. - A .ttf path holding anything but a TrueType file is refused, not handed to the texture builder; an oversized rebuild is a refusal, so it is cached. - Fixtures: a fan-out of empty leaves, ttf-parser's argument reading, a scaled composite, a 1.3M-group format 13 flood under 16 MB, an oversized rebuild, a typed refusal through build_bundle. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Re-review addressed in 4ad1654. All three blockers were real; I took the stronger option on #1 and #2 and read #1 Composite pre-walk. #2 cmap format 13. #3 Windows CI. Lookup indices are read by position ( P2s: oversized rebuild is a Font tests 35/35, clippy and rustfmt clean locally. Still deferred, as before: a FreeType/OTS load in CI, and |
decentraland-bot
left a comment
There was a problem hiding this comment.
Re-review of 4ad1654: changes requested
This commit closes the cmap, kerning and Windows CI findings. Reading glyf and cmap raw was the right move. The composite sizing is now nearly sound, but it has two holes, and each one lets a crafted font of a few hundred bytes to ~1 MB abort the converter process with a stack overflow. That is worse than a hang: it kills the whole worker, and catch_unwind cannot catch it.
These findings come from reading the code against ttf-parser 0.25.1. I did not run any crafted fonts.
Previous findings
- Composite pre-walk (was P1): partly fixed.
- Visits are now counted, and composites are parsed the same way in the sizing and flattening passes.
- The memoised, saturating sizing is sound for glyph ids below
numGlyphs: about 1M visits × 65 components, plus 4M points, bounds the flattener's work. - Two new holes remain; see below.
- cmap format 13 (was P1): fixed.
- Every record, segment and code point is charged before it is read, and ranges are clamped.
- Work is at most ~2.2M units per font, and every read is bounds-checked.
is_supportedandbakenow shareunicode_map, so the gate and the rebuild cannot disagree.
- Windows CI / u16 iterator (was P1): fixed.
- Lookup indices are read by position.
- The other iterators still walked on source data (
RecordListIter,LookupSubtablesIter) check bounds before incrementing. - The overlay fixture now really kerns, so the 2000-record test fails if the cap is removed.
- P2s from the last round: fixed.
- An oversized rebuild is a typed refusal.
- A
.ttfpath holding anything other than TrueType is refused in bothbuild_bundleandbuild_bundle_multi. - The memo stores height.
- The cmap pre-pass is charged.
Blocking
- P0: sizing and flattening use different glyph counts (
rebuild.rs:418-425vs:508,:210-213).size_glyphstreats any component id at or abovemaxp.numGlyphsas a one-node leaf and never visits it.flattenrecurses into any id belowglyph_count(), which comes from the rawlocalength. It has no memo, no depth limit and no cycle check.- ttf-parser cuts
locadown tonumGlyphs + 1entries, butGlyfTablesreads the raw table. - Repro:
numGlyphs = 2,locawith 4 entries, glyph 1 → glyph 2, glyph 2 → glyph 2. Sizing passes, thenflattenrecurses until the stack overflows. - A fan-out placed above
numGlyphsgives unbounded expansion instead (64^k).
- P0: the depth check runs after the recursion (
rebuild.rs:444-456).visitfollows the full chain before testingheight > MAX_COMPOSITE_DEPTH.- 65,535 composites, each referencing the next one, take about 1.1 MB. Sizing then goes ~65k frames deep, which overflows a 2 MB worker stack before the depth-32 refusal is reached.
Minor (P2)
- Depth off by one (
:454): ttf-parser stops atdepth >= 32(glyf.rs:439). A chain exactly 32 deep is drawn here, but would be empty when ttf-parser outlines the original. - One bad glyph refuses the whole font (
:328-391): truncated flags or coordinates, or non-increasing contour ends, refuse the font. ttf-parser drops only that glyph. A real font with one damaged glyph that is never pre-filled would get no bundle at all. - Point-matched composites (
:240): whenARGS_ARE_XY_VALUESis clear, the spec still stores the two point-number arguments, but no bytes are consumed here. ttf-parser has the same bug. Copying it was needed while ttf-parser did the outlining. Now that one parser does both passes, being spec-correct no longer opens a gap between sizing and flattening. As written, anchored composites decode wrongly, andcomponents_are_read_as_ttf_parser_reads_themlocks the misparse in. - cmap details that differ from ttf-parser:
- (3,10) records are accepted in any format; ttf-parser requires format 12/13.
- Format 12 uses
wrapping_addwhere ttf-parser useschecked_add. - Format 6 maps past 0xFFFF.
- A linear first-match walk resolves overlapping segments differently from ttf-parser's binary search.
- These only affect malformed fonts.
- Kerning: one dangling lookup index drops all GPOS kerning and does not fall back to
kern. This is documented and deliberate, so it is a policy point. - Tests:
- The composite-bomb test is now refused by the node cap, so it would still pass if the per-glyph point check were removed.
- No test reaches
MAX_FONT_POINTS,MAX_FONT_NODESorMAX_COMPOSITE_DEPTH. The oversized-rebuild fixture is about 3.94M points and is refused by output size. flattened_composites_match_ttf_parsernests only one level, so it would pass if theTransform::combinearguments were swapped.TWO_BY_TWO,WE_HAVE_A_SCALEand negative i8 offsets are untested.- The
fixturehelper always writes alocathat matches maxp, which is why the first P0 has no coverage.
Suggested fix
- Set
GlyfTables::glyph_count = min(loca entries - 1, numGlyphs), and use that same bound invisitandflatten. - Pass
depthintovisitand refuse before recursing oncedepth >= 32(this also fixes the off-by-one). - Add fixtures for a
localonger than maxp and for a 65k-long chain.
CI
Passing: x86 build and nextest, arm nextest, Lambda build, node addon, wasm and the windows gate. Still pending: windows tests + sanity and arm lints + sanity. Address sanitizer was skipped.
Reviewed by Jarvis 🤖 · Requested by Juan Ignacio Molteni (<@U03JSUQ5Z7U>) via Slack
…d on the way down Re-review on #126, two stack overflows a small font could cause: - The sizing stopped at maxp's glyph count while the flattener followed loca's, so a glyph past maxp was flattened unsized: no memo, no depth limit, no cycle check. GlyfTables now holds the one bound, min(loca entries - 1, numGlyphs), and both passes use it. - The depth was checked after the recursion, so a 65k chain of single-component composites was followed to the bottom first. visit takes a depth and refuses before recursing at 32, which is also where ttf-parser gives up; a memo hit checks depth + height the same way. Also: composites are flattened in each glyph's own space, so anchored components (arguments as point numbers) are placed by their matched points per the specification instead of copying ttf-parser's misread; a malformed simple glyph empties that glyph and its parents as ttf-parser does, instead of refusing the font; (3,10) cmap records count only for formats 12/13, format 12 uses checked_add, format 6 stops at the BMP. Fixtures: loca longer than maxp with a self-referencing glyph past it, a 65k chain, a chain of exactly 32 and of 31, the per-glyph point cap without the node cap, the per-font point and node caps, anchored components, three nested transforms (2x2, x/y scale with a negative byte offset, uniform scale with word offsets) against ttf-parser, and a malformed glyph. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Both P0s addressed in 8668606, and they were exactly as described: the two passes used different glyph bounds, and the depth check sat below the recursion. P0 — glyph bound. P0 — depth. P2s taken:
Kept as policy: a dangling GPOS lookup index yields no kerning rather than a CI on |
decentraland-bot
left a comment
There was a problem hiding this comment.
Re-review of 8668606: approved
Both P0s from the last round are fixed. A crafted font can no longer overflow the stack or recurse without a bound, and I found no new P0 or P1. Two things are left before merge: a CPU-cost P2, and the Unity check of the rebuilt font that you already listed as open.
These findings come from reading the code against ttf-parser 0.25.1. No crafted fonts were run.
Previous findings
- P0: sizing and flattening used different glyph counts: fixed.
GlyfTables::glyphs = min(loca entries − 1, numGlyphs)is now the only bound.glyph(),size_glyphsandflattenall use it.size_glyphsvisits every glyph id below the bound.flattenonly recurses into ids below it, so it never reaches a glyph that wasn't sized.- This matches ttf-parser:
locais cut down tonumGlyphs + 1entries, andglyph_rangeskips any component outside it.
- P0: the depth check ran after the recursion: fixed.
visitnow refuses on entry oncedepth >= 32, and refuses a memo hit oncedepth + height >= 32. So sizing goes at most 33 frames deep.- Every glyph that passes has a height of 31 or less, so
flattenis bounded the same way. - The depth off-by-one is gone: a chain of exactly 32 is refused, which is where ttf-parser's
outline_implreturnsNone.
- P2s: all fixed.
- Anchored components are read as the spec lays them out, and placed by their matched points after the component's own scale.
- A malformed simple glyph now empties that glyph and its parents, matching ttf-parser's
?on the component outline. - The cmap fixes are in: (3,10) records only count with format 12/13, format 12 uses
checked_add, and format 6 stops at the BMP. - The new tests are real regressions: the 65k chain and the over-long
locawould both overflow the stack on4ad1654. There is a per-glyph point-cap test that the node cap doesn't catch, plusMAX_FONT_POINTS/MAX_FONT_NODEStests and a three-level transform test.
- Kerning policy (a dangling lookup index means no kerning): accepted as a policy decision.
Checks with no findings
- Point counts:
simple_pointsandsimple_contoursagree. A malformed glyph yields fewer points than were sized, never more. - Panics: none reachable in the changed code.
- Indexing in
simple_contoursis guarded byend > total. - Coordinate sums fit in i32.
- Composite scales stay finite in f32, and
to_pointsrejects them. - The
encode_glyphcasts are safe under the 8,192-point cap.
- Indexing in
Minor (P2)
- Uncharged work in
flatten(rebuild.rs:499-541).flattenhas no memo. Each nesting level copies its child's contours, and each anchored component finds its point without.iter().flatten().nth(i), which costs O(contours).- Possible construction, from reading the code: a glyph with 8,192 one-point contours, about 15 levels of composites, each adding 63 anchored references to an empty glyph with parent point
0xFFFF, and about 470locaaliases of the top glyph. - That stays under every cap, at about 3.6e9 iterator steps plus about 1e8 small allocations. My estimate is roughly 5–15 s of CPU from a font under 30 KB.
- It is bounded, so this is not a hang. But the font is not refused, and this work happens before the render budget applies.
- Possible fixes:
- Flatten into one point buffer with contour end indices, so anchors are an index lookup and there is one allocation per level.
- Memoise
flattenper glyph id. - Charge contours against a font-wide budget.
- Fidelity nit: ttf-parser empties a composite when a component's data is under 10 bytes but not empty, because the header read fails. Here, the other components are kept. This only affects malformed fonts.
Before merge
- CI on
8668606is still running:windows tests + sanity,arm lints + sanityandx86 build + sanitywere pending when I checked. The other jobs pass, and address sanitizer was skipped. This approval assumes those three go green. - The Unity check of a bundle carrying the rebuilt font is still open. Please confirm the points from earlier rounds:
- glyphs render without hinting;
- TextCore reads the minimal GPOS without duplicating pairs;
- the kerning scale matches Unity's rounding;
- accented composites render correctly. Anchored ones are now placed by matched points, so they are worth a look.
Reviewed by Jarvis 🤖 · Requested by Juan Ignacio Molteni (<@U03JSUQ5Z7U>) via Slack
…own points Review on #126 (P2): flatten copied every nesting level's contours and found anchors with nth() over nested contours, so a contrived font under 30 KB could spend seconds of CPU inside the caps, before the render budget applied. A glyph now flattens into one point buffer with contour end indices: an anchor is an index lookup and each component costs one copy of its own points, so the work is bounded by the point caps the sizing already enforces. The review's other nit, that ttf-parser empties a composite whose component has under ten bytes of data, did not reproduce against ttf-parser 0.25.1: it reads such a glyph as empty and keeps the other components. The rebuild does the same, and a test pins it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Thanks for the approval. The remaining P2 is taken in 221598e: The fidelity nit didn't reproduce: against ttf-parser 0.25.1, a composite whose component has six bytes of data is drawn with its other components, not emptied. I kept the rebuild matching that and added a test that compares the two outlines, so if ttf-parser changes its mind the test says so. CI on |
decentraland-bot
left a comment
There was a problem hiding this comment.
Re-review of 221598e: approved
The flattening P2 from the last round is fixed. This commit adds no new P0 or P1, and CI is green on 221598e.
These findings come from reading the code against ttf-parser 0.25.1. No crafted fonts were run.
Previous findings
- Uncharged work in
flatten(was P2): fixed.Flat { points, ends }keeps one buffer per glyph.- An anchor is now looked up by index:
out.points[..first]for the parent point andout.points[first..]for the child point, which is taken after the component's own transform and before its offset. That matches the old semantics exactly. - The quadratic
.nth()scan and the per-contour allocations are gone. - Work is now bounded by expanded points × composite height, at most about 4M × 32 point copies across the font.
- Small doc nit: each point is copied once per nesting level, not once per glyph as the doc comment says. It is still bounded.
- Panics: none reachable.
first <= lenalways holds, so both slices are valid.endsonly increases and never passes the buffer length: simple glyphs check that contour ends increase, and composite ends arefirst + child_end. So the slicing ininto_contoursstays in bounds.
Fidelity nit (P2): partly right on both sides
My last note was imprecise. Your test is correct for the case it covers, but the gap is still there for some other short stubs.
- Zero contour count (your all-zero 6-byte stub): ttf-parser draws it empty and keeps the composite's other components, and so does the rebuild.
- Negative contour count on a 2–9 byte stub (e.g.
FF FF 00 00 00 00):- In ttf-parser,
outline_implreads −1, advances to offset 10, ands.tail()?returnsNone. The parent's?passes that up, so the whole composite comes out empty. - The rebuild's
components()reads nothing andsimple_contoursreturnsSome(empty), so the parent still draws.
- In ttf-parser,
- A 1-byte glyph: ttf-parser's
read::<i16>()?fails and empties the parent, but the rebuild treats the glyph as empty and keeps the parent. - Positive count on a short stub: the two agree.
- Suggested fix: in
flatten, returnOk(None)for a glyph whose contour count is negative and that is shorter than 10 bytes, or that is 1 byte long. Add anFF FFstub test that expects an empty outline. - This only affects malformed fonts and has no security impact.
Before merge
- CI: green on
221598e, all nine jobs. Address sanitizer was skipped. - Unity check of the rebuilt font: still open; you said it is in progress. The points to confirm are unchanged:
- glyphs render without hinting;
- TextCore reads the minimal GPOS without duplicating pairs;
- the kerning scale matches Unity's rounding;
- accented and anchored composites render correctly.
Reviewed by Jarvis 🤖 · Requested by Juan Ignacio Molteni (<@U03JSUQ5Z7U>) via Slack
Scene font bundles are a new asset lane with a new recipe and template, so a minor bump. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
What
Scene
.ttffiles (the explorer'sfont_src, decentraland/unity-explorer#10317) now convert to a font bundle. Each bundle holds:Fontcarrying a font rebuilt from validated values, never the uploaded file (see Security);TMP_FontAsset, forTextShape;FontAsset, for scene UI.Each font asset has its own
SDFAAatlas, pre-filled with:The assets use the explorer's runtime settings: 90pt, padding 9, 1024²,
SDFAA. Kerning between the pre-filled glyphs ships with them.With the runtime path, every glyph is drawn by FreeType on the main thread the first time it's shown. With the bundle, common text renders on the first frame. The assets stay dynamic, so other characters are still added at runtime through the bundled font, into the same atlas's free space.
How
crate/src/fontgencopies what TextCore writes, fitted against atlases Unity 6000.5 baked itself:FaceInfofrom FreeType's face metrics;FT_MulFixscaling;SDFAAencodes it. Along glyph edges it matches Unity's field; further out it is smooth where TextCore's bitmap transform is jagged.fontgen::rebuildrewrites the font from validated values.fontgen::kerningreads its kerning (see Security).builder::fontassembles the bundle fromtemplate/font-types.mac.bundle. That's a 44 KB type donor Unity built itself; the script that regenerates it istemplate/src/FontTypesTemplate.cs.m_GlyphPairAdjustmentRecords, scaled the way TextCore stores what it reads from GPOS.unity::serialized_filegains SerializedFile format 23 (Unity 6000.5) read and write support, needed to read the donor. Output bundles stay format 22, like everything else.live: fonts convert for scenes only, and are digest-named under a newFontrecipe at generation 0. A font the lane refuses (not TrueType, over a limit, not rebuildable) gets no bundle and counts as a tolerated failure: the manifest'sexitCodegoes non-zero, as with an undecodable image, without listing it as a failed bundle.sfdumpdumps any bundle's objects (SFDUMP_NODES=<field>prints a type tree).fontcalre-fitsfontgenagainst a Unity-baked asset after a Unity upgrade.Security
From @popuz's review: scene fonts are untrusted, and the client's FreeType reads the bundled font whenever text needs a glyph outside the pre-fill.
No uploaded bytes reach the client.
fontgen::rebuildwrites a new TrueType file from whatttf-parserparsed:glyf/loca, re-encoded without hinting instructions. Composites are flattened, glyph ids are kept, and fractional midpoints stay implied.cmapformats 4 and 12.head,hhea,hmtx,maxp,OS/2andpost, with the source's metrics.nametable.GPOSholding onekernlookup of the pairsfontgen::kerningread.Hinting programs, variation, colour/bitmap data and every other layout table are dropped. The bake runs on the rebuilt font, so the atlas and the font FreeType later reads agree. A test checks that every glyph of the donor font draws the same path, that coordinates move at most half a unit (scaled composites snap to whole units), and that mappings, advances and face metrics are unchanged.
Bounded inputs, enforced before anything expands. A crafted font under 1 KB must not be able to exhaust the converter. The two tables that can be made expensive,
glyfandcmap, are read raw by this crate rather than throughttf-parser, so the walk that is bounded is the only walk that runs:fontgen::rebuildfrom the rawglyf, withttf-parser0.25's semantics where they are a choice (no grid rounding or scaled offsets, one-point and malformed glyphs empty, components outside the font skipped, depth 32) and the specification's layout for arguments, so anchored components are placed by their matched points; every glyph is sized first from the component graph with memoised, saturating counts of visits, points and height, under one glyph bound (min(loca entries − 1, numGlyphs)) shared by sizing and flattening, with the depth checked before each recursion: over 1,024 visits or 8,192 points per glyph, 1M visits or 4M points per font, 64 direct components, nesting to 32 or a cycle is refused before anything expands;cmapis resolved per format from the raw subtables (0/4/6/10/12/13), never throughFace::glyph_index: one unit of work per record read and per code point visited, under a budget of 2 × 0x110000 for the font, at most 32 encoding records; the pre-fill andis_supporteduse this map too;kernlookup indices are counted into a fixed bit set (65,536 named, 256 lookups + subtables visited) and read by position, not iterated; a GPOS past a cap, or with an unresolvable lookup index, ships without kerning;A font past any of these is
fontgen::Refused, a typed error. The conversion tolerates only that (no bundle, non-zeroexitCode, cached per hash); a template, cache or fetch error fails the bundle so a retry builds the font. A.ttfpath holding anything but a TrueType file is refused rather than handed to the texture builder. Fixtures cover each cap: a composite bomb (100 points × 2³⁰), a fan-out of empty leaves (64³⁰ visits, zero points), a composite cycle, alocalonger thanmaxpwith a self-referencing glyph past it, a 65k-long chain (refused at depth 32, not followed), chains of exactly 32 and 31, the per-glyph point cap without the node cap, the per-font point and node caps, anchored components, three nested transforms againstttf-parser, a malformed glyph, acmapgroup over all of Unicode, ranges past the budget, a 1.3M-group format 13 flood under 16 MB, 40 encoding records, a 2000-recordkernfeature overlay (which kerns when under the cap), an oversized rebuild, and a typed refusal throughbuild_bundle.TrueType only: sfnt version
0x00010000and.ttfpaths, matching the protocol (feat: add font_src to TextShape, UiText, UiDropdown and UiInput protocol#489: "a ttf font file") and the client's header check. OTF/CFF,true, collections and web fonts get no bundle.Bounded conversion work: the distance field no longer evaluates every segment at every texel.
fontcalprints the figure).Verified
--workspace --all-targets -D warnings) and rustfmt are clean.CreateFontAssetoutput;AB:tmp_v49_…).Not covered yet
fontin its recipes reads as current.abgen-corpusbatch builds skip fonts; the lambda and the JIT server include them.GPOSonly holds pairs among the pre-filled glyphs.ttf-parserand by hand against the spec; a FreeType load would need a new CI dependency.ttf-parser's flattening ignoresUSE_MY_METRICSand point-matched anchors. Advances come fromhmtx, so metrics are unaffected; anchors are for the Unity check on an accent-heavy font.🤖 Generated with Claude Code