1243466 - Heap-use-after-free crash [@mozilla::layers::ImageContainer::~ImageContainer]

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect, P3)

Description

[Christoph Diehl [:posidron]]

The following testcase crashes on en-us.linux-x86_64-asan.tar.bz2 revision 9a1977d9b21b1a4ba1fc5fdaff4240af83088271

See attachment.

Backtrace:

==13625==ERROR: AddressSanitizer: heap-use-after-free on address 0x6060004abf10 at pc 0x7f354fcc82a4 bp 0x7fffe60d4dd0 sp 0x7fffe60d4dc8
READ of size 8 at 0x6060004abf10 thread T0 (Web Content)
    #0 0x7f354fcc82a3 in Lock /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/dist/include/mozilla/Mutex.h:69
    #1 0x7f354fcc82a3 in BaseAutoLock /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/dist/include/mozilla/Mutex.h:164
    #2 0x7f354fcc82a3 in ForgetImageContainer /builds/slave/m-in-l64-asan-0000000000000000/build/src/gfx/layers/ImageContainer.cpp:109
    #3 0x7f354fcc82a3 in mozilla::layers::ImageContainer::~ImageContainer() /builds/slave/m-in-l64-asan-0000000000000000/build/src/gfx/layers/ImageContainer.cpp:151
    #4 0x7f3552cdcfe7 in Release /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/dist/include/ImageContainer.h:349
    #5 0x7f3552cdcfe7 in AddRefTraitsReleaseHelper /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:362
    #6 0x7f3552cdcfe7 in Release /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:372
    #7 0x7f3552cdcfe7 in ~Mutex /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:56
    #8 0x7f3552cdcfe7 in mozilla::VideoFrameContainer::~VideoFrameContainer() /builds/slave/m-in-l64-asan-0000000000000000/build/src/dom/media/VideoFrameContainer.cpp:30
    #9 0x7f35528eb146 in Release /builds/slave/m-in-l64-asan-0000000000000000/build/src/dom/media/VideoFrameContainer.h:39
    #10 0x7f35528eb146 in AddRefTraitsReleaseHelper /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:362
    #11 0x7f35528eb146 in Release /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:372
    #12 0x7f35528eb146 in ~RefPtr /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:56
    #13 0x7f35528eb146 in mozilla::dom::HTMLMediaElement::~HTMLMediaElement() /builds/slave/m-in-l64-asan-0000000000000000/build/src/dom/html/HTMLMediaElement.cpp:2175
    #14 0x7f3552998d6e in ~HTMLVideoElement /builds/slave/m-in-l64-asan-0000000000000000/build/src/dom/html/HTMLVideoElement.cpp:51
    #15 0x7f3552998d6e in mozilla::dom::HTMLVideoElement::~HTMLVideoElement() /builds/slave/m-in-l64-asan-0000000000000000/build/src/dom/html/HTMLVideoElement.cpp:50
    #16 0x7f354de6ea88 in SnowWhiteKiller::~SnowWhiteKiller() /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/base/nsCycleCollector.cpp:2655
    #17 0x7f354de6e6ae in nsCycleCollector::FreeSnowWhite(bool) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/base/nsCycleCollector.cpp:2829
    #18 0x7f354f49e0c9 in AsyncFreeSnowWhite::Run() /builds/slave/m-in-l64-asan-0000000000000000/build/src/js/xpconnect/src/XPCJSRuntime.cpp:151
    #19 0x7f354df8a8df in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/threads/nsThread.cpp:995
    #20 0x7f354e005bda in NS_ProcessNextEvent(nsIThread*, bool) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/glue/nsThreadUtils.cpp:297
    #21 0x7f354e9af939 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/glue/MessagePump.cpp:95
    #22 0x7f354e91b97c in RunInternal /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:234
    #23 0x7f354e91b97c in RunHandler /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:227
    #24 0x7f354e91b97c in MessageLoop::Run() /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:201
    #25 0x7f3553d23a17 in nsBaseAppShell::Run() /builds/slave/m-in-l64-asan-0000000000000000/build/src/widget/nsBaseAppShell.cpp:156
    #26 0x7f3555c6c7a2 in XRE_RunAppShell /builds/slave/m-in-l64-asan-0000000000000000/build/src/toolkit/xre/nsEmbedFunctions.cpp:789
    #27 0x7f354e91b97c in RunInternal /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:234
    #28 0x7f354e91b97c in RunHandler /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:227
    #29 0x7f354e91b97c in MessageLoop::Run() /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:201
    #30 0x7f3555c6be9a in XRE_InitChildProcess /builds/slave/m-in-l64-asan-0000000000000000/build/src/toolkit/xre/nsEmbedFunctions.cpp:625
    #31 0x48d760 in content_process_main(int, char**) /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/app/../contentproc/plugin-container.cpp:237
    #32 0x7f354b839ec4 (/lib/x86_64-linux-gnu/libc.so.6+0x21ec4)

0x6060004abf10 is located 48 bytes inside of 64-byte region [0x6060004abee0,0x6060004abf20)
freed by thread T28 (ImageBridgeChil) here:
    #0 0x474ed1 in __interceptor_free /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:64
    #1 0x7f354ff36b44 in mozilla::layers::ImageBridgeChild::DeallocPImageContainerChild(mozilla::layers::PImageContainerChild*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/gfx/layers/ipc/ImageBridgeChild.cpp:1057
    #2 0x7f354eba4fc7 in mozilla::layers::PImageBridgeChild::DeallocSubtree() /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/ipc/ipdl/PImageBridgeChild.cpp:1132
    #3 0x7f354eba55ea in mozilla::layers::PImageBridgeChild::OnChannelError() /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/ipc/ipdl/PImageBridgeChild.cpp:970
    #4 0x7f354e9aac9d in NotifyMaybeChannelError /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/glue/MessageChannel.cpp:1864
    #5 0x7f354e9aac9d in mozilla::ipc::MessageChannel::OnNotifyMaybeChannelError() /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/glue/MessageChannel.cpp:1893
    #6 0x7f354e91cdf4 in RunTask /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:364
    #7 0x7f354e91cdf4 in MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:372
    #8 0x7f354e91dea7 in MessageLoop::DoWork() /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:459
    #9 0x7f354e91ffc8 in base::MessagePumpDefault::Run(base::MessagePump::Delegate*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_pump_default.cc:34
    #10 0x7f354e91b97c in RunInternal /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:234
    #11 0x7f354e91b97c in RunHandler /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:227
    #12 0x7f354e91b97c in MessageLoop::Run() /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:201
    #13 0x7f354e933923 in base::Thread::ThreadMain() /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/thread.cc:172
    #14 0x7f354e9351dc in ThreadFunc(void*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/platform_thread_posix.cc:36
    #15 0x7f355c4b9181 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x8181)

previously allocated by thread T0 (Web Content) here:
    #0 0x4750d1 in __interceptor_malloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:74
    #1 0x48dd7d in moz_xmalloc /builds/slave/m-in-l64-asan-0000000000000000/build/src/memory/mozalloc/mozalloc.cpp:83
    #2 0x7f354fcc7c2f in operator new /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/dist/include/mozilla/mozalloc.h:186
    #3 0x7f354fcc7c2f in mozilla::layers::ImageContainer::ImageContainer(mozilla::layers::ImageContainer::Mode) /builds/slave/m-in-l64-asan-0000000000000000/build/src/gfx/layers/ImageContainer.cpp:137
    #4 0x7f354fcde1a7 in mozilla::layers::LayerManager::CreateImageContainer(mozilla::layers::ImageContainer::Mode) /builds/slave/m-in-l64-asan-0000000000000000/build/src/gfx/layers/Layers.cpp:204
    #5 0x7f35528f28c8 in mozilla::dom::HTMLMediaElement::GetVideoFrameContainer() /builds/slave/m-in-l64-asan-0000000000000000/build/src/dom/html/HTMLMediaElement.cpp:4030
    #6 0x7f3552b286c7 in mozilla::MediaDecoder::MediaDecoder(mozilla::MediaDecoderOwner*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/dom/media/MediaDecoder.cpp:506
    #7 0x7f3552aa9467 in operator new /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/dist/include/WebMDecoder.h:17
    #8 0x7f3552aa9467 in InstantiateDecoder /builds/slave/m-in-l64-asan-0000000000000000/build/src/dom/media/DecoderTraits.cpp:577
    #9 0x7f3552aa9467 in mozilla::DecoderTraits::CreateDecoder(nsACString_internal const&, mozilla::MediaDecoderOwner*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/dom/media/DecoderTraits.cpp:598
    #10 0x7f35528c9278 in mozilla::dom::HTMLMediaElement::InitializeDecoderForChannel(nsIChannel*, nsIStreamListener**) /builds/slave/m-in-l64-asan-0000000000000000/build/src/dom/html/HTMLMediaElement.cpp:2811
    #11 0x7f35528c895d in mozilla::dom::HTMLMediaElement::MediaLoadListener::OnStartRequest(nsIRequest*, nsISupports*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/dom/html/HTMLMediaElement.cpp:393
    #12 0x7f354e0fd20b in nsBaseChannel::OnStartRequest(nsIRequest*, nsISupports*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/netwerk/base/nsBaseChannel.cpp:800
    #13 0x7f354e145c7d in nsInputStreamPump::OnStateStart() /builds/slave/m-in-l64-asan-0000000000000000/build/src/netwerk/base/nsInputStreamPump.cpp:525
    #14 0x7f354e145242 in nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/netwerk/base/nsInputStreamPump.cpp:427
    #15 0x7f354df45db9 in nsInputStreamReadyEvent::Run() /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/io/nsStreamUtils.cpp:94
    #16 0x7f354df8a8df in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/threads/nsThread.cpp:995
    #17 0x7f354e005bda in NS_ProcessNextEvent(nsIThread*, bool) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/glue/nsThreadUtils.cpp:297
    #18 0x7f354e9af939 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/glue/MessagePump.cpp:95
    #19 0x7f354e91b97c in RunInternal /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:234
    #20 0x7f354e91b97c in RunHandler /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:227
    #21 0x7f354e91b97c in MessageLoop::Run() /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:201
    #22 0x7f3553d23a17 in nsBaseAppShell::Run() /builds/slave/m-in-l64-asan-0000000000000000/build/src/widget/nsBaseAppShell.cpp:156
    #23 0x7f3555c6c7a2 in XRE_RunAppShell /builds/slave/m-in-l64-asan-0000000000000000/build/src/toolkit/xre/nsEmbedFunctions.cpp:789
    #24 0x7f354e91b97c in RunInternal /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:234
    #25 0x7f354e91b97c in RunHandler /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:227
    #26 0x7f354e91b97c in MessageLoop::Run() /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:201
    #27 0x7f3555c6be9a in XRE_InitChildProcess /builds/slave/m-in-l64-asan-0000000000000000/build/src/toolkit/xre/nsEmbedFunctions.cpp:625
    #28 0x48d760 in content_process_main(int, char**) /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/app/../contentproc/plugin-container.cpp:237
    #29 0x7f354b839ec4 (/lib/x86_64-linux-gnu/libc.so.6+0x21ec4)

Thread T28 (ImageBridgeChil) created by T0 (Web Content) here:
    #0 0x461945 in pthread_create /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_interceptors.cc:175
    #1 0x7f354e933504 in CreateThread /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/platform_thread_posix.cc:135
    #2 0x7f354e933504 in Create /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/platform_thread_posix.cc:146
    #3 0x7f354e933504 in base::Thread::StartWithOptions(base::Thread::Options const&) /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/thread.cc:94
    #4 0x7f354e93333e in base::Thread::Start() /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/thread.cc:83
    #5 0x7f354ff32a5d in mozilla::layers::ImageBridgeChild::StartUpInChildProcess(IPC::Channel*, int) /builds/slave/m-in-l64-asan-0000000000000000/build/src/gfx/layers/ipc/ImageBridgeChild.cpp:713
    #6 0x7f354f1b1766 in mozilla::dom::PContentChild::OnMessageReceived(IPC::Message const&) /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/ipc/ipdl/PContentChild.cpp:8427
    #7 0x7f354e9a8a9c in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/glue/MessageChannel.cpp:1461
    #8 0x7f354e9a644c in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message const&) /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/glue/MessageChannel.cpp:1381
    #9 0x7f354e9973c2 in mozilla::ipc::MessageChannel::OnMaybeDequeueOne() /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/glue/MessageChannel.cpp:1350
    #10 0x7f354e91cdf4 in RunTask /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:364
    #11 0x7f354e91cdf4 in MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:372
    #12 0x7f354e91dea7 in MessageLoop::DoWork() /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:459
    #13 0x7f354e9b0635 in mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/glue/MessagePump.cpp:284
    #14 0x7f354e91b97c in RunInternal /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:234
    #15 0x7f354e91b97c in RunHandler /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:227
    #16 0x7f354e91b97c in MessageLoop::Run() /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:201
    #17 0x7f3553d23a17 in nsBaseAppShell::Run() /builds/slave/m-in-l64-asan-0000000000000000/build/src/widget/nsBaseAppShell.cpp:156
    #18 0x7f3555c6c7a2 in XRE_RunAppShell /builds/slave/m-in-l64-asan-0000000000000000/build/src/toolkit/xre/nsEmbedFunctions.cpp:789
    #19 0x7f354e91b97c in RunInternal /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:234
    #20 0x7f354e91b97c in RunHandler /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:227
    #21 0x7f354e91b97c in MessageLoop::Run() /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:201
    #22 0x7f3555c6be9a in XRE_InitChildProcess /builds/slave/m-in-l64-asan-0000000000000000/build/src/toolkit/xre/nsEmbedFunctions.cpp:625
    #23 0x48d760 in content_process_main(int, char**) /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/app/../contentproc/plugin-container.cpp:237
    #24 0x7f354b839ec4 (/lib/x86_64-linux-gnu/libc.so.6+0x21ec4)

SUMMARY: AddressSanitizer: heap-use-after-free /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/dist/include/mozilla/Mutex.h:69 Lock
Shadow bytes around the buggy address:
  0x0c0c8008d790: fa fa fa fa fd fd fd fd fd fd fd fa fa fa fa fa
  0x0c0c8008d7a0: fa fa fa fa fa fa fa fa fa fa fa fa fd fd fd fd
  0x0c0c8008d7b0: fd fd fd fa fa fa fa fa 00 00 00 00 00 00 00 00
  0x0c0c8008d7c0: fa fa fa fa fd fd fd fd fd fd fd fa fa fa fa fa
  0x0c0c8008d7d0: fd fd fd fd fd fd fd fa fa fa fa fa fd fd fd fd
=>0x0c0c8008d7e0: fd fd[fd]fd fa fa fa fa fd fd fd fd fd fd fd fa
  0x0c0c8008d7f0: fa fa fa fa fd fd fd fd fd fd fd fa fa fa fa fa
  0x0c0c8008d800: fd fd fd fd fd fd fd fa fa fa fa fa fd fd fd fd
  0x0c0c8008d810: fd fd fd fa fa fa fa fa fd fd fd fd fd fd fd fa
  0x0c0c8008d820: fa fa fa fa fd fd fd fd fd fd fd fa fa fa fa fa
  0x0c0c8008d830: fd fd fd fd fd fd fd fa fa fa fa fa 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 right 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
  Contiguous container OOB:fc
  ASan internal:           fe

Comment 1

[Christoph Diehl [:posidron]]

Attachment: Testcase

Comment 2

[Josh Matthews [:jdm]]

Summary - IPC error notification causes PImageContainerChild to be deallocated, but the destructor for the ImageContainer that's stored in the VideoFrameContainer later tries to the use the IPC actor.

Comment 3

[Andrew McCreight [:mccr8]]

Nicolas, this sec-high use after free has been sitting around for almost two months. Are you going to have time to look at this or should I look for somebody else to investigate it? Thanks.

Comment 4

[Nicolas Silva [:nical]]

I am working on a fix.

Comment 5

[Nicolas Silva [:nical]]

(In reply to Andrew McCreight [:mccr8] from comment #3)
> Nicolas, this sec-high use after free has been sitting around for almost two
> months. Are you going to have time to look at this or should I look for
> somebody else to investigate it? Thanks.

Actually, this is tricky. This is a shutdown crash where we try to use the ImageBridge after it's been shut down already. The only reliable way to fix this is to make sure that the media element that holds the ImageContainerChild (and other gfx stuff) is destroyed before shutting gfx down.
I have a patch that papers over this specific crash but it's pointless because if we get into this situation we will crash shortly after with another gfx resource being used past gfx shutdown (most likely a TextureClient).

This is also what blocks bug 1215265.

Comment 6

[Nicolas Silva [:nical]]

cc Jean-Yves who helped a lot with fixing the late shutdown of some of the media stuff.

Comment 7

[Nicolas Silva [:nical]]

Bug 1215265 Attachment 8735902 [details] [diff] should help with this.

Comment 8

[Nicolas Silva [:nical]]

Attachment: Release ImageContainer's ipc resource during media shutdown.

This is a bit of a shot in the dark. This patch assumes that media shutdown correctly goes through media resources to release all of the textures, and that the only required addition is to release the ImageContainer's IPDL actor as well.
So the patch tries to do that by plugging into ReleaseMediaResources and and calling DetachIPC() on the VideoFrameContainer/ImageContainer. I may be missing places where it should be called (this was mostly driven by grepping around).

I am not sure that this will fix 100% of the problems, but releasing the PImageContainerChild should happen during MediaShutdown and I haven't found code that explicitly does that.

Comment 9

[Nicolas Silva [:nical]]

Comment on attachment 8736301 [details] [diff] [review]
Release ImageContainer's ipc resource during media shutdown.

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

::: dom/media/MediaDecoder.cpp
@@ +653,5 @@
>    ChangeState(PLAY_STATE_SHUTDOWN);
>  
>    MediaShutdownManager::Instance().Unregister(this);
>  
> +  mVideoFrameContainer->DetachIPC();

erratum: added a null check for mVideoFrameContainer.

::: dom/media/MediaDecoderStateMachine.cpp
@@ +1351,5 @@
>    }
>  
>    DECODER_LOG("Shutdown started");
>  
> +  mVideoFrameContainer->DetachIPC();

erratum: added a null check for mVideoFrameContainer.

Comment 10

[Jean-Yves Avenard [:jya]]

Comment on attachment 8736301 [details] [diff] [review]
Release ImageContainer's ipc resource during media shutdown.

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

I don't see how that could work.

I believe the VideoFrameContainer is shared across MediaDecoder, MDSM and MediaFormatReader. But those are all accessed on different threads.

First, it seems to me that if you call ImageContainer::DetachIPC(); more than once, you'll end up with a null deref as mIPDLChild is now null.

Shouldn't VideoFrameContainer::mVideoFrameContainer be set to null after a call to VideoFrameContainer::DetachIPC()?

Seconds, this doesn't seem thread safe to me at all. In the event VideoFrameContainer::DetachIPC is called simultaneously between the MediaDecoder, the MDSM or the MediaFormatReader, you end up with a race when accessing VideoFrameContainer::mImageContainer and ImageContainer::mIPDLChild and ImageContainer::mImageClient. If you do clear VideoFrameContainer::mVideoFrameContainer after a call to VideoFrameContainer::DetachIPC() then I believe you should have a monitor protecting this operation.

JW knows the shutdown process and MediaDecoder better than I do. He should be the one reviewing this.

I think the easiest approach would be to only call DetachIPC in the MediaDecoderReader::Shutdown()

::: dom/media/MediaFormatReader.cpp
@@ +1639,5 @@
>  {
>    // Before freeing a video codec, all video buffers needed to be released
>    // even from graphics pipeline.
>    if (mVideoFrameContainer) {
> +    mVideoFrameContainer->DetachIPC();

We get here when we enter dormant mode. We aren't shutting down per say.

Can we resume once mVideoFrameContainer has been detached?

Comment 11

[Jean-Yves Avenard [:jya]]

(In reply to Nicolas Silva [:nical] from comment #5)
> Actually, this is tricky. This is a shutdown crash where we try to use the
> ImageBridge after it's been shut down already. The only reliable way to fix
> this is to make sure that the media element that holds the
> ImageContainerChild (and other gfx stuff) is destroyed before shutting gfx
> down.

I'm fairly certain this is already the case following bug 1207220.

gfx is shutdown there:
https://dxr.mozilla.org/mozilla-central/source/xpcom/build/XPCOMInit.cpp#848

while media is shutdown via the MediaShutdownManager there:
https://dxr.mozilla.org/mozilla-central/source/xpcom/build/XPCOMInit.cpp#840

this operation observerService->NotifyObservers is synchronous. It will not exit this loop until the MediaShutdownManager has fully shut down and there's nothing left holding a reference to the image container.

gfx is only shutdown once all the media objects have shutdown.

So I don't think this patch will do anything to address the issue.

Comment 12

[Nicolas Silva [:nical]]

(In reply to Jean-Yves Avenard [:jya] from comment #10)
> Comment on attachment 8736301 [details] [diff] [review]
> Release ImageContainer's ipc resource during media shutdown.
> 
> Review of attachment 8736301 [details] [diff] [review]:
> -----------------------------------------------------------------
> I believe the VideoFrameContainer is shared across MediaDecoder, MDSM and
> MediaFormatReader. But those are all accessed on different threads.

I don't have a good picture of what happens on what threads so yeah, the patch is apparently not good in its current shape. The point I'd want to get to after the threading issues are figured out is for ImageContainer to release its ipdl actor before the end of the media shutdown.

> 
> First, it seems to me that if you call ImageContainer::DetachIPC(); more
> than once, you'll end up with a null deref as mIPDLChild is now null.

In DetachIPC we null out mImageContainerChild which means IsAsync() will return false the second time.

But it falls down if DetachIPC is called on multiple threads in parallel, obviously. 


> 
> Shouldn't VideoFrameContainer::mVideoFrameContainer be set to null after a
> call to VideoFrameContainer::DetachIPC()?
> 
> Seconds, this doesn't seem thread safe to me at all. [...] 

Ideally we'd call DetachIPC only once no matter on which thread. 

> I think the easiest approach would be to only call DetachIPC in the
> MediaDecoderReader::Shutdown()

Sounds good to me.

> [...]
> We get here when we enter dormant mode. We aren't shutting down per say.
> 
> Can we resume once mVideoFrameContainer has been detached?

Ok, this part is bad as well. We can't resume after that. The idea is to call DetachIPC during shutdown only.

Comment 13

[Jean-Yves Avenard [:jya]]

(In reply to Nicolas Silva [:nical] from comment #12)

> In DetachIPC we null out mImageContainerChild which means IsAsync() will
> return false the second time.

you mean mImageContainer right?

Ok that's good. But it's still not threadsafe, access to ImageContainer::mImageClient (from ImageContainer::IsAsync()) is done on multiple threads with no monitor.

(assuming you allow VideoFrameContainer::DetachIPC() to be called on any threads as your current patch does)

Comment 14

[Jean-Yves Avenard [:jya]]

(In reply to Nicolas Silva [:nical] from comment #12)

> patch is apparently not good in its current shape. The point I'd want to get
> to after the threading issues are figured out is for ImageContainer to
> release its ipdl actor before the end of the media shutdown.

why would the ipdl actor need to be cleared *before* gfx being shutdown or before the end of the media shutdown?

Why can't the gfx clear that when shutting down?

again, there's nothing left of media by the time gfxPlatform::ShutdownLayersIPC(); is called in ShutdownXPCOM

Comment 15

[Nicolas Silva [:nical]]

(In reply to Jean-Yves Avenard [:jya] from comment #13)
> (In reply to Nicolas Silva [:nical] from comment #12)
> 
> > In DetachIPC we null out mImageContainerChild which means IsAsync() will
> > return false the second time.
> 
> you mean mImageContainer right?

Sorry, I meant ImageContainer::mImageClient.

Comment 16

[Nicolas Silva [:nical]]

(In reply to Jean-Yves Avenard [:jya] from comment #14)
> why would the ipdl actor need to be cleared *before* gfx being shutdown or
> before the end of the media shutdown?
> 
> Why can't the gfx clear that when shutting down?

I'll look into that, but generally, it means that the destruction of the IPDLChild can be scheduled concurrently from other threads, since some ImageContainers are still alive past the media shutdown and I don't know for sure on which thread their destructor may run. I would like to avoid breaking the bi-directional reference between ImageContainer and ImageContainerChild concurrently. Actually, maybe the only thing that may hold on to ImageContainers past the media shutdown is cycle collected HTMLMediaElements and we can assume the destructor will only run on the main thread past that point which makes it simpler.

> 
> again, there's nothing left of media by the time
> gfxPlatform::ShutdownLayersIPC(); is called in ShutdownXPCOM

HTMLMediaElements are cycle collected and they appear to be able to maintain ImageContainers alive past shutdown. If you look at the stack here https://treeherder.mozilla.org/logviewer.html#?job_id=18592065&repo=try the HTMLMediaElement ends up being destroyed late during the cycle collector's shutdown.

Comment 17

[Jean-Yves Avenard [:jya]]

(In reply to Nicolas Silva [:nical] from comment #16)

> HTMLMediaElements are cycle collected and they appear to be able to maintain
> ImageContainers alive past shutdown. If you look at the stack here
> https://treeherder.mozilla.org/logviewer.html#?job_id=18592065&repo=try the
> HTMLMediaElement ends up being destroyed late during the cycle collector's
> shutdown.

Then it's the HTMLMediaElement::mVideoFrameContainer that you need to null out. Doing anything in MediaDecoder / MDSM or MediaFormatReader isn't going to help (those aren't cycle collected)

Comment 18

[Nicolas Silva [:nical]]

(In reply to Jean-Yves Avenard [:jya] from comment #17)
> Then it's the HTMLMediaElement::mVideoFrameContainer that you need to null
> out. Doing anything in MediaDecoder / MDSM or MediaFormatReader isn't going
> to help (those aren't cycle collected)

Ok, I assumed that they were referring to the same ImageContainers and that it would be enough.

So, to summarize this discussion: the preferred approach is to make sure that all (async) ImageContainers are destroyed/nulled-out when gfx shuts down.
I'll give it another shot with this in mind, any help is very welcome!

Comment 19

[Nicolas Silva [:nical]]

Attachment: null out ImageContainers during shutdown

This patch tries to get rid of ImageContainers during shutdown in the hope that none of them will keep a PImageContainerChild past the shutdown. This focuses on HMTLMediaElement and MediaDecoder (the latter can be kept alive by the former and also holds on to a VideoFrameContainer), so there may be other places where this kind of thing needs to be done as well.

All of the added code is run only on the main thread, supposedly at a point where no other media related thread should be able to do things concurrently.

I had to remove the constness of MediaDecoder::mVideoFrameContainer which is not very nice, but the alternatives were:
 - To make it possible to null out VideoFrameContainer::mImageContainer which sounds worse because all of VideoFrameContainer's methids expect mImageContainer to never be null.
 - To make sure MediaDecoder itself gets deleted at this point, but it means removing all HTMLMediaElements from the global table, since the latter needs HTMLMediaElement::mDecoder to not be null, and generally it ends up venturing into more stuff that I don't know at all.

Comment 20

[Nicolas Silva [:nical]]

Hm, so there are other things holding on to VideoFrameContainers and triggering the issue (like MediaFormatReader), I'll update the patch as I find them. I can reproduce this very intermittently so it's going to take a while.
Still interested in confirming whether or not this approach is the one you guys want to pursue, though. The risk here is that as soon as someone adds a strong reference to a VideoFrameContainer or an ImageContainer, it can create an intermittent shutdown bug.

Comment 21

[Nicolas Silva [:nical]]

Attachment: Don't delete an ImageContainerChild if its ImageContainer is still holding on to it

Complementary approach (or perhaps alternative). This specifically addresses the crash stack on this bug (trying to destroy the ipdl actor too late). If we do anything else with the actor we'll run into issues, but in theory the only thing that hurts us is the liveness of objects past shutdown so we shouldn't run into them being *used* past shutdown. We'll also have problems if a late ImageContainer is holding on to textures, but Jean-Yves being confident that this is a thing of the past, I am more than happy to believe him!

Comment 22

[Jean-Yves Avenard [:jya]]

Comment on attachment 8736687 [details] [diff] [review]
null out ImageContainers during shutdown

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

You only need to null the HTMLMediaElement::mVideoFrameContainer from MediaDecoder::FinishShutdown(), no need to loop in a static function. Every HTMLMediaElement own a MediaDecoder, if not mVideoFrameContainer is already null.

But having said that, the issue is in the ImageContainer destructor. It holds a weak reference to an ImageContainerChild (mIPDLChild) and attempts to use it in its destructor even if it has already been destroyed.

Doesn't gfx hold a list of all existing ImageContainer somewhere?

Comment 23

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8736687 [details] [diff] [review]
null out ImageContainers during shutdown

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

::: dom/media/MediaDecoder.cpp
@@ +693,5 @@
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>    mDecoderStateMachine->BreakCycles();
>    SetStateMachine(nullptr);
> +  mVideoFrameContainer = nullptr;

MediaShutdownManager::Shutdown() ensures all MediaDecoder instances will be deleted before return. It is pointless to nullify the member here since the destructor of MediaDecoder will release the reference.

Comment 24

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Jean-Yves Avenard [:jya] from comment #22)
> But having said that, the issue is in the ImageContainer destructor. It
> holds a weak reference to an ImageContainerChild (mIPDLChild) and attempts
> to use it in its destructor even if it has already been destroyed.
> 
> Doesn't gfx hold a list of all existing ImageContainer somewhere?

As jya said, the crash can be fixed if an ImageContainer knows whether mIPDLChild is deleted or not.

Comment 25

[JW Wang [:jwwang] [:jw_wang]]

All the work here is ensuring all VideoFrameContainers are deleted before IPC error happens so we won't access mIPDLChild after it is deleted implicitly by OnNotifyMaybeChannelError(). However, I think the real problem is the poor life cycle management of mIPDLChild.

Comment 26

[Jean-Yves Avenard [:jya]]

Couldn't you use a proper weak pointer rather than a raw pointer to detect that the object has been deleted. Xpcom provides such weak pointer nsWeakPtr (similar to std::weak_ptr). 
I'm guessing you used raw pointer to avoid reference cycles

Comment 27

[Nicolas Silva [:nical]]

(In reply to JW Wang [:jwwang] from comment #23)
> MediaShutdownManager::Shutdown() ensures all MediaDecoder instances will be
> deleted before return. It is pointless to nullify the member here since the
> destructor of MediaDecoder will release the reference.

It probably tries to ensure that, but the reality is that some MediaDecoders are still alive past the shutdown, see comment 16 for example.

(In reply to Jean-Yves Avenard [:jya] from comment #26)
> Couldn't you use a proper weak pointer rather than a raw pointer to detect
> that the object has been deleted. Xpcom provides such weak pointer nsWeakPtr
> (similar to std::weak_ptr). 

As far as I know (correct me if it's changed) our weak pointers are not thread-safe.


OK, I give up about enforcing that PImageContainerChild actors must be deleted before IPDL shuts down. It will bite us as soon as we add a bit of complexity to PImageContainerChild, but attachment 8736709 [details] [diff] [review] works around this crash and will do for now.

Comment 28

[Jean-Yves Avenard [:jya]]

(In reply to Nicolas Silva [:nical] from comment #27)
> (In reply to JW Wang [:jwwang] from comment #23)
> > MediaShutdownManager::Shutdown() ensures all MediaDecoder instances will be
> > deleted before return. It is pointless to nullify the member here since the
> > destructor of MediaDecoder will release the reference.
> 
> It probably tries to ensure that, but the reality is that some MediaDecoders
> are still alive past the shutdown, see comment 16 for example.

I can well believe that for HTMLMediaElement which are cycle counted.

But not MediaDecoder.

Comment 29

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8736709 [details] [diff] [review]
Don't delete an ImageContainerChild if its ImageContainer is still holding on to it

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

The patch looks good.

::: gfx/layers/ImageContainer.cpp
@@ +133,3 @@
>  };
>  
> +void ImageContainer::DeallocActor(PImageContainerChild* aActor)

// nit. It seems nice if the function has a comment that it is static like "/* static */".

@@ +144,5 @@
> +    actor->mIPCOpen = false;
> +  }
> +}
> +
> +void ImageContainer::AsyncDestroyActor(PImageContainerChild* aActor)

// nit. It seems nice if the function has a comment that it is static like "/* static */".

Comment 30

[Nicolas Silva [:nical]]

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

Comment 31

[Ryan VanderMeulen [:RyanVM]]

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

Comment 32

[Daniel Veditz [:dveditz]]

[Tracking Requested - why for this release]:
This didn't go through the sec-approval process. What branches are affected? It was reported in Firefox 47 so at least that one needs the fix, but does it affect Firefox 46 or either of the supported ESR branches?

Comment 33

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

We need the sec-approval? flag set on the patch and the template for sec-approval filled out so we can know what we need to do here to avoid exposing ourselves now that this is checked in.

Comment 34

[Nicolas Silva [:nical]]

Comment on attachment 8736709 [details] [diff] [review]
Don't delete an ImageContainerChild if its ImageContainer is still holding on to it

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

I don't think an exploit can be constructed easily, if at all (though someone who knows more about this topic could make a more educated statement, here).
The state of the objects involved in this UAF are independent of the web content itself. Web content can cause them to exist, but it can't see them in any form and has no effect on the values they store and on whether or not the destruction of the two objects will happen in the wrong order.
Even if this UAF was systematic, it can only happen during the shutdown of the process (either normal or abnormal shutdown), so one can't repeatedly trigger the bug to enhance the likelihood of something bad happening. Moreover, the two objects are destroyed in the wrong order but no script or other kind of web-served content is run between the destruction of the two objects, so no occasion for web content to attempt to allocate something after one of the objects is destroyed and before the other one uses its memory. And no web content will run after the first object is destroyed at all because the process is already shutting down.


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?

My guess is that this has been theoretically possible since gecko 42, although its likelihood may have fluctuated and if it has I don't know because of what. 

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

My guess is bug 1143575.

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

Not yet. I would confident about an aurora uplift since the patch has been in central for 10 days without causing trouble, I wouldn't uplift/backport it further.

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

Unlikely. Manual testing resources are probably better spent elsewhere. I think that our nightly population is enough testing in this case.

Comment 35

[Daniel Veditz [:dveditz]]

Christoph: please verify that this fixes the bug. Since IPC is implicated does this crash require e10s? If so then we don't need to worry about ESR-45

Comment 36

[Nicolas Silva [:nical]]

(In reply to Daniel Veditz [:dveditz] from comment #35)
> Christoph: please verify that this fixes the bug. Since IPC is implicated
> does this crash require e10s? If so then we don't need to worry about ESR-45

Sorry, I say "IPC" when I should be saying "IPDL" and this also applies to non-e10s shutdown.

Comment 37

[Christoph Diehl [:posidron]]

(In reply to Daniel Veditz [:dveditz] from comment #35)
> Christoph: please verify that this fixes the bug. Since IPC is implicated
> does this crash require e10s? If so then we don't need to worry about ESR-45

It seems so, I have also not seen this anymore on our cluster.

Comment 38

[Sylvestre Ledru [:Sylvestre]]

Not affecting esr45 if I understood correctly, right?

Comment 39

[Nicolas Silva [:nical]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #38)
> Not affecting esr45 if I understood correctly, right?

Theoretically it does affect 45. In practice I don't know. I think that the risk of this being exploitable is fairly low (see comment 34, although I would be interested to hear what someone more experienced about security than me thinks about that). I Also think that writing and uplifting a patch that applies that far back is riskier than not patching esr 45.

Comment 40

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hello Nicolas, this is a sec-high issue that affects Fx47. Could you please nominate for uplift to Beta? Thanks!

Comment 41

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

Sotaro reviewed this code and Nical is on PTO, may save us a day for Sotaro to nominate and explain.

Comment 42

[Nicolas Silva [:nical]]

Comment on attachment 8736709 [details] [diff] [review]
Don't delete an ImageContainerChild if its ImageContainer is still holding on to it

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Shutdown use-after-free crashes
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: Not too risky. This patch is touching trickier stuff than what I'm usually happy with uplifting to beta, but it applies cleanly on the branch and has been on central and aurora for a while now without causing any issue, so I feel good about uplifting.
[String/UUID change made/needed]: none.

Comment 43

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8736709 [details] [diff] [review]
Don't delete an ImageContainerChild if its ImageContainer is still holding on to it

Sec-high, baked in Nightly for a month, Beta47+

Comment 44

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/39fce55feada