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

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase

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

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

Nightly:
No symptoms.

Comment 1

[Jesse Ruderman]

Security-sensitive in case this is a long-standing bug revealed by bug 729519.

Comment 2

[Mats Palmgren (inactive)]

Attachment: frame dump #1

The check for when to forget about mOverflowContList is wrong.
http://hg.mozilla.org/mozilla-central/annotate/4420f27742c7/layout/generic/nsContainerFrame.cpp#l1770

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...

Comment 3

[Mats Palmgren (inactive)]

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).

Comment 4

[Mats Palmgren (inactive)]

Attachment: 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?

Comment 5

[Mats Palmgren (inactive)]

Attachment: fix

Call BeginFinish() before DeleteNextInFlowChild/StealFrame on the kid's
next-in-flow and EndFinish() after, with the help of the new AutoFinish
class.

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?

https://tbpl.mozilla.org/?tree=Try&rev=c3cebb6cf160

Comment 6

[Jesse Ruderman]

WFM on mozilla-central??

Comment 7

[Jesse Ruderman]

The first good revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/52127bafd50b
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 8

[Mats Palmgren (inactive)]

Comment on attachment 740090 [details] [diff] [review]
fix

I still think we should fix this.

Comment 9

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

Comment on attachment 740090 [details] [diff] [review]
fix

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

I'm rubber-stamping this.

Comment 10

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/48bbd10759f7

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/48bbd10759f7

Comment 12

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

(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?

Comment 13

[Mats Palmgren (inactive)]

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.

Comment 14

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

Same for 23?

Comment 15

[Mats Palmgren (inactive)]

Yes, 22 and all later versions.

Comment 17

[Mats Palmgren (inactive)]

Landed a crashtest:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4e37220caed

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c4e37220caed