471594 - "ASSERTION: negative length" & crash with XBL, rtl

Status: VERIFIED FIXED

(Core :: Layout: Text and Fonts, defect, P2)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: negative length: 'GetContentEnd() - mContentOffset >= 0', file layout/generic/nsTextFrame.h, line 307

###!!! ASSERTION: bad index: 'PRUint32(aIndex) < mState.mLength', file nsTextFragment.h, line 184

###!!! ASSERTION: integer overflow: 'mMaxTextLength <= mMaxTextLength + aFrame->GetContentLength()', file layout/generic/nsTextFrameThebes.cpp, line 1187

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

Security-sensitive because these assertions scare me.  Related to bug 429458?

Comment 1

[Daniel Holbert [:dholbert]]

I can repro all the assertions from comment 0 using my up-to-date Linux mozilla-central build. (I actually get 3 consecutive copies of the "negative length" assertion.)

Platfom --> All/All

Comment 2

[Daniel Holbert [:dholbert]]

If I add a character inside the second binding's <content> (as exemplified in this attached testcase), I get:
 - 2 copies of the "negative length" assertion
 - 1 copy of the "integer overflow" assertion
 - a crash in nsTextFrameUtils::TransformText (I'll post a backtrace in a minute)

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090102 Minefield/3.2a1pre

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (crashes Firefox when loaded)

(oops, here's the testcase)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: backtrace of testcase 2 crash, on linux

Here's the GDB log of the crash & the backtrace.

Strangely, the backtrace only includes 2 stack levels, and then it hits "0x00000000 in ?? ()".  Does that mean we're accidentally overwriting chunks of the stack?  That sounds bad...

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: testcase 3 (crashes Firefox when loaded)

You can replace "2!" in the earlier testcases with 
  "<alphanumeric><punctuation_or_space>"
(in either order), and the testcase still asserts and crashes, as demonstrated here.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: testcase 4 (crashes Firefox when loaded)

(In reply to comment #2)
> If I add a character inside the second binding's <content> [I get a crash]

I get the same crash if I put the character directly inside span 's'. A space character triggers the crash there, as well (though it doesn't trigger a crash when put into the <content> of binding 'y' instead of into span 's').

One more note: If I put any character (I've tested alphanumeric, space, & punctuation) just inside div 'd' (before span 's') then I get no crash & no assertions.  I think this is true of all testcases posted so far.

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to comment #4)
> Strangely, the backtrace only includes 2 stack levels, and then it hits
> "0x00000000 in ?? ()".  Does that mean we're accidentally overwriting chunks of
> the stack?  That sounds bad...

Yup, this is a classic integer overflow + buffer-overrun situation.  What happens is:
 A) In BuildTextRunForFrames, we have contentLength = -1
 B) That gets passed into nsTextFrameUtils::TransformText as "PRUint32 aLength".  The conversion to unsigned yields a value of 4294967295.
 C) TransformText writes aLength characters to the buffer 'aOutput', which only has capacity 4096.  We write (way) beyond the end of the buffer, & we fail.
 
Note: aOutput's memory is allocated up a few stack levels in  BuildTextRunsScanner::FlushFrames, quoted here:

1156     nsAutoTArray<PRUint8,BIG_TEXT_NODE_SIZE> buffer;
[snip]
1159     textRun = BuildTextRunForFrames(buffer.Elements());
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsTextFrameThebes.cpp#1156

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: testcase 5 (crashes onclick instead of onload)

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: backtrace just before testcase 5 crash, on linux

(In reply to comment #4)
> Strangely, the backtrace only includes 2 stack levels

Here's a more useful backtrace, taken just after we enter TransformText but before we've blown away the stack.  Note the 'aLength=4294967295' at level 0.

Comment 10

[Daniel Holbert [:dholbert]]

This crashes Firefox v3.0, too, btw. (I just tried using testcase #5)
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.5) Gecko/2008121622 Ubuntu/8.10 (intrepid) Firefox/3.0.5

Comment 11

[Jeff Walden [:Waldo]]

Is there a compiler warning which would have fingered this if we'd made the effort to fix it?  The warning-blame site seems to be down now, not entirely surprising given it was promoted as an experimental service or somesuch, so I can't track down if one is present (and don't have the time now to do a build to see for myself).

Comment 12

[Daniel Holbert [:dholbert]]

I looked for a regression range, using testcase 4.  It turns out there were two regressions -- first we started hanging, and then we started crashing.  (I don't know when the various assertions appeared, because I'm using non-debug nightly builds to find the regression range.)

No Problem --> Hang:
====================
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a8pre) Gecko/2007081504 Minefield/3.0a8pre
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a8pre) Gecko/2007081604 Minefield/3.0a8pre
Bonsai link: http://tinyurl.com/8v57p4 -- possibly bug 385270?

Hang --> Crash:
===============
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b2pre) Gecko/2007111104 Minefield/3.0b2pre
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b2pre) Gecko/2007111204 Minefield/3.0b2pre
Bonsai link: http://tinyurl.com/8rgst4 -- possibly bug 402427?

Comment 13

[Daniel Holbert [:dholbert]]

(In reply to comment #12)
> No Problem --> Hang:
> Bonsai link: http://tinyurl.com/8v57p4 -- possibly bug 385270?

Confirmed that this initial regression was from bug 385270.  I took a CVS checkout from just after that bug's landing (15 Aug 2007 11:40 PDT), and I get 2000 asserts[1] and then an abort[2].  When I back out that bug's patch[3], I get good behavior -- no asserts & no crash/hang/abort.

[1] The asserts are all repetitions of this pair:
ASSERTION: redo line on totally empty line: 'aState.IsImpactedByFloat()', file /mozilla/layout/generic/nsBlockFrame.cpp, line 3428
ASSERTION: unconstrained height on totally empty line: 'NS_UNCONSTRAINEDSIZE != aState.mAvailSpaceRect.height', file /mozilla/layout/generic/nsBlockFrame.cpp, line 3430

[2] The abort is accompanied by this message:
   Block(div)(0)@0x9c6d504: yikes! spinning on a line over 1000 times!
and it's here in the code:    http://tinyurl.com/6wem6x

[3] The patch posted on the bug didn't back out cleanly, so I backed out the patch as-it-was-landed, via the commands here: http://tinyurl.com/9l6jwp

Comment 14

[Daniel Holbert [:dholbert]]

(In reply to comment #12)
> Hang --> Crash:
> ===============
> Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b2pre) Gecko/2007111104
> Minefield/3.0b2pre
> Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b2pre) Gecko/2007111204
> Minefield/3.0b2pre
> Bonsai link: http://tinyurl.com/8rgst4 -- possibly bug 402427?

Turns out the hang-to-crash change was caused by bug 397961's checkin.  (Verified by backing its patch out of a trunk-checkout from just after the behavior change.)

Comment 15

[Samuel Sidler (old account; do not CC)]

Not wanted on 1.8.1 given the regression range and bugs involved.

Comment 16

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

In testcase 5, the problem seems to be the frame reconstruction that happens when we apply the binding on click.

The frame tree looks correct before the click. But afterwards we have
              Block(div)(0)@0x2178fed4 {0,0,59520,1152} [state=00101010] sc=0x2178dea0(i=1,b=0)<
                line 0x11ecdc4: count=3 state=inline,dirty,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0xc141] {58346,0,1174,1152} ca={0,0,59520,1152} <
                  Inline(span)(-1)@0x2178ff50 next=0x11eceb8 next-continuation=0x11eceb8 {59040,96,480,960} [state=00a00400] [content=0x8caf4c0] [sc=0x2178f7e4]<
                    Text(0)@0x11ecc10[0,1,F]  next=0x11eccfc next-continuation=0x11ece74 {0,0,480,960} [state=d0220000] [content=0x8caf510] sc=0x2178ff88 pst=:-moz-non-element<
                      "x"
                    >
                    Inline(span)(0)@0x11eccfc {0,0,0,0} [state=00000402] [content=0x8cb6650] [sc=0x11ecc74]<
                      Text(0)@0x11ecd84[0,1,T]  {0,0,0,0} [state=00000402] [content=0x8cb66a0] sc=0x11ecd34 pst=:-moz-non-element<
                        "s"
                      >
                    >
                  >
                  Inline(span)(-1)@0x11eceb8 next=0x11ecef0 prev-continuation=0x2178ff50 next-continuation=0x11ecef0 {58720,96,320,960} [state=00200000] [content=0x8caf4c0] [sc=0x2178f7e4]<
                    Text(0)@0x11ece74[1,1,T]  prev-continuation=0x11ecc10 {0,0,320,960} [state=d0020000] [content=0x8caf510] sc=0x2178ff88 pst=:-moz-non-element<
                      "!"
                    >
                  >
                  Inline(span)(-1)@0x11ecef0 prev-continuation=0x11eceb8 {58346,96,374,960} [state=00600400] [content=0x8caf4c0] [sc=0x2178f7e4]<>
                >
              >
            >
which looks bad. In particular, "x"s next-continuation is "!" but somehow the "s" got jammed in the middle. Then I suspect bidi resolution gets all confused and sets up the wrong offsets which causes reflow to explode.

The "s" shouldn't even be in the frame tree anymore, if I read this XBL right.

Comment 17

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

Attachment: testcase 6

Slightly simplified version of testcase 5; we don't need the inner XBL binding, all we need to trigger the bug is frame reconstruction of the span "s".

Comment 18

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

OK, so GetInsertionPoint is returning the wrong frame. It's returning the first continuation of the span (0x2178ff50 above), not the last continuation.

Comment 19

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

No, GetInsertionPoint is OK, the problem is that ContentInserted is not skipping all the way to the last continuation of parentFrame when it decides to append (because we have no specified prevSibling or nextSibling). ContentAppended has code for this, we just need to use that in ContentInserted too.

Comment 20

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

Attachment: visual test

This testcase shows the bug visually, without crashing or asserting and without involving RTL. The "s" should be after the "b", but it's rendered before the "b".

Comment 21

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

Attachment: fix

Comment 22

[Boris Zbarsky [:bzbarsky]]

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

Looks good.

Comment 23

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

Pushed http://hg.mozilla.org/mozilla-central/rev/7af1fe6b7ab4

I pushed the reftest. The crashtests should be added when this bug gets decloaked.

Comment 24

[Samuel Sidler (old account; do not CC)]

roc: Can you get this landed on 1.9.1 soon so it can bake and we can get a 1.9.0 patch worked up and ready?

Comment 25

[Daniel Holbert [:dholbert]]

Patch pushed to 1.9.1 on roc's behalf:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3b8c370b2be9

Comment 26

[Samuel Sidler (old account; do not CC)]

roc: Can you work up a 1.9.0 patch for this blocking bug?

Comment 27

[Daniel Holbert [:dholbert]]

Attachment: fix, as applied to 1.9.0.x branch

This bug's patch (attachment 357266 [details] [diff] [review]) applies cleanly to 1.9.0.x branch, aside from some contextual differences in reftest.list and crashtests.list.

I'm attaching a 1.9.0.x-specific version here, with the *.list conflicts resolved.  (I'll request approval1.9.0.x? after I've verified that this fixes the bug on 1.9.0.x.)

Comment 28

[Daniel Holbert [:dholbert]]

Comment on attachment 360094 [details] [diff] [review]
fix, as applied to 1.9.0.x branch

I just confirmed that this patch fixes the bug on 1.9.0.x.  (I tried 'visual test' as well as a number of the crashing testcases, and they're broken before-patching but fixed after.)

Requesting approval for landing it there.

Comment 29

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

Thanks Daniel!

Comment 30

[Daniel Veditz [:dveditz]]

Comment on attachment 360094 [details] [diff] [review]
fix, as applied to 1.9.0.x branch

Approved for 1.9.0.7, a=dveditz for release-drivers.

Comment 31

[Daniel Holbert [:dholbert]]

Committed to CVS trunk.  I left out the crashtests, per comment 23.

Checking in layout/base/nsCSSFrameConstructor.cpp;
/cvsroot/mozilla/layout/base/nsCSSFrameConstructor.cpp,v  <--  nsCSSFrameConstructor.cpp
new revision: 1.1477; previous revision: 1.1476
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/471594-1-ref.html,v
done
Checking in layout/reftests/bugs/471594-1-ref.html;
/cvsroot/mozilla/layout/reftests/bugs/471594-1-ref.html,v  <--  471594-1-ref.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/471594-1.xhtml,v
done
Checking in layout/reftests/bugs/471594-1.xhtml;
/cvsroot/mozilla/layout/reftests/bugs/471594-1.xhtml,v  <--  471594-1.xhtml
initial revision: 1.1
done
Checking in layout/reftests/bugs/reftest.list;
/cvsroot/mozilla/layout/reftests/bugs/reftest.list,v  <--  reftest.list
new revision: 1.469; previous revision: 1.468
done

Comment 32

[Al Billings [:abillings - ex-MoCo]]

Tomcat, another debug based testcase for verification for 1.9.0.7. This seems to be the day for them.

Comment 33

[Carsten Book [:Tomcat]]

(In reply to comment #32)
> Tomcat, another debug based testcase for verification for 1.9.0.7. This seems
> to be the day for them.

done :)

verified 1.9.0.7 using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.7pre) Gecko/2009021407 Firefox/3.0.7pre (DEBUG BUILD) - no assertion on the testcases

Comment 34

[Marc Bejarano]

do the crashtests still need to be committed?

Comment 35

[Martijn Wargers (dead)]

No, afaict. This bug has the in-testsuite+ flag, so the crashtests are committed.

Comment 36

[Jesse Ruderman]

I interpreted comment 23 as meaning crashtests still need to be added.

Comment 37

[Boris Zbarsky [:bzbarsky]]

That's correct.  The crashtests still need adding, I think.

Comment 38

[Marc Bejarano]

roc: are you going to commit the  crashtests?

Comment 39

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

commited the test: http://hg.mozilla.org/mozilla-central/rev/95d2b19be8f7

Comment 40

[Aakash Desai [:aakashd]]

verified FIXED (no crashes or assertions) on debug builds:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090608 Shiretoko/3.5pre ID:20090608122057

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090608 Minefield/3.6a1pre ID:20090608122028