Core Text rendering path does not implement size-dependent tracking in AAT fonts

RESOLVED FIXED in Firefox 58

Status

()

P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

Trunk
mozilla58
Points:
---

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox55 wontfix, firefox56 wontfix, firefox57 fix-optional, firefox58 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

a year ago
Created attachment 8906632 [details]
testcase for tracking in Apple system font

The ".SFNS Text" and ".SFNS Display" fonts (aka "-apple-system"), and probably other AAT fonts, include a 'trak' (tracking) table that specifies adjustments to inter-glyph spacing depending on the point size.

Safari supports this, while Firefox doesn't (nor does Chrome, FWIW), which results in inferior rendering of a waterfall of different sizes; see attachment.
(Assignee)

Comment 1

a year ago
Created attachment 8906633 [details]
screenshot comparing Firefox (left) and Safari (right) rendering of the testcase

Note in particular the discontinuity in the Firefox waterfall between 19px and 20px. This is the point where the -apple-system font switches from the Text to Display face; the latter has tighter default glyph spacing.

Proper application of the per-size tracking data would smooth this out, as seen in the Safari rendering.
status-firefox55: --- → wontfix
status-firefox56: --- → wontfix
status-firefox57: affected → fix-optional
status-firefox-esr52: --- → wontfix
Priority: -- → P3
Whiteboard: [gfx-noted]
(Assignee)

Comment 2

a year ago
Created attachment 8908241 [details] [diff] [review]
Support the AAT 'trak' table when shaping macOS fonts via Core Text

This adds 'trak' table support, and results in a nice smooth waterfall in the testcase above. It applies only to fonts we're shaping with Core Text (which includes things like -apple-system), as it's debatable how an AAT table like 'trak' should interact -- if at all -- with OpenType-based shaping. (In practice, it's unlikely to be present in any fonts except Apple's, anyhow.) It's also debatable, I think, how tracking should interact with page zoom: should it be applied based on the nominal CSS font size involved, or on the zoomed (physical) size of the text? For now, I've gone with the former option, mostly because it was easier, but also because it provides a result that remains stable when zooming, which is probably a good thing.
Attachment #8908241 - Flags: review?(jmuizelaar)
(Assignee)

Comment 3

a year ago
Created attachment 8908247 [details]
screenshot of the tracking testcase with patched Nightly build

Here's a screenshot of the result we get with the patch applied. The metrics are not precisely identical to Safari's in all cases, but they're within a pixel or two; I think the discrepancies must be due to slight differences in how rounding is handled in some places. Visually, it certainly looks OK.
Comment on attachment 8908241 [details] [diff] [review]
Support the AAT 'trak' table when shaping macOS fonts via Core Text

Review of attachment 8908241 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxMacPlatformFontList.mm
@@ +531,5 @@
> +        uint16_t(trak->reserved) != 0) {
> +        return false;
> +    }
> +    // Find the horizontal trackData, and check it doesn't overrun the buffer.
> +    if (horizOffset + sizeof(TrackData) > len) {

Probably worth making this:
horizOffset > len - sizeof(TrackData)
to avoid integer overflow. The same kind of thing applies below.

This code overall seems a little hairy and it feels like it's not obviously safe. Is there a way that we can write to give more confidence that it's not introducing security problems?
(Assignee)

Comment 5

a year ago
(In reply to Jeff Muizelaar [:jrmuizel] from comment #4)
> Comment on attachment 8908241 [details] [diff] [review]
> Support the AAT 'trak' table when shaping macOS fonts via Core Text
> 
> Review of attachment 8908241 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxMacPlatformFontList.mm
> @@ +531,5 @@
> > +        uint16_t(trak->reserved) != 0) {
> > +        return false;
> > +    }
> > +    // Find the horizontal trackData, and check it doesn't overrun the buffer.
> > +    if (horizOffset + sizeof(TrackData) > len) {
> 
> Probably worth making this:
> horizOffset > len - sizeof(TrackData)
> to avoid integer overflow.

IIUC, integer overflow can't actually occur here, because horizOffset is a 16-bit unsigned value, but the C++ integral promotion rules[1] mean that the arithmetic operation will be performed on (32-bit) ints. (And sizeof(TrackData) is obviously a small constant value.)

Still, I'll do your suggested rearrangement (here and below) as I agree it's generally better practice to write such checks that way.

> The same kind of thing applies below.
> 
> This code overall seems a little hairy and it feels like it's not obviously
> safe. Is there a way that we can write to give more confidence that it's not
> introducing security problems?

Aside from re-ordering these checks to avoid any appearance of overflow risk, I'm not sure what else we can do here.

Note that this code isn't directly exposed to the web, in that a webfont can't deliver a 'trak' table (it will be stripped by OTS), so we're only concerned here about locally-installed AAT fonts. If a user has malicious fonts installed on their local system, they've got bigger worries than this.

(Generally, we ought to be able to trust the tables in Apple's installed fonts, and simply follow the offsets, etc.; but on principle I included length checks throughout, to ensure we'd bail out safely in case of a font with a bad table -- or some kind of misinterpretation on our side.)


[1] http://en.cppreference.com/w/cpp/language/implicit_conversion#Integral_promotion
(Assignee)

Comment 6

a year ago
Created attachment 8912161 [details] [diff] [review]
Support the AAT 'trak' table when shaping macOS fonts via Core Text

Rearranged a couple of overflow checks as suggested above.
Attachment #8912161 - Flags: review?(jmuizelaar)
(Assignee)

Updated

a year ago
Attachment #8908241 - Attachment is obsolete: true
Attachment #8908241 - Flags: review?(jmuizelaar)
(Assignee)

Updated

a year ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Comment on attachment 8912161 [details] [diff] [review]
Support the AAT 'trak' table when shaping macOS fonts via Core Text

Review of attachment 8912161 [details] [diff] [review]:
-----------------------------------------------------------------

Ah, I had not realized that these were only local fonts.
Attachment #8912161 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 8

a year ago
Yeah, at least currently we don't support any of the AAT-specific layout tables in downloadable fonts; OTS doesn't know about them, and will drop them from any @font-face resources that attempt to include them.

(I thought we had a [low priority] bug on file to consider adding such support to OTS some day, but I can't seem to find it offhand; maybe I thought about it but never actually filed one.)

Comment 9

a year ago
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9129ef3f95a
Support the AAT 'trak' table when shaping macOS fonts via Core Text. r=jrmuizel

Comment 10

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c9129ef3f95a
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.