Jason Kratzer [:jkratzer] Reporter
	Description • 7 years ago

Testcase found while fuzzing mozilla-central rev 6d82e132348f.

==4360==ERROR: AddressSanitizer: heap-use-after-free on address 0x6150002ede48 at pc 0x7f107ecf555f bp 0x7ffe1c4fa1a0 sp 0x7ffe1c4fa198
READ of size 8 at 0x6150002ede48 thread T0 (file:// Content)
    #0 0x7f107ecf555e in ~lock_block /builds/worker/workspace/build/src/obj-firefox/dist/include/mtransport/sigslot.h:318:13
    #1 0x7f107ecf555e in operator() /builds/worker/workspace/build/src/obj-firefox/dist/include/mtransport/sigslot.h:2424
    #2 0x7f107ecf555e in mozilla::PeerConnectionMedia::IceGatheringStateChange_m(mozilla::NrIceCtx*, mozilla::NrIceCtx::GatheringState) /builds/worker/workspace/build/src/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:1359
    #3 0x7f107ed45472 in apply<mozilla::PeerConnectionMedia *, void (mozilla::PeerConnectionMedia::*)(mozilla::NrIceCtx *, mozilla::NrIceCtx::GatheringState), mozilla::NrIceCtx *, mozilla::NrIceCtx::GatheringState, 0, 1> /builds/worker/workspace/build/src/obj-firefox/dist/include/mtransport/runnable_utils.h:104:5
    #4 0x7f107ed45472 in mozilla::runnable_args_memfn<mozilla::PeerConnectionMedia*, void (mozilla::PeerConnectionMedia::*)(mozilla::NrIceCtx*, mozilla::NrIceCtx::GatheringState), mozilla::NrIceCtx*, mozilla::NrIceCtx::GatheringState>::Run() /builds/worker/workspace/build/src/obj-firefox/dist/include/mtransport/runnable_utils.h:174
    #5 0x7f107cf713ee in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1033:14
    #6 0x7f107cf8cd50 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:508:10
    #7 0x7f107de1760a in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:97:21
    #8 0x7f107dd6afa9 in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10
    #9 0x7f107dd6afa9 in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319
    #10 0x7f107dd6afa9 in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299
    #11 0x7f10843a43da in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:157:27
    #12 0x7f1088adb7cb in XRE_RunAppShell() /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:875:22
    #13 0x7f107dd6afa9 in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10
    #14 0x7f107dd6afa9 in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319
    #15 0x7f107dd6afa9 in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299
    #16 0x7f1088adb1bd in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:701:34
    #17 0x4f2dfc in content_process_main /builds/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:63:30
    #18 0x4f2dfc in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:280
    #19 0x7f109c08c82f in __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:291
    #20 0x42243c in _start (/home/forb1dden/builds/mc-asan/firefox+0x42243c)

0x6150002ede48 is located 72 bytes inside of 456-byte region [0x6150002ede00,0x6150002edfc8)
freed by thread T0 (file:// Content) here:
    #0 0x4c2e92 in __interceptor_free /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:68:3
    #1 0x7f107ed44fa0 in apply<mozilla::PeerConnectionMedia *, void (mozilla::PeerConnectionMedia::*)()> /builds/worker/workspace/build/src/obj-firefox/dist/include/mtransport/runnable_utils.h:104:5
    #2 0x7f107ed44fa0 in mozilla::runnable_args_memfn<mozilla::PeerConnectionMedia*, void (mozilla::PeerConnectionMedia::*)()>::Run() /builds/worker/workspace/build/src/obj-firefox/dist/include/mtransport/runnable_utils.h:174
    #3 0x7f107cf713ee in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1033:14
    #4 0x7f107cf8cd50 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:508:10
    #5 0x7f1083b23b2c in SpinEventLoopUntil<mozilla::ProcessFailureBehavior::ReportToCaller, (lambda at /builds/worker/workspace/build/src/dom/ipc/ContentChild.cpp:1091:24)> /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:323:25
    #6 0x7f1083b23b2c in mozilla::dom::ContentChild::ProvideWindowCommon(mozilla::dom::TabChild*, mozIDOMWindowProxy*, bool, unsigned int, bool, bool, bool, nsIURI*, nsTSubstring<char16_t> const&, nsTSubstring<char> const&, bool, nsIDocShellLoadInfo*, bool*, mozIDOMWindowProxy**) /builds/worker/workspace/build/src/dom/ipc/ContentChild.cpp:1091
    #7 0x7f1083baed1c in mozilla::dom::TabChild::ProvideWindow(mozIDOMWindowProxy*, unsigned int, bool, bool, bool, nsIURI*, nsTSubstring<char16_t> const&, nsTSubstring<char> const&, bool, nsIDocShellLoadInfo*, bool*, mozIDOMWindowProxy**) /builds/worker/workspace/build/src/dom/ipc/TabChild.cpp:1061:16
    #8 0x7f1088a2aedb in nsWindowWatcher::OpenWindowInternal(mozIDOMWindowProxy*, char const*, char const*, char const*, bool, bool, bool, nsIArray*, bool, bool, nsIDocShellLoadInfo*, mozIDOMWindowProxy**) /builds/worker/workspace/build/src/toolkit/components/windowwatcher/nsWindowWatcher.cpp:852:24
    #9 0x7f1088a308cc in OpenWindow2 /builds/worker/workspace/build/src/toolkit/components/windowwatcher/nsWindowWatcher.cpp:444:10
    #10 0x7f1088a308cc in non-virtual thunk to nsWindowWatcher::OpenWindow2(mozIDOMWindowProxy*, char const*, char const*, char const*, bool, bool, bool, nsISupports*, bool, bool, nsIDocShellLoadInfo*, mozIDOMWindowProxy**) /builds/worker/workspace/build/src/toolkit/components/windowwatcher/nsWindowWatcher.cpp
    #11 0x7f107fecce64 in nsGlobalWindowOuter::OpenInternal(nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, bool, bool, bool, bool, bool, nsIArray*, nsISupports*, nsIDocShellLoadInfo*, bool, nsPIDOMWindowOuter**) /builds/worker/workspace/build/src/dom/base/nsGlobalWindowOuter.cpp:7193:21
    #12 0x7f107fecbc8d in OpenJS /builds/worker/workspace/build/src/dom/base/nsGlobalWindowOuter.cpp:5599:10
    #13 0x7f107fecbc8d in nsGlobalWindowOuter::OpenOuter(nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, mozilla::ErrorResult&) /builds/worker/workspace/build/src/dom/base/nsGlobalWindowOuter.cpp:5574
    #14 0x7f107fe6c022 in nsGlobalWindowInner::Open(nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, mozilla::ErrorResult&) /builds/worker/workspace/build/src/dom/base/nsGlobalWindowInner.cpp:3793:3
    #15 0x7f1081823204 in mozilla::dom::WindowBinding::open(JSContext*, JS::Handle<JSObject*>, nsGlobalWindowInner*, JSJitMethodCallArgs const&) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/WindowBinding.cpp:2189:56
    #16 0x7f1081821600 in mozilla::dom::WindowBinding::genericMethod(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/WindowBinding.cpp:15271:13
    #17 0x7f1088dc4464 in CallJSNative /builds/worker/workspace/build/src/js/src/jscntxtinlines.h:291:15
    #18 0x7f1088dc4464 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:473
    #19 0x7f1088daa548 in CallFromStack /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:528:12
    #20 0x7f1088daa548 in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3096
    #21 0x7f1088d96cb0 in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:423:12
    #22 0x7f1088dc7401 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:706:15
    #23 0x7f1088dc7b9f in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:738:12
    #24 0x7f10898d6646 in ExecuteScript(JSContext*, JS::AutoObjectVector&, JS::Handle<JSScript*>, JS::Value*) /builds/worker/workspace/build/src/js/src/jsapi.cpp:4678:12
    #25 0x7f10802efc66 in nsJSUtils::ExecutionContext::CompileAndExec(JS::CompileOptions&, JS::SourceBufferHolder&, JS::MutableHandle<JSScript*>) /builds/worker/workspace/build/src/dom/base/nsJSUtils.cpp:266:8
    #26 0x7f1084217f21 in mozilla::dom::ScriptLoader::EvaluateScript(mozilla::dom::ScriptLoadRequest*) /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp:2287:25
    #27 0x7f10842133c9 in mozilla::dom::ScriptLoader::ProcessRequest(mozilla::dom::ScriptLoadRequest*) /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp:1929:10
    #28 0x7f10841f6315 in mozilla::dom::ScriptLoader::ProcessScriptElement(nsIScriptElement*) /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp:1627:10
    #29 0x7f10841f22e7 in mozilla::dom::ScriptElement::MaybeProcessScript() /builds/worker/workspace/build/src/dom/script/ScriptElement.cpp:147:18
    #30 0x7f107f0c5e26 in AttemptToExecute /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIScriptElement.h:226:18
    #31 0x7f107f0c5e26 in nsHtml5TreeOpExecutor::RunScript(nsIContent*) /builds/worker/workspace/build/src/parser/html/nsHtml5TreeOpExecutor.cpp:735
    #32 0x7f107f0bf0cd in nsHtml5TreeOpExecutor::RunFlushLoop() /builds/worker/workspace/build/src/parser/html/nsHtml5TreeOpExecutor.cpp:539:7
    #33 0x7f107f0cb23b in nsHtml5ExecutorFlusher::Run() /builds/worker/workspace/build/src/parser/html/nsHtml5StreamParser.cpp:130:20
    #34 0x7f107cf4ab04 in mozilla::SchedulerGroup::Runnable::Run() /builds/worker/workspace/build/src/xpcom/threads/SchedulerGroup.cpp:396:25
    #35 0x7f107cf713ee in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1033:14
    #36 0x7f107cf8cd50 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:508:10

previously allocated by thread T0 (file:// Content) here:
    #0 0x4c31d3 in malloc /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:88:3
    #1 0x4f3c9d in moz_xmalloc /builds/worker/workspace/build/src/memory/mozalloc/mozalloc.cpp:70:17
    #2 0x7f107eca9eea in operator new /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/mozalloc.h:159:12
    #3 0x7f107eca9eea in mozilla::PeerConnectionImpl::Initialize(mozilla::dom::PeerConnectionObserver&, nsGlobalWindowInner*, mozilla::PeerConnectionConfiguration const&, nsISupports*) /builds/worker/workspace/build/src/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:655
    #4 0x7f107ecaf3da in mozilla::PeerConnectionImpl::Initialize(mozilla::dom::PeerConnectionObserver&, nsGlobalWindowInner&, mozilla::dom::RTCConfiguration const&, nsISupports*, mozilla::ErrorResult&) /builds/worker/workspace/build/src/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:735:9
    #5 0x7f1080b3fae7 in mozilla::dom::PeerConnectionImplBinding::initialize(JSContext*, JS::Handle<JSObject*>, mozilla::PeerConnectionImpl*, JSJitMethodCallArgs const&) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/PeerConnectionImplBinding.cpp:83:9
    #6 0x7f108220c5e7 in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:3042:13
    #7 0x7f1088dc4464 in CallJSNative /builds/worker/workspace/build/src/js/src/jscntxtinlines.h:291:15
    #8 0x7f1088dc4464 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:473
    #9 0x7f1088daa548 in CallFromStack /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:528:12
    #10 0x7f1088daa548 in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3096
    #11 0x7f1088d96cb0 in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:423:12
    #12 0x7f1088dc499c in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:495:15
    #13 0x7f1088dc54c2 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:541:10
    #14 0x7f10898c119c in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/jsapi.cpp:2995:12
    #15 0x7f1080d0cab7 in mozilla::dom::RTCPeerConnectionJSImpl::__Init(mozilla::dom::RTCConfiguration const&, mozilla::dom::Optional<JS::Handle<JSObject*> > const&, mozilla::ErrorResult&, JSCompartment*) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/RTCPeerConnectionBinding.cpp:8588:8
    #16 0x7f1080d27976 in mozilla::dom::RTCPeerConnection::Constructor(mozilla::dom::GlobalObject const&, JSContext*, mozilla::dom::RTCConfiguration const&, mozilla::dom::Optional<JS::Handle<JSObject*> > const&, mozilla::ErrorResult&, JS::Handle<JSObject*>) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/RTCPeerConnectionBinding.cpp:9989:16
    #17 0x7f1080e4f772 in mozilla::dom::RTCPeerConnectionBinding::_constructor(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/RTCPeerConnectionBinding.cpp:5755:63
    #18 0x7f1088dc5b0a in CallJSNative /builds/worker/workspace/build/src/js/src/jscntxtinlines.h:291:15
    #19 0x7f1088dc5b0a in CallJSNativeConstructor /builds/worker/workspace/build/src/js/src/jscntxtinlines.h:324
    #20 0x7f1088dc5b0a in InternalConstruct(JSContext*, js::AnyConstructArgs const&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:580
    #21 0x7f1088daa3c2 in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3088:18
    #22 0x7f1088d96cb0 in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:423:12
    #23 0x7f1088dc7401 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:706:15
    #24 0x7f1088dc7b9f in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:738:12
    #25 0x7f10898d6646 in ExecuteScript(JSContext*, JS::AutoObjectVector&, JS::Handle<JSScript*>, JS::Value*) /builds/worker/workspace/build/src/js/src/jsapi.cpp:4678:12
    #26 0x7f10802efc66 in nsJSUtils::ExecutionContext::CompileAndExec(JS::CompileOptions&, JS::SourceBufferHolder&, JS::MutableHandle<JSScript*>) /builds/worker/workspace/build/src/dom/base/nsJSUtils.cpp:266:8
    #27 0x7f1084217f21 in mozilla::dom::ScriptLoader::EvaluateScript(mozilla::dom::ScriptLoadRequest*) /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp:2287:25
    #28 0x7f10842133c9 in mozilla::dom::ScriptLoader::ProcessRequest(mozilla::dom::ScriptLoadRequest*) /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp:1929:10
    #29 0x7f10841f6315 in mozilla::dom::ScriptLoader::ProcessScriptElement(nsIScriptElement*) /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp:1627:10
    #30 0x7f10841f22e7 in mozilla::dom::ScriptElement::MaybeProcessScript() /builds/worker/workspace/build/src/dom/script/ScriptElement.cpp:147:18
    #31 0x7f107f0c5e26 in AttemptToExecute /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIScriptElement.h:226:18
    #32 0x7f107f0c5e26 in nsHtml5TreeOpExecutor::RunScript(nsIContent*) /builds/worker/workspace/build/src/parser/html/nsHtml5TreeOpExecutor.cpp:735
    #33 0x7f107f0bf0cd in nsHtml5TreeOpExecutor::RunFlushLoop() /builds/worker/workspace/build/src/parser/html/nsHtml5TreeOpExecutor.cpp:539:7
    #34 0x7f107f0cb23b in nsHtml5ExecutorFlusher::Run() /builds/worker/workspace/build/src/parser/html/nsHtml5StreamParser.cpp:130:20
    #35 0x7f107cf4ab04 in mozilla::SchedulerGroup::Runnable::Run() /builds/worker/workspace/build/src/xpcom/threads/SchedulerGroup.cpp:396:25

SUMMARY: AddressSanitizer: heap-use-after-free /builds/worker/workspace/build/src/obj-firefox/dist/include/mtransport/sigslot.h:318:13 in ~lock_block
Shadow bytes around the buggy address:
  0x0c2a80055b70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c2a80055b80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c2a80055b90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c2a80055ba0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c2a80055bb0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x0c2a80055bc0: fd fd fd fd fd fd fd fd fd[fd]fd fd fd fd fd fd
  0x0c2a80055bd0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2a80055be0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2a80055bf0: fd fd fd fd fd fd fd fd fd fa fa fa fa fa fa fa
  0x0c2a80055c00: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2a80055c10: 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
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==4360==ABORTING

	Nils Ohlmeier [:drno]
	Comment 2 • 7 years ago

I'm not sure I understand the stack trace which causes the freeing.

Maybe the assumption here https://dxr.mozilla.org/mozilla-central/rev/947b1418ebb23e1faf716cec11237c6861c5d061/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp#1255
no longer holds true?

	Nils Ohlmeier [:drno]
	Comment 3 • 7 years ago

Michael, can you have a look at this?

	Michael Froman [:mjf] Assignee
	Comment 4 • 7 years ago

I'll take a look.

	Michael Froman [:mjf] Assignee
	Comment 5 • 7 years ago

The first error I see when running trigger.html with an asan build is:
Assertion failure: sIPCConnection.IsValid(), at /builds/worker/workspace/build/src/dom/media/CubebUtils.cpp:458

Let me see if this happens on a local debug build.

	Michael Froman [:mjf] Assignee
	Comment 6 • 7 years ago

I have a debug build from c9bec96cc789 on m-c that also asserts when running the trigger.html attachment. For reference:
https://dxr.mozilla.org/mozilla-central/source/dom/media/CubebUtils.cpp#458

	Michael Froman [:mjf] Assignee
	Comment 7 • 7 years ago

Paul, can you take a look at this assertion?

	Michael Froman [:mjf] Assignee
	Comment 8 • 7 years ago

For clarification, I was running this on my linux machine.  I do not get the assert when running with --disable-e10s on my local debug build.

	Paul Adenot (:padenot)
	Comment 9 • 7 years ago

You need to copy the file `pref.js` to the root of the profile directory. Also it appears you have an issue with audio remoting. Running without e10s is a way to disable remoting to sidestep this (probably unrelated) issue.

	Michael Froman [:mjf] Assignee
	Comment 10 • 7 years ago

We're spinning the event loop here[1] which causes main thread execution to resume before we've exited PeerConnectionImpl::IceGatheringStateChange.  In this case, execution resuming means resuming teardown on PeerConnectionMedia and we free the PeerConnectionMedia that calls PeerConnectionImpl::IceGatheringStateChange here via a signal[2].  The cause of the UAF is release the sigslot lockblock which no longer exists.

Replacing the RUN_ON_THREAD (which immediately executes the PeerConnectionObserver::OnStateChange because we're already on mThread) with a mThread->Dispatch fixes this crash.

Randell, does this fall under the queue jumping bug?  Should we fix this single issue here, possibly under a bug for a broken PeerConnectionMedia teardown ordering?

[1] https://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp#3257
[2] https://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp?q=%2Bfunction%3Amozilla%3A%3APeerConnectionMedia%3A%3ASelfDestruct%28%29&redirect_type=single#1359

	Randell Jesup [:jesup] (needinfo me)
	Comment 11 • 7 years ago

Yes, this is the queue-jumping bug I think.  I'm open with how to land a fix - Nils?

	Nils Ohlmeier [:drno]
	Comment 12 • 7 years ago

Lets land the fix for this issue here, since I need to get back to the queue jumping thing (but I don't see that happening soon).

	Michael Froman [:mjf] Assignee
	Comment 13 • 7 years ago

The queue jumping issue is causing a tear-down ordering problem that results in signals getting dispatched to an already deleted instance.

	Byron Campen [:bwc]
	Comment 14 • 7 years ago

Comment on attachment 8941483 [details] [diff] [review]
Don't allow queue-jumping for gathering state change events.

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

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +3254,4 @@
>      return;
>    }
>    WrappableJSErrorResult rv;
> +  // use Dispatch rather than RUN_ON_THREAD (which immediately executes

Let's avoid this comment, to make it harder for someone to figure out the security hole we're plugging.

	Michael Froman [:mjf] Assignee
	Comment 16 • 7 years ago

Remove comment per bwc.

	Michael Froman [:mjf] Assignee
	Comment 17 • 7 years ago

Comment on attachment 8941489 [details] [diff] [review]
Don't allow queue-jumping for gathering state change events.

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

carry forward r+ from bwc after removing comment.

	Michael Froman [:mjf] Assignee
	Comment 18 • 7 years ago

Comment on attachment 8941489 [details] [diff] [review]
Don't allow queue-jumping for gathering state change events.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not very easily since there is an open queue-jumping bug, and knowning that doesn't reveal what problem the queue jumping caused.

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?
All.

If not all supported branches, which bug introduced the flaw?
The queue-jumping behavior was introduced back in 2012.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Not yet, but they should be easy to create because it is a very small change - one place in one file.

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

	Al Billings [:abillings - ex-MoCo]
	Comment 19 • 7 years ago

I'd like to give this sec-approval+ and then beta+ to make the last beta but I need Gerry's approval to do so.

If the patch applies, this should be nominated for Beta. 

Gerry, this is a very simple patch and it affects things forever. We should consider it for ESR52 as well.

	Gerry Chang [:gchang]
	Comment 20 • 7 years ago

Al,
Yes, I also want to include this in beta 16.

	Al Billings [:abillings - ex-MoCo]
	Comment 21 • 7 years ago

Comment on attachment 8941489 [details] [diff] [review]
Don't allow queue-jumping for gathering state change events.

sec-approval+.
Let's get a beta nomination too.

	Michael Froman [:mjf] Assignee
	Comment 22 • 7 years ago

Comment on attachment 8941489 [details] [diff] [review]
Don't allow queue-jumping for gathering state change events.

Approval Request Comment
[Feature/Bug causing the regression]:
The queue-jumping behavior was introduced back in 2012 by bug 824851.

[User impact if declined]:
In this case, a UAF during PeerConnection teardown is fairly easy to provoke over a small number of attempts.

[Is this code covered by automated tests?]:
Many mochitests exercise this code, but none are specifically looking for timing-related teardown issue.

[Has the fix been verified in Nightly?]:
Not yet.

[Needs manual test from QE? If yes, steps to reproduce]:
No.

[List of other uplifts needed for the feature/fix]:
None.

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
It is one small change in one file that ensures dispatch to a thread vs queue-jumping if already on the requested thread.

[String changes made/needed]:
None.

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 23 • 7 years ago

Hi Al, Dan, should we consider uplifting this to ESR52.6?

	Al Billings [:abillings - ex-MoCo]
	Comment 24 • 7 years ago

I'd like to see this in ERS52.

	Al Billings [:abillings - ex-MoCo]
	Comment 25 • 7 years ago

Comment on attachment 8941489 [details] [diff] [review]
Don't allow queue-jumping for gathering state change events.

This was approved for 58 (in comments) but didn't get checked into the last Beta. This needs to be in RC1.

	Ryan VanderMeulen [:RyanVM]
	Comment 26 • 7 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/2fa8c2b1c9917c035e0d9cc7f7b18549d67e5ade

https://hg.mozilla.org/releases/mozilla-beta/rev/7aa26468189e (FIREFOX_58b_RELBRANCH)
https://hg.mozilla.org/releases/mozilla-release/rev/01c8d651c351

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 27 • 7 years ago

https://hg.mozilla.org/mozilla-central/rev/2fa8c2b1c991

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 28 • 7 years ago

Comment on attachment 8941489 [details] [diff] [review]
Don't allow queue-jumping for gathering state change events.

Sec-high, ESR52+

	Ryan VanderMeulen [:RyanVM]
	Comment 29 • 7 years ago
uplift

https://hg.mozilla.org/releases/mozilla-esr52/rev/5f753161df25