fix #353: guard empty glyph in TTFGlyph._getCBox#379
Open
gediz wants to merge 1 commit into
Open
Conversation
Fixes foliojs#353. _getCBox always decodes a 10-byte GlyfHeader. A glyph with a zero-length loca entry (space, U+200D ZWJ, and similar) has no glyf data, so the decode reads the following glyph's header. When that empty glyph is the last glyph, the read runs past the end of the glyf table and throws "RangeError: Offset is outside the bounds of the DataView" (older builds: "Trying to access beyond buffer length"). This surfaces when embedding or laying out a subsetted font, where a trailing empty glyph is common, and is reported downstream through pdf-lib and pdfmake. Add the same glyfPos === nextPos guard that _decode already uses and return an empty bounding box. The default new BBox() would set the Infinity sentinel, which _getMetrics turns into a non-finite topBearing, so the guard returns an explicit zero-area box. Add a regression test in test/issues.js that reads the cbox of the OpenSans space glyph and asserts it has no positive extent.
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
1 participant
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.
Fixes #353, #378, #303, #312 (the earliest report is #179).
The bug
TTFGlyph._getCBoxalways decodes a 10-byteGlyfHeader. A glyph with azero-length
locaentry (space, NBSP, U+200D ZWJ, ideographic space) has noglyfdata. The decode then reads the next glyph's header, and when the emptyglyph is the last glyph the read runs past the end of the
glyftable andthrows.
_decodealready guards this withglyfPos === nextPos._getCBoxdoes not, and it runs for every glyph through
_getMetrics(advance width,layout).
Reproduction
Any font whose last glyph is empty. The repro in #353 uses Arimo
(github.com/Vikeo/pdfmake-buffer-error). On current master, reading the empty
trailing glyph's metrics throws:
Older builds report "Trying to access beyond buffer length" from the same line
(the text in #353); the message changed with the move to a native DataView. The
crash usually surfaces downstream when a subsetted font is embedded or laid out.
The fix
Add the same
glyfPos === nextPosguard_decodealready uses, returning anempty bounding box. #378 proposes this same change. The box is
new BBox(0, 0, 0, 0)rather than the defaultnew BBox(): the default is the±Infinity sentinel, which
_getMetricsturns into a non-finitetopBearing, soan explicit zero-area box keeps the metrics finite. The earlier PR #305 and
FreeType zero the empty-glyph box for the same reason.
Tests
Added a regression test in
test/issues.jsthat reads the cbox of the OpenSansspace glyph and asserts it has no positive extent (before the fix it returned
the next glyph's box).
npm testpasses. The one failing test (i18ndefault-language) fails on master too and is unrelated.
Related
vheaandglyffixes #305 adds the same loca guard but bundles several unrelatedchanges (vhea, maxp, post). This PR is the focused version.
react-pdf#1055, svg-text-to-path#27.