1015844 - use-after-poison (read) at nsIFrame::GetPrevInFlow()

Status: VERIFIED FIXED

(Core :: Layout, defect, P3)

Description

[Aki Helin]

Attachment: ff-uap-getprevinflow.html

ASan spots a use-after-poison error when the attached page is opened at least in current Aurora builds. Poisoning may make this harmless, but filing as a security bug to be on the safe side.

==888==ERROR: AddressSanitizer: use-after-poison on address 0x62500058adb8 at pc 0x7f5cd76be30b bp 0x7fff5df86370 sp 0x7fff5df86368
READ of size 8 at 0x62500058adb8 thread T0
==888==WARNING: Trying to symbolize code, but external symbolizer is not initialized!
    #0 0x7f5cd76be30a in nsIFrame::GetPrevInFlow() const /home/aki/src/mozilla-aurora/layout/generic/nsIFrame.h:1504
    #1 0x7f5cd766e6de in ~AutoFinish /home/aki/src/mozilla-aurora/layout/generic/nsContainerFrame.h:616
    #2 0x7f5cd76ef8a5 in nsFrame::ReflowAbsoluteFrames(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&, bool) /home/aki/src/mozilla-aurora/layout/generic/nsFrame.cpp:4339
    #3 0x7f5cd76ef51d in nsFrame::FinishReflowWithAbsoluteFrames(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&, bool) /home/aki/src/mozilla-aurora/layout/generic/nsFrame.cpp:4309
    #4 0x7f5cd76b4ff4 in nsColumnSetFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) /home/aki/src/mozilla-aurora/layout/generic/nsColumnSetFrame.cpp:1013
    #5 0x7f5cd76991a6 in nsBlockReflowContext::ReflowBlock(nsRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, nsHTMLReflowState&, unsigned int&, nsBlockReflowState&) /home/aki/src/mozilla-aurora/layout/generic/nsBlockReflowContext.cpp:261
    #6 0x7f5cd76829cc in nsBlockFrame::ReflowBlockFrame(nsBlockReflowState&, nsLineList_iterator, bool*) /home/aki/src/mozilla-aurora/layout/generic/nsBlockFrame.cpp:3132
    #7 0x7f5cd767a782 in nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) /home/aki/src/mozilla-aurora/layout/generic/nsBlockFrame.cpp:2076
    #8 0x7f5cd7676253 in nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) /home/aki/src/mozilla-aurora/layout/generic/nsBlockFrame.cpp:1069
    #9 0x7f5cd76991a6 in nsBlockReflowContext::ReflowBlock(nsRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, nsHTMLReflowState&, unsigned int&, nsBlockReflowState&) /home/aki/src/mozilla-aurora/layout/generic/nsBlockReflowContext.cpp:261
    #10 0x7f5cd76829cc in nsBlockFrame::ReflowBlockFrame(nsBlockReflowState&, nsLineList_iterator, bool*) /home/aki/src/mozilla-aurora/layout/generic/nsBlockFrame.cpp:3132
[...]
0x62500058adb8 is located 5304 bytes inside of 8192-byte region [0x625000589900,0x62500058b900)

Comment 1

[Mats Palmgren (inactive)]

Attachment: frame dump + stack for StealFrame assertion

There are a few of these assertions:
ASSERTION: Column set should be complete if the available height is unconstrained ..., layout/generic/nsColumnSetFrame.cpp, line 1021

and then:
ASSERTION: StealFrame: can't find aChild: 'removed', layout/generic/nsContainerFrame.cpp, line 1279

leading up to the crash.  I suspect the last one is what causes the crash.

Attached is the frame tree + stack when the StealFrame assertion
occurs.  It appears aChild is a NS_FRAME_IS_OVERFLOW_CONTAINER
on an ExcessOverflowContainersList (highlighted in red color),
but nsColumnSetFrame::StealFrame forces the search to the
principal list.
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsContainerFrame.cpp?rev=18214a2cfdb3#1251

Comment 2

[Mats Palmgren (inactive)]

Attachment: part 1

This part fixes the crash.

Comment 3

[Mats Palmgren (inactive)]

Attachment: part 2

This part adds:
* MOZ_ASSERT that all frames on the [Excess]OverflowContainersList
lists are NOT true overflow containers, since that's an invariant
we depend on
* add ReflowOverflowContainerChildren to reflow said frames - this is
boiler plate for any abs.pos. container and I don't see any reason
why we shouldn't do it for nsColumnSetFrame
* move the NS_ASSERTION about being fully complete - I think that
only applies to the status from reflowing normal flow children

Note that we still assert for this testcase even after these fixes:

ASSERTION: Column set should be complete if the available height is unconstrained: 'NS_FRAME_IS_FULLY_COMPLETE(aStatus) || aReflowState.AvailableHeight() != NS_UNCONSTRAINEDSIZE', layout/generic/nsColumnSetFrame.cpp, line 1017

ASSERTION: overflow containers must be zero-height: 'aMetrics.Height() == 0', layout/generic/nsBlockFrame.cpp, line 1495

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

Comment 4

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/516ad717d7f4
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb0790d28846

(I'll file bugs for the remaining assertions above separately, if needed)

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/516ad717d7f4
https://hg.mozilla.org/mozilla-central/rev/fb0790d28846

Is this worth uplifting to Aurora31, being an ESR and all?

Comment 6

[Mats Palmgren (inactive)]

I don't think so.  It seems unlikely this crash would occur on
any real web pages so it's not needed for robustness, and frame-
poisoning should make it non-exploitable.

Comment 7

[Mats Palmgren (inactive)]

(the remaining assertions are already on file: bug 569193 and bug 574889)

Comment 8

[Daniel Veditz [:dveditz]]

(In reply to Mats Palmgren (:mats) from comment #6)
> and frame-poisoning should make it non-exploitable.

Where is the framepoisoning? The ASAN use-after-poison warning is not the same thing as our release-build framepoisoning, and in fact pretty much can only occur in arenas we didn't framepoison.

Comment 9

[Mats Palmgren (inactive)]

Attachment: the crash in a *non*-ASAN build

Our pres arena frame poisoning is definitely mitigating this crash.

Comment 10

[Mats Palmgren (inactive)]

(In reply to Daniel Veditz [:dveditz] from comment #8)
> The ASAN use-after-poison warning is not the
> same thing as our release-build framepoisoning, 

Yes.

> and in fact pretty much can
> only occur in arenas we didn't framepoison.

I don't think that is true.  When Free-ing pres arena objects in 
an ASAN build we do both frame-poisoning and tell ASAN the memory
is "noaccess", so in an ASAN build it will NOT show up as a frame-
poison crash but rather as an ASAN-noaccess crash.
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresArena.cpp?rev=d1e2767da13f#109

Comment 11

[Andrew McCreight [:mccr8]]

Marking sec-other per comment 6.

Comment 12

[Daniel Veditz [:dveditz]]

Comment 9 is useful, thanks. Definitely frame-poisoning.

Comment 13

[Mats Palmgren (inactive)]

Landed the crashtest:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a49e1d956526

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed crash in ASan build of Fx32, 2014-06-03.
Verified fixed in ASan build of Fx32, 2014-08-20.
Verified fixed in release build of Fx32, release candidate from today.