Closed Bug 1278080 Opened 3 years ago Closed 3 years ago

AddressSanitizer: use-after-poison [@ TopLeft] with READ of size 4

Categories

(Core :: Layout, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: truber, Assigned: mats)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [post-critsmash-triage][adv-main50-])

Attachments

(6 files)

Attached testcase causes:

(m-c-1464872875-opt-asan)
==19004==ERROR: AddressSanitizer: use-after-poison on address 0x6250006ac5a0 at pc 0x7f189d1a3527 bp 0x7ffe66ca7630 sp 0x7ffe66ca7628
READ of size 4 at 0x6250006ac5a0 thread T0
    #0 0x7f189d1a3526 in TopLeft /builds/slave/m-cen-l64-asan-000000000000000/build/src/obj-firefox/dist/include/mozilla/gfx/BaseRect.h:314
    #1 0x7f189d1a3526 in nsRect /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/generic/nsIFrame.h:672
    #2 0x7f189d1a3526 in PresShell::MarkFramesInSubtreeApproximatelyVisible(nsIFrame*, nsRect const&, bool) /builds/slave/m-cen-l64-asan-000000000000000/build/src/layout/base/nsPresShell.cpp:6164


(m-c-1464948162-dbg-asan)
[19144] ###!!! ASSERTION: StealFrame: can't find aChild: 'removed', file /builds/slave/m-cen-l64-asan-d-ntly-00000000/build/src/layout/generic/nsContainerFrame.cpp, line 1399
==19144==ERROR: AddressSanitizer: use-after-poison on address 0x6250009ceda0 at pc 0x7f18495e9d1d bp 0x7fff8304c9d0 sp 0x7fff8304c9c8
READ of size 4 at 0x6250009ceda0 thread T0
    #0 0x7f18495e9d1c in mozilla::gfx::BaseRect<int, nsRect, nsPoint, nsSize, nsMargin>::TopLeft() const /builds/slave/m-cen-l64-asan-d-ntly-00000000/build/src/obj-firefox/dist/include/mozilla/gfx/BaseRect.h:314
    #1 0x7f184c5d62f1 in nsIFrame::GetPosition() const /builds/slave/m-cen-l64-asan-d-ntly-00000000/build/src/obj-firefox/dist/include/nsIFrame.h:672
    #2 0x7f184c806f2c in PresShell::MarkFramesInSubtreeApproximatelyVisible(nsIFrame*, nsRect const&, bool) /builds/slave/m-cen-l64-asan-d-ntly-00000000/build/src/layout/base/nsPresShell.cpp:6164
Attached file Testcase
Also saw this stack from the same testcase/build.
Group: core-security → gfx-core-security
Group: gfx-core-security → layout-core-security
Assignee: nobody → mats
Attached file frame dump
nsContainerFrame::RemoveFrame is trying to remove the blue frame.
It has a next-in-flow that is an overflow container.  This is a valid
frame tree as far as I know, so the problem is that we pass 'true'
to StealFrame here:
https://hg.mozilla.org/mozilla-central/annotate/ec20b463c04f/layout/generic/nsContainerFrame.cpp#l167
'true' means we skip the overflow-container lists even if the frame bit
says it's a an overflow container:
https://hg.mozilla.org/mozilla-central/annotate/ec20b463c04f/layout/generic/nsContainerFrame.cpp#l1372
This is rather insane.

It seems we have always done that, here's the version that introduce
StealFrame and it also pass true:
https://hg.mozilla.org/mozilla-central/annotate/10266/layout/generic/nsContainerFrame.cpp#l223
and even before that, when nsContainerFrame::RemoveFrame did the job
itself, it never checked the overflow lists so roc@ was just
preserving the existing behavior.
I think we should remove the aForceNormal flag and let nsContainerFrame::StealFrame
deal with it, i.e. if NS_FRAME_IS_OVERFLOW_CONTAINER then try to remove it from
the overflow lists, then if not found there, try the normal lists.  We can then
remove the nsColumnSetFrame and nsCanvasFrame impls.

I think that should be fine from a performance POV, because overflow containers
are rare, and removing a child frame from its frame list is O(1) these days.
> nsColumnSetFrame has its overflow containers on the normal lists

FTR, nsCanvasFrame says it does that too.
(FTR, the bug that roc@ fixed when adding the StealFrame call is bug 405271)
Attached patch fixSplinter Review
Attachment #8761421 - Flags: review?(dholbert)
Attachment #8760007 - Attachment mime type: text/plain → text/html
Regression from when? Even narrowing it down to an affected release ("47 is OK, 48 isn't") would help.
Those versions were examples. I don't crash in opt builds on 46 or 48 which I'd expect to if it's true framepoisoning.
Keywords: regression
(In reply to Daniel Veditz [:dveditz] from comment #12)
> Regression from when? Even narrowing it down to an affected release ("47 is
> OK, 48 isn't") would help.

Sorry, was marked regression by accident.
(I'd bet this is technically a regression from Bug 1144096, from the perspective of the attached testcase at least -- since the testcase uses a CSS grid that's fragmented (and Bug 1144096 is where we added support for grid fragmentation).  But we haven't enabled CSS grid by default in release builds yet, so no release builds should ever end up being affected by this bug.)
Comment on attachment 8761421 [details] [diff] [review]
fix

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

r=me, just one wording nit:

::: layout/generic/nsContainerFrame.cpp
@@ +1387,5 @@
>  #endif
>  
> +  bool removed = MaybeStealOverflowContainerFrame(aChild);
> +  if (!removed) {
> +    // NOTE nsColumnSetFrame and nsCanvasFrame have its overflow containers on

s/have its/have their/

(and then wrap "on" to the next line, to avoid going over 80 characters)
Attachment #8761421 - Flags: review?(dholbert) → review+
https://hg.mozilla.org/mozilla-central/rev/913b86ac15a7
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Group: layout-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main50-]
Group: core-security-release
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.