310267 - crash [@ nsBidiPresUtils::Resolve] [@ nsCachedStyleData::GetStyleData]

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Filing as security-sensitive because I believe the crash is exploitable. 
(Before I reduced the testcase, I got similar stack traces with large non-zero
numbers on top instead of zero on top.)

Comment 1

[Jesse Ruderman]

Attachment: reduced testcase

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20050926
Firefox/1.6a1

Crashes within a second of loading the testcase.

Comment 2

[Jesse Ruderman]

Attachment: apple crash report with stack trace

Comment 3

[Asa Dotzler [:asa]]

If you come up with a very safe fix in the next couple of days, please request
approval for the patch and we'll evaluate.

Comment 4

[Simon Montagu :smontagu]

TB10975241E

Comment 5

[Simon Montagu :smontagu]

In a debug build I get:

###!!! ASSERTION: frame not in line: 'line->Contains(aDeletedFrame)', file c:/ho
me/simon/projects/mozwork/debugtree/mozilla/layout/generic/nsBlockFrame.cpp, lin
e 5681

followed by: 

First-chance exception in seamonkey.exe (GKLAYOUT.DLL): 0xC0000005: Access Violation.

with this at the top of the stack:
nsCachedStyleData::GetStyleData(const nsStyleStructID & eStyleStruct_Display) line 210 + 3 bytes
nsStyleContext::GetStyleData(nsStyleStructID eStyleStruct_Display) line 248 + 15 bytes
nsIFrame::GetStyleData(nsStyleStructID eStyleStruct_Display) line 612
nsIFrame::GetStyleDisplay() line 90 + 17 bytes
nsBidiPresUtils::InitLogicalArray(nsPresContext * 0x0490e170, nsIFrame * 0x04994a10, nsIFrame * 0x00000000, int 0x00000001) line 375 + 8 bytes
nsBidiPresUtils::Resolve(nsPresContext * 0x0490e170, nsIFrame * 0x049923ac, nsIFrame * 0x04994a10, int & 0x00000000, int 0x00000000) line 197 + 23 bytes
nsBlockFrame::Reflow(nsBlockFrame * const 0x049923ac, nsPresContext * 0x0490e170, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, unsigned int & 0x00000000) line 794 + 66 bytes

and at nsIFrame::GetStyleDisplay(), the frame's members are all 0xdddddddd

Comment 6

[Boris Zbarsky [:bzbarsky]]

That first assert is key, I think.  We're hitting it when we're removing an nsTextFrame.  The text frame has a nextBidi, so we remove that first (in nsFrameManager::RemoveFrame).  We hit the assert while removing said nextBidi.  I have no idea what that code is really doing, so Simon or Uri, could you check out what's happening there?

Comment 7

[Uri Bernstein (Google)]

As much as I'd like to help, I don't really know this code. But I'll try to have a look tomorrow anyway (unless Simon figures it out by then).

Comment 8

[Simon Montagu :smontagu]

Thanks for the analysis, Boris. The situation is actually a little more complicated: the original frame acquires a nextBidi in Bidi resolution, and then the nextBidi acquires an nsContinuingTextFrame in ReflowLine (probably because of the  newline character in the text). This third frame is the one we get the assert on and which crashes later.

I think the way to fix this is to make nsTextManager::RemoveFrame treat an nsContinuingTextFrame of a nextBidi the same way it treats a nextBidi, and I can see two ways to achieve this. Patches to follow.

Do we still think this is exploitable?

Comment 9

[Simon Montagu :smontagu]

Attachment: alternative patch #1: extend the condition in nsFrameManager::RemoveFrame

Comment 10

[Simon Montagu :smontagu]

Attachment: alternative patch #2: make the continuation of a nextBidi a nextBidi

Comment 11

[Simon Montagu :smontagu]

This could do with testing with variations on the original them, but a local copy of the testcase doesn't crash for me.

Comment 12

[Boris Zbarsky [:bzbarsky]]

So why does this actually break as things stand?  Is the nsContinuingTextFrame for the nextBidi on a line before the nextBidi itself?

Comment 13

[Simon Montagu :smontagu]

No, it's 2 lines after it. Changing http://lxr.mozilla.org/seamonkey/source/layout/generic/nsBlockFrame.cpp#5774 to loop until |line->Contains(deletedNextInFlow)| is true prevents the assertion, but it still crashes (in a different place) because the next time through the loop aDeletedFrame is not the nextSibling of prevSibling.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Hmm.  Do we have any documentation around that actually describes the frame structures bidi creates?

Comment 15

[Simon Montagu :smontagu]

Not really, no. I'm willing to write some, especially if you can point me to some documentation about the frame structures that regular reflow creates that I can use as a starting point.

Comment 16

[Boris Zbarsky [:bzbarsky]]

I think the closest we have to that is http://wiki.mozilla.org/Gecko:Continuation_Model

Comment 17

[Simon Montagu :smontagu]

OK, suppose I add a bullet to that page after "nsBlockFrame creates multiple inline children...", something like:

When an inline frame contains bidirectional text, nsBlockFrame also creates multiple inline frames associated with the same inline element, so that each frame contains a run of text with the same directionality. In this case, all of the frames associated with the inline element will also have the status bit NS_FRAME_IS_BIDI set, and the frames will be linked via the nextBidi property.

Comment 18

[Boris Zbarsky [:bzbarsky]]

OK... And how does that interact with the flow?  That is, it appears that with bidi an immediate next-in-flow may be two lines down from where we are.  That can't happen without bidi, right?  So how does that happen?

Comment 19

[Uri Bernstein (Google)]

(In reply to comment #17)
> When an inline frame contains bidirectional text, nsBlockFrame also creates
> multiple inline frames associated with the same inline element,

Shouldn't that more accurately say "text frames", rather than "inline frames"? AFAIK only text frames are currently handled this way (and bug 299065 is about doing this for inline frames as well).

Comment 20

[Simon Montagu :smontagu]

I will need to do more debugging to answer comment 18, i.e. it won't happen before tomorrow morning. In principle Bidi should not have any effect on the line breaking.

Comment 21

[Simon Montagu :smontagu]

It seems that the root of the bug is not the deletion but the insertion. In the non-bidi case the <div> is being inserted after the nextInFlow of a text frame, but in the bidi case (where the text frame has a nextBidi) it is being inserted after the nextBidi and before the nextInFlow, forcing the nextInFlow down by one line.

Comment 22

[Simon Montagu :smontagu]

Attachment: alternative patch #3: look for nextBidis when inserting frames

Maybe this should be rolled into GetLastInFlow() instead?

Comment 23

[Boris Zbarsky [:bzbarsky]]

So before the insertion happens, what does the frame model look like?  We have various places that walk the in-flow chain, get last-in-flows, etc, and it sounds to me like a lot of them need to become bidi-aware either implicitly (change GetLastInFlow()) or explicitly.  Just changing GetLastInFlow() is not so great, since we probably have code that depends on this being the same result as just walking the flow chain...

Comment 24

[Simon Montagu :smontagu]

I'm also unsure whether GetNextInFlow() should return a nextBidi if there is a nextBidi and no nextInFlow.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Which brings me back to my question.  Could we please, please document what sort of frame structures bidi creates?  Right now I can't usefully think about this problem, because I don't know what I'm working with.

Comment 26

[Simon Montagu :smontagu]

ipso facto I'm clearly the wrong person to do that. Uri, are you interested?

Comment 27

[Uri Bernstein (Google)]

(In reply to comment #26)
> ipso facto I'm clearly the wrong person to do that. Uri, are you interested?

I was actually looking forward to *reading* this documentation when it's ready, so that I'll finally understand what's going on...

I can perhaps try reading the code and writing what I can make of it (asking you lots of questions in the process). Still, I would think that it would be better for someone who already knows the system to be the one documenting it. I'm not sure what factum you're basing your assertion upon.

Comment 28

[Daniel Veditz [:dveditz]]

Could not reproduce the crash in 1.0.7 fwiw

Comment 29

[Uri Bernstein (Google)]

"alternative patch #1" will be effectively incorporated into my patch for bug 299065, if and when that patch would reach a complete and stable state.

Getting the "nextBidi" property is replaced by a call to aOldFrame->GetNextContinuation() (which returns both bidi continuations and NiFs).

Comment 30

[Uri Bernstein (Google)]

Actually, the latest version of my patch for bug 299065 also effectively implements "alternative solution #3" (by using GetLastContinuation rather than GetLastInFlow).

...which leavs me confused as to what nsFrameManager::RemoveFrame() and nsFrameManager::InsertFrames() are really expected to do with next-in-flows and bidi continuations.

Comment 31

[Uri Bernstein (Google)]

This is fixed by the fix for bug 299065.

Comment 32

[Uri Bernstein (Google)]

I think we should consider taking one of these patches for the 1.8 branch.

In addition to fixing this crash, there was a report on the mozilla.org.il forum about RTL text being inverted in several situations (specifically, when using "highlight all" in the search bar in Firefox, where the searched text appears in a bidi wrapping line). These problems are fixed by the patch for bug 299065, but I also verified they are fixed by alternative patch #3 (I didn't check the others), so that might be another reason to take it for 1.8.

Comment 33

[Uri Bernstein (Google)]

Comment on attachment 200876 [details] [diff] [review]
alternative patch #3: look for nextBidis when inserting frames

I'm taking the liberty of r+'ing Simon's patch #3 (for the branch), and requesting sr and approval form ROC.

Comment 34

[Simon Montagu :smontagu]

Checked in to MOZILLA_1_8_BRANCH

Comment 35

[Bob Clary [:bc] (inactive)]

https://bugzilla.mozilla.org/attachment.cgi?id=197660
ff2b2 winxp, linux no crash

verified fixed 1.8

Comment 36

[Daniel Veditz [:dveditz]]

Comment on attachment 200876 [details] [diff] [review]
alternative patch #3: look for nextBidis when inserting frames

approved for 1.8.0 branch, a=dveditz for drivers

Comment 37

[Simon Montagu :smontagu]

Checked in to MOZILLA_1_8_0_BRANCH

Comment 38

[Jay Patel [:jay]]

v.fixed on 1.8.0 branch with Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.8pre) Gecko/20061020 Firefox/1.5.0.8pre, no crash with reduced testcase.

Comment 39

[Mike Hommey [:glandium]]

Attachment: Patch for 1.7

Here is an adapted patch for the 1.7 branch. Could you confirm that it is okay ? I'm unable to reproduce the crash on debian with both 1.7 and 1.8 branch with the pango or the xft backend, so I'm kind of blind here...

Comment 40

[Uri Bernstein (Google)]

Comment on attachment 245404 [details] [diff] [review]
Patch for 1.7

Rubber-stamping, since this is essentially the same as the 1.8 patch I reviewed. I don't have the means to test this in 1.7.

Comment 41

[Jesse Ruderman]

Crashtest checked in.