1387845 - heap-use-after-free in [@ mozilla::layers::SharedPlanarYCbCrImage::~SharedPlanarYCbCrImage]

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: log.txt

I am try to get a working testcase at the moment.

==8962==ERROR: AddressSanitizer: heap-use-after-free on address 0x6100004f2568 at pc 0x7f4d233a3397 bp 0x7f4caa8952d0 sp 0x7f4caa8952c8
READ of size 8 at 0x6100004f2568 thread T65 (MediaPl~back #1)
    #0 0x7f4d233a3396 in mozilla::AtomicRefCountedWithFinalize<mozilla::layers::TextureClient>::Release() src/obj-firefox/dist/include/mozilla/layers/AtomicRefCountedWithFinalize.h:121:7
    #1 0x7f4d235d386f in RefPtr<mozilla::layers::TextureClient>::operator=(decltype(nullptr)) src/gfx/layers/../../mfbt/RefPtr.h:166:5
    #2 0x7f4d23708989 in mozilla::layers::SharedPlanarYCbCrImage::~SharedPlanarYCbCrImage() src/gfx/layers/ipc/SharedPlanarYCbCrImage.cpp:43:22
    #3 0x7f4d23708a8d in mozilla::layers::SharedPlanarYCbCrImage::~SharedPlanarYCbCrImage() src/gfx/layers/ipc/SharedPlanarYCbCrImage.cpp:36:51
    #4 0x7f4d233c2fee in mozilla::layers::Image::Release() src/obj-firefox/dist/include/ImageContainer.h:200:3
    #5 0x7f4d25c7cde4 in mozilla::VideoData::~VideoData() src/dom/media/MediaData.cpp:164:1
    #6 0x7f4d25c7ce1d in mozilla::VideoData::~VideoData() src/dom/media/MediaData.cpp:163:1
    #7 0x7f4d2182581e in mozilla::MediaData::Release() src/obj-firefox/dist/include/MediaData.h:282:3
    #8 0x7f4d25d18642 in mozilla::detail::RunnableMethodImpl<mozilla::detail::Listener<RefPtr<mozilla::VideoData> >*, void (mozilla::detail::Listener<RefPtr<mozilla::VideoData> >::*)(RefPtr<mozilla::VideoData>&&), true, (mozilla::RunnableKind)0, RefPtr<mozilla::VideoData>&&>::~RunnableMethodImpl() src/obj-firefox/dist/include/nsThreadUtils.h:1147:45
    #9 0x7f4d25d1867d in mozilla::detail::RunnableMethodImpl<mozilla::detail::Listener<RefPtr<mozilla::VideoData> >*, void (mozilla::detail::Listener<RefPtr<mozilla::VideoData> >::*)(RefPtr<mozilla::VideoData>&&), true, (mozilla::RunnableKind)0, RefPtr<mozilla::VideoData>&&>::~RunnableMethodImpl() src/obj-firefox/dist/include/nsThreadUtils.h:1147:33
    #10 0x7f4d21a5e6ae in mozilla::Runnable::Release() src/xpcom/threads/nsThreadUtils.cpp:44:1
    #11 0x7f4d218e8397 in nsTArray_Impl<nsCOMPtr<nsIRunnable>, nsTArrayInfallibleAllocator>::DestructRange(unsigned long, unsigned long) src/obj-firefox/dist/include/nsTArray.h:2012:7
    #12 0x7f4d218e827d in nsTArray_Impl<nsCOMPtr<nsIRunnable>, nsTArrayInfallibleAllocator>::RemoveElementsAt(unsigned long, unsigned long) src/obj-firefox/dist/include/nsTArray.h:2060:3
    #13 0x7f4d218c3ca9 in nsTArray_Impl<nsCOMPtr<nsIRunnable>, nsTArrayInfallibleAllocator>::~nsTArray_Impl() src/obj-firefox/dist/include/nsTArray.h:885:7
    #14 0x7f4d21a2b862 in mozilla::AutoTaskDispatcher::PerThreadTaskGroup::~PerThreadTaskGroup() src/obj-firefox/dist/include/mozilla/TaskDispatcher.h:177:65
    #15 0x7f4d21a2b825 in mozilla::DefaultDelete<mozilla::AutoTaskDispatcher::PerThreadTaskGroup>::operator()(mozilla::AutoTaskDispatcher::PerThreadTaskGroup*) const src/obj-firefox/dist/include/mozilla/UniquePtr.h:528:5
    #16 0x7f4d21a2b48a in mozilla::AutoTaskDispatcher::TaskGroupRunnable::~TaskGroupRunnable() src/obj-firefox/dist/include/mozilla/TaskDispatcher.h:185:9
    #17 0x7f4d21a2b4bd in mozilla::AutoTaskDispatcher::TaskGroupRunnable::~TaskGroupRunnable() src/obj-firefox/dist/include/mozilla/TaskDispatcher.h:185:9
    #18 0x7f4d21a5e6ae in mozilla::Runnable::Release() src/xpcom/threads/nsThreadUtils.cpp:44:1
    #19 0x7f4d21a2066f in RefPtr<nsIRunnable>::operator=(decltype(nullptr)) src/obj-firefox/dist/include/mozilla/RefPtr.h:166:5
    #20 0x7f4d21a1fe2b in mozilla::TaskQueue::Runner::Run() src/xpcom/threads/TaskQueue.cpp:254:9
    #21 0x7f4d21a5c12e in nsThreadPool::Run() src/xpcom/threads/nsThreadPool.cpp:225:14
    #22 0x7f4d21a5c5ac in non-virtual thunk to nsThreadPool::Run() src/xpcom/threads/nsThreadPool.cpp:154:15
    #23 0x7f4d21a53c80 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1446:14
    #24 0x7f4d21a598c0 in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:480:10
    #25 0x7f4d225baaf4 in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:339:20
    #26 0x7f4d2250c277 in MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:326:10
    #27 0x7f4d2250c109 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:299:3
    #28 0x7f4d21a4bdeb in nsThread::ThreadFunc(void*) src/xpcom/threads/nsThread.cpp:506:11
    #29 0x7f4d3d7ed5ed in _pt_root src/nsprpub/pr/src/pthreads/ptthread.c:216:5
    #30 0x7f4d40df76b9 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76b9)
    #31 0x7f4d3fe803dc in clone /build/glibc-bfm8X4/glibc-2.23/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:109

Comment 1

[Tyson Smith [:tsmith]]

Maybe a dev should have a quick look at this before we close it since there is a log.

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

SharedPlanarYCbCrImage  allocates the TextureClient and give it to TextureClientRecycleAllocator to hold.

ImageBridgeChild::NotifyNotUsed gets called in ImageBridgeThread, finds a matching texture ID in mTexturesWaitingRecycled on a TextureClient with reference count set to 2, and removes it from the table.  The calls ::Release, which  causes a callback (ref count 1), which causes the second/nested ::Release call, a part of the callback that clears TextureClientHolder entry from TextureClientRecycleAllocator.  With reference count now at zero, the actual delete happens.

On MediaPlaybackThread, SharedPlanarYCbCrImage destructor increments the reference count for the mTextureClient it holds, then dispatches a call to the ImageBridgeThread to ImageBridgeChild::ReleaseTextureClientNow. Then we get in trouble because mTextureClient has already been released, by the time we call mTextureClient == null (trouble shows up in  AtomicRefCountedWithFinalize::Release.)  We're going after the TextureClient that got deleted as described above.

Sotaro, you spent some time in this code, is there anything we can do just with the given log?

Comment 3

[Sotaro Ikeda [:sotaro]]

Hmm, the code of SharedPlanarYCbCrImage::~SharedPlanarYCbCrImage() is wired. I am not sure why the destructor does such things. It seems not necessary any more on current gecko.

Comment 4

[Sotaro Ikeda [:sotaro]]

(In reply to Milan Sreckovic [:milan] (PTO through 8/30) from comment #2)
> SharedPlanarYCbCrImage  allocates the TextureClient and give it to
> TextureClientRecycleAllocator to hold.
> 
> ImageBridgeChild::NotifyNotUsed gets called in ImageBridgeThread, finds a
> matching texture ID in mTexturesWaitingRecycled on a TextureClient with
> reference count set to 2, and removes it from the table.  The calls
> ::Release, which  causes a callback (ref count 1), which causes the
> second/nested ::Release call, a part of the callback that clears
> TextureClientHolder entry from TextureClientRecycleAllocator.  With
> reference count now at zero, the actual delete happens.
> 
> On MediaPlaybackThread, SharedPlanarYCbCrImage destructor increments the
> reference count for the mTextureClient it holds, then dispatches a call to
> the ImageBridgeThread to ImageBridgeChild::ReleaseTextureClientNow. Then we
> get in trouble because mTextureClient has already been released, by the time
> we call mTextureClient == null (trouble shows up in 
> AtomicRefCountedWithFinalize::Release.)  We're going after the TextureClient
> that got deleted as described above.
> 

Hmm, I am still not sure how this STR happens :( SharedPlanarYCbCrImage still holds ref of TextureClient when ImageBridgeChild::NotifyNotUsed gets called, therefore reference count should be more than or equal to 3.

Comment 5

[Sotaro Ikeda [:sotaro]]

Destructor of ~SharedPlanarYCbCrImage() is wired. SharedPlanarYCbCrImage is derived from PlanarYCbCrImage, but ~SharedPlanarYCbCrImage() is not virtual.

Comment 6

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #5)
> Destructor of ~SharedPlanarYCbCrImage() is wired. SharedPlanarYCbCrImage is
> derived from PlanarYCbCrImage, but ~SharedPlanarYCbCrImage() is not virtual.

It seems not a problem, if base destructor virtual means all descendants have virtual destructors.

Comment 7

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #4)
> 
> Hmm, I am still not sure how this STR happens :( SharedPlanarYCbCrImage
> still holds ref of TextureClient when ImageBridgeChild::NotifyNotUsed gets
> called, therefore reference count should be more than or equal to 3.

Oh, I just misunderstood attachment 8894233 [details]. It detected heap-use-after-free at the following. I misunderstood that TextureClient treid to delete twice.

>   NS_LOG_RELEASE(this, currCount, mName);
  https://dxr.mozilla.org/mozilla-central/source/gfx/layers/AtomicRefCountedWithFinalize.h#121

The above is just log of releasing ref counted object. It was added by Bug 1280297. And I could happen when MediaPl~back was preenpted until end of TextureClient deletion on ImageBridge thread.

Comment 8

[Sotaro Ikeda [:sotaro]]

NS_LOG_RELEASE assumed to be called after update reference count. Therefore the problem like Comment 7 could happen on multi threaded usages.

Comment 9

[Sotaro Ikeda [:sotaro]]

For now, it seems simpler to address the problem like Bug 1287155. It used NS_BUILD_REFCNT_LOGGING def to avoid the crash.

Comment 10

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add more NS_BUILD_REFCNT_LOGGING in AtomicRefCountedWithFinalize

Comment 11

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf97878d1c9e11bc0cd0ee0c2aeda1192b70bb1f

Comment 12

[Nicolas Silva [:nical]]

Comment on attachment 8899311 [details] [diff] [review]
patch - Add more NS_BUILD_REFCNT_LOGGING in AtomicRefCountedWithFinalize

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

::: gfx/layers/AtomicRefCountedWithFinalize.h
@@ +101,5 @@
>  private:
>      void AddRef() {
>        MOZ_ASSERT(mRefCount >= 0, "AddRef() during/after Finalize()/dtor.");
>        mRefCount++;
> +#ifdef NS_BUILD_REFCNT_LOGGING

Since this is multi-threaded, passing mRefCount to NS_LOG_ADDREF is incorrect because it may have changed after the increment. Instead we should do something like:

> auto count = ++mRefCount;
> //...
> NS_LOG_ADDREF(this, count, mName, sizeof(*this));

I know that the error doesn't come from your patch, but it would be great if you fixed it in the process.

Comment 13

[Sotaro Ikeda [:sotaro]]

Thanks! I am going to update it in a next patch.

Comment 14

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add more NS_BUILD_REFCNT_LOGGING in AtomicRefCountedWithFinalize

Comment 15

[Sotaro Ikeda [:sotaro]]

:tsmith, did you enabled " --enable-logrefcnt"? When it is enabled, this could hit the problem. It seems like the problem of NS_LOG_RELEASE on multi threaded environment like in comment 7.

Comment 16

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f67fcb23e666e126308ed75c9fc403692f9e3be0

Comment 17

[Tyson Smith [:tsmith]]

(In reply to Sotaro Ikeda [:sotaro] from comment #15)
> :tsmith, did you enabled " --enable-logrefcnt"? When it is enabled, this
> could hit the problem. It seems like the problem of NS_LOG_RELEASE on multi
> threaded environment like in comment 7.

I'm not sure. I assume that is a build flag. I used a build from TC[1] (I'm not 100% if it was an opt or debug build though)

[1] https://tools.taskcluster.net/index/artifacts/gecko.v2.mozilla-central.latest.firefox/linux64-asan-opt

Comment 18

[Sotaro Ikeda [:sotaro]]

Thanks!

Comment 19

[Sotaro Ikeda [:sotaro]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6099148d38ae7021d7bce779f524f26ce141f047

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6099148d38ae

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Is there a user impact here that warrants backport consideration or can it ride the 57 train?

Comment 22

[Sotaro Ikeda [:sotaro]]

The fix does not affect to release build. Then back port seems not necessary. It can ride the 57 train.