463763 - Crash [@ nsHTMLFramesetFrame::Reflow] with MathML msup, display:block

Status: VERIFIED FIXED

(Core :: MathML, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Filing as security-sensitive because of comments in older (fixed) bugs involving the same crash signature.

Comment 1

[Zack Weinberg (:zwol)]

Attachment: minimized test case

Here's a smaller and more readable test case.

This looks like another manifestation of the double-reflow problem that caused bug 416907, but I'm not sure how we managed to do it this time.  Still investigating.

Comment 2

[Zack Weinberg (:zwol)]

Oof, this is a nasty one.

The problem structure is <msup><mo><frameset/></mo></msup>; to trigger a crash you have to have display:block on the outer <msup>, but the bug doesn't have anything to do with that.

We hit nsHTMLFramesetFrame::Reflow for the first time with this stack trace...

  <msup>.nsMathMLContainerFrame::ReflowChild()
   <msup>.nsContainerFrame::ReflowChild()
    <mo>.nsMathMLmoFrame::Reflow()
     <mo>.nsMathMLTokenFrame::Reflow()
      <mo>.nsMathMLContainerFrame::ReflowChild()
       <mo>.nsContainerFrame::ReflowChild()
        <frameset>.nsHTMLFramesetFrame::Reflow()

It returns successfully, and so do the immediate two callers.  <mo>.nsMathMLTokenFrame::Reflow() then calls nsMathMLContainerFrame::FinalizeReflow(), which in turn calls nsMathMLTokenFrame::Place(), but not nsMathMLTokenFrame::Stretch(), because it 
needs to give the <msup> a chance to do that.  We keep returning, all the way to <msup>.nsMathMLContainerFrame::ReflowChild().  Nothing has called DidReflow() for the <frameset>.  At this point we are hosed, because nsMathMLmsupFrame::Place() will detect invalid markup and return, and the <mo> will get finalized, but the <frameset> won't.

Comment 3

[Zack Weinberg (:zwol)]

Attachment: fix for bug

After a great deal of staring at gdb I came to the conclusion that the simplest, safest fix for this bug was to have DidReflowChildren() recurse over *all* descendant frames of the problem frame.

The first included test case crashes FF3.0.3 and does not crash trunk after this patch.  The second test case does not crash Firefox 3.0.3, even though it uses some of the same code paths, but then, the actual crash here is easy to perturb out of existence.  So I'm including it anyway.

This patch should apply unmodified to 1.9.0.x, so I'm flagging it for approval to that branch; please let me know if it doesn't actually apply.

Comment 4

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

+      DidReflowChildren(grandchild, nsnull);

This should be frame->DidReflowChildren I guess. Or else make DidReflowChildren static.

Also, the DidReflowChildren should be inside the NS_FRAME_IN_REFLOW guard, right?

Comment 5

[Zack Weinberg (:zwol)]

Attachment: (rev 2) static DidReflowChildren

(In reply to comment #4)
> +      DidReflowChildren(grandchild, nsnull);
> 
> This should be frame->DidReflowChildren I guess. Or else make
> DidReflowChildren static.

I made it static.  'frame' doesn't necessarily point to a nsMathMLContainerFrame object.

> Also, the DidReflowChildren should be inside the NS_FRAME_IN_REFLOW guard,
> right?

That would miss a frame that has its NS_FRAME_IN_REFLOW bit set but is a descendant of a frame that doesn't have that bit set.  I'm not prepared to swear that that never happens.  (I imagine it's not *supposed* to happen, but this function's job is to clean up broken frame tree invariants, so it seems best to be as defensive as possible.)

Comment 6

[Jesse Ruderman]

If the crash is exploitable, it might be best to have the tests in a separate patch that only gets checked in once the bug is revealed.

Comment 7

[Zack Weinberg (:zwol)]

I have no idea whether it's exploitable.  The crash is a read dereference of a NULL pointer, and as far as I can tell, both pointer and offset are going to be zero when we get there with the bug triggered.

Comment 8

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

(In reply to comment #5)
> (In reply to comment #4)
> > Also, the DidReflowChildren should be inside the NS_FRAME_IN_REFLOW guard,
> > right?
> 
> That would miss a frame that has its NS_FRAME_IN_REFLOW bit set but is a
> descendant of a frame that doesn't have that bit set.  I'm not prepared to
> swear that that never happens.  (I imagine it's not *supposed* to happen, but
> this function's job is to clean up broken frame tree invariants, so it seems
> best to be as defensive as possible.)

I think we should ensure that never happens. We should add debug code in DidReflow that checks all children (in all child lists) and verifies that their NS_FRAME_IN_REFLOW has already been cleared, so if this invariant is violated we'll get clear assertions near the problem. If we find violations of that we can fix them separately.

That helps here because it means DidReflowChildren can't cause a real increase in the amount of work we do. The MathML code that defers DidReflow should not get us into situations where we've marked a frame as done with reflow but not its children.

Comment 9

[Zack Weinberg (:zwol)]

Attachment: (rev 2+1i) with sanity checking in nsFrame::DidReflow

So something like this?  But I don't think we want to do it now, because the assertion fires nearly twenty thousand times in the reftests, and 1300 times in the mochitests; most of these cases seem to have nothing to do with MathML.

Moving the recursion in DidReflowChildren into the NS_FRAME_IN_REFLOW conditional does not break the test cases for this bug again, though.  So we could maybe do that part now and the assertions after 3.1?

Comment 10

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

Checking for FRAME_IS_DIRTY or NS_FRAME_HAS_DIRTY_CHILDREN is not correct. There are reasons why we might exit reflow with dirty children --- in a few cases we rely on iterative reflow.

If there are still assertion failures for children which are still IN_REFLOW or FIRST_REFLOW, then yeah, let's just leave the extra assertions out for now and have DidReflowChildren recursion guarded by NS_FRAME_IN_REFLOW.

Comment 11

[Zack Weinberg (:zwol)]

Yeah, we still get the same number of assertion failures for IN_REFLOW|FIRST_REFLOW only.

I'll attach another iteration of the patch shortly, and file a bug for adding the assertions post-1.9.1.

Comment 12

[Zack Weinberg (:zwol)]

Attachment: (rev 3) only recurse when calling DidReflow

Hopefully final revision as discussed above.  I'll file a separate bug for the DidReflow() sanity checks.

Comment 13

[Daniel Veditz [:dveditz]]

On the 1.9.0 branch both testcases crash on a null dereference at
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsFrameSetFrame.cpp&rev=3.210&mark=1156#1154
with no sign of using freed objects. Not an exploitable crash, but the investigation seems to have revealed some ugliness we should fix in case there are other paths in that would yield more exploitable results.

I see the assertion "Someone didn't override Reflow or ComputeAutoSize: 'Not Reached'" from nsLeafFrame.cpp line 131 (twice) before the crash with both testcases.

Comment 14

[Zack Weinberg (:zwol)]

(In reply to comment #13)
> On the 1.9.0 branch both testcases crash on a null dereference at
> http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsFrameSetFrame.cpp&rev=3.210&mark=1156#1154

That's the same place it crashes on trunk.

> with no sign of using freed objects. Not an exploitable crash, but the
> investigation seems to have revealed some ugliness we should fix in case there
> are other paths in that would yield more exploitable results.

I don't know if that means you're saying this should get fixed on 1.9.0 branch or not.  Did you try the patch?

> I see the assertion "Someone didn't override Reflow or ComputeAutoSize: 'Not
> Reached'" from nsLeafFrame.cpp line 131 (twice) before the crash with both
> testcases.

I believe that to be harmless, but the patch in bug 416907 fixes it.  It'll appear any time <frameset/> is used inside MathML, whether or not in a context that provokes a crash.

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 347877 [details] [diff] [review]
(rev 3) only recurse when calling DidReflow

Moving to post beta 2 queue; should this be nominated for blocking final release?

Comment 16

[Zack Weinberg (:zwol)]

It's a crash, so I'm nominating it for blocking final.

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 347877 [details] [diff] [review]
(rev 3) only recurse when calling DidReflow

a191=beltzner

Comment 18

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/1695db512c46 for m-c and http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2407bc6d2773 for 1.9.1

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 347877 [details] [diff] [review]
(rev 3) only recurse when calling DidReflow

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

Comment 20

[Daniel Veditz [:dveditz]]

Fix checked into the 1.9.0 branch

Comment 21

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

Verified for 1.9.0.6 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.6pre) Gecko/2009011304 GranParadiso/3.0.6pre. It no longer crashes but reports "invalid markup". Good enough for me.

Comment 22

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

Verified fixed for 1.9.1 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090112 Shiretoko/3.1b3pre.

Comment 23

[Tony Chung [:tchung]]

sorry for the inconvenience.  i meant to detect any bugs before the branch merge that were still RESO fixed, but had a verified1.9.1 keyword next to them.

Comment 24

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090115 Minefield/3.2a1pre