Closed Bug 395623 (CVE-2008-5501) Opened 17 years ago Closed 17 years ago

[FIX]"ASSERTION: not in child list" and crash with position:inherit, counters, :first-line


(Core :: Layout, defect, P2)






(Reporter: jruderman, Assigned: bzbarsky)



(Keywords: assertion, crash, testcase, Whiteboard: [sg:critical?][dbaron-1.9:Rs])


(2 files, 1 obsolete file)

Attached file testcase
Loading the testcase triggers three assertions:

###!!! ASSERTION: not in child list: 'found', file /Users/jruderman/trunk/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 1767

###!!! ASSERTION: didn't find frame to delete: 'result', file /Users/jruderman/trunk/mozilla/layout/generic/nsAbsoluteContainingBlock.cpp, line 128

###!!! ASSERTION: no placeholder frame for out-of-flow frame: 'Not Reached', file /Users/jruderman/trunk/mozilla/layout/generic/nsFrame.cpp, line 5447

If I try to resize the Firefox window at this point, it often crashes 
[@ nsSpaceManager::CreateFrameInfo] [@ nsIFrame::GetStyleDisplay] dereferencing 0xddddddf5.  This happens most reliably if I launch Firefox with the testcase filename on the command line.
Flags: blocking1.9?
Whiteboard: [sg:critical?]
OK. This is the first-line chicken coming home to roost.

The problem is that we reparent the body's style context to the first-line, which means that since its position is "inherit" it would suddenly change position.... except that ReParentStyleContext assumes that no restyling will be needed as a result of the reparent.  To make a long story short, we end up with the body having a POSITION_ABSOLUTE and FLOAT_NONE, but it's on the float list.  Which is just all bad.
I tried hacking around this by making the first-line rule set position/float to inherit, but then the first-line ends up display:block, which causes no end of grief (blockframe tries to reflow it as a block, reflow state tries to treat it as a block, etc).

I really don't see a sane way to fix this in our arch.  :(  I would strongly suggest that we just back out first-line support until we can do it without security holes.  If ever.
I'd be fine with removing first-line support.  It's not worth the memory safety bugs (looks like we've had 5 others involving first-line before this one), and the use cases are less clear than for first-letter.
I'd be fine with either:

 * removing :first-line, or

 * switching to the model in early drafts of css3-selectors, which the working group agreed was better, but which then wasn't implemented, where the :first-line and :first-letter pseudo-elements end up innermost (and potentially repeated if there are multiple elements in the line).
Blocks: 388529
Attached patch Another option (obsolete) — Splinter Review
This basically implements hixie's proposal: reset props do not inherit from ::first-line.  The ReParentStyleContext changes are nice, but not strictly necessary.

The tests include tests for this bug, bug 380012, bug 367489, bug 322348, bug 360599, bug 350370, bug 364220, bug 390417, bug 368568, bug 368863, and bug 343293.
Attachment #281415 - Flags: superreview?(dbaron)
Attachment #281415 - Flags: review?(dbaron)
Attached patch Merged to tipSplinter Review
Attachment #281415 - Attachment is obsolete: true
Attachment #283305 - Flags: superreview?(dbaron)
Attachment #283305 - Flags: review?(dbaron)
Attachment #281415 - Flags: superreview?(dbaron)
Attachment #281415 - Flags: review?(dbaron)
Flags: blocking1.9? → blocking1.9+
Blocks: 398803
Whiteboard: [sg:critical?] → [sg:critical?][dbaron-1.9:Rs]
Comment on attachment 283305 [details] [diff] [review]
Merged to tip


But you might want to comment a little more about the idea that ReParentStyleContext shouldn't need change hints (which was always my understanding, anyway), since it should never produce anything requiring frame-change (though we should enforce that better) and whenever it's used we're already going to reflow and repaint.
Attachment #283305 - Flags: superreview?(dbaron)
Attachment #283305 - Flags: superreview+
Attachment #283305 - Flags: review?(dbaron)
Attachment #283305 - Flags: review+
Comment on attachment 283305 [details] [diff] [review]
Merged to tip

Requesting approval.  It would be great to get testing for this in beta, and to land bug 398803 too.
Attachment #283305 - Flags: approval1.9?
Boris, is this essential for M9?  Not sure what the risks/upsides are.
It's probably not essential except insofar as we would like to get testing of the changes sooner than later.

The risk is that rendering of first-line will break somehow or that we'll introduce crashes.

The upside is that this fixes exploitable crashes.

At this stage in the game, I'd probably put this off for M10.
Assignee: nobody → bzbarsky
Summary: "ASSERTION: not in child list" and crash with position:inherit, counters, :first-line → [FIX]"ASSERTION: not in child list" and crash with position:inherit, counters, :first-line
Target Milestone: --- → mozilla1.9 M10
Comment on attachment 283305 [details] [diff] [review]
Merged to tip

a=drivers for after M9 freeze
Attachment #283305 - Flags: approval1.9? → approval1.9+
Checked in, with the documentation changes dbaron suggested in comment 7.
Closed: 17 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Verified fixed with current debug trunk build.
Fwiw, it also doesn't seem to crash on branch.
Group: security
Flags: wanted1.8.1.x-
Alias: CVE-2008-5501
You need to log in before you can comment on or make changes to this bug.