802485 - Heap-use-after-free in mozilla::image::RasterImage::Discard

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Abhishek Arya]

Reproduces on trunk. Looks like a recent regression and similar to https://bugzilla.mozilla.org/buglist.cgi?quicksearch=RasterImage%3A%3ADiscard;list_id=4692622 [None of those bug had a repro and I don't seem have permission to restrict view them, so didn't add the repro there.]

=================================================================
==24004== ERROR: AddressSanitizer heap-use-after-free on address 0x7fd2752a08d8 at pc 0x7fd29c1ecb87 bp 0x7fff64cc69f0 sp 0x7fff64cc69e8
READ of size 1 at 0x7fd2752a08d8 thread T0
    #0 0x7fd29c1ecb86 in imgRequestProxy::NotificationsDeferred() const image/src/imgRequestProxy.h:89
    #1 0x7fd29c1e6d0c in imgStatusTracker::SendDiscard(imgRequestProxy*) image/src/imgStatusTracker.cpp:618
    #2 0x7fd29c1e67d8 in imgStatusTrackerObserver::OnDiscard() image/src/imgStatusTracker.cpp:206
    #3 0x7fd29c0a0ac8 in mozilla::image::RasterImage::Discard(bool) image/src/RasterImage.cpp:2288
    #4 0x7fd29c05a8b3 in mozilla::image::DiscardTracker::DiscardNow() image/src/DiscardTracker.cpp:268
    #5 0x7fd29c05d032 in mozilla::image::DiscardTracker::TimerCallback(nsITimer*, void*) image/src/DiscardTracker.cpp:249
    #6 0x7fd2a80825d2 in nsTimerImpl::Fire() xpcom/threads/nsTimerImpl.cpp:472
    #7 0x7fd2a8083ada in nsTimerEvent::Run() xpcom/threads/nsTimerImpl.cpp:555
    #8 0x7fd2a80471e0 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:612
    #9 0x7fd2a7cd804b in NS_ProcessNextEvent_P(nsIThread*, bool) objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:220
    #10 0x7fd2a6662cbd in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:117
    #11 0x7fd2a8324d91 in MessageLoop::RunInternal() ipc/chromium/src/base/message_loop.cc:215
    #12 0x7fd2a8324bc6 in MessageLoop::RunHandler() ipc/chromium/src/base/message_loop.cc:208
    #13 0x7fd2a8324aab in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:182
    #14 0x7fd2a5b08b4a in nsBaseAppShell::Run() widget/xpwidgets/nsBaseAppShell.cpp:163
    #15 0x7fd2a47373b4 in nsAppStartup::Run() toolkit/components/startup/nsAppStartup.cpp:290
    #16 0x7fd29a8154ad in XREMain::XRE_mainRun() toolkit/xre/nsAppRunner.cpp:3792
    #17 0x7fd29a81b325 in XREMain::XRE_main(int, char**, nsXREAppData const*) toolkit/xre/nsAppRunner.cpp:3858
    #18 0x7fd29a81e1d4 in XRE_main toolkit/xre/nsAppRunner.cpp:3933
    #19 0x40bb13 in do_main(int, char**) browser/app/nsBrowserApp.cpp:174
    #20 0x409255 in main browser/app/nsBrowserApp.cpp:279
    #21 0x7fd2b9f6d76c in ?? ??:0
0x7fd2752a08d8 is located 88 bytes inside of 96-byte region [0x7fd2752a0880,0x7fd2752a08e0)
freed by thread T0 here:
    #0 0x4c3660 in __interceptor_free ??:?
    #1 0x7fd2b6de5406 in moz_free memory/mozalloc/mozalloc.cpp:51
    #2 0x7fd29c1b771d in operator delete(void*) ../../dist/include/mozilla/mozalloc.h:224
    #3 0x7fd29c1b5c3b in imgRequestProxy::Release() image/src/imgRequestProxy.cpp:29
    #4 0x7fd29a933724 in nsCOMPtr_base::assign_assuming_AddRef(nsISupports*) ../../dist/include/nsCOMPtr.h:440
    #5 0x7fd2a7c90403 in nsCOMPtr_base::assign_with_AddRef(nsISupports*) objdir-ff-asan-sym/xpcom/build/nsCOMPtr.cpp:49
    #6 0x7fd29cc61fda in nsCOMPtr<imgIRequest>::operator=(imgIRequest*) ../../../../../../dist/include/nsCOMPtr.h:622
    #7 0x7fd29e469342 in nsImageLoadingContent::ClearCurrentRequest(tag_nsresult) content/base/src/nsImageLoadingContent.cpp:1023
    #8 0x7fd29e468f83 in nsImageLoadingContent::DestroyImageLoadingContent() content/base/src/nsImageLoadingContent.cpp:100
    #9 0x7fd29f541546 in ~nsHTMLImageElement content/html/content/src/nsHTMLImageElement.cpp:80
    #10 0x7fd29f5411bc in ~nsHTMLImageElement content/html/content/src/nsHTMLImageElement.cpp:79
    #11 0x7fd29e54374d in nsNodeUtils::LastRelease(nsINode*) content/base/src/nsNodeUtils.cpp:260
    #12 0x7fd29e7e869d in mozilla::dom::FragmentOrElement::Release() content/base/src/FragmentOrElement.cpp:1766
    #13 0x7fd29f541917 in nsHTMLImageElement::Release() content/html/content/src/nsHTMLImageElement.cpp:85
    #14 0x7fd29a7d927b in ~nsCOMPtr_base ../../../dist/include/nsCOMPtr.h:408
    #15 0x7fd29c39fa09 in nsCOMPtr<nsIContent>::~nsCOMPtr() ../../dist/include/nsCOMPtr.h:447
    #16 0x7fd29c39f6d6 in nsCOMPtr<nsIContent>::~nsCOMPtr() ../../dist/include/nsCOMPtr.h:447
    #17 0x7fd29e80a6d4 in ContentUnbinder::UnbindSubtree(nsIContent*) content/base/src/FragmentOrElement.cpp:1086
    #18 0x7fd29e80a63c in ContentUnbinder::UnbindSubtree(nsIContent*) content/base/src/FragmentOrElement.cpp:1084
    #19 0x7fd29e809149 in ContentUnbinder::Run() content/base/src/FragmentOrElement.cpp:1097
    #20 0x7fd2a80471e0 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:612
    #21 0x7fd2a7cd804b in NS_ProcessNextEvent_P(nsIThread*, bool) objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:220
    #22 0x7fd2a6662656 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:82
    #23 0x7fd2a8324d91 in MessageLoop::RunInternal() ipc/chromium/src/base/message_loop.cc:215
    #24 0x7fd2a8324bc6 in MessageLoop::RunHandler() ipc/chromium/src/base/message_loop.cc:208
    #25 0x7fd2a8324aab in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:182
    #26 0x7fd2a5b08b4a in nsBaseAppShell::Run() widget/xpwidgets/nsBaseAppShell.cpp:163
    #27 0x7fd2a47373b4 in nsAppStartup::Run() toolkit/components/startup/nsAppStartup.cpp:290
    #28 0x7fd29a8154ad in XREMain::XRE_mainRun() toolkit/xre/nsAppRunner.cpp:3792
    #29 0x7fd29a81b325 in XREMain::XRE_main(int, char**, nsXREAppData const*) toolkit/xre/nsAppRunner.cpp:3858
previously allocated by thread T0 here:
    #0 0x4c3720 in malloc ??:?
    #1 0x7fd2b6de555a in moz_xmalloc memory/mozalloc/mozalloc.cpp:57
    #2 0x7fd29c135850 in operator new(unsigned long) ../../dist/include/mozilla/mozalloc.h:200
    #3 0x7fd29c148e76 in imgLoader::ValidateRequestWithNewChannel(imgRequest*, nsIURI*, nsIURI*, nsIURI*, nsILoadGroup*, imgINotificationObserver*, nsISupports*, unsigned int, imgIRequest*, imgIRequest**, nsIChannelPolicy*, nsIPrincipal*, int) image/src/imgLoader.cpp:1227
    #4 0x7fd29c15046c in imgLoader::ValidateEntry(imgCacheEntry*, nsIURI*, nsIURI*, nsIURI*, nsILoadGroup*, imgINotificationObserver*, nsISupports*, unsigned int, bool, imgIRequest*, imgIRequest**, nsIChannelPolicy*, nsIPrincipal*, int) image/src/imgLoader.cpp:1390
    #5 0x7fd29c155d99 in imgLoader::LoadImage(nsIURI*, nsIURI*, nsIURI*, nsIPrincipal*, nsILoadGroup*, imgINotificationObserver*, nsISupports*, unsigned int, nsISupports*, imgIRequest*, nsIChannelPolicy*, imgIRequest**) image/src/imgLoader.cpp:1616
    #6 0x7fd29e005d4b in nsContentUtils::LoadImage(nsIURI*, nsIDocument*, nsIPrincipal*, nsIURI*, imgINotificationObserver*, int, imgIRequest**) content/base/src/nsContentUtils.cpp:2786
    #7 0x7fd29e474fc6 in nsImageLoadingContent::LoadImage(nsIURI*, bool, bool, nsIDocument*, unsigned int) content/base/src/nsImageLoadingContent.cpp:664
    #8 0x7fd29e4774f0 in nsImageLoadingContent::LoadImage(nsAString_internal const&, bool, bool) content/base/src/nsImageLoadingContent.cpp:578
    #9 0x7fd29f54e14c in nsHTMLImageElement::MaybeLoadImage() content/html/content/src/nsHTMLImageElement.cpp:443
    #10 0x7fd29f5644fe in nsRunnableMethodImpl<void (nsHTMLImageElement::*)(), true>::Run() ../../../../dist/include/nsThreadUtils.h:349
    #11 0x7fd29e03f85c in nsContentUtils::RemoveScriptBlocker() content/base/src/nsContentUtils.cpp:5019
    #12 0x7fd29e228065 in nsDocument::EndUpdate(unsigned int) content/base/src/nsDocument.cpp:4162
    #13 0x7fd29fb662f3 in nsHTMLDocument::EndUpdate(unsigned int) content/html/document/src/nsHTMLDocument.cpp:2308
    #14 0x7fd2a16b934a in nsHtml5TreeOpExecutor::EndDocUpdate() parser/html/nsHtml5TreeOpExecutor.h:248
    #15 0x7fd2a16bd244 in nsHtml5TreeOpExecutor::RunFlushLoop() parser/html/nsHtml5TreeOpExecutor.cpp:589
    #16 0x7fd2a16f9629 in nsHtml5ExecutorFlusher::Run() parser/html/nsHtml5StreamParser.cpp:127
    #17 0x7fd2a80471e0 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:612
    #18 0x7fd2a7cd804b in NS_ProcessNextEvent_P(nsIThread*, bool) objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:220
    #19 0x7fd2a6662656 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:82
    #20 0x7fd2a8324d91 in MessageLoop::RunInternal() ipc/chromium/src/base/message_loop.cc:215
    #21 0x7fd2a8324bc6 in MessageLoop::RunHandler() ipc/chromium/src/base/message_loop.cc:208
    #22 0x7fd2a8324aab in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:182
    #23 0x7fd2a5b08b4a in nsBaseAppShell::Run() widget/xpwidgets/nsBaseAppShell.cpp:163
    #24 0x7fd2a47373b4 in nsAppStartup::Run() toolkit/components/startup/nsAppStartup.cpp:290
Shadow byte and word:
  0x1ffa4ea5411b: fd
  0x1ffa4ea54118: fd fd fd fd fd fd fd fd
More shadow bytes:
  0x1ffa4ea540f8: fd fd fd fd fd fd fd fd
  0x1ffa4ea54100: fa fa fa fa fa fa fa fa
  0x1ffa4ea54108: fa fa fa fa fa fa fa fa
  0x1ffa4ea54110: fd fd fd fd fd fd fd fd
=>0x1ffa4ea54118: fd fd fd fd fd fd fd fd
  0x1ffa4ea54120: fa fa fa fa fa fa fa fa
  0x1ffa4ea54128: fa fa fa fa fa fa fa fa
  0x1ffa4ea54130: fd fd fd fd fd fd fd fd
  0x1ffa4ea54138: fd fd fd fd fd fd fd fd
Stats: 96M malloced (128M for red zones) by 303852 calls
Stats: 8M realloced by 17844 calls
Stats: 74M freed by 183182 calls
Stats: 0M really freed by 0 calls
Stats: 256M (65573 full pages) mmaped in 64 calls
  mmaps   by size class: 8:278511; 9:24573; 10:12285; 11:8188; 12:2048; 13:1536; 14:512; 15:256; 16:768; 17:96; 18:128; 19:16; 20:8; 21:2;
  mallocs by size class: 8:262726; 9:21920; 10:8126; 11:6617; 12:1819; 13:1068; 14:505; 15:138; 16:705; 17:80; 18:126; 19:15; 20:6; 21:1;
  frees   by size class: 8:158102; 9:13578; 10:4879; 11:3469; 12:945; 13:870; 14:367; 15:105; 16:666; 17:73; 18:111; 19:12; 20:4; 21:1;
  rfrees  by size class:
Stats: malloc large: 228 small slow: 1388
==24004== ABORTING

Comment 1

[Abhishek Arya]

Attachment: Testcase zip (part1)

Comment 2

[Abhishek Arya]

Attachment: Testcase zip (part2)

Comment 3

[Abhishek Arya]

Attachment: Testcase zip (part3)

I had no option but to split the archive (4mb is too small a limit for attachment these days :( ). There is a large mp4 file in there as dependency.

Comment 4

[Bob Clary [:bc] (inactive)]

Was your build created after bug 801405 was fixed at 2012-10-16 01:21:23 ? This may have been fixed already.

Comment 5

[Abhishek Arya]

(In reply to Bob Clary [:bc:] from comment #4)
> Was your build created after bug 801405 was fixed at 2012-10-16 01:21:23 ?
> This may have been fixed already.

I updated my build today, but it might be before 01:21. Let me resync and check.

Comment 6

[Abhishek Arya]

(In reply to Abhishek Arya from comment #5)
> (In reply to Bob Clary [:bc:] from comment #4)
> > Was your build created after bug 801405 was fixed at 2012-10-16 01:21:23 ?
> > This may have been fixed already.
> 
> I updated my build today, but it might be before 01:21. Let me resync and
> check.

Yes my testcase still crashes on bleeding edge trunk (give it like 30-60 sec to run on asanified build). I did make sure to check that the fix is in source file as well.

Comment 7

[Josh Matthews [:jdm]]

Thank you, I've been bashing my head trying to fire out the crash stacks involving discarding and coming up empty so far.

Comment 8

[Josh Matthews [:jdm]]

Attachment: Testcase

Comment 9

[Josh Matthews [:jdm]]

Attachment: Testcase resource 1

Comment 10

[Josh Matthews [:jdm]]

Attachment: Testcase resource 2 (place under images/)

Comment 11

[Josh Matthews [:jdm]]

Thanks Abhishek. I have narrowed it down to this testcase; I'll start digging into the problem now.

Comment 12

[Abhishek Arya]

(In reply to Josh Matthews [:jdm] from comment #11)
> Thanks Abhishek. I have narrowed it down to this testcase; I'll start
> digging into the problem now.

You are very welcome!

Comment 13

[Daniel Veditz [:dveditz]]

UAF read of size 1 ? what kind of object are we re-using here? Just image data, or some object that has pointers in it?

Comment 14

[Josh Matthews [:jdm]]

We're using pointers to imgRequestProxy objects that no longer exist.

Comment 15

[Josh Matthews [:jdm]]

Attachment: Ensure owner of imgRequestProxy is not updated for cancelled proxies, while ensuring that image references are still updated.

Comment 16

[Josh Matthews [:jdm]]

What's the policy here for landing? This is a topcrash (bug 801453), and it only applies to trunk. Does this actually need to be s-s?

Comment 17

[Josh Matthews [:jdm]]

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

Comment 18

[Joe Drew (not getting mail)]

Comment on attachment 672903 [details] [diff] [review]
Ensure owner of imgRequestProxy is not updated for cancelled proxies, while ensuring that image references are still updated.

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

I am very hesitant to r+ this fix. I'd much rather we not special-case canceled imgRequestProxies in ChangeOwner(). Can you try that out and see what happens?

Comment 19

[Josh Matthews [:jdm]]

Attachment: Remove special handling for canceled image request proxies that are changing owners.

Comment 20

[Josh Matthews [:jdm]]

Attachment: Remove special handling for canceled image request proxies that are changing owners.

qref

Comment 21

[Joe Drew (not getting mail)]

Comment on attachment 673346 [details] [diff] [review]
Remove special handling for canceled image request proxies that are changing owners.

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

r++++++++++++++++++++++++++++++++++++++++++++++

Comment 22

[Josh Matthews [:jdm]]

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

Comment 23

[Daniel Veditz [:dveditz]]

(In reply to Josh Matthews [:jdm] from comment #16)
> What's the policy here for landing? This is a topcrash (bug 801453), and it
> only applies to trunk. Does this actually need to be s-s?

In practice probably not, but when security bugs are filed we can't always guarantee they'll be fixed before they get uplifted to branches with more users. If this is m-c only then sec-approval+ for landing whenever you're ready.

Comment 25

[Josh Matthews [:jdm]]

https://tbpl.mozilla.org/?tree=Try&rev=1259035ab796

Comment 26

[Josh Matthews [:jdm]]

Attachment: Remove special handling for canceled image request proxies that are changing owners.

Now not causing test failures. There's a tricky case where we CancelAndForgetObserver, which does an immediate RemoveProxy. We later ChangeOwner, which now causes us to be added to the new owner's list, but in the destructor we were not removing outselves from it.

Comment 27

[Josh Matthews [:jdm]]

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

Comment 28

[Ryan VanderMeulen [:RyanVM]]

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

Comment 29

[Ryan VanderMeulen [:RyanVM]]

And a follow-up push:
https://hg.mozilla.org/mozilla-central/rev/3d3039772d4a