Crash [@ IsPercentageAware] with first-letter float and direction: rtl

RESOLVED FIXED in mozilla1.9.2



8 years ago
6 years ago


(Reporter: Martijn Wargers (dead), Assigned: tnikkel)


(5 keywords)

Windows XP
crash, regression, testcase, verified1.9.0.16, verified1.9.1
Dependency tree / graph
Bug Flags:
wanted1.9.0.x +
in-testsuite +

Firefox Tracking Flags

(status1.9.2 beta2-fixed, status1.9.1 .6-fixed)


(Whiteboard: [sg:dos] null deref; blocks further testing, crash signature)


(4 attachments)



8 years ago
Created attachment 375849 [details]

See testcase, which crashes current trunk build after 100ms.
It doesn't crash in a 2009-05-01 build, it does crash in a 2009-05-02 build:
No idea what could have caused it.
0  	xul.dll  	IsPercentageAware  	 layout/generic/nsLineLayout.cpp:672
1 	xul.dll 	xul.dll@0x8fbb0b
My, which is the last changeset before that range, looks like a likely candidate. Can you check in about:buildconfig whether the build that doesn't crash includes that changeset?
Reverting did not remove the crash.

I get a series of assertions:

###!!! ASSERTION: prev sibling not in line list: 'Not Reached', file ...layout/generic/nsBlockFrame.cpp, line 4913

###!!! ASSERTION: unknown out of flow frame type: 'disp->mDisplay == NS_STYLE_DISPLAY_POPUP', file ...layout/generic/nsHTMLReflowState.cpp, line 518

###!!! 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 6107

###!!! ASSERTION: unknown out of flow frame type: 'disp->mDisplay == NS_STYLE_DISPLAY_POPUP', file ...layout/generic/nsHTMLReflowState.cpp, line 518

###!!! ASSERTION: shouldn't use unconstrained widths anymore: '(mFrameType == NS_CSS_FRAME_TYPE_INLINE && !frame->IsFrameOfType(nsIFrame::eReplaced)) || frame->GetType() == nsGkAtoms::textFrame || mComputedWidth != NS_UNCONSTRAINEDSIZE', file ...layout/generic/nsHTMLReflowState.cpp, line 287

###!!! ASSERTION: null frame is not allowed: 'aFrame', file ...layout/generic/nsLineLayout.cpp, line 670
I am seeing a different regression range: 2009-04-22-03 - 2009-04-23-03. Reverting the bidi-related checkin in that range, from bug 489517, prevents the crash with this testcase, but it still happens when changing direction manually, so this looks like an older bug that bug 489517 is exposing.

Comment 4

8 years ago
Ok, I guess my regression range didn't make sense then.
I found the regression range for the older bug, and it turns out to be regression from bug 332655 (which is actually what I suspected in the first place).
Assignee: nobody → smontagu
Blocks: 332655


8 years ago
Duplicate of this bug: 502560
Blocks: 502560

Comment 7

8 years ago
This bug interferes with fuzzing by triggering a bunch of assertions before crashing, sometimes including the dreaded "StealFrame failure" assertion.  As a result, I can't look for other bugs that trigger these assertions.
!exploitable output with the current 3.5.2 debug nightly 

(aec.9bc): Access violation - code c0000005 (!!! second chance !!!)
eax=00000000 ebx=7ffdb000 ecx=00000000 edx=00000026 esi=0012ee2c edi=0012ee34
eip=01ac6952 esp=0012e034 ebp=0012e058 iopl=0         nv up ei pl zr na pe nc
cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000             efl=00000246
01ac6952 8b11            mov     edx,dword ptr [ecx]  ds:0023:00000000=????????
0:000> cdb: Reading initial command '!load winext\msec.dll;.logappend;!exploitable;k;q'

Exploitability Classification: PROBABLY_EXPLOITABLE
Recommended Bug Title: Probably Exploitable - Data from Faulting Address controls Code Flow starting at gklayout!IsPercentageAware+0x0000000000000032 (Hash=0x1d0e112e.0x416e3159)

The data from the faulting address is later used as the target for a branch.
ChildEBP RetAddr
0012e058 01ac59c1 gklayout!IsPercentageAware+0x32
0012e1e0 01aba585 gklayout!nsLineLayout::ReflowFrame+0x81
0012e3c0 01ac5ceb gklayout!nsFirstLetterFrame::Reflow+0x235
0012e558 01a854fe gklayout!nsLineLayout::ReflowFrame+0x3ab
0012e600 01a84f10 gklayout!nsBlockFrame::ReflowInlineFrame+0x5e
0012e6a0 01a84ad2 gklayout!nsBlockFrame::DoReflowInlineFrames+0x210
0012e79c 01a82842 gklayout!nsBlockFrame::ReflowInlineFrames+0xf2
0012e89c 01a81491 gklayout!nsBlockFrame::ReflowLine+0x2c2
0012ea5c 01a7ed71 gklayout!nsBlockFrame::ReflowDirtyLines+0x561
0012ee2c 01a56ec2 gklayout!nsBlockFrame::Reflow+0x251
0012f02c 01a5627e gklayout!nsFrame::BoxReflow+0x522
0012f110 01a56860 gklayout!nsFrame::RefreshSizeCache+0x11e
0012f128 01bb3824 gklayout!nsFrame::GetBoxAscent+0x60
0012f158 01bc6972 gklayout!nsSprocketLayout::GetAscent+0x44
0012f170 01bb134b gklayout!nsBoxFrame::GetBoxAscent+0x72
0012f308 01bc6d55 gklayout!nsSprocketLayout::Layout+0xeb
0012f328 01bb499b gklayout!nsBoxFrame::DoLayout+0x55
0012f34c 01bb612d gklayout!nsIFrame::Layout+0x6b
0012f41c 01bc6d55 gklayout!nsStackLayout::Layout+0x17d
0012f43c 01bb499b gklayout!nsBoxFrame::DoLayout+0x55
Group: core-security

Comment 9

8 years ago
I actually see this signature come in from users sometime.  yesterday was a big day.

IsPercentageAware Firefox	3.0.13	2009073022	1.9	Windows NT	5.1.2600 Service Pack 3	x86	

IsPercentageAware  Firefox	3.5.2	20090729225027	1.9.1	Windows NT	5.1.2600 Service Pack 2	x86	

IsPercentageAware Firefox	3.5.2	 Windows NT	5.1.2600 Szervizcsomag 

IsPercentageAware Firefox	3.5.2	

IsPercentageAware	about:blank

IsPercentageAware	about:blank



IsPercentageAware	about:blank
Comment 8 shows a null deref DoS, consistent with the last assertion in comment 2. But this is blocking further fuzz testing of this area so that adds more importance to getting this fixed.
Whiteboard: [sg:dos] null deref; blocks further testing
See bug 460389 comment 19. Note also that not all the crashes with this signature are this bug. IsPercentageAware is just the first place where nsLineLayout::ReflowFrame will crash when called with null aFrame.
Depends on: 460389
Created attachment 406904 [details] [diff] [review]
Wallpaper patch

This prevents the crash, but leaves a bundle of assertions, so I'm not sure if it adds enough value to be worth pursuing.
Attachment #406904 - Flags: superreview?(roc)
Attachment #406904 - Flags: review?(roc)
First-letter frames definitely should have children. Is this just the general "first-letter and bidi reordering don't work together"? What are we going to do about this problem? I wonder if bidi-reordering could just ignore first-letter frames?

Comment 14

8 years ago
When ltr the first letter frame should contain "&m&", but when rtl it should contain just "&" (correct me if I'm wrong, I don't know how bidi works, just basing this on how a static rendering works). When we dynamically change the direction that triggers a reflow, and when we ResolveBidi we create a non-fluid continuation of the first letter frame breaking between "&" and "m&". Leaving us with two first letter frames (NS_NewFirstLetterFrame is a good place to set a breakpoint to follow this). When reflowing that continuation the block frame creates a continuation (not totally sure why) of that first letter frame, and it stays empty and that is where we crash.

I don't think we should be creating continuations of first letter frames at all (?). So either we can teach Bidi resolution about first letter frames, or let the frame constructor deal with it by causing a reconstruct on containing blocks that have first letter style and "bidi stuff" (direction on the block changes, but I also imagine maybe changing some character data could do the same by mixing ltr and rtl characters?) changes on the block or its contents.

Comment 15

8 years ago
Created attachment 408559 [details] [diff] [review]

Create an ugly special case in bidi resolution for floating first letter frames.

This also fixes bug 429865, bug 436982, bug 460389, and bug 477731.

I discovered two other bugs while fixing this. The overflow rect of the the child of a floating first letter frame doesn't get updated properly, likely due to the half-init'ed nsLineLayout that is used to reflow it. You can see this by converting the testcase to html. Then changing the direction will create a horizontal scrollbar to handle the overflow.

The other is that if you replace the "&m&m" in the testcase with a mixture of RTL and LTR characters, they won't get interleaved correctly when switching the direction from rtl to ltr and back.


8 years ago
Attachment #408559 - Flags: review?(roc)

Comment 16

8 years ago
Comment on attachment 408559 [details] [diff] [review]

Not sure who needs/wants to look at this. Please adjust as necessary.
Attachment #408559 - Flags: review?(smontagu)
Attachment #408559 - Flags: review?(roc) → review+
Comment on attachment 408559 [details] [diff] [review]

Looks good to me! Simon should definitely look at it too. Thanks!!!
Attachment #408559 - Flags: review?(smontagu) → review+
(In reply to comment #15)

> The other is that if you replace the "&m&m" in the testcase with a mixture of
> RTL and LTR characters, they won't get interleaved correctly when switching the
> direction from rtl to ltr and back.

That sounds bad. What does the frame tree look like in this case?

Comment 19

8 years ago
Created attachment 409000 [details]

A quick look at the frame tree looks okay. But here is a testcase so you can have a look. After some investigation changing the direction dynamically wasn't needed. I think the "bc" get incorrectly placed with the "ef".
Assignee: smontagu → tnikkel
That looks as if the embedding levels of the frames aren't getting set properly.


8 years ago
No longer depends on: 460389

Comment 21

8 years ago
Last Resolved: 8 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1

Comment 22

8 years ago
All branches are affected.
blocking1.9.1: --- → ?
Flags: wanted1.9.2?
Flags: blocking1.9.0.16?


8 years ago
Attachment #408559 - Flags: approval1.9.2?
Attachment #408559 - Flags: approval1.9.1.5?
Attachment #408559 - Flags: approval1.9.0.16?
I have a fix for attachment 409000 [details]: the embedding levels are set properly, but nsBidiPresUtils::GetFrameEmbeddingLevel is checking the embedding level of the placeholder instead of the placeholdee.
Opened bug 525740 for the reordering bug.
Attachment #406904 - Flags: superreview?(roc)
Attachment #406904 - Flags: review?(roc)
Attachment #408559 - Flags: approval1.9.2? → approval1.9.2+
Not blocking, but we'll look at the patch when we get to approvals.
blocking1.9.1: ? → ---
status1.9.1: --- → wanted
Flags: blocking1.9.0.16? → wanted1.9.0.x+

Comment 26

8 years ago

And the tests from the public bugs
status1.9.2: --- → final-fixed
Flags: wanted1.9.2?
Target Milestone: mozilla1.9.3a1 → mozilla1.9.2
Group: core-security
Attachment #408559 - Flags: approval1.9.1.6?
Attachment #408559 - Flags: approval1.9.1.6+
Attachment #408559 - Flags: approval1.9.0.16?
Attachment #408559 - Flags: approval1.9.0.16+
Comment on attachment 408559 [details] [diff] [review]

Approved for and, a=dveditz for release-drivers

Comment 28

8 years ago
I might be more conservative about making this bug public, although on 1.9.1+ we crash with a null deref, we are just getting lucky, the frame tree is messed up. On 1.9.0 there's even more room for things to go wrong as you have to flip the direction back and forth a few times before it crashes, so there is plenty of time to mess with a broken frame tree.

Comment 29

8 years ago
I forgot to paste this above: a push for a bustage fix

Comment 30

8 years ago

And the tests from the public bugs
status1.9.1: wanted → .6-fixed
Checking in nsBidiPresUtils.cpp;
/cvsroot/mozilla/layout/base/nsBidiPresUtils.cpp,v  <--  nsBidiPresUtils.cpp
new revision: 1.117; previous revision: 1.116

Checking in layout/base/crashtests/503936-1.html;
/cvsroot/mozilla/layout/base/crashtests/503936-1.html,v  <--  503936-1.html
initial revision: 1.1
Checking in layout/base/crashtests/crashtests.list;
/cvsroot/mozilla/layout/base/crashtests/crashtests.list,v  <--  crashtests.list
new revision: 1.103; previous revision: 1.102
Keywords: fixed1.9.0.16
Er, that second part should have been:

Checking in layout/base/crashtests/429865-1.html;
/cvsroot/mozilla/layout/base/crashtests/429865-1.html,v  <--  429865-1.html
initial revision: 1.1
Checking in layout/base/crashtests/436982-1.html;
/cvsroot/mozilla/layout/base/crashtests/436982-1.html,v  <--  436982-1.html
initial revision: 1.1
Checking in layout/base/crashtests/477731-1.html;
/cvsroot/mozilla/layout/base/crashtests/477731-1.html,v  <--  477731-1.html
initial revision: 1.1
Checking in layout/base/crashtests/crashtests.list;
/cvsroot/mozilla/layout/base/crashtests/crashtests.list,v  <--  crashtests.list
new revision: 1.104; previous revision: 1.103
Verified crash with attached testcase in and lack of crash with (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/20091119 Shiretoko/3.5.6pre (.NET CLR 3.5.30729)) so this is fixed for 1.9.1.

On 1.9.0, the testcase does not cause a crash in and I see no difference in behavior between and .16. Does this really affect 1.9.0 directly or is this just defense in depth across all of the code lines?
Keywords: verified1.9.1

Comment 34

8 years ago
I have a testcase that crashes 1.9.0 but I would prefer not to post it in a public bug. I checked that 1.9.0 nightlies no longer crash on that testcase.
That's good enough for me, Timothy. I'm marking this verified for
Keywords: fixed1.9.0.16 → verified1.9.0.16
Timothy, please be sure to attach it to this bug after we ship a release with this fix in it. (Alternatively, send it to someone in the security-group who can attach it as a private attachment.)

Comment 37

8 years ago
Landed the testcase of this bug and the testcase mentioned in comment 34 on mozilla-central
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ IsPercentageAware]
You need to log in before you can comment on or make changes to this bug.