font: avoid panic on malformed composite glyphs

[ChrisJr404]

Problem

Face.GlyphData / Face.GlyphDataOutline panic with slice bounds out of range [:-4] on certain malformed TrueType fonts.

getPointsForGlyph is documented to always return the four phantom points, so glyphDataFromGlyf slices them off unconditionally:

points := f.getPointsForGlyph(glyph)
segments := buildSegments(points[:len(points)-phantomCount])

That invariant does not actually hold for composite glyphs. In getPointsForGlyphRec, when a component resolves to fewer than phantomCount points (e.g. it references an out-of-range glyph index, or nesting exceeds maxCompositeNesting), the function returns early before appending the phantom points:

LC := len(compPoints)
if LC < phantomCount { // in case of max depth reached
    delete(currentGlyphs, item.GlyphIndex)
    return
}

So a composite glyph whose (first) component is unresolvable makes getPointsForGlyph return 0–3 points, and points[:len(points)-phantomCount] slices with a negative bound and panics. Any caller passing untrusted font data to font.ParseTTF and then reading glyph outlines can be crashed by a crafted font.

getGlyfPoints (reached from GlyphExtents on variable fonts) has the same unchecked assumption at allPoints[len(allPoints)-phantomCount:].

Fix

Guard both consumers: if getPointsForGlyph returns fewer than phantomCount points, treat the glyph as empty instead of slicing out of range. This matches the existing behaviour for glyphs with no outline (a blank glyph), and leaves well-formed fonts completely unaffected.

Test

Added TestGlyphDataMalformedComposite, which builds a composite glyph referencing an out-of-range component and checks that the glyph-data accessors return an empty outline rather than panicking. It panics without the fix and passes with it. Found by fuzzing ParseTTF + glyph-outline extraction; the original crashing font and a hand-built minimal reproducer both go through the same code path.

[whereswaldon]

@benoitkugler Is it better to guard the consumers, or to alter getPointsForGlyph to always return four points, even if they are zeroes? I don't love making all callers perform a length-checking ceremony, but maybe that is more desirable that making up phantom points? I really don't know.

[benoitkugler]

@ChrisJr404 Thank you for the bug report !

@benoitkugler Is it better to guard the consumers, or to alter getPointsForGlyph to always return four points, even if they are zeroes? I don't love making all callers perform a length-checking ceremony, but maybe that is more desirable that making up phantom points? I really don't know.

I indeed think it would be a bit cleaner to actually enforce the phantom points creation. It will only penalize cases which happen barely in practice, and keep the code simpler.

[ChrisJr404]

Thanks, that makes sense to me too. I've reworked it so the guarantee lives in getPointsForGlyphRec itself: the early-return path (missing glyph id, or nesting past maxCompositeNesting) now appends the phantom points before returning, so getPointsForGlyph always yields at least phantomCount points and a malformed composite just produces an empty-but-valid outline. With the invariant enforced by the producer I dropped the two caller-side length checks and the redundant internal one, which keeps the code simpler as you noted. Regression test and the font/shaping/harfbuzz suites still pass.

[benoitkugler]

This looks great to me, thank you for this nice fix !