[
https://issues.apache.org/jira/browse/FOP-3328?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18124660#comment-18124660
]
Jason Harrop commented on FOP-3328:
-----------------------------------
The reproduction is the one in the description: load a variable Noto Sans with
debug logging on the complex-script package. As files, attached
FOP-3328-variable-font.fo, noto-variable.xconf and logging.properties
(java.util.logging, org.apache.fop.complexscripts at FINE); the font is Google
Fonts' NotoSans[wdth,wght].ttf, renamed NotoSans-VF.ttf since FOP rejects the
brackets in a file URI ("Illegal character in path"), a separate nit.
{noformat}
java -Djava.util.logging.config.file=logging.properties -cp ...
org.apache.fop.cli.Main -c noto-variable.xconf -fo FOP-3328-variable-font.fo
-pdf out.pdf 2> fop.log
grep -c "invalid device table delta count" fop.log
{noformat}
On main at 5be8c69b6 the log has 85,586 device-table lines for that one font,
2,728 of them "invalid device table delta count", from reading the anchors'
device tables at the wrong base. With -ea nothing throws here, as the
description says: on this font every bad read is caught by the n < 0 guard.
With the pull request's change there are 0 invalid counts; the same reads are
43,755 "unsupported device table delta format" lines, the VariationIndex tables
(format 0x8000) recognised and ignored. The rendered text is the same either
way, since neither path applies a device adjustment; the defect is in what is
parsed, not yet in what is drawn.
> [PATCH] GPOS: Anchor Table format 3 device tables are read from the wrong
> offset
> --------------------------------------------------------------------------------
>
> Key: FOP-3328
> URL: https://issues.apache.org/jira/browse/FOP-3328
> Project: FOP
> Issue Type: Bug
> Components: font/opentype
> Affects Versions: 2.11
> Reporter: Jason Harrop
> Priority: Minor
> Attachments: FOP-3328-variable-font.fo,
> FOP-anchor-device-table.patch, logging.properties, noto-variable.xconf
>
> Original Estimate: 20m
> Remaining Estimate: 20m
>
> There are two defects in {{OTFAdvancedTypographicTableReader}}, one of which
> makes FOP misparse the GPOS table of any variable font.
> h3. 1. Anchor device table offsets use the wrong base (root cause)
> {{readPosAnchor(long anchorTableOffset)}} saves the reader's position on
> entry:
> {code:java}
> long cp = in.getCurrentPos();
> in.seekSet(anchorTableOffset);
> ...
> xd = readPosDeviceTable(cp, xdo);
> yd = readPosDeviceTable(cp, ydo);
> {code}
> Per the OpenType spec, an Anchor Table format 3's {{xDeviceOffset}} /
> {{yDeviceOffset}} are measured "from beginning of Anchor table" - so the base
> must be {{anchorTableOffset}}, not {{cp}}. As written, the reader seeks into
> an
> unrelated part of the font and parses whatever bytes it finds as a device
> table.
> This has gone unnoticed because anchor device tables are rare in non-variable
> fonts. In a variable font they are VariationIndex tables and are everywhere,
> so
> the bad path is taken constantly.
> h3. 2. A delta count of zero slips past the guard
> {{readPosDeviceTable}} rejects a negative delta count:
> {code:java}
> int n = (es - ss) + 1;
> if (n < 0) {
> log.debug("invalid device table delta count: " + n + ", ignoring device
> table");
> return null;
> }
> {code}
> but {{n == 0}} (ie {{es == ss - 1}}) passes, and then trips
> {{GlyphPositioningTable.DeviceTable}}'s {{assert startSize <= endSize}}. So
> with
> assertions enabled the misparse in (1) turns from a silently-ignored bad read
> into an {{AssertionError}} that aborts font loading.
> h3. Reproducing
> Load {{NotoSans\[wght\].ttf}} - Fedora's {{google-noto-sans-vf-fonts}}
> package,
> also available from Google Fonts - through
> {{org.apache.fop.fonts.autodetect.FontInfoFinder}}, with
> {{org.apache.fop.complexscripts}} at DEBUG. FOP 2.11 logs 22,700 device table
> reads for that one font, thousands of them rejected as garbage:
> {noformat}
> DEBUG OTFAdvancedTypographicTableReader - invalid device table delta count:
> -101, ignoring device table
> DEBUG OTFAdvancedTypographicTableReader - invalid device table delta count:
> -107, ignoring device table
> DEBUG OTFAdvancedTypographicTableReader - invalid device table delta count:
> -113, ignoring device table
> {noformat}
> Those counts are nonsense because the offsets are wrong. With the base
> corrected,
> the same tables are recognised as VariationIndex (delta format 0x8000) and are
> cleanly ignored by the existing "unsupported device table delta format"
> branch.
> To be precise about what I did and did not reproduce: on these particular
> fonts,
> stock FOP 2.11 has every bad read caught by the {{n < 0}} guard, so no
> {{AssertionError}} is thrown - the garbage reads are the observable symptom.
> The
> identical code in a downstream fork (docx4j, which repackages this class) does
> hit {{n == 0}} on the same font file and throws. Originally reported there as
> https://github.com/plutext/docx4j/issues/686, by a user on Fedora with
> {{-ea}} enabled:
> {noformat}
> java.lang.AssertionError
> at
> ...GlyphPositioningTable$DeviceTable.<init>(GlyphPositioningTable.java:1778)
> at ...OTFAdvancedTypographicTableReader.readPosDeviceTable(...:1732)
> at ...OTFAdvancedTypographicTableReader.readPosAnchor(...:2063)
> at
> ...OTFAdvancedTypographicTableReader.readMarkToLigaturePosTableFormat1(...:2350)
> at ...OTFAdvancedTypographicTableReader.readGPOSSubtable(...:3127)
> at ...OTFAdvancedTypographicTableReader.readAll(...:89)
> at ...OpenFont.handleCharacterSpacing(OpenFont.java:902)
> at ...OFFontLoader.read(OFFontLoader.java:123)
> at ...FontLoader.getFont(FontLoader.java:130)
> at ...FontInfoFinder.find(FontInfoFinder.java:268)
> {noformat}
> (package names differ in the fork; the code at those lines is yours,
> unmodified)
> Whether a given font trips the assertion or merely reads garbage depends on
> what
> happens to be at the wrong offset, so I would treat (1) as the defect to fix
> and
> (2) as hardening.
> h3. Effect
> * With {{-ea}}: {{AssertionError}} during font loading. In the fork's case
> this
> aborted discovery of every remaining font, not just the offending one.
> * Without {{-ea}}: any bogus device table that survives the guard feeds wrong
> adjustments into glyph positioning.
> h3. Patch
> Attached: {{FOP-anchor-device-table.patch}}. It applies cleanly to trunk and
> to
> the {{fop-2_11}} tag - the file is byte-identical between them.
> I verified in the downstream fork that the fix does not disturb ordinary
> fonts:
> font discovery over a machine with 1242 system fonts produces byte-identical
> results before and after, and the five Noto variable fonts that previously
> failed
> now load. Longest added line is 96 characters, so checkstyle should be happy.
> Happy to raise this as a GitHub PR instead, or as well, if that is easier for
> you.
> Contribution is offered under the Apache License 2.0.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
