Closed Bug 459968 Opened 16 years ago Closed 16 years ago

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

Categories

(Core :: Layout, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.1b3

People

(Reporter: jruderman, Assigned: roc)

References

Details

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

Attachments

(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]
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
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:
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.
Status: NEW → RESOLVED
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?
Status: RESOLVED → VERIFIED
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:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fbbab9a6a0e
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.