836225 - crash when shaping supplementary-plane text with graphite font

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[martin_hosken]

Attachment: font and test html

The enclosed iswa font is graphite based and crashes firefox. There is a crash report with my name on it somewhere.

Comment 1

[Jonathan Kew [:jfkthame]]

Martin's crash: http://crash-stats.mozilla.com/report/index/bp-46755649-b3bf-4ad7-8f9c-cfc802130130

Also crashes for me on OS X.

Provisionally marking security-sensitive, pending further investigation.

Comment 2

[Jonathan Kew [:jfkthame]]

In a debug build, this hits a bunch of assertions before crashing, starting with
###!!! ASSERTION: unexpected offset: 'offs >= j && offs < aLength', file /Users/jkew/mozdev/mc-clean/gfx/thebes/gfxGraphiteShaper.cpp, line 380

Either graphite is giving us strange output, or we're doing something wrong with the output it gives. Will investigate shortly.

Comment 3

[Christoph Diehl [:posidron]]

Attachment: callstack

This is already crashing earlier and shows an invalid read of 4 bytes in an ASan enabled build.

Comment 4

[Christoph Diehl [:posidron]]

Attachment: testcase-minimized

Minimized the testcase and replaced the glyphs with HTML entities:

󽠃

Comment 5

[Jonathan Kew [:jfkthame]]

This is looking like a graphite engine bug - it seems to be returning a spurious value for the number of glyphs in the run (gr_seg_n_slots), which leads to us accessing lots of out-of-bounds stuff when trying to process glyphs that aren't actually there.

Comment 6

[Jonathan Kew [:jfkthame]]

Turns out the bug is in our integration (gfxGraphiteShaper) after all - we pass the number of utf16 code units to the gr_make_seg function, but what it actually wants is the number of Unicode characters. So this affects any non-BMP text. Patch coming up.

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: handle supplementary-plane chars properly in graphite shaper

In addition to correcting the length parameter to gr_make_seg, we should convert character indices back to utf16 code point offsets immediately during the cluster-finding loop, so that all our char/glyph processing is consistently done in terms of utf16 codes. With this fix, the example here (using Plane 15 characters) works properly, AFAICS.

Comment 8

[Jonathan Kew [:jfkthame]]

(We should also have a crashtest for this, though we might want to hold off on that until the fix is in place.)

Comment 9

[John Daggett (:jtd)]

(In reply to Jonathan Kew (:jfkthame) from comment #7)
> Created attachment 708153 [details] [diff] [review]
> handle supplementary-plane chars properly in graphite shaper
> 
> In addition to correcting the length parameter to gr_make_seg, we should
> convert character indices back to utf16 code point offsets immediately
> during the cluster-finding loop, so that all our char/glyph processing is
> consistently done in terms of utf16 codes. With this fix, the example here
> (using Plane 15 characters) works properly, AFAICS.

Hmmm, shouldn't the checks in the asserts also be if (xxx) continue statements to be very sure we avoid out-of-bounds array references?

Comment 10

[Jonathan Kew [:jfkthame]]

(In reply to John Daggett (:jtd) from comment #9)
> (In reply to Jonathan Kew (:jfkthame) from comment #7)
> > Created attachment 708153 [details] [diff] [review]
> > handle supplementary-plane chars properly in graphite shaper
> > 
> > In addition to correcting the length parameter to gr_make_seg, we should
> > convert character indices back to utf16 code point offsets immediately
> > during the cluster-finding loop, so that all our char/glyph processing is
> > consistently done in terms of utf16 codes. With this fix, the example here
> > (using Plane 15 characters) works properly, AFAICS.
> 
> Hmmm, shouldn't the checks in the asserts also be if (xxx) continue
> statements to be very sure we avoid out-of-bounds array references?

Possibly, though they'd then have an impact on release build performance. In general, we don't expect release builds to include all the same sanity-checks for internal consistency that we have in debug builds.

Comment 11

[Jonathan Kew [:jfkthame]]

Attachment: crashtest for graphite and supplementary-plane characters

Crashtest using a copy of the PigLatin graphite font with characters mapped into plane 15. With graphite enabled, this at least throws a bunch of assertions (may or may not actually crash the browser) without the patch here; loads cleanly once the patch is applied.

Comment 12

[Jonathan Kew [:jfkthame]]

Attachment: crashtest for graphite and supplementary-plane characters

And now with a test-pref() annotation, so that the test can run usefully even before we pref-on graphite by default.

Comment 13

[Daniel Veditz [:dveditz]]

Were you planning on enabling graphite in Firefox 21, or in a later cycle?

Comment 14

[Jonathan Kew [:jfkthame]]

(In reply to Daniel Veditz [:dveditz] from comment #13)
> Were you planning on enabling graphite in Firefox 21, or in a later cycle?

I was hoping to try for FF21, and I think we're getting really close to being ready; but before moving forward with that, I want to get the tests in bug 700022 safely landed. Which may take a little while, as other priorities keep popping up...

Comment 15

[Jonathan Kew [:jfkthame]]

Review ping... would be nice to get this into the tree soon.

Comment 16

[Jonathan Kew [:jfkthame]]

Comment on attachment 708153 [details] [diff] [review]
handle supplementary-plane chars properly in graphite shaper

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
It would be pretty easy to construct an example that hits the faulty codepath (for users with graphite preffed on), and is likely to crash.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The code change makes it pretty clear the problem related to the use of supplementary-plane characters with graphite.

Which older supported branches are affected by this flaw?
All current branches, but the feature is preffed off by default, so nobody is exposed unless they have deliberately preffed on the graphite font support in about:config.

If not all supported branches, which bug introduced the flaw?
All branches.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Backports would be pretty simple - the patch doesn't apply as-is due to changes in the context, but the actual code fix needed remains the same so the fixup would be straightforward.
Given that the feature is preffed off by default, it's not clear to me that it is worth backporting the fix, but if desired I will prepare the patches for older branches.

How likely is this patch to cause regressions; how much testing does it need?
Simple fix for incorrect use of the API, should be very little risk of regression.

Comment 17

[Alex Keybl [:akeybl]]

(In reply to Jonathan Kew (:jfkthame) from comment #16)
> Which older supported branches are affected by this flaw?
> All current branches, but the feature is preffed off by default, so nobody
> is exposed unless they have deliberately preffed on the graphite font
> support in about:config.

Given this, no need to take code change.

Comment 18

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/118c857ffee0

Comment 19

[Jonathan Kew [:jfkthame]]

Comment on attachment 708153 [details] [diff] [review]
handle supplementary-plane chars properly in graphite shaper

Although this isn't critical to track for FF20 release, as it is preffed off by default, requesting approval to land the fix on Aurora so that it will work for those users who know they need it and specifically enable the feature.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): graphite font support

User impact if declined: users wanting to use graphite fonts and supplementary-plane text (there's a project actively developing fonts for exactly this scenario) will not be able to work with FF20

Testing completed (on m-c, etc.): tested locally with the sign-language font prototype, verified to prevent the assertions in debug builds as well as the actual crash

Risk to taking this patch (and alternatives if risky): the feature is preffed off by default, so no risk to users in general

String or UUID changes made by this patch: none

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/118c857ffee0

Comment 21

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 708153 [details] [diff] [review]
handle supplementary-plane chars properly in graphite shaper

Approving since it's low risk (preffed off by default).

Comment 22

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/49a5773f3ed0

Updated status-firefox20 to "fixed", as the potential crash bug is now fixed for FF20, although the feature remains preffed off by default anyway.

Comment 23

[Jonathan Kew [:jfkthame]]

Pushed the crashtest, as the fix here has long since shipped to all relevant channels.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7299b4f7f43

Comment 24

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

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