Bug 538065 (CVE-2010-0166)

"ASSERTION: invalid array index" with glyphruns at YouTube

RESOLVED FIXED

Status

()

Core
Layout: Text
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Jesse Ruderman, Assigned: jfkthame)

Tracking

({assertion, verified1.9.2})

Trunk
x86
Mac OS X
assertion, verified1.9.2
Points:
---
Bug Flags:
blocking1.9.2 -
in-testsuite +

Firefox Tracking Flags

(status1.9.2 .2-fixed, status1.9.1 unaffected)

Details

(Whiteboard: [sg:critical?][3.6.x][rc-ridealong], URL)

Attachments

(3 attachments)

(Reporter)

Description

8 years ago
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
...
Maybe Jonathan can look at this...
(Assignee)

Comment 2

8 years ago
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....
(Assignee)

Comment 3

8 years ago
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....)
(Assignee)

Comment 4

8 years ago
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.
(Assignee)

Comment 5

8 years ago
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.
Assignee: nobody → jfkthame
Attachment #420347 - Flags: review?(roc)
(Assignee)

Comment 6

8 years ago
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.
Flags: blocking1.9.2?
Hmm, does my patch in bug 533393 fix this? I forgot to land it :-(
Oh, it's the same patch actually. Sorry about wasting your time.
Attachment #420347 - Flags: review?(roc) → review+
Let's take your patch.
Duplicate of this bug: 533393
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.
Flags: blocking1.9.2? → blocking1.9.2-
Whiteboard: [3.6.x][rc-ridealong]
(Assignee)

Comment 12

8 years ago
Pushed to m-c: http://hg.mozilla.org/mozilla-central/rev/8d23bc013df6
(Assignee)

Comment 13

8 years ago
(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?
(Assignee)

Updated

8 years ago
Attachment #420347 - Flags: approval1.9.2.1?
(Reporter)

Updated

8 years ago
Whiteboard: [3.6.x][rc-ridealong] → [3.6.x][rc-ridealong][sg:critical?]
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.
status1.9.1: --- → ?
status1.9.2: --- → wanted
Whiteboard: [3.6.x][rc-ridealong][sg:critical?] → [3.6.x][rc-ridealong][sg:critical?][needs answer to comment 14]
(Assignee)

Comment 15

8 years ago
(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.
Attachment #420347 - Flags: approval1.9.2.2? → approval1.9.2.2+
a1922=beltzner
status1.9.1: ? → unaffected
(Assignee)

Comment 17

8 years ago
Pushed to 1.9.2:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/e6a4e7433b27
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Updated

8 years ago
status1.9.2: wanted → .2-fixed
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.
Keywords: verified1.9.2
Whiteboard: [3.6.x][rc-ridealong][sg:critical?][needs answer to comment 14] → [sg:critical?][3.6.x][rc-ridealong]
Alias: CVE-2010-0166
Group: core-security
(Reporter)

Comment 19

8 years ago
Crashtest added: http://hg.mozilla.org/mozilla-central/rev/d3e7c18f58fc
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.