Repository navigation
[font] fix panics and wrong results in cmap, svg, post, bitmaps, and variation tables - #288
Merged
Merged
Conversation
egonelbre
requested review from
andydotxyz,
benoitkugler and
whereswaldon
as code owners
September 28, 2026 17:52
benoitkugler
requested changes
Sep 29, 2026
benoitkugler
left a comment
benoitkugler
left a comment
Contributor
There was a problem hiding this comment.
Nice fixes, I've suggested a minor change.
benoitkugler
approved these changes
Sep 30, 2026
benoitkugler
left a comment
benoitkugler
left a comment
Contributor
There was a problem hiding this comment.
Thank you !
Could you resolve the merge conflicts ?
indexStart went negative when a segment's idRangeOffset pointed before the glyph array, for instance idRangeOffset=2 on segment 0. newCmap4 then sliced GlyphIDArray with a negative index and panicked inside NewFont. It now returns an error. The 0xFFFF sentinel check also tested the segment start rather than idRangeOffset itself, so newCmap4 ignored real offsets on segments starting at 0xFFFF. It now tests idRangeOffset.
cmap4Iter.Char added the segment delta at GID width, while Lookup adds it in uint16 and wraps. When glyph plus delta passed 0xFFFF the iterator and Lookup disagreed on the glyph. Char now wraps the same way.
newCmap0 tested the byte value instead of the glyph value. Byte 0 lost its mapping even when it had one, and bytes mapped to glyph 0 stayed in the map, so Lookup reported them as present.
A StartCharCode of 0x80000000 or more became a negative firstCode. Lookup then overflowed on r - firstCode and indexed entries out of range, and RuneRanges returned a wrong range. No rune can map to such a subtable, so newCmap10 now returns an empty one.
Lookup for formats 4, 6, 10, 12 and 13 reported a code point as mapped even when the computed glyph was 0, and RuneRanges counted those code points as covered. Lookup now returns false for glyph 0. RuneRanges skips the code points that map to glyph 0. For the explicit glyph arrays of formats 4, 6 and 10 it checks each entry, before and after the delta. An arithmetic segment or group of formats 4 and 12 has at most one such code point, the one whose glyph wraps to 0. A format 13 group maps every code point to one glyph, so a group with glyph 0 is skipped whole. Adjacent ranges still join.
newCoveragesFromCmap added every rune the cmap iterator yields, so a code point mapped to .notdef counted as covered. The iterator path now skips glyph 0. This covers only cmaps without RuneRanges. Formats 4, 6, 10, 12 and 13 implement font.CmapRuneRanger and never expose a glyph id here. For them, cmap.RuneRanges excludes glyph 0 since the previous commit, "[font] cmap: exclude glyph 0 from Lookup and RuneRanges". Caches written by older versions may still count those code points as covered, so the cache format moves to version 7. The fix/fontscan branch bumps the same constant for a different reason, so merging both means resolving that one line to a single higher version.
SvgDocOffset+SvgDocLength wrapped in uint32. The length check passed and slicing the raw data panicked inside NewFont. newSvg now adds the two in uint64.
When the gzip header parsed but decompression failed, glyphData swallowed the error and returned the compressed bytes as the SVG source. It now reports the glyph as missing.
sanitize accepted a maximum index equal to numBuiltInPostNames+len(Strings). GlyphName then indexed Strings out of range and panicked.
Every index format sized its glyph array from the glyph range and panicked on a negative make length. newBitmapSubtable now rejects the range once, before the format switch. It also rejects an empty format 4 glyph array, which happens when numGlyphs+1 wraps to 0.
parseIndexSubTable5 ended glyph i at (ImageSize+1)*i instead of ImageSize*(i+1), so every index format 5 strike returned garbage image data. The format 3 error text said format 1.
egonelbre
force-pushed
the
fix/font-parsing-bugs
branch
from
September 30, 2026 09:49
2f5cc43 to
89ee4bd
Compare
Contributor
|
Thank you. We have a new staticcheck warning coming from #277 (in 3 test files). Could you apply the straightforward fix ? |
Index formats 1 and 3 mark a glyph without a bitmap by two equal consecutive offsets. imageFor still returned a pointer to the empty entry, so the renderer drew an empty bitmap instead of falling back to the outline. imageFor now returns nil for those glyphs.
applyDeltasToPoints indexed deltas with tuple.pointNumbers[i] and never checked it against the glyph point count, so a malformed font panicked. parsePointNumbers now rejects any point number at or above the point count, for shared and private point numbers alike.
calculateScalar indexes peak and intermediate tuples with fvar's axis count, but gvar sizes them with its own axisCount. newGvar now checks every shared and embedded tuple against fvar, as newMvar already does for MVAR. NewFont treats gvar as optional and ignores constructor errors. When newGvar returned partially parsed variations before building the shared-axis cache, the earlier glyphs applied their deltas with wrong scalars. newGvar now returns an empty gvar on serialized-data errors, as it does for the axis count and the other validation failures.
NewFont kept any avar that parsed, even one with fewer or more axis maps than fvar has axes. Variation normalization walks the avar maps and writes normalized[i] for each, so a short table left axes unmapped and a long one indexed past the coordinates and panicked. NewFont now drops an avar whose map count does not match fvar.
NewFont documents that only cmap, head and maxp are required, yet it aborted on a bad CPAL while dropping a bad COLR without a word. CPAL is now optional too. A bad CPAL drops COLR with it, since one without the other is unusable.
egonelbre
force-pushed
the
fix/font-parsing-bugs
branch
from
September 30, 2026 09:56
89ee4bd to
79b33e6
Compare
Contributor
|
Amazing, thank you ! |
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
2 participants
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.
Sixteen small fixes in the font package, grouped by table so each group can be reviewed on its own. Seven turn panics on malformed fonts into errors or dropped tables, eight fix wrong results from fonts that parse fine.
cmap:
svg:
post:
bitmaps:
Variations:
Also: NewFont documents that only cmap, head, and maxp are required, but aborted on a bad CPAL. A bad CPAL now drops COLR with it.
Merge note: fix/fontscan bumps the same cache version constant to 7 for a different reason. Whichever lands second resolves that one line to a single higher number.