Closed Bug 606714 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Graphics, defect, critical)

x86
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+
status1.9.2 --- unaffected
status1.9.1 --- unaffected

People

(Reporter: jruderman, Assigned: jfkthame)

Details

(Keywords: crash, testcase, Whiteboard: [sg:critical])

Crash Data

Attachments

(3 files, 1 obsolete file)

Bug 583077's testcase crashes in an exploitable-looking way on Mac, too.  Using Firefox trunk on Mac OS X 10.5.
Attached file crash report #1
Top line and crash address vary.
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).
Assignee: nobody → jfkthame
Attachment #486059 - Flags: review?(jdaggett)
Whiteboard: [sg:critical]
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.
Attachment #486059 - Flags: review?(jdaggett) → review+
Can this land now?
Attachment #486059 - Flags: approval2.0?
(In reply to comment #4)
> Can this land now?

Yes, if it gets approved for landing.
Attachment #486059 - Attachment is obsolete: true
Attachment #489433 - Flags: approval2.0?
Attachment #486059 - Flags: approval2.0?
Exploitable-looking crash = blocks! Land away.
blocking2.0: --- → final+
Attachment #489433 - Flags: approval2.0?
http://hg.mozilla.org/mozilla-central/rev/7e3e4c91c0f7
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Backed out due to failures on Linux - probably conflicting with recently-landed patches from bug 597147. Will re-test and update....
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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).
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
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.
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
status1.9.1: --- → ?
status1.9.2: --- → ?
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.
Whiteboard: [sg:critical] → [sg:critical][need answer to comment 11 from 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.
Whiteboard: [sg:critical][need answer to comment 11 from jfkthame] → [sg:critical]
blocking1.9.1: ? → ---
blocking1.9.2: ? → ---
Crash Signature: [@ TGlyphEncoder::EncodeChars]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.