Closed Bug 863935 Opened 9 years ago Closed 9 years ago

Use-after-poison in nsFrameList::UnhookFrameFromSiblings with moz-column


(Core :: Layout: Block and Inline, defect)

Not set



Tracking Status
firefox22 --- wontfix
firefox23 --- wontfix
firefox24 --- fixed
firefox-esr17 --- unaffected
b2g18 --- unaffected


(Reporter: jruderman, Assigned: mats)


(Blocks 1 open bug)


(5 keywords, Whiteboard: [adv-main24+])


(4 files)

Attached file testcase
Assertion failure: !GetPropTableFrames(aPresContext, aProperty), at layout/generic/nsContainerFrame.cpp:1459

Use-after-poison of pres arena memory in nsFrameList::UnhookFrameFromSiblings.

No symptoms.
Security-sensitive in case this is a long-standing bug revealed by bug 729519.
Assignee: nobody → matspal
Attached file frame dump #1
The check for when to forget about mOverflowContList is wrong.

Here's the stack / frame tree for the call to Finish() when we
shouldn't forget about it, because only the first two children
(ColumnSet frames) will be removed.

It seems we also update mSentry/mPrevOverflowCont wrong -
if I fix the above bug, then the resulting mSentry/mPrevOverflowCont
points to a frame (red) that is going to be destroyed.  I think mSentry
should point to the green block, and mPrevOverflowCont its prev-in-flow
(pink), but I'm not really sure...
I'm pretty sure this is mitigated by frame poisoning.
After nsOverflowContinuationTracker::Insert creates a new list and overwrites
the ExcessOverflowContainers property (the assertion in comment 0) there's
no way to access the old list anymore.  The crash is either going to be
accessing a destroyed frame through the bogus mSentry/mPrevOverflowCont
or when DeleteNextInFlowChild is trying to unhook one of the frames and finds
a next-sibling that is destroyed because we failed to remove it from the new
frame list earlier so it's still hooked up (the ASan error above).
OS: Mac OS X → All
Hardware: x86_64 → All
Attached file frame dump #2
BEFORE: mSentry=0x7f4fe5b03330 mPrevOverflowCont=(nil)
AFTER:  mSentry=0x7f4fe5b03af0 mPrevOverflowCont=(nil)

fantasai, does this makes sense to you for the given frame trees?
Attachment #740062 - Flags: feedback?(fantasai.bugs)
Attached patch fixSplinter Review
Call BeginFinish() before DeleteNextInFlowChild/StealFrame on the kid's
next-in-flow and EndFinish() after, with the help of the new AutoFinish

The old Finish() method tried to figure out whether the OC frame list
would become empty, and thus deleted.  This is too fragile and never
really worked.  Instead, compare the list pointer with mParent's 
OC lists after the kid's next-in-flows are gone, in EndFinish(),
and re-setup the tracker's list if it was deleted.

Break out the list setup code from the ctor to a new method,
SetupOverflowContList(), so that it can be used for that purpose.

In BeginFinish, only try to detect if mSentry/mPrevOverflowCont
are going stale and repair those later in EndFinish().
(to my surprise "f == mPrevOverflowCont" actually do occur, which seems
to contradict the documentation that "it's a known good point; this
pointer won't be deleted on us")

I'm assuming that if we bump into mSentry without seeing
mPrevOverflowCont then mPrevOverflowCont is still valid - does this
assumption hold?

It would be great if you could elaborate on what mPrevOverflowCont/
mSentry is used for, and the relation between them.  If one is null,
what does that mean? and what values can the other have in that case?
Attachment #740090 - Flags: review?(fantasai.bugs)
WFM on mozilla-central??
The first good revision is:
user:        Scott Johnson
date:        Wed Apr 24 10:02:36 2013 -0500
summary:     Bug 857324: Make column set reflow continue without balancing rather than restarting if computed height is exceeded. [r=mats]
Comment on attachment 740090 [details] [diff] [review]

I still think we should fix this.
Attachment #740090 - Flags: review?(fantasai.bugs) → review?(roc)
Comment on attachment 740090 [details] [diff] [review]

Review of attachment 740090 [details] [diff] [review]:

I'm rubber-stamping this.
Attachment #740090 - Flags: review?(roc) → review+
Blocks: 881090
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
(In reply to Jesse Ruderman from comment #1)
> Security-sensitive in case this is a long-standing bug revealed by bug
> 729519.

This means 22 and 23 are affected by this and would, ideally, have the fix ported to them, right?
Bug 729519 was fixed in 22 so this crash is mitigated by arena poisoning
from that version, so it's not necessary to uplift this IMO.
Same for 23?
Yes, 22 and all later versions.
Duplicate of this bug: 881090
Whiteboard: [adv-main24+]
Landed a crashtest:
Group: core-security
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.