459968 - gfxTextRun::GetAdjustedSpacingArray "Attempting to allocate excessively large array" with pre-line

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Loading the testcase causes loads of badness:

###!!! ASSERTION: Invalid offset: 'aOffset <= mSkipChars->mCharCount', file gfx/thebes/src/gfxSkipChars.cpp, line 92

###!!! ASSERTION: Text run does not map enough text for our reflow: 'gfxSkipCharsIterator(iter).ConvertOriginalToSkipped(offset + length) <= mTextRun->GetLength()', file layout/generic/nsTextFrameThebes.cpp, line 5768

###!!! ASSERTION: Invalid offset: 'aOffset <= mSkipChars->mCharCount', file gfx/thebes/src/gfxSkipChars.cpp, line 92

###!!! ASSERTION: Substring out of range: 'aStart + aMaxLength <= mCharacterCount', file gfx/thebes/src/gfxFont.cpp, line 1960

###!!! ASSERTION: Substring out of range: 'aStart + aLength <= mCharacterCount', file gfx/thebes/src/gfxFont.cpp, line 1911

###!!! ASSERTION: Bad offset looking for glyphrun: 'aOffset <= mCharacterCount', file gfx/thebes/src/gfxFont.cpp, line 2158

###!!! ASSERTION: Attempting to allocate excessively large array: 'Error', file nsTArray.cpp, line 69

###!!! ABORT: file nsTArray.cpp, line 70


(gdb) f 9
#9  0x018a8fea in gfxTextRun::GetAdjustedSpacingArray (this=0x1c9f0790, aStart=6, aEnd=4, aProvider=0xbfffa83c, aSpacingStart=6, aSpacingEnd=4, aSpacing=0xbfff91f4) at /Users/jruderman/central/gfx/thebes/src/gfxFont.cpp:1560
1560	    if (!aSpacing->AppendElements(aEnd - aStart))
(gdb) p aEnd
$1 = 4
(gdb) p aStart
$2 = 6

Comment 1

[Jesse Ruderman]

"Attempting to allocate excessively large array" is only an abort in my local tree.  Should it be an abort for all debug-build users?  It seems to lead to buffer overflows and stack corruption more often than not.

Comment 2

[Brandon Sterne (:bsterne)]

This didn't crash for me using a trunk debug build so it may be fixed there.  I'll attach the assertions I still see, though.  Jesse, any idea on a severity rating?

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b1pre) Gecko/20081023100737 Minefield/3.1b1pre

Comment 3

[Brandon Sterne (:bsterne)]

Attachment: trunk assertions

Comment 4

[Brandon Sterne (:bsterne)]

(In reply to comment #2)
> This didn't crash for me using a trunk debug build

I only tested it on Linux though, and just realized this was filed as a Mac bug.  Doh.

Comment 5

[Jesse Ruderman]

You're right, the only reason it "crashes" for me is that I have "Attempting to allocate excessively large array" set to abort.  Without that, it doesn't crash.  Nothing Mac-specific about that :)

Comment 6

[Jesse Ruderman]

The other assertions still scare me, though, because they sound buffer-overflowy .

Comment 7

[Jesse Ruderman]

roc agrees that it looks scary and volunteered to try to fix it.

Comment 8

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

Attachment: fix

This is all integer overflow stuff. This patch fixes some of those issues by clamping conversions from float to nscoord and by using NSCoordSaturatingAdd in a few places.

With this patch, we assert like this:
###!!! ASSERTION: shouldn't have unconstrained widths anymore: 'psd->mRightEdge != NS_UNCONSTRAINEDSIZE', file /Users/roc/mozilla-trunk/layout/generic/nsLineLayout.cpp, line 2411
###!!! ASSERTION: should no longer be using unconstrained widths: 'aWidth != NS_UNCONSTRAINEDSIZE', file /Users/roc/mozilla-trunk/layout/generic/nsLineLayout.cpp, line 179
because widths have been clamped to nscoord_MIN/nscoord_MAX. I'm not sure what to do about that. The assertions are innocuous, maybe this long after the reflow branch landing, we should take them out?

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 346388 [details] [diff] [review]
fix

>-      return NSToCoordRound(aValue.GetFloatValue() *
>+      return NSToCoordRoundWithClamp(aValue.GetFloatValue() *
>                             NS_ceil(aPresContext->AppUnitsPerDevPixel() *
>                                     zeroWidth));

Maybe fix up the indentation here if there's a nice way to do it without crossing the 80th column.

r+sr=dbaron

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Initially landed on mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/57e6a1e9041e
http://hg.mozilla.org/mozilla-central/rev/026147c91538
and then backed out due to crashes caused by bug 430332:
http://hg.mozilla.org/mozilla-central/rev/9eaf91dad68c
and then relanded on mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/54946665b7c0
and thus fixed on both mozilla-central and releases/mozilla-1.9.1.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I tried to verify the fix on 1.9.1 but I'm also not able to reproduce the crash with an older build from November last year and the given testcase. Jesse, do I have to meet special conditions?

Comment 12

[Jesse Ruderman]

Are you using a debug build?  In comment 0, I incorrectly identified this bug as an abort.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Ok, the crash is fixed. At least on trunk. Have to build a Shiretoko debug build first.

Jesse, I still can see the following assertion:

###!!! ASSERTION: bad width: 'Not Reached', file /data/mozilla/build/mozilla-central/mozilla/layout/generic/nsLineLayout.cpp, line 182
nsLineLayout::BeginLineReflow(int, int, int, int, int, int) (nsUnicharUtils.cpp:)
nsBlockFrame::DoReflowInlineFrames(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, int*, LineReflowStatus*, int) (nsUnicharUtils.cpp:)
nsBlockFrame::ReflowInlineFrames(nsBlockReflowState&, nsLineList_iterator, int*) (nsUnicharUtils.cpp:)
nsBlockFrame::ReflowLine(nsBlockReflowState&, nsLineList_iterator, int*) (nsUnicharUtils.cpp:)
nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) (nsUnicharUtils.cpp:)

There is no bug filed on that. I believe I should do that. It hasn't be security related, right?

Comment 14

[Jesse Ruderman]

If you're not satisfied with bug 371561, bug 323381, or bug 387080, sure.  If you can reduce the testcase significantly, you can file a new public bug report with the new testcase.  If you can't reduce the testcase much, I guess the best thing to do is to file a new bug but refer to the testcase in this bug.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Oh, bug 323381 does it cover. Thanks for pointing it out. It's summary was a bit too short, so I wasn't able to find it.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Also no crash anymore with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090211 Shiretoko/3.1b3pre ID:20090211221038

Comment 17

[Mats Palmgren (inactive)]

Landed the crashtest:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fbbab9a6a0e

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0fbbab9a6a0e