806483 - Heap-use-after-free (read) nsIFrame::GetStyleContext

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

Attachment: ASAN dump

ASAN debug build on Linux64, compiled with
ac_add_options --enable-optimize="-faddress-sanitizer -Dxmalloc=myxmalloc -DDEBUG_TRACEMALLOC_PRESARENA"
ac_add_options --enable-debug

That is, disabling the pres shell arena.

STEPS TO REPRODUCE
1. run Firefox with a fresh profile
2. click on a menu item, say Tools, then close it
3. exit

ACTUAL RESULTS

==8983== ERROR: AddressSanitizer heap-use-after-free on address 0x7ff31bfc6aa0 at pc 0x7ff35b68f777 bp 0x7fff120279d0 sp 0x7fff120279c8
READ of size 8 at 0x7ff31bfc6aa0 thread T0
    #0 0x7ff35b68f777 in nsIFrame::GetStyleContext() const ???:0
    #1 0x7ff35b68f2c3 in nsIFrame::PresContext() const ../../dist/include/nsIFrame.h:572
    #2 0x7ff35b962a99 in nsIFrame::Properties() const layout/svg/../generic/nsIFrame.h:2619
    #3 0x7ff35b965dd5 in ~DisplayItemData layout/base/FrameLayerBuilder.cpp:142
    #4 0x7ff35b9aa50a in mozilla::FrameLayerBuilder::DisplayItemData::Release() layout/base/FrameLayerBuilder.h:498
    #5 0x7ff35b9aa1de in ~nsRefPtr layout/base/../../dist/include/nsAutoPtr.h:874
    #6 0x7ff35b97a183 in ~nsRefPtr layout/base/../../dist/include/nsAutoPtr.h:875
    #7 0x7ff35ba1074d in ~nsRefPtrHashKey layout/base/../../dist/include/nsHashKeys.h:294
    #8 0x7ff35ba10633 in ~nsRefPtrHashKey layout/base/../../dist/include/nsHashKeys.h:294
    #9 0x7ff35ba0ff64 in nsTHashtable<nsRefPtrHashKey<mozilla::FrameLayerBuilder::DisplayItemData> >::s_ClearEntry(PLDHashTable*, PLDHashEntryHdr*) layout/base/../../dist/include/nsTHashtable.h:464
    #10 0x7ff3676a1e3e in PL_DHashTableFinish xpcom/build/pldhash.cpp:363
    #11 0x7ff35ba0f503 in ~nsTHashtable layout/base/../../dist/include/nsTHashtable.h:385
    #12 0x7ff35ba0f3a3 in ~nsTHashtable layout/base/../../dist/include/nsTHashtable.h:385
    #13 0x7ff35ba0f289 in ~LayerManagerData layout/base/FrameLayerBuilder.cpp:185
    #14 0x7ff35ba0eff3 in ~LayerManagerData layout/base/FrameLayerBuilder.cpp:185
    #15 0x7ff35ba0f10b in ~LayerManagerData layout/base/FrameLayerBuilder.cpp:183
    #16 0x7ff35b9c3de2 in LayerManagerUserDataDestroy layout/base/../../dist/include/Layers.h:119
    #17 0x7ff35b8c3ca3 in mozilla::gfx::UserData::Destroy() gfx/2d/UserData.h:98
    #18 0x7ff3683d6d64 in mozilla::layers::LayerManager::Destroy() gfx/layers/Layers.h:164
    #19 0x7ff3656bc710 in nsWindow::Destroy() widget/gtk2/nsWindow.cpp:618
    #20 0x7ff35fdb1f40 in nsView::DestroyWidget() view/src/nsView.cpp:108
    #21 0x7ff35fdb0ab0 in ~nsView view/src/nsView.cpp:92
    #22 0x7ff35fdb021b in ~nsView view/src/nsView.cpp:43
    #23 0x7ff35fdb1042 in nsIView::Destroy() view/src/nsView.cpp:152
    #24 0x7ff35c426031 in nsFrame::DestroyFrom(nsIFrame*) layout/generic/nsFrame.cpp:674
    #25 0x7ff35c84237a in nsSplittableFrame::DestroyFrom(nsIFrame*) layout/generic/nsSplittableFrame.cpp:44
    #26 0x7ff35c3d23a9 in nsContainerFrame::DestroyFrom(nsIFrame*) layout/generic/nsContainerFrame.cpp:238
    #27 0x7ff35d430435 in nsBoxFrame::DestroyFrom(nsIFrame*) layout/xul/base/src/nsBoxFrame.cpp:949
    #28 0x7ff35d58d140 in nsMenuPopupFrame::DestroyFrom(nsIFrame*) layout/xul/base/src/nsMenuPopupFrame.cpp:1827
...

Comment 1

[Mats Palmgren (inactive)]

(for posterity, the above is for mozilla-central rev 324985f4c4ea)

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Mats want to take a stab at rating this one? Do you think this is triggerable in other ways (not user action)?

Comment 3

[Mats Palmgren (inactive)]

> Mats want to take a stab at rating this one?

It looks like [sg:critical (mitigated by frame poisoning)] to me, but I don't
know deeply enough about LayerManager stuff to say for sure.

> Do you think this is triggerable in other ways (not user action)?

The bug occurs while destroying a pres shell, which you can do from script by
removing an <iframe> from the document for example.  I don't know what content
would trigger it.

Comment 4

[Mats Palmgren (inactive)]

The bug is reproducible in rev d26f6917e986 (2012-05-24).  I tried building some
revs from 2011 as well, but they failed to build under memory/.  The code involved
is from 2010 or so, eg. bug 579276.

Comment 5

[Mats Palmgren (inactive)]

Attachment: fix

Explicitly remove the LayerManagerDataProperty() from a destroyed frame also when destroying the shell.

Comment 6

[Mats Palmgren (inactive)]

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

[Security approval request comment]
How easily can the security issue be deduced from the patch?
not at all

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
not really

Which older supported branches are affected by this flaw?
the bug was introduced in 2010 or so

If not all supported branches, which bug introduced the flaw?
unknown

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
same patch should apply

How likely is this patch to cause regressions; how much testing does it need?
low risk

Comment 7

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

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

sec-approval+.

Could someone suggest a security rating for this issue while we're here?

Comment 8

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea8d002c4d6d

Comment 9

[Ed Morley [:emorley]]

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

Resolving fixed, but this outstanding:

(In reply to Al Billings [:abillings] from comment #7)
> sec-approval+.
> 
> Could someone suggest a security rating for this issue while we're here?

Comment 10

[bhavana bajaj [:bajaj]]

(In reply to Mats Palmgren [:mats] from comment #8)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/ea8d002c4d6d

Looks like this patch is been on m-c for a week now. Please uplift the patch to aurora as soon as we are comfortable with the bake time/testing on m-c .

Comment 11

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

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): DLBI
User impact if declined: possible crash/security issue
Testing completed (on m-c, etc.): one week landed
Risk to taking this patch (and alternatives if risky): Simple patch with relatively low risk
String or UUID changes made by this patch: None

Comment 12

[bhavana bajaj [:bajaj]]

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

Low risk , well tested patch , tracking 18. Approving on aurora.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2636c23bd023

Comment 14

[Mats Palmgren (inactive)]

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

see comment 6 and comment 11

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

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

thank you - please go ahead and uplift this to esr10 branch

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr17/rev/1f276706e60f
https://hg.mozilla.org/releases/mozilla-esr10/rev/70ce65da3666

(status-esr17 was accidentally set to fixed in comment 13. I received approval from bajaj to land it there as well)

Comment 17

[Daniel Holbert [:dholbert]]

This failed to build on esr10, with this build error:
{
/builds/slave/m-esr10-osx64-dbg/build/layout/base/FrameLayerBuilder.h:415: error: 'static const mozilla::FramePropertyDescriptor* mozilla::FrameLayerBuilder::LayerManagerDataProperty()' is protected

/builds/slave/m-esr10-osx64-dbg/build/layout/base/nsPresShell.cpp:2294: error: within this context
}

Looks like this function did indeed used to be protected, but it was made public (on trunk) in this change:
https://hg.mozilla.org/mozilla-central/diff/25af0febf329/layout/base/FrameLayerBuilder.h#l1.115

But on esr10, that function is still protected & hence can't be called directly by nsPresShell.

The fix could be as simple as just moving the LayerManagerDataProperty declaration to a "public" chunk of the header file, but it's probably worth a once-over from mats and/or mattwoodrow to be sure that's a good idea.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

What he said. Backed out.

https://hg.mozilla.org/releases/mozilla-esr10/rev/2cab6f96723d
https://hg.mozilla.org/releases/mozilla-esr17/rev/26392b810c4b

Comment 19

[Mats Palmgren (inactive)]

https://hg.mozilla.org/releases/mozilla-esr10/rev/9bd7cdfc311f
I'll push it to esr17 also - as soon as my clone is complete...

Comment 20

[Mats Palmgren (inactive)]

https://hg.mozilla.org/releases/mozilla-esr17/rev/741b75ca2e09

Comment 21

[Mats Palmgren (inactive)]

It requires a special ASAN build to see this problem so it's not easily verifiable.

Comment 22

[u279076]

(In reply to Mats Palmgren [:mats] from comment #21)
> It requires a special ASAN build to see this problem so it's not easily
> verifiable.

Thanks Mats. Flagging this [qa-].

Comment 23

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren [:mats] from comment #21)
> It requires a special ASAN build to see this problem so it's not easily
> verifiable.

Note that Christian Holler (:decoder) generates ASAN nightly builds and posts them here:
  https://people.mozilla.com/~choller/firefox/asan/

Currently, it looks like the oldest build there is from 12/26/2012, so they probably won't help for verification in this bug's case. But in the future, we may be able to grab ASAN nightlies from there for fix-verification.

Comment 24

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #23)
> Note that Christian Holler (:decoder) generates ASAN nightly builds...

Yes, but they wont help verifying this bug.  What I meant with "special" is an
ASan build that was built with the extra -DDEBUG_TRACEMALLOC_PRESARENA.
(btw, I'm fixing that in bug 827150)

Comment 25

[Christian Holler (:decoder)]

If necessary, https://developer.mozilla.org/en/Building_Firefox_with_Address_Sanitizer describes also how to do manual builds or try pushes with ASan support.