538065 - (CVE-2010-0166) "ASSERTION: invalid array index" with glyphruns at YouTube

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Jesse Ruderman]

Attachment: stack trace

In debug Firefox trunk, loading any YouTube video page triggers:

###!!! ASSERTION: invalid array index: 'i < Length()', file ../../../dist/include/nsTArray.h, line 317

#5  0x041418ae in NS_DebugBreak_P (aSeverity=1, aStr=0x42f9140 "invalid array index", aExpr=0x42e7e60 "i < Length()", aFile=0x42f84dc "../../../dist/include/nsTArray.h", aLine=317) at /Users/jruderman/central/xpcom/base/nsDebugImpl.cpp:360
#6  0x0419b602 in nsTArray<gfxTextRun::GlyphRun>::ElementAt (this=0x213333d4, i=1) at nsTArray.h:317
#7  0x0419b636 in nsTArray<gfxTextRun::GlyphRun>::operator[] (this=0x213333d4, i=1) at nsTArray.h:350
#8  0x04197fb7 in gfxTextRun::SanitizeGlyphRuns (this=0x213333c0) at /Users/jruderman/central/gfx/thebes/src/gfxFont.cpp:2725
...

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Maybe Jonathan can look at this...

Comment 2

[Jonathan Kew [:jfkthame]]

I'm not seeing this assertion with the youtube page. I suspect it was triggered by some particular (unusual) text string, perhaps in an ad or some transient listing of videos or comments. It'd be good to have a cut-down test case that reproduces it....

Comment 3

[Jonathan Kew [:jfkthame]]

Ah - looks like I can reliably trigger this by loading the Unicode sample page at http://alanwood.net/unicode/unicode_samples.html. Should be able to narrow it down from there.

(That page triggers several other assertions too....)

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: testcase, generates 4 assertions with current trunk build

The assertion can be triggered by the presence of particular Unicode characters. This testcase has 4 characters (U+FEFF, FFF9, FFFA, FFFB) that each trigger it.

I'm guessing the occurrence on youtube was probably temporary, caused by a character present in some dynamic content at that time.

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: patch v1: update lastRunIndex variable when deleting a run

The problem arises when SanitizeGlyphRuns deletes the last glyph run from the array, but fails to update the "last" variable; then on the next iteration it would try to look at the no-longer-valid "last" element of the array. Simple fix is to decrement "last" when deleting an array element.

This patch does that, and the assertions no longer occur. I've also renamed "last" to "lastRunIndex" just because I think it's clearer.

Incidentally, the problem only occurred with the Core Text backend, not ATSUI, because they generate different glyph arrays in the case of invisible characters such as those in the testcase here: Core Text deletes them (resulting in empty glyph runs, which SanitizeGlyphRuns is designed to eliminate), whereas ATSUI leaves a "deleted" placeholder glyph in the glyph array.

Comment 6

[Jonathan Kew [:jfkthame]]

I think we should take this on 1.9.2 as well, as soon as it's landed on trunk. The chance of the bug actually being exploitable seems pretty slim, IMO, but I see no reason NOT to fix it; it's clear what the error is, and the patch is simple and safe.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hmm, does my patch in bug 533393 fix this? I forgot to land it :-(

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Oh, it's the same patch actually. Sorry about wasting your time.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Let's take your patch.

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

If this isn't a hard release blocker, which I don't think it is as per comment 6, then I don't think it would cause a RC respin on its own. If we have to do a new RC we'll take it as a ridealong, otherwise, 3.6.1.

Comment 12

[Jonathan Kew [:jfkthame]]

Pushed to m-c: http://hg.mozilla.org/mozilla-central/rev/8d23bc013df6

Comment 13

[Jonathan Kew [:jfkthame]]

(In reply to comment #11)
> If this isn't a hard release blocker, which I don't think it is as per comment
> 6, then I don't think it would cause a RC respin on its own. If we have to do a
> new RC we'll take it as a ridealong, otherwise, 3.6.1.

Right - sorry, I didn't mean to suggest this needs to affect RC now, it's clearly not critical. Just that we should take it on the branch when we have the opportunity. Blocking was the wrong flag to request; how about approval1.9.2.1 on the patch?

Comment 14

[Daniel Veditz [:dveditz]]

Does this affect the 1.9.1 branch? It doesn't have the regression keyword so it might, and we don't want to land in a 3.6.x security update and not the corresponding 3.5.x security update if it affects both.

Comment 15

[Jonathan Kew [:jfkthame]]

(In reply to comment #14)
> Does this affect the 1.9.1 branch? It doesn't have the regression keyword so it
> might, and we don't want to land in a 3.6.x security update and not the
> corresponding 3.5.x security update if it affects both.

No, it doesn't affect 1.9.1 because this bug is in code that was added specifically as part of the CoreText support, and we don't have CoreText support in 1.9.1 / 3.5.x.

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

a1922=beltzner

Comment 17

[Jonathan Kew [:jfkthame]]

Pushed to 1.9.2:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/e6a4e7433b27

Comment 18

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.2 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.2pre) Gecko/20100312 Namoroka/3.6.2pre using the attached testcase.

Comment 19

[Jesse Ruderman]

Crashtest added: http://hg.mozilla.org/mozilla-central/rev/d3e7c18f58fc