606714 - CoreText crash [@ TGlyphEncoder::EncodeChars] with gigantic text run

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jesse Ruderman]

Attachment: testcase (from bug 583077)

Bug 583077's testcase crashes in an exploitable-looking way on Mac, too.  Using Firefox trunk on Mac OS X 10.5.

Comment 1

[Jesse Ruderman]

Attachment: crash report #1

Top line and crash address vary.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: patch, v1 - handle large textruns in sections to avoid shaper failure

We currently have code in the Uniscribe shaper path to split long runs and shape them in sections; I suggest that we do this at the gfxFont::InitTextRun level instead, regardless of which shaper is being called. This will make it much less likely that we'll run into either allocation failures during the shaping process, or shaper library bugs where extremely long runs lead to arithmetic overflow of indexes or other issues.

This patch limits the length of text runs passed to shapers from gfxFont::InitTextRun, using a similar approach to the existing code in gfxUniscribeShaper.cpp. Note that because gfxGDIFont currently overrides this method, we can't immediately remove the gfxUniscribeShaper version of the code, but we may be able to do that in a followup by reworking how gfxGDIFont interacts with its platform shapers.

With this patch, the testcase displays without crashing (it still makes the browser unresponsive for some time, but that's normal for tests that create ridiculously long strings).

Comment 3

[John Daggett (:jtd)]

Comment on attachment 486059 [details] [diff] [review]
patch, v1 - handle large textruns in sections to avoid shaper failure

One small nit:

Move the 'ok = PR_FALSE' to just below the def'n of thisRunLength and eliminate the initialization above the loop.  I think that makes it a little clearer that you're setting this to false each time through the loop unless shaping is successful.

Comment 4

[Johnny Stenback (:jst)]

Can this land now?

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: patch for checkin, addressing nit from comment #3

(In reply to comment #4)
> Can this land now?

Yes, if it gets approved for landing.

Comment 6

[Joe Drew (not getting mail)]

Exploitable-looking crash = blocks! Land away.

Comment 7

[Jonathan Kew [:jfkthame]]

http://hg.mozilla.org/mozilla-central/rev/7e3e4c91c0f7

Comment 8

[Jonathan Kew [:jfkthame]]

Backed out due to failures on Linux - probably conflicting with recently-landed patches from bug 597147. Will re-test and update....

Comment 9

[Jonathan Kew [:jfkthame]]

http://hg.mozilla.org/mozilla-central/rev/f7e0aeb5462b

Relanded with fix for the linux bustage (occurred because of bug 597147, which had landed just before this).

Comment 10

[Daniel Veditz [:dveditz]]

Would this be worth taking on older branches? I can't reproduce the crash on a recent 3.6.x on Mac (10.5.8) but I don't know if it's because Apple fixed something or if older Gecko's weren't vulnerable in the same way. Breaking up text runs seems like a sane thing to do regardless, though.

Comment 11

[Daniel Veditz [:dveditz]]

I assume the branches will need a completely different patch (no harfbuzz). Is backporting even possible? We're going to keep getting these simplistic DoS-type testcases where clever kids wonder what happens if they keep growing a text string in a loop, and sometimes expose underlying OS bugs. I'd rather stop things on our end if possible.

Comment 12

[Jonathan Kew [:jfkthame]]

This particular crash (within CoreText) won't happen on 3.5/3.6 because we use the ATSUI backend instead (for 10.4 compatibility). There is code in gfxAtsuiFonts.cpp to break up strings because of a 32K-pixel limitation on the length of a line ATSUI can lay out. So although it's using a somewhat different approach, and for a different original reason, the end result is that we don't send these really massive strings to the platform shaping code.

None of this entirely protects us from situations where we're on the edge of OOM, due to scripts creating huge strings, and then the platform doesn't handle a malloc()/new failure safely - this could happen even if we're only shaping small chunks at a time. But it does become much harder to hit exactly those conditions if we work on smaller sub-runs.