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

Status: RESOLVED FIXED

(Core :: Graphics: Text, enhancement, P3)

Description

[Jonathan Kew [:jfkthame]]

Attachment: 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.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: 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.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: 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.

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: 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 4

[Jeff Muizelaar [:jrmuizel]]

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?

Comment 5

[Jonathan Kew [:jfkthame]]

(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

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: Support the AAT 'trak' table when shaping macOS fonts via Core Text

Rearranged a couple of overflow checks as suggested above.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 8

[Jonathan Kew [:jfkthame]]

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

[Pulsebot]

Pushed by [e-mail]:
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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c9129ef3f95a