1064117 - intermittent AddressSanitizer: heap-use-after-free content/media/../../dist/include/nsAutoPtr.h:1017 get

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect)

Description

[Karl Tomlinson (:karlt)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=47575268&full=1&branch=mozilla-inbound#error0

==1770==ERROR: AddressSanitizer: heap-use-after-free on address 0x60b0004e1230 at pc 0x7f5658e80f90 bp 0x7f5600057630 sp 0x7f5600057628
READ of size 8 at 0x60b0004e1230 thread T3134 (MediaStreamGrph)
    #0 0x7f5658e80f8f in get /builds/slave/m-in-l64-asan-0000000000000000/build/obj-firefox/content/media/../../dist/include/nsAutoPtr.h:1017
    #1 0x7f5658e80f8f in operator mozilla::GraphDriver * /builds/slave/m-in-l64-asan-0000000000000000/build/obj-firefox/content/media/../../dist/include/nsAutoPtr.h:1030
    #2 0x7f5658e80f8f in mozilla::ThreadedDriver::RunThread() /builds/slave/m-in-l64-asan-0000000000000000/build/content/media/GraphDriver.cpp:297
    #3 0x7f5658e9bc02 in mozilla::MediaStreamGraphInitThreadRunnable::Run() /builds/slave/m-in-l64-asan-0000000000000000/build/content/media/GraphDriver.cpp:214
    #4 0x7f5654c5c9b1 in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/m-in-l64-asan-0000000000000000/build/xpcom/threads/nsThread.cpp:823
    #5 0x7f5654cb9cea in NS_ProcessNextEvent(nsIThread*, bool) /builds/slave/m-in-l64-asan-0000000000000000/build/xpcom/glue/nsThreadUtils.cpp:265
    #6 0x7f56554ae3a7 in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) /builds/slave/m-in-l64-asan-0000000000000000/build/ipc/glue/MessagePump.cpp:326
    #7 0x7f565545db00 in RunInternal /builds/slave/m-in-l64-asan-0000000000000000/build/ipc/chromium/src/base/message_loop.cc:229
    #8 0x7f565545db00 in RunHandler /builds/slave/m-in-l64-asan-0000000000000000/build/ipc/chromium/src/base/message_loop.cc:222
    #9 0x7f565545db00 in MessageLoop::Run() /builds/slave/m-in-l64-asan-0000000000000000/build/ipc/chromium/src/base/message_loop.cc:196
    #10 0x7f5654c596f5 in nsThread::ThreadFunc(void*) /builds/slave/m-in-l64-asan-0000000000000000/build/xpcom/threads/nsThread.cpp:350
    #11 0x7f566b14d405 in _pt_root /builds/slave/m-in-l64-asan-0000000000000000/build/nsprpub/pr/src/pthreads/ptthread.c:212
    #12 0x7f566e88ae99 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x7e99)
    #13 0x7f566d99bdbc (/lib/x86_64-linux-gnu/libc.so.6+0xf3dbc)
0x60b0004e1230 is located 80 bytes inside of 104-byte region [0x60b0004e11e0,0x60b0004e1248)
freed by thread T0 here:
    #0 0x470d21 in __interceptor_free /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:64
    #1 0x7f5658ee624e in Release /builds/slave/m-in-l64-asan-0000000000000000/build/content/media/GraphDriver.h:77
    #2 0x7f5658ee624e in ~nsRefPtr /builds/slave/m-in-l64-asan-0000000000000000/build/obj-firefox/content/media/../../dist/include/nsAutoPtr.h:852
    #3 0x7f5658ee624e in mozilla::MediaStreamGraphImpl::~MediaStreamGraphImpl() /builds/slave/m-in-l64-asan-0000000000000000/build/content/media/MediaStreamGraph.cpp:74
    #4 0x7f5658ee726d in mozilla::MediaStreamGraphImpl::~MediaStreamGraphImpl() /builds/slave/m-in-l64-asan-0000000000000000/build/content/media/MediaStreamGraph.cpp:69
    #5 0x7f5658effabc in mozilla::MediaStreamGraphImpl::Release() /builds/slave/m-in-l64-asan-0000000000000000/build/content/media/MediaStreamGraph.cpp:2822
    #6 0x7f5658f223f7 in ~nsRefPtr /builds/slave/m-in-l64-asan-0000000000000000/build/obj-firefox/content/media/../../dist/include/nsAutoPtr.h:852
    #7 0x7f5658f223f7 in ~MediaStreamGraphShutDownRunnable /builds/slave/m-in-l64-asan-0000000000000000/build/content/media/MediaStreamGraph.cpp:1471
    #8 0x7f5658f223f7 in mozilla::(anonymous namespace)::MediaStreamGraphShutDownRunnable::~MediaStreamGraphShutDownRunnable() /builds/slave/m-in-l64-asan-0000000000000000/build/content/media/MediaStreamGraph.cpp:1471
previously allocated by thread T0 here:
    #0 0x470f21 in malloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:74
    #1 0x7f5667631bed in moz_xmalloc /builds/slave/m-in-l64-asan-0000000000000000/build/memory/mozalloc/mozalloc.cpp:52
    #2 0x7f5658eff2cb in operator new /builds/slave/m-in-l64-asan-0000000000000000/build/obj-firefox/content/media/../../dist/include/mozilla/mozalloc.h:201
    #3 0x7f5658eff2cb in mozilla::MediaStreamGraphImpl::MediaStreamGraphImpl(bool, int, unsigned char, mozilla::dom::AudioChannel) /builds/slave/m-in-l64-asan-0000000000000000/build/content/media/MediaStreamGraph.cpp:2735
    #4 0x7f5658eff6eb in mozilla::MediaStreamGraph::CreateNonRealtimeInstance(int) /builds/slave/m-in-l64-asan-0000000000000000/build/content/media/MediaStreamGraph.cpp:2798
    #5 0x7f5658feafff in mozilla::dom::AudioDestinationNode::AudioDestinationNode(mozilla::dom::AudioContext*, bool, mozilla::dom::AudioChannel, unsigned int, unsigned int, float) /builds/slave/m-in-l64-asan-0000000000000000/build/content/media/webaudio/AudioDestinationNode.cpp:324
Thread T3134 (MediaStreamGrph) created by T0 here:
    #0 0x45d795 in __interceptor_pthread_create /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_interceptors.cc:175
    #1 0x7f566b149d8d in _PR_CreateThread /builds/slave/m-in-l64-asan-0000000000000000/build/nsprpub/pr/src/pthreads/ptthread.c:453
    #2 0x7f566b14990a in PR_CreateThread /builds/slave/m-in-l64-asan-0000000000000000/build/nsprpub/pr/src/pthreads/ptthread.c:544
    #3 0x7f5654c5ac0b in nsThread::Init() /builds/slave/m-in-l64-asan-0000000000000000/build/xpcom/threads/nsThread.cpp:455
    #4 0x7f5654c600fc in nsThreadManager::NewThread(unsigned int, unsigned int, nsIThread**) /builds/slave/m-in-l64-asan-0000000000000000/build/xpcom/threads/nsThreadManager.cpp:269
    #5 0x7f5654cb941c in NS_NewThread(nsIThread**, nsIRunnable*, unsigned int) /builds/slave/m-in-l64-asan-0000000000000000/build/xpcom/glue/nsThreadUtils.cpp:68
    #6 0x7f5658e7fd44 in NS_NewNamedThread<16> /builds/slave/m-in-l64-asan-0000000000000000/build/obj-firefox/content/media/../../dist/include/nsThreadUtils.h:74
    #7 0x7f5658e7fd44 in mozilla::ThreadedDriver::Start() /builds/slave/m-in-l64-asan-0000000000000000/build/content/media/GraphDriver.cpp:226
    #8 0x7f5658ef7dd8 in mozilla::MediaStreamGraphImpl::RunInStableState(bool) /builds/slave/m-in-l64-asan-0000000000000000/build/content/media/MediaStreamGraph.cpp:1673

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

ASAN UAF -> sec bug.  'this' being freed out from under it in a processing loop; sec-high or sec-critical

In RunThread(), the main driver loop:
    if (mNextDriver && stillProcessing) {

'this' appears to have been freed out from under us.

It was freed at mozilla::MediaStreamGraphImpl::~MediaStreamGraphImpl():74 or :69, from the ShutdownRunnable, on Mainthread.

Comment 3

[Paul Adenot (:padenot)]

Attachment: possible patch

Maybe this ?

The ASAN report shows that the thread for the OfflineAudioContext still runs
while we are deleting the graph. At this point mForceShutdown should be true, so
we just have to wait for the driver thread to exit and continue.

It's not clear to me if nsThread::Shutdown does a sync wait. It seems to do so.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8487166 [details] [diff] [review]
possible patch

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

r+; but I'd like to see a Try run.
nsThread::Shutdown() is absolutely synchronous

Comment 5

[Paul Adenot (:padenot)]

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

Comment 6

[Karl Tomlinson (:karlt)]

(In reply to Paul Adenot (:padenot) from comment #3)
> It's not clear to me if nsThread::Shutdown does a sync wait. It seems to do
> so.

It won't return until the thread is shut down.

However, I wouldn't usually describe running main (current) thread events as a "wait".

The world may be quite different when Shutdown() returns.

Comment 8

[Paul Adenot (:padenot)]

Ok, so that's what I want I think.

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Yup - the target thread is shut down when it returned, which is what I meant by synchronous and "wait"

Comment 10

[Ryan VanderMeulen [:RyanVM]]

FWIW, this appears to reproduce pretty reliably on ASAN e10s runs (hidden by default on TBPL).
https://tbpl.mozilla.org/?showall=1&jobname=Ubuntu%20ASAN%20VM%2012.04%20x64%20mozilla-inbound%20opt%20test%20mochitest-e10s-1&tree=Mozilla-Inbound

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Are we ready to land this?  Since it's in 34, we'll need security approval

This should only apply to 34 & 35 as it was due to bug 848954

Comment 12

[Paul Adenot (:padenot)]

Comment on attachment 8487166 [details] [diff] [review]
possible patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not easily, this is a quite rare race condition.

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?
34, 35

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

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

How likely is this patch to cause regressions; how much testing does it need?
It's green on try,

Comment 13

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

Comment on attachment 8487166 [details] [diff] [review]
possible patch

Sec-approval+ for trunk. Let's get this on Aurora as well.

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8487166 [details] [diff] [review]
possible patch

Approval Request Comment
[Feature/regressing bug #]: bug 848954

[User impact if declined]: sec UAF

[Describe test coverage new/current, TBPL]: see s-a request

[Risks and why]: see s-a request

[String/UUID change made/needed]: none

Comment 16

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

Comment on attachment 8487166 [details] [diff] [review]
possible patch

Thanks, Mr. Jesup!

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/938f95281dd3

Comment 18

[Phil Ringnalda (:philor)]

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

Comment 19

[Kamil Jozwiak [:kjozwiak]]

Marking this as qe‑verify- as there's no STR/POC attached. Quickly talked to Paul via IRC to double check and see if there's anything QE can do here in terms of verification.