Last Comment Bug 538065 - (CVE-2010-0166) "ASSERTION: invalid array index" with glyphruns at YouTube
(CVE-2010-0166)
: "ASSERTION: invalid array index" with glyphruns at YouTube
Status: RESOLVED FIXED
[sg:critical?][3.6.x][rc-ridealong]
: assertion, verified1.9.2
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Jonathan Kew (:jfkthame)
:
:
Mentors:
http://www.youtube.com/watch?v=SUX9ja...
: 533393 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-01-05 17:54 PST by Jesse Ruderman
Modified: 2010-04-11 17:01 PDT (History)
9 users (show)
mbeltzner: blocking1.9.2-
jruderman: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.2-fixed
unaffected


Attachments
stack trace (28.69 KB, text/plain)
2010-01-05 17:54 PST, Jesse Ruderman
no flags Details
testcase, generates 4 assertions with current trunk build (349 bytes, text/html)
2010-01-06 07:16 PST, Jonathan Kew (:jfkthame)
no flags Details
patch v1: update lastRunIndex variable when deleting a run (1.64 KB, patch)
2010-01-06 09:20 PST, Jonathan Kew (:jfkthame)
roc: review+
mbeltzner: approval1.9.2.2+
Details | Diff | Splinter Review

Description Jesse Ruderman 2010-01-05 17:54:49 PST
Created attachment 420240 [details]
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) 2010-01-05 18:46:33 PST
Maybe Jonathan can look at this...
Comment 2 Jonathan Kew (:jfkthame) 2010-01-06 03:22:29 PST
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) 2010-01-06 03:27:13 PST
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) 2010-01-06 07:16:19 PST
Created attachment 420323 [details]
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) 2010-01-06 09:20:11 PST
Created attachment 420347 [details] [diff] [review]
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) 2010-01-06 10:02:50 PST
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) 2010-01-06 13:54:31 PST
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) 2010-01-06 13:56:02 PST
Oh, it's the same patch actually. Sorry about wasting your time.
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-01-06 13:56:51 PST
Let's take your patch.
Comment 10 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-01-06 13:57:32 PST
*** Bug 533393 has been marked as a duplicate of this bug. ***
Comment 11 Mike Beltzner [:beltzner, not reading bugmail] 2010-01-06 14:50:55 PST
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) 2010-01-07 09:54:25 PST
Pushed to m-c: http://hg.mozilla.org/mozilla-central/rev/8d23bc013df6
Comment 13 Jonathan Kew (:jfkthame) 2010-01-07 10:00:11 PST
(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] 2010-02-05 13:24:41 PST
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) 2010-02-05 13:34:50 PST
(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] 2010-02-26 13:50:57 PST
a1922=beltzner
Comment 17 Jonathan Kew (:jfkthame) 2010-02-26 14:21:47 PST
Pushed to 1.9.2:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/e6a4e7433b27
Comment 18 Al Billings [:abillings] 2010-03-15 16:01:07 PDT
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 2010-04-11 17:01:46 PDT
Crashtest added: http://hg.mozilla.org/mozilla-central/rev/d3e7c18f58fc

Note You need to log in before you can comment on or make changes to this bug.