1403117 - use-after-poison in nsLayoutUtils::GetFloatContainingBlock

Status: RESOLVED DUPLICATE of bug 1382213

(Core :: Layout, defect)

Description

[Holger Fuhrmannek]

Attachment: testcase.html

The current nightly build of firefox crashs with the attached testcase. 
ASAN:
==4008==ERROR: AddressSanitizer: use-after-poison on address 0x625000e81fb0 at pc 0x7fffe02bfe16 bp 0x7ffffffeddc0 sp 0x7ffffffeddb8
READ of size 8 at 0x625000e81fb0 thread T0
    #0 0x7fffe02bfe15 in nsLayoutUtils::GetFloatContainingBlock(nsIFrame*) /builds/worker/workspace/build/src/layout/base/nsLayoutUtils.cpp:9308 (discriminator 2)
    #2 0x7fffe051cb99 in ReparentFloatsForInlineChild /builds/worker/workspace/build/src/layout/generic/nsInlineFrame.cpp:312 (discriminator 1)
    #3 0x7fffe051cb99 in DrainSelfOverflowListInternal /builds/worker/workspace/build/src/layout/generic/nsInlineFrame.cpp:505 (discriminator 1)
    #5 0x7fffe051c90c in nsInlineFrame::DestroyFrom(nsIFrame*) /builds/worker/workspace/build/src/layout/generic/nsInlineFrame.cpp:197
    #7 0x7fffe039b66d in Destroy /builds/worker/workspace/build/src/layout/generic/nsIFrame.h:661
...

Comment 1

[Holger Fuhrmannek]

Attachment: asanlog.txt

Comment 2

[Tyson Smith [:tsmith]]

Dup of bug 1382213?

Comment 3

[Ryan VanderMeulen [:RyanVM]]

This appears to have an older regression range than the other bug, so I'm going to tentatively say no to that question for now. Note that the testcase needs modification to work around bug 1300895 on older builds.

INFO: Last good revision: b6683e141c47c022598c0caac3ea8ba8c6236d42 (2016-04-07)
INFO: First bad revision: d9b1a9829c8ee2862955043f28183efa07de3d2b (2016-04-08)
INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b6683e141c47c022598c0caac3ea8ba8c6236d42&tochange=d9b1a9829c8ee2862955043f28183efa07de3d2b

Bug 1259296 in that range does touch layout/base/nsLayoutUtils.cpp and layout/generic/nsGfxScrollFrame.cpp which appear on the stack, so maybe that's a start?

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

In DrainSelfOverflowListInternal, the overflow frames of "this" have a parent not "this", and that parent is poisoned, which is weird...

Comment 5

[Botond Ballo [:botond]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #3)
> Bug 1259296 in that range does touch layout/base/nsLayoutUtils.cpp and
> layout/generic/nsGfxScrollFrame.cpp which appear on the stack, so maybe
> that's a start?

I doubt it - those layout changes look like pretty harmless code removal.

Given that the testcase uses insertAdjacentText(), and the regression range contains bug 811259, "implement Element.insertAdjacentText and Element.insertAdjacentElement", I think that's a more likely culprit.

Comment 6

[Botond Ballo [:botond]]

(I tried to needinfo the assignee of bug 811259, but Bugzilla told me:

"You asked Jocelyn Liu [:jocelyn] [:joliu] <[e-mail]> for needinfo on bug 1403117, but that bug has been restricted to users in certain groups, and the user you asked isn't in all the groups to which the bug has been restricted. Please choose someone else to ask, or make the bug accessible to users on its CC: list and add that user to the list."

So ni?smaug, who reviewed the patches in the bug.)

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Ah yes, thank you. That's a lot saner :)

Comment 8

[Andrew McCreight [:mccr8]]

(In reply to Botond Ballo [:botond] from comment #6)
> (I tried to needinfo the assignee of bug 811259, but Bugzilla told me:

You just have to manually CC the person in order to needinfo them.

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

This is really unrelated to insertAdjacentText. The same crash can be reproduced with an equivalent code:
> a.parentNode.insertBefore(document.createTextNode("aaaaaaa"), a.nextSibling);

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Attachment: updated testcase

This can be used to test earlier version.

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

It ends up giving me this range: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1dc6b294800d&tochange=c45f6e6d6005

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

In this range, only bug 1001994 seems very suspicious. The patch creates nsInlineFrame::DestroyFrom function, so it is likely to be the cause of this.

Comment 13

[Daniel Veditz [:dveditz]]

Opt build crashes with a null deref on this testcase, so it's not protected by our "framepoisoning" mitigation (which would show a unique address range).

Comment 14

[Mats Palmgren (inactive)]

Attachment: stack + frame dump

We crash b/c nsLayoutUtils::GetFloatContainingBlock is trying to use
a destroyed ancestor frame (red).  The reason is that nsBlockFrame::DoRemoveFrame
removes frame in order, so (red) is destroyed before (blue) which is
the parent of (green).  The problem is that due to "lazy reparenting",
the (green) frame has not yet been reparented so its 'mParent' still points
to (red) even though it's a child of (blue).

So this is exactly the same underlying bug as in bug 1382213.
Also, frame-poisoning should make it unexploitable on all platforms.

(I don't know why it looked like a null-pointer crash to dveditz
in comment 13, but I'm guessing the local variable 'ancestor' was
optimized away and on some platforms that looks like a null pointer?)

Comment 16

[Mats Palmgren (inactive)]

Sorry, duped to the wrong bug.

Comment 17

[Mats Palmgren (inactive)]

(tracking this in bug 1382213)