840353 - Heap-use-after-free in nsAsyncDOMEvent::Run

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Joe Drew (not getting mail)]

+++ This bug was initially created as a clone of Bug #835814 +++

Unfortunately this bug still exists after the patch of bug 835814, it just comes up on fewer occasions. (It's also probably harder to create a testcase for.)

Further unfortunately, it blocks my work on bug 716140. I'm getting a build up and running on the record-and-replay box.

>==6846== ERROR: AddressSanitizer: heap-use-after-free on address 0x7f1873a98298 at pc 0x7f18a5b0eb72 bp 0x7fff6a3d1ad0 sp 0x7fff6a3d1ac8
>READ of size 8 at 0x7f1873a98298 thread T0
>    #0 0x7f18a5b0eb71 in nsCOMPtr<nsINodeInfo>::get() const src/../../dist/include/nsCOMPtr.h:764
>    #1 0x7f18a5b0eda9 in nsCOMPtr<nsINodeInfo>::operator->() const src/../../dist/include/nsCOMPtr.h:784
>    #2 0x7f18a5ab6ff3 in nsINode::OwnerDoc() const src/../../dist/include/nsINode.h:476
>    #3 0x7f18973cc3c0 in nsAsyncDOMEvent::Run() src/content/events/src/nsAsyncDOMEvent.cpp:35
>    #4 0x7f18a477fbdf in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:627
>    #5 0x7f189205ef95 in NS_ProcessNextEvent_P(nsIThread*, bool) src/objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:238
>    #6 0x7f18a259a7dc in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:82
>    #7 0x7f18945742c2 in MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:215
>    #8 0x7f1894573429 in MessageLoop::RunHandler() src/ipc/chromium/src/base/message_loop.cc:208
>    #9 0x7f18945795ae in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:182
>    #10 0x7f1896aa0f47 in nsBaseAppShell::Run() src/widget/xpwidgets/nsBaseAppShell.cpp:163
>    #11 0x7f1895007b35 in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:288
>    #12 0x7f189eb04534 in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:3823
>    #13 0x7f189eb0f92a in XREMain::XRE_main(int, char**, nsXREAppData const*) src/toolkit/xre/nsAppRunner.cpp:3890
>    #14 0x7f1891a692b0 in XRE_main src/toolkit/xre/nsAppRunner.cpp:4093
>    #15 0x4520f3 in do_main(int, char**, nsIFile*) src/browser/app/nsBrowserApp.cpp:185
>    #16 0x4d9392 in main src/browser/app/nsBrowserApp.cpp:377
>    #17 0x7f18b37c876c in
>0x7f1873a98298 is located 24 bytes inside of 208-byte region [0x7f1873a98280,0x7f1873a98350)
>freed by thread T0 here:
>    #0 0x40f082 in __interceptor_free
>    #1 0x7f18b47e54b9 in moz_free src/memory/mozalloc/mozalloc.cpp:48
>    #2 0x7f18a02cf830 in operator delete(void*) src/../../../../dist/include/mozilla/mozalloc.h:224
>    #3 0x7f18a02cf830 in mozilla::dom::HTMLImageElement::~HTMLImageElement() src/content/html/content/src/HTMLImageElement.cpp:83
>    #4 0x7f18948851f7 in nsNodeUtils::LastRelease(nsINode*) src/content/base/src/nsNodeUtils.cpp:258
>    #5 0x7f18a045dc10 in mozilla::dom::FragmentOrElement::Release() src/content/base/src/FragmentOrElement.cpp:1685
>    #6 0x7f18a02c7f8a in mozilla::dom::HTMLImageElement::Release() src/content/html/content/src/HTMLImageElement.cpp:89
>    #7 0x7f1895c1fa9f in nsCOMPtr_base::~nsCOMPtr_base() src/objdir-ff-asan-sym/media/webrtc/signaling/signaling_ecc/../../../../dist/include/nsCOMPtr.h:410
>    #8 0x7f18a411c48c in nsCOMPtr<nsINode>::~nsCOMPtr() src/../../dist/include/nsCOMPtr.h:449
>    #9 0x7f18a411c379 in nsCOMPtr<nsINode>::~nsCOMPtr() src/../../dist/include/nsCOMPtr.h:449
>    #10 0x7f18973ce1a7 in nsAsyncDOMEvent::~nsAsyncDOMEvent() src/../../../dist/include/nsAsyncDOMEvent.h:23
>    #11 0x7f189b565ce0 in nsLoadBlockingAsyncDOMEvent::~nsLoadBlockingAsyncDOMEvent() src/content/events/src/nsAsyncDOMEvent.cpp:63
>    #12 0x7f189b565a3f in nsLoadBlockingAsyncDOMEvent::~nsLoadBlockingAsyncDOMEvent() src/content/events/src/nsAsyncDOMEvent.cpp:59
>    #13 0x7f1894468a35 in nsRunnable::Release() src/objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:31
>    #14 0x7f1895c1fa9f in nsCOMPtr_base::~nsCOMPtr_base() src/objdir-ff-asan-sym/media/webrtc/signaling/signaling_ecc/../../../../dist/include/nsCOMPtr.h:410
>    #15 0x7f18a3e0c58c in nsCOMPtr<nsIRunnable>::~nsCOMPtr() src/../../dist/include/nsCOMPtr.h:449
>    #16 0x7f18a3e0c479 in nsCOMPtr<nsIRunnable>::~nsCOMPtr() src/../../dist/include/nsCOMPtr.h:449
>    #17 0x7f18a477fcdf in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:633
>    #18 0x7f189205ef95 in NS_ProcessNextEvent_P(nsIThread*, bool) src/objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:238
>    #19 0x7f18a259a7dc in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:82
>    #20 0x7f18945742c2 in MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:215
>    #21 0x7f1894573429 in MessageLoop::RunHandler() src/ipc/chromium/src/base/message_loop.cc:208
>    #22 0x7f18945795ae in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:182
>    #23 0x7f1896aa0f47 in nsBaseAppShell::Run() src/widget/xpwidgets/nsBaseAppShell.cpp:163
>    #24 0x7f1895007b35 in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:288
>    #25 0x7f189eb04534 in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:3823
>    #26 0x7f189eb0f92a in XREMain::XRE_main(int, char**, nsXREAppData const*) src/toolkit/xre/nsAppRunner.cpp:3890
>    #27 0x7f1891a692b0 in XRE_main src/toolkit/xre/nsAppRunner.cpp:4093
>    #28 0x4520f3 in do_main(int, char**, nsIFile*) src/browser/app/nsBrowserApp.cpp:185
>    #29 0x4d9392 in main src/browser/app/nsBrowserApp.cpp:377
>previously allocated by thread T0 here:
>    #0 0x40f162 in __interceptor_malloc
>    #1 0x7f18b47e5604 in moz_xmalloc src/memory/mozalloc/mozalloc.cpp:54
>    #2 0x7f18920c186f in operator new(unsigned long) src/../../../../dist/include/mozilla/mozalloc.h:200
>    #3 0x7f18920c186f in NS_NewHTMLImageElement(already_AddRefed<nsINodeInfo>, mozilla::dom::FromParser) src/content/html/content/src/HTMLImageElement.cpp:66
>    #4 0x7f18958d8c95 in nsVideoFrame::CreateAnonymousContent(nsTArray<nsIAnonymousContentCreator::ContentInfo>&) src/layout/generic/nsVideoFrame.cpp:75
>    #5 0x7f18a68218d9 in non-virtual thunk to nsVideoFrame::CreateAnonymousContent(nsTArray<nsIAnonymousContentCreator::ContentInfo>&) src/layout/generic/nsVideoFrame.cpp:116
>    #6 0x7f189a43bf19 in nsCSSFrameConstructor::GetAnonymousContent(nsIContent*, nsIFrame*, nsTArray<nsIAnonymousContentCreator::ContentInfo>&) src/layout/base/nsCSSFrameConstructor.cpp:3903
>    #7 0x7f189a41c7b6 in nsCSSFrameConstructor::ProcessChildren(nsFrameConstructorState&, nsIContent*, nsStyleContext*, nsIFrame*, bool, nsFrameItems&, bool, PendingBinding*, nsIFrame*) src/layout/base/nsCSSFrameConstructor.cpp:9967
>    #8 0x7f189a49b656 in nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem&, nsFrameConstructorState&, nsIFrame*, nsFrameItems&) src/layout/base/nsCSSFrameConstructor.cpp:3745
>    #9 0x7f189a46d015 in nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsIFrame*, nsFrameItems&) src/layout/base/nsCSSFrameConstructor.cpp:5542
>    #10 0x7f189a49211b in nsCSSFrameConstructor::ConstructFramesFromItemList(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList&, nsIFrame*, nsFrameItems&) src/layout/base/nsCSSFrameConstructor.cpp:9903
>    #11 0x7f189a4194d6 in nsCSSFrameConstructor::ContentAppended(nsIContent*, nsIContent*, bool) src/layout/base/nsCSSFrameConstructor.cpp:6725
>    #12 0x7f189a4367e8 in nsCSSFrameConstructor::CreateNeededFrames(nsIContent*) src/layout/base/nsCSSFrameConstructor.cpp:6380
>    #13 0x7f189a436c61 in nsCSSFrameConstructor::CreateNeededFrames() src/layout/base/nsCSSFrameConstructor.cpp:6405
>    #14 0x7f18a48722c2 in PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) src/layout/base/nsPresShell.cpp:3867
>    #15 0x7f18a48710d3 in PresShell::FlushPendingNotifications(mozFlushType) src/layout/base/nsPresShell.cpp:3757
>    #16 0x7f18942da2cd in nsDocument::FlushPendingNotifications(mozFlushType) src/content/base/src/nsDocument.cpp:6727
>    #17 0x7f1894729766 in nsDocLoader::DocLoaderIsEmpty(bool) src/uriloader/base/nsDocLoader.cpp:729
>    #18 0x7f18947240f5 in nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) src/uriloader/base/nsDocLoader.cpp:659
>    #19 0x7f18a763055b in non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) src/uriloader/base/nsDocLoader.cpp:663
>    #20 0x7f1894823461 in nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, tag_nsresult) src/netwerk/base/src/nsLoadGroup.cpp:676
>    #21 0x7f189428efd0 in nsDocument::DoUnblockOnload() src/content/base/src/nsDocument.cpp:7600
>    #22 0x7f1894280a41 in nsDocument::UnblockOnload(bool) src/content/base/src/nsDocument.cpp:7542
>    #23 0x7f18942de953 in nsDocument::DispatchContentLoadedEvents() src/content/base/src/nsDocument.cpp:4408
>    #24 0x7f189a1a3062 in nsRunnableMethodImpl<void (nsDocument::*)(), true>::Run() src/../../../dist/include/nsThreadUtils.h:367
>    #25 0x7f18a477fbdf in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:627
>Shadow bytes around the buggy address:
>  0x1fe30e753000: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>  0x1fe30e753010: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>  0x1fe30e753020: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>  0x1fe30e753030: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>  0x1fe30e753040: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>=>0x1fe30e753050: fd fd fd[fd]fd fd fd fd fd fd fd fd fd fd fd fd
>  0x1fe30e753060: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>  0x1fe30e753070: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>  0x1fe30e753080: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>  0x1fe30e753090: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>  0x1fe30e7530a0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>Shadow byte legend (one shadow byte represents 8 application bytes):
>  Addressable:           00
>  Partially addressable: 01 02 03 04 05 06 07
>  Heap left redzone:     fa
>  Heap righ redzone:     fb
>  Freed Heap region:     fd
>  Stack left redzone:    f1
>  Stack mid redzone:     f2
>  Stack right redzone:   f3
>  Stack partial redzone: f4
>  Stack after return:    f5
>  Stack use after scope: f8
>  Global redzone:        f9
>  Global init order:     f6
>  Poisoned by user:      f7
>  ASan internal:         fe
>Stats: 274M malloced (295M for red zones) by 451143 calls
>Stats: 48M realloced by 25728 calls
>Stats: 245M freed by 320606 calls
>Stats: 110M really freed by 202828 calls
>Stats: 492M (492M-0M) mmaped; 123 maps, 0 unmaps
>  mmaps   by size class: 8:294894; 9:40955; 10:12285; 11:14329; 12:4096; 13:1536; 14:1280; 15:384; 16:1216; 17:1312; 18:64; 19:40; 20:24;
>  mallocs by size class: 8:368736; 9:40833; 10:12926; 11:17620; 12:3670; 13:2004; 14:1736; 15:459; 16:1577; 17:1440; 18:77; 19:42; 20:23;
>  frees   by size class: 8:256057; 9:30657; 10:9412; 11:15499; 12:2386; 13:1682; 14:1564; 15:329; 16:1481; 17:1417; 18:63; 19:39; 20:20;
>  rfrees  by size class: 8:175259; 9:10652; 10:2677; 11:10244; 12:779; 13:679; 14:773; 15:177; 16:980; 17:577; 18:26; 19:4; 20:1;
>Stats: malloc large: 1582 small slow: 2700
>Stats: StackDepot: 0 ids; 0M mapped
>==6846== ABORTING
>

Comment 1

[Joe Drew (not getting mail)]

To be clear, this comes up at least when you run mochitest-browser-chrome with my patches from bug 716140 applied. I don't know how to reproduce this without those patches.

Comment 2

[Joe Drew (not getting mail)]

Attachment: don't send OnStopRequest() if we're canceled

The fundamental problem here is that we're trying to notify something that's in the middle of destroying itself. It's even *told us* that it's destroying itself by calling CancelAndForgetObserver(). Some leak problems made us hesitate to ignore OnStopRequest in bug 393936, where that interface method was introduced, but I think we're OK to do so now.

This patch is running (alone) through try: https://tbpl.mozilla.org/?tree=Try&rev=c126127e3f03

The thing we'll need to look out for most is leaks because of something that calls CancelAndForgetObserver while still wanting OnStopRequest.

Comment 3

[Joe Drew (not getting mail)]

One other question for Kyle is whether we should revert the change in bug 835814 in favour of this one.

Comment 4

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

Joe do you think OMT-ImageDecode might land in 21?

Comment 5

[Joe Drew (not getting mail)]

No, definitely not. (If it did, by some miracle, I'd back it out of Aurora.)

Comment 6

[Joe Drew (not getting mail)]

(In reply to David Bolter [:davidb] from comment #4)
> Joe do you think OMT-ImageDecode might land in 21?

BTW - this is an underlying bug that is easily exposed with the patches on bug OMT-ImageDecode, but I expect that it is exploitable without those patches.

Comment 7

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

(In reply to Joe Drew (:JOEDREW! \o/) from comment #6)
> (In reply to David Bolter [:davidb] from comment #4)
> > Joe do you think OMT-ImageDecode might land in 21?
> 
> BTW - this is an underlying bug that is easily exposed with the patches on
> bug OMT-ImageDecode, but I expect that it is exploitable without those
> patches.

Oh darn and my flags are wrong. Does it affect all branches?

Comment 8

[Joe Drew (not getting mail)]

Almost certainly. But with no testcase.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 713549 [details] [diff] [review]
don't send OnStopRequest() if we're canceled

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

Skipping just this one notification seems wrong.  Why not skip them all?

Comment 10

[Joe Drew (not getting mail)]

This is adding OnStopRequest to the (vast majority) of notifications that we skip if (mCanceled).

Comment 11

[Joe Drew (not getting mail)]

(The only one we don't skip is OnStartRequest, I believe.)

Comment 12

[Seth Fowler [:seth] [:s2h]]

That lack of symmetry is a little weird, though, don't you think? Intuitively you'd expect to get OnStopRequest if you get OnStartRequest.

Comment 13

[Joe Drew (not getting mail)]

Certainly. The problem is that, since we drop our ref to the listener *before* OnStopRequest (to avoid leaks), if a listener Cancel()s us and then goes away, there's no object there to get confused.

The real solution here is cycle collecting Imagelib observers, but that's hard.

Comment 14

[Joe Drew (not getting mail)]

Comment on attachment 713549 [details] [diff] [review]
don't send OnStopRequest() if we're canceled

[Security approval request comment]
How easily could an exploit be constructed based on the patch? 

With some difficulty. We don't actually have a testcase that exercises this bug on trunk, but the patches in bug make it happen with mochitest-browser-chrome. A very determined (or observant—we've had similar bugs in the past) attacker could probably reverse-engineer an exploit, but I'm not sure how.

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

No.

Which older supported branches are affected by this flaw?

Likely all.

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

In most cases we'd crash in the cases that this patch would have any effect, but I can conceive of ways that it could affect correctness. I'd want it to bake on mozilla-central for a little bit if we were going to uplift it.

Comment 15

[Joe Drew (not getting mail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/40b4bc5cac71

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/40b4bc5cac71

Can we get a test for this too once it hits all the necessary branches?

Comment 17

[Joe Drew (not getting mail)]

I have no idea how to test this without my patches in bug 716140. With it, we already have tests that crash. :-)

Comment 18

[Alex Keybl [:akeybl]]

Please nominate this patch for uplift to Aurora/Beta/ESR17 when you get the chance. Thanks!

Comment 19

[Robert Kaiser]

This is marked tracking+ for 20, which is closing its doors soon. Joe, what's the plan on uplift here?

Comment 20

[Joe Drew (not getting mail)]

Comment on attachment 713549 [details] [diff] [review]
don't send OnStopRequest() if we're canceled

Sorry, I missed Alex's comment! 

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Not a regression
User impact if declined: Security bug
Testing completed (on m-c, etc.): On m-c for a few days
Risk to taking this patch (and alternatives if risky): Not terribly risky.
String or UUID changes made by this patch: none

Comment 21

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 713549 [details] [diff] [review]
don't send OnStopRequest() if we're canceled

Thanks Joe - we'll get this into our fourth week Beta if it lands today.

Comment 22

[Joe Drew (not getting mail)]

https://hg.mozilla.org/releases/mozilla-beta/rev/7fa8722aa84a
https://hg.mozilla.org/releases/mozilla-aurora/rev/6f3311695469
https://hg.mozilla.org/releases/mozilla-esr17/rev/d754fa307c59

Comment 23

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

Because there is no repository of old ASan builds for 17esr - or any repository at all - reproducing this crash on 17esr is more difficult. It does not repro on non-ASan 17.0.4esr. However, to see the crash, I can use a nightly m-c build (with ASan) from around that date to see the crash. I can also use my own 17.0.5esr ASan build to verify that it's fixed.

Also, used bug file from related bug 835814.

Confirmed crash with m-c 2012-02-11 (ASan)
Confirmed fixed with 17.0.5esr 2013-03-28 (custom local ASan build, changeset cd7d672b40ea)

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/06a2afad6fab