Closed Bug 459968 Opened 16 years ago Closed 16 years ago

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


(Core :: Layout, defect, P2)






(Reporter: jruderman, Assigned: roc)



(5 keywords, Whiteboard: [sg:critical?])


(3 files)

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
"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.
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
(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.
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 :)
The other assertions still scare me, though, because they sound buffer-overflowy .
roc agrees that it looks scary and volunteered to try to fix it.
Assignee: nobody → roc
Flags: blocking1.9.1?
Whiteboard: [sg:critical?]
Attached patch fixSplinter Review
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?
Attachment #346388 - Flags: superreview?(dbaron)
Attachment #346388 - Flags: review?(dbaron)
Whiteboard: [sg:critical?] → [sg:critical?][needs review]
Comment on attachment 346388 [details] [diff] [review]

>-      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.

Attachment #346388 - Flags: superreview?(dbaron)
Attachment #346388 - Flags: superreview+
Attachment #346388 - Flags: review?(dbaron)
Attachment #346388 - Flags: review+
Whiteboard: [sg:critical?][needs review] → [sg:critical?][needs landing]
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Initially landed on mozilla-central:
and then backed out due to crashes caused by bug 430332:
and then relanded on mozilla-central:
and thus fixed on both mozilla-central and releases/mozilla-1.9.1.
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: [sg:critical?][needs landing] → [sg:critical?]
Target Milestone: --- → mozilla1.9.1b3
Flags: in-testsuite?
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?
OS: Mac OS X → All
Hardware: x86 → All
Are you using a debug build?  In comment 0, I incorrectly identified this bug as an abort.
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?
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.
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.
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
Group: core-security
Landed the crashtest:
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.