1371889 - (CVE-2017-7793) Heap-use-after-free in mozilla::dom::(anonymous namespace)::ConsumeBodyDoneObserver<mozilla::dom::Request>::OnStreamComplete

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Abhishek Arya]

Attachment: t.zip

>=================================================================
>==107957==ERROR: AddressSanitizer: heap-use-after-free on address 0x60d000066990 at pc 0x7f6f64bc192f bp 0x7ffcea9dbcf0 sp 0x7ffcea9dbce8
>READ of size 8 at 0x60d000066990 thread T0 (Web Content)
>    #0 0x7f6f64bc192e in mozilla::dom::(anonymous namespace)::ConsumeBodyDoneObserver<mozilla::dom::Request>::OnStreamComplete(nsIStreamLoader*, nsISupports*, nsresult, unsigned int, unsigned char const*) dom/fetch/Fetch.cpp:1006:21
>    #1 0x7f6f613b41e7 in mozilla::net::nsStreamLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) netwerk/base/nsStreamLoader.cpp:105:30
>    #2 0x7f6f6131fc95 in nsInputStreamPump::OnStateStop() netwerk/base/nsInputStreamPump.cpp:734:20
>    #3 0x7f6f6131e44e in nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) netwerk/base/nsInputStreamPump.cpp:448:25
>    #4 0x7f6f611532fd in nsInputStreamReadyEvent::Run() xpcom/io/nsStreamUtils.cpp:96:20
>    #5 0x7f6f6119a898 in mozilla::ThrottledEventQueue::Inner::ExecuteRunnable() xpcom/threads/ThrottledEventQueue.cpp:190:22
>    #6 0x7f6f6119a47f in mozilla::ThrottledEventQueue::Inner::Executor::Run() xpcom/threads/ThrottledEventQueue.cpp:74:15
>    #7 0x7f6f6119a898 in mozilla::ThrottledEventQueue::Inner::ExecuteRunnable() xpcom/threads/ThrottledEventQueue.cpp:190:22
>    #8 0x7f6f6119a47f in mozilla::ThrottledEventQueue::Inner::Executor::Run() xpcom/threads/ThrottledEventQueue.cpp:74:15
>    #9 0x7f6f611827c8 in mozilla::SchedulerGroup::Runnable::Run() xpcom/threads/SchedulerGroup.cpp:359:25
>    #10 0x7f6f611ad373 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:1406:14
>    #11 0x7f6f611b7278 in NS_ProcessNextEvent(nsIThread*, bool) xpcom/threads/nsThreadUtils.cpp:472:10
>    #12 0x7f6f61d66358 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:96:21
>    #13 0x7f6f61ccd9ed in RunHandler ipc/chromium/src/base/message_loop.cc:231:3
>    #14 0x7f6f61ccd9ed in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:211
>    #15 0x7f6f65f1b0fa in nsBaseAppShell::Run() widget/nsBaseAppShell.cpp:156:27
>    #16 0x7f6f693d6aac in XRE_RunAppShell() toolkit/xre/nsEmbedFunctions.cpp:896:22
>    #17 0x7f6f61ccd9ed in RunHandler ipc/chromium/src/base/message_loop.cc:231:3
>    #18 0x7f6f61ccd9ed in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:211
>    #19 0x7f6f693d6598 in XRE_InitChildProcess(int, char**, XREChildData const*) toolkit/xre/nsEmbedFunctions.cpp:712:34
>    #20 0x524fc1 in content_process_main browser/app/../../ipc/contentproc/plugin-container.cpp:64:30
>    #21 0x524fc1 in main browser/app/nsBrowserApp.cpp:286
>    #22 0x7f6f79ee7f44 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21f44)
>    #23 0x41d905 in _start (objdir-ff-asan/dist/bin/firefox-bin+0x41d905)
>
>0x60d000066990 is located 16 bytes inside of 144-byte region [0x60d000066980,0x60d000066a10)
>freed by thread T104 (DOM Worker) here:
>    #0 0x4e8890 in __interceptor_free /build/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:47
>    #1 0x7f6f6108dd75 in SnowWhiteKiller::~SnowWhiteKiller() xpcom/base/nsCycleCollector.cpp:2651:25
>    #2 0x7f6f6108d1f1 in nsCycleCollector::FreeSnowWhite(bool) xpcom/base/nsCycleCollector.cpp:2826:3
>    #3 0x7f6f61093ee8 in nsCycleCollector::BeginCollection(ccType, nsICycleCollectorListener*) xpcom/base/nsCycleCollector.cpp:3829:3
>    #4 0x7f6f61093852 in nsCycleCollector::Collect(ccType, js::SliceBudget&, nsICycleCollectorListener*, bool) xpcom/base/nsCycleCollector.cpp:3650:9
>    #5 0x7f6f61094f74 in ShutdownCollect xpcom/base/nsCycleCollector.cpp:3591:10
>    #6 0x7f6f61094f74 in nsCycleCollector::Shutdown(bool) xpcom/base/nsCycleCollector.cpp:3886
>    #7 0x7f6f61096b4c in nsCycleCollector_shutdown(bool) xpcom/base/nsCycleCollector.cpp:4247:23
>    #8 0x7f6f65a486cb in (anonymous namespace)::WorkerJSContext::~WorkerJSContext() dom/workers/RuntimeService.cpp:1144:5
>    #9 0x7f6f65a47f3a in (anonymous namespace)::WorkerThreadPrimaryRunnable::Run() dom/workers/RuntimeService.cpp:2952:3
>    #10 0x7f6f611ad373 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:1406:14
>    #11 0x7f6f611b7278 in NS_ProcessNextEvent(nsIThread*, bool) xpcom/threads/nsThreadUtils.cpp:472:10
>    #12 0x7f6f61d67896 in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:338:20
>    #13 0x7f6f61ccd9ed in RunHandler ipc/chromium/src/base/message_loop.cc:231:3
>    #14 0x7f6f61ccd9ed in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:211
>    #15 0x7f6f611a650f in nsThread::ThreadFunc(void*) xpcom/threads/nsThread.cpp:501:11
>    #16 0x7f6f775d222c in _pt_root nsprpub/pr/src/pthreads/ptthread.c:216:5
>    #17 0x7f6f7aec2183 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x8183)
>
>previously allocated by thread T104 (DOM Worker) here:
>    #0 0x4e8be8 in malloc /build/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:66
>    #1 0x52622d in moz_xmalloc memory/mozalloc/mozalloc.cpp:83:17
>    #2 0x7f6f64b9c3d9 in operator new objdir-ff-asan/dist/include/mozilla/mozalloc.h:194:12
>    #3 0x7f6f64b9c3d9 in mozilla::dom::Request::Constructor(mozilla::dom::GlobalObject const&, mozilla::dom::RequestOrUSVString const&, mozilla::dom::RequestInit const&, mozilla::ErrorResult&) dom/fetch/Request.cpp:581
>    #4 0x7f6f63c1c7c6 in mozilla::dom::RequestBinding::_constructor(JSContext*, unsigned int, JS::Value*) objdir-ff-asan/dom/bindings/RequestBinding.cpp:1851:53
>    #5 0x7f6f697c0069 in CallJSNative js/src/jscntxtinlines.h:293:15
>    #6 0x7f6f697c0069 in CallJSNativeConstructor js/src/jscntxtinlines.h:326
>    #7 0x7f6f697c0069 in InternalConstruct(JSContext*, js::AnyConstructArgs const&) js/src/vm/Interpreter.cpp:573
>    #8 0x7f6f697bfe2c in js::ConstructFromStack(JSContext*, JS::CallArgs const&) js/src/vm/Interpreter.cpp:599:12
>    #9 0x7f6f6979b493 in Interpret(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:3056:18
>    #10 0x7f6f697941bb in js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:410:12
>    #11 0x7f6f697c153a in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) js/src/vm/Interpreter.cpp:699:15
>    #12 0x7f6f697c1ec1 in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) js/src/vm/Interpreter.cpp:731:12
>    #13 0x7f6f69efa8ea in Evaluate(JSContext*, js::ScopeKind, JS::Handle<JSObject*>, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, JS::MutableHandle<JS::Value>) js/src/jsapi.cpp:4662:19
>    #14 0x7f6f69efac60 in JS::Evaluate(JSContext*, JS::ReadOnlyCompileOptions const&, JS::SourceBufferHolder&, JS::MutableHandle<JS::Value>) js/src/jsapi.cpp:4727:12
>    #15 0x7f6f65a537ee in (anonymous namespace)::ScriptExecutorRunnable::WorkerRun(JSContext*, mozilla::dom::workers::WorkerPrivate*) dom/workers/ScriptLoader.cpp:1969:10
>    #16 0x7f6f65ae4ad1 in mozilla::dom::workers::WorkerRunnable::Run() dom/workers/WorkerRunnable.cpp:374:12
>    #17 0x7f6f611ad373 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:1406:14
>    #18 0x7f6f611b7278 in NS_ProcessNextEvent(nsIThread*, bool) xpcom/threads/nsThreadUtils.cpp:472:10
>    #19 0x7f6f65ad11b3 in mozilla::dom::workers::WorkerPrivate::RunCurrentSyncLoop() dom/workers/WorkerPrivate.cpp:5874:7
>    #20 0x7f6f65a16e51 in (anonymous namespace)::LoadAllScripts(mozilla::dom::workers::WorkerPrivate*, nsTArray<(anonymous namespace)::ScriptLoadInfo>&, bool, mozilla::dom::workers::WorkerScriptType, mozilla::ErrorResult&) dom/workers/ScriptLoader.cpp:2127:12
>    #21 0x7f6f65a169b0 in mozilla::dom::workers::scriptloader::LoadMainScript(mozilla::dom::workers::WorkerPrivate*, nsAString const&, mozilla::dom::workers::WorkerScriptType, mozilla::ErrorResult&) dom/workers/ScriptLoader.cpp:2245:3
>    #22 0x7f6f65af35bb in (anonymous namespace)::CompileScriptRunnable::WorkerRun(JSContext*, mozilla::dom::workers::WorkerPrivate*) dom/workers/WorkerPrivate.cpp:586:5
>    #23 0x7f6f65ae4ad1 in mozilla::dom::workers::WorkerRunnable::Run() dom/workers/WorkerRunnable.cpp:374:12
>    #24 0x7f6f611ad373 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:1406:14
>    #25 0x7f6f611b7278 in NS_ProcessNextEvent(nsIThread*, bool) xpcom/threads/nsThreadUtils.cpp:472:10
>    #26 0x7f6f65acb4fb in mozilla::dom::workers::WorkerPrivate::DoRunLoop(JSContext*) dom/workers/WorkerPrivate.cpp:5115:7
>    #27 0x7f6f65a47ebb in (anonymous namespace)::WorkerThreadPrimaryRunnable::Run() dom/workers/RuntimeService.cpp:2919:25
>    #28 0x7f6f611ad373 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:1406:14
>    #29 0x7f6f611b7278 in NS_ProcessNextEvent(nsIThread*, bool) xpcom/threads/nsThreadUtils.cpp:472:10
>    #30 0x7f6f61d67896 in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:338:20
>    #31 0x7f6f61ccd9ed in RunHandler ipc/chromium/src/base/message_loop.cc:231:3
>    #32 0x7f6f61ccd9ed in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:211
>    #33 0x7f6f611a650f in nsThread::ThreadFunc(void*) xpcom/threads/nsThread.cpp:501:11
>
>Thread T104 (DOM Worker) created by T0 (Web Content) here:
>    #0 0x4362cd in __interceptor_pthread_create /build/llvm/projects/compiler-rt/lib/asan/asan_interceptors.cc:314
>    #1 0x7f6f775cee35 in _PR_CreateThread nsprpub/pr/src/pthreads/ptthread.c:457:14
>    #2 0x7f6f775cea2e in PR_CreateThread nsprpub/pr/src/pthreads/ptthread.c:548:12
>    #3 0x7f6f611a83d1 in nsThread::Init(nsACString const&) xpcom/threads/nsThread.cpp:683:8
>    #4 0x7f6f65aeeecb in mozilla::dom::workers::WorkerThread::Create(mozilla::dom::workers::WorkerThreadFriendKey const&) dom/workers/WorkerThread.cpp:90:7
>    #5 0x7f6f65a0edc1 in mozilla::dom::workers::RuntimeService::ScheduleWorker(mozilla::dom::workers::WorkerPrivate*) dom/workers/RuntimeService.cpp:1918:14
>    #6 0x7f6f65a0d298 in mozilla::dom::workers::RuntimeService::RegisterWorker(mozilla::dom::workers::WorkerPrivate*) dom/workers/RuntimeService.cpp:1745:19
>    #7 0x7f6f65ac8533 in mozilla::dom::workers::WorkerPrivate::Constructor(JSContext*, nsAString const&, bool, mozilla::dom::WorkerType, nsAString const&, nsACString const&, mozilla::dom::workers::WorkerLoadInfo*, mozilla::ErrorResult&) dom/workers/WorkerPrivate.cpp:4657:24
>    #8 0x7f6f65ac7e1d in mozilla::dom::workers::WorkerPrivate::Constructor(mozilla::dom::GlobalObject const&, nsAString const&, bool, mozilla::dom::WorkerType, nsAString const&, mozilla::dom::workers::WorkerLoadInfo*, mozilla::ErrorResult&) dom/workers/WorkerPrivate.cpp:4574:10
>    #9 0x7f6f65ac7daf in mozilla::dom::workers::WorkerPrivate::Constructor(mozilla::dom::GlobalObject const&, nsAString const&, mozilla::dom::WorkerOptions const&, mozilla::ErrorResult&) dom/workers/WorkerPrivate.cpp:4515:10
>    #10 0x7f6f642848bf in mozilla::dom::WorkerBinding::_constructor(JSContext*, unsigned int, JS::Value*) objdir-ff-asan/dom/bindings/WorkerBinding.cpp:971:68
>    #11 0x7f6f697c0069 in CallJSNative js/src/jscntxtinlines.h:293:15
>    #12 0x7f6f697c0069 in CallJSNativeConstructor js/src/jscntxtinlines.h:326
>    #13 0x7f6f697c0069 in InternalConstruct(JSContext*, js::AnyConstructArgs const&) js/src/vm/Interpreter.cpp:573
>    #14 0x7f6f697bfe2c in js::ConstructFromStack(JSContext*, JS::CallArgs const&) js/src/vm/Interpreter.cpp:599:12
>    #15 0x7f6f6979b493 in Interpret(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:3056:18
>    #16 0x7f6f697941bb in js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:410:12
>    #17 0x7f6f697c153a in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) js/src/vm/Interpreter.cpp:699:15
>    #18 0x7f6f697c1ec1 in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) js/src/vm/Interpreter.cpp:731:12
>    #19 0x7f6f69ef98ee in ExecuteScript(JSContext*, JS::AutoObjectVector&, JS::Handle<JSScript*>, JS::Value*) js/src/jsapi.cpp:4578:12
>    #20 0x7f6f69ef95c4 in JS_ExecuteScript(JSContext*, JS::AutoObjectVector&, JS::Handle<JSScript*>, JS::MutableHandle<JS::Value>) js/src/jsapi.cpp:4599:12
>    #21 0x7f6f63547b68 in nsJSUtils::ExecutionContext::CompileAndExec(JS::CompileOptions&, JS::SourceBufferHolder&, JS::MutableHandle<JSScript*>) dom/base/nsJSUtils.cpp:267:8
>    #22 0x7f6f65e2fd93 in mozilla::dom::ScriptLoader::EvaluateScript(mozilla::dom::ScriptLoadRequest*) dom/script/ScriptLoader.cpp:2129:25
>    #23 0x7f6f65e2d254 in mozilla::dom::ScriptLoader::ProcessRequest(mozilla::dom::ScriptLoadRequest*) dom/script/ScriptLoader.cpp:1725:10
>    #24 0x7f6f65e1bf5b in mozilla::dom::ScriptLoader::ProcessScriptElement(nsIScriptElement*) dom/script/ScriptLoader.cpp:1427:10
>    #25 0x7f6f65e19a3c in mozilla::dom::ScriptElement::MaybeProcessScript() dom/script/ScriptElement.cpp:149:18
>    #26 0x7f6f6289453d in nsIScriptElement::AttemptToExecute() objdir-ff-asan/dist/include/nsIScriptElement.h:225:18
>    #27 0x7f6f62893b13 in nsHtml5TreeOpExecutor::RunScript(nsIContent*) parser/html/nsHtml5TreeOpExecutor.cpp:698:22
>    #28 0x7f6f6288f693 in nsHtml5TreeOpExecutor::RunFlushLoop() parser/html/nsHtml5TreeOpExecutor.cpp:499:7
>    #29 0x7f6f62897265 in nsHtml5ExecutorFlusher::Run() parser/html/nsHtml5StreamParser.cpp:129:20
>    #30 0x7f6f611827c8 in mozilla::SchedulerGroup::Runnable::Run() xpcom/threads/SchedulerGroup.cpp:359:25
>    #31 0x7f6f611ad373 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:1406:14
>    #32 0x7f6f611b7278 in NS_ProcessNextEvent(nsIThread*, bool) xpcom/threads/nsThreadUtils.cpp:472:10
>    #33 0x7f6f61d66358 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:96:21
>    #34 0x7f6f61ccd9ed in RunHandler ipc/chromium/src/base/message_loop.cc:231:3
>    #35 0x7f6f61ccd9ed in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:211
>    #36 0x7f6f65f1b0fa in nsBaseAppShell::Run() widget/nsBaseAppShell.cpp:156:27
>    #37 0x7f6f693d6aac in XRE_RunAppShell() toolkit/xre/nsEmbedFunctions.cpp:896:22
>    #38 0x7f6f61ccd9ed in RunHandler ipc/chromium/src/base/message_loop.cc:231:3
>    #39 0x7f6f61ccd9ed in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:211
>    #40 0x7f6f693d6598 in XRE_InitChildProcess(int, char**, XREChildData const*) toolkit/xre/nsEmbedFunctions.cpp:712:34
>    #41 0x524fc1 in content_process_main browser/app/../../ipc/contentproc/plugin-container.cpp:64:30
>    #42 0x524fc1 in main browser/app/nsBrowserApp.cpp:286
>    #43 0x7f6f79ee7f44 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21f44)
>
>SUMMARY: AddressSanitizer: heap-use-after-free dom/fetch/Fetch.cpp:1006:21 in mozilla::dom::(anonymous namespace)::ConsumeBodyDoneObserver<mozilla::dom::Request>::OnStreamComplete(nsIStreamLoader*, nsISupports*, nsresult, unsigned int, unsigned char const*)
>Shadow bytes around the buggy address:
>  0x0c1a80004ce0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>  0x0c1a80004cf0: fa fa fa fa fa fa fa fa fa fa fa fa fd fd fd fd
>  0x0c1a80004d00: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fa fa
>  0x0c1a80004d10: fa fa fa fa fa fa fd fd fd fd fd fd fd fd fd fd
>  0x0c1a80004d20: fd fd fd fd fd fd fd fd fa fa fa fa fa fa fa fa
>=>0x0c1a80004d30: fd fd[fd]fd fd fd fd fd fd fd fd fd fd fd fd fd
>  0x0c1a80004d40: fd fd fa fa fa fa fa fa fa fa fd fd fd fd fd fd
>  0x0c1a80004d50: fd fd fd fd fd fd fd fd fd fd fd fa fa fa fa fa
>  0x0c1a80004d60: fa fa fa fa fd fd fd fd fd fd fd fd fd fd fd fd
>  0x0c1a80004d70: fd fd fd fd fd fa fa fa fa fa fa fa fa fa fa fa
>  0x0c1a80004d80: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>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
>==107957==ABORTING
>

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Note that adding a <meta http-equiv="refresh" content="1"> to the top of the testcase makes it easier to reproduce in my experience. Affected builds will crash within 10-15sec.

INFO: Last good revision: 9a8f2342fb3116d23989087e026448d38a3768c5 (2015-10-27)
INFO: First bad revision: fc706d376f0658e560a59c3dd520437b18e8c4a4 (2015-10-28)
INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9a8f2342fb3116d23989087e026448d38a3768c5&tochange=fc706d376f0658e560a59c3dd520437b18e8c4a4

Too old to get a narrower range with mozregression, unfortuantely. Bug 1217501 maybe?

Comment 2

[Julien Cristau [:jcristau] (back April 22)]

tracking this uaf for 55/56

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 1 - the fix

The fix for this issue is to create a wrapper, refcounted, thread-safe, able to keep alive the FetchBody. I'm also writing another patch that moves the ConsumeBody part into the wrapper. This makes the code cleaner, but I prefer to land it in a separate bug.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 2 - Move FetchBodyWrapper in a separate file

This and the following 2 patches can land in a separate bug. Up to the reviewer.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 3 - Rename FetchBodyWrapper to FetchBodyConsumer

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 4 - Move the consumer body logic into FetchBodyConsumer

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Unrelated changes should not land in this bug. Only minimal fix, since I assume we want this on branches too, right?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8877587 [details] [diff] [review]
part 1 - the fix

I think I'm not the right reviewer for Fetch.
Lots of this code is r=baku,bkelly
bkelly, do you think you had time?
If not, put this back to my queue.

And, raw pointers bite us again, even in modern code :/

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

> And, raw pointers bite us again, even in modern code :/

Yea, I'm almost stole it this morning. :-)  I'll review tomorrow.

Comment 10

[Andrea Marchesini [:baku]]

Thanks Ben. Some comments: we have to keep FetchBody alive somehow because the current setup is fragile.
I decided to introduce a FetchBodyWrapper: a thread-safe object able to keep FetchBody alive and to release it on the current thread. (patch 1).

Then I also decided to rename FetchBodyWrapper in FetchBodyConsumer and move the entire logic from FetchBody to that new object. (parts 2, 3 and 4). This makes the code cleaner, but as I says, it can land in a separate bug.

Comment 11

[Andrea Marchesini [:baku]]

Comment on attachment 8877681 [details] [diff] [review]
part 2 - Move FetchBodyWrapper in a separate file

Just code moving

Comment 12

[Andrea Marchesini [:baku]]

Comment on attachment 8877682 [details] [diff] [review]
part 3 - Rename FetchBodyWrapper to FetchBodyConsumer

Code renaming.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

As I said, since we need to land the fix for branches, code cleanups should happen in a separate bug.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8877587 [details] [diff] [review]
part 1 - the fix

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

::: dom/fetch/Fetch.cpp
@@ +844,5 @@
> +public:
> +  explicit FetchBodyWorkerHolder(FetchBodyWrapper<Derived>* aWrapper)
> +    : mWrapper(aWrapper)
> +    , mWasNotified(false)
> +  {}

Assert mWrapper is not nullptr.

@@ +939,5 @@
> +  RefPtr<FetchBody<Derived>> mBody;
> +
> +  // Set when consuming the body is attempted on a worker.
> +  // Unset when consumption is done/aborted.
> +  nsAutoPtr<workers::WorkerHolder> mWorkerHolder;

UniquePtr<> instead of nsAutoPtr<>.  Also, please mention that this holds the wrapper alive via a cycle.

@@ +950,5 @@
> +  MOZ_ASSERT(aStatus > workers::Running);
> +  if (!mWasNotified) {
> +    mWasNotified = true;
> +    mWrapper->Body()->ContinueConsumeBody(mWrapper, NS_BINDING_ABORTED, 0,
> +                                          nullptr);

Maybe add a comment that this will cause the FetchBodyWorkerHolder<> to be destroyed.  This means:

1. This method better not do anything else after calling this.
2. Will break the cycle between the holder and the wrapper.

@@ +957,5 @@
> +  return true;
> +}
> +
> +template <class Derived>
> +class MOZ_STACK_CLASS FetchObjectWrapperReleaseHelper final

I don't think you need this class.  You can just use MakeScopeExit() instead.  We've had this since FF42 AFAICT, so it should be safe for uplift.

@@ +1087,5 @@
>   * In case of failure to create a stream pump or dispatch stream completion to
>   * worker, ensure we cleanup properly. Thread agnostic.
>   */
>  template <class Derived>
>  class MOZ_STACK_CLASS AutoFailConsumeBody final

You can replace this stack class with MakeScopeExit as well.  This can be a follow-up bug, though, since its an existing class.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Andrea, can we wait to do the later patches in a separate bug after this lands?  It would be nice to keep this bug limited to things that can be uplifted to ESR.

Comment 16

[Andrea Marchesini [:baku]]

Attachment: fetch_crash.patch

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

not easy. this is a use-after-free crash.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

The comments and the patch are about improving how FetchBody is kept alive. It doesn't involve a description of the security problem.

Which older supported branches are affected by this flaw?

all.

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

Easy to backport.

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

it should be fine. It's totally green on try. Basically I replaced a manual addref/release call, with a thread-safe refcounted object.

Comment 17

[Andrea Marchesini [:baku]]

Attachment: fetch_crash.patch

see previous comment.

Comment 18

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

Dan, do you have an opinion about how far into the cycle this should go in?

Comment 20

[Daniel Veditz [:dveditz]]

Given the size of the path we should land soon for as much testing as possible

Comment 21

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

Comment on attachment 8878370 [details] [diff] [review]
fetch_crash.patch

sec-approval+ for landing on trunk.

Comment 22

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

https://hg.mozilla.org/mozilla-central/rev/9c4f43ce54f1

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Please request Beta & ESR52 approval on this when you get a chance.

Comment 24

[Andrea Marchesini [:baku]]

Comment on attachment 8878370 [details] [diff] [review]
fetch_crash.patch

[Feature/Bug causing the regression]: Fetch API in workers
[User impact if declined]: UAF can happen in Fetch API when the worker or the window go away.
[Is this code covered by automated tests?]: no. It's racy
[Has the fix been verified in Nightly?]: n/a
[Needs manual test from QE? If yes, steps to reproduce]: n/a 
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not too much.
[Why is the change risky/not risky?]: The patch introduces a wrapper able to keep a reference of Fetch keeping it alive. It's big patch but it has green on try for a while.
[String changes made/needed]: none

Comment 25

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8878370 [details] [diff] [review]
fetch_crash.patch

what could possibly go wrong

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Needs rebased patches for Beta and ESR52.

Comment 27

[Andrea Marchesini [:baku]]

I'm working on bug 1375749. I would like to suspend the uplift until I figure out if that crash is related to this patch.

Comment 28

[Andrea Marchesini [:baku]]

Is it OK if I submit a patch for beta and esr52 containing this one (rebased) plus bug 1375749?

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Sure!

Comment 30

[Andrea Marchesini [:baku]]

Attachment: m-b

Comment 31

[Andrea Marchesini [:baku]]

Attachment: esr52

patches ready. They compile but I haven't pushed them to treeherder.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/bfed06697cb3
https://hg.mozilla.org/releases/mozilla-esr52/rev/b38fed9a9772

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Backed out from Beta temporarily to avoid shipping the topcrashes caused by it to Beta users (per bug 1374922 comment 31). Setting NI on myself to make sure we get it re-landed once those issues are resolved (mainly bug 1374922 and bug 1375659). I'm going to leave ESR52 alone under the assumption that we'll get the fixes landed and uplifted before we attempt any sort of release candidate builds off that branch.

https://hg.mozilla.org/releases/mozilla-beta/rev/eaea82aeb9b5

Comment 34

[Andrei Vaida [:avaida]]

(In reply to Andrea Marchesini [:baku] from comment #24)
> [Is this code covered by automated tests?]: no. It's racy
> [Has the fix been verified in Nightly?]: n/a
> [Needs manual test from QE? If yes, steps to reproduce]: n/a 

Setting qe-verify- based on Andrea's assessment on manual testing needs.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Re-landed with the fixes for bug 1374922 included. Unfortunately, bug 1375659 needs a rebased patch.

https://hg.mozilla.org/releases/mozilla-beta/rev/bc1c85daa17a

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Backed out again because bug 1374922 had to be backed out.
https://hg.mozilla.org/releases/mozilla-beta/rev/d94ac92ea7c3

Comment 37

[Ryan VanderMeulen [:RyanVM]]

Given that whack-a-mole we continue to play with this bug with the various crashes tracked in the blocking bugs and where we are in the cycle, I think we should consider dropping this for 55/52.3 and instead aim for 56/52.4 so we have more time to find any other lingering fallout. I'm worried we'll be continuing to chase issues right up to the point of release otherwise.

At this point in time, that would require me to backout rev b38fed9a9772 from ESR52. Beta55 does not currently have anything landed for this bug. Does that sound like a sane/rational idea to everyone else?

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Any thoughts on comment 37, Dan/Al?

Comment 39

[Daniel Veditz [:dveditz]]

Yes, that sounds sane. I worry about ESR 52.4 because we don't have nightly users. We might know the fallout that happens on trunk and fix that, but there could be different fallout on the older branch because it's missing something that landed in 53/54 that we wouldn't notice :-(

Comment 40

[Ryan VanderMeulen [:RyanVM]]

Backed out from ESR52. We'll revisit during the next cycle.

https://hg.mozilla.org/releases/mozilla-esr52/rev/63f6d360e304

Comment 41

[Ryan VanderMeulen [:RyanVM]]

Given that we're still dealing with crashes on 56/57 related to this bug and its subsequent follow-ups, I'm worried that the risk for landing on ESR52 is simply too high. Is it maybe possible to come up with a lower-risk fix for ESR52, like maybe crashing safely instead of getting into UAF territory?

Comment 42

[Andrea Marchesini [:baku]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #41)
> Given that we're still dealing with crashes on 56/57 related to this bug and
> its subsequent follow-ups, I'm worried that the risk for landing on ESR52 is
> simply too high. Is it maybe possible to come up with a lower-risk fix for
> ESR52, like maybe crashing safely instead of getting into UAF territory?

I think it's better to do not land any patch yet here. Bug 1381748 is not in central yet: I have written some patches for bug 1381748 basically rewriting the logic of Fetch() from scratch. But there is 1 android test still orange with this refactoring.

I need to finish those patches and then I'll try to come out with a simpler fix for ESR52. I want to land bug 1381748 in this week.

Comment 43

[Andrea Marchesini [:baku]]

Patches for bug 1381748 seem stable. If we want to uplift bugs we need: 1371889 1373555 1374922 1375749 and 1381748. I can create a single patch with all those ones merged. RyanVM, feedback?

Comment 44

[Ryan VanderMeulen [:RyanVM]]

I think RelMan should weigh in on that. I'm a bit nervous about it, personally.

Comment 45

[Julien Cristau [:jcristau] (back April 22)]

Dan, can we live with leaving this bug unfixed in esr52?  I'm afraid of the magnitude of changes we're talking about here.

Comment 46

[Daniel Veditz [:dveditz]]

I understand the nervousness but this is exactly the kind of bug we promise to fix on ESR. If one external reporter can find this, others could too especially when we release advisories for 56 and they get clues where to look. Given the paucity of test resources on that branch waiting until the next release isn't going to accomplish anything so we might as well land now.

Comment 47

[Ryan VanderMeulen [:RyanVM]]

Looks like we'll be wanting that ESR52 roll-up patch after all :)

Comment 48

[Julien Cristau [:jcristau] (back April 22)]

Andrea, 52.4 gtb is scheduled for today, is there a chance you'll have a patch ready?

Comment 49

[Andrea Marchesini [:baku]]

Attachment: esr52

Comment 50

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8909363 [details] [diff] [review]
esr52

let's try and get this in to 52.4.

Comment 51

[Ryan VanderMeulen [:RyanVM]]

I attempted to land the patch on ESR52, but unfortunately had to backout for breaking nearly every test suite.

First, there was debug build bustage. That was fixed by:
https://hg.mozilla.org/releases/mozilla-esr52/rev/841a30c9111e5424de14f163e7210d9e011eab60

Valgrind failure:
https://treeherder.mozilla.org/logviewer.html#?job_id=131773193&repo=mozilla-esr52

Tons of mochitest, web-platform-test, and xpcshell failures:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-esr52&revision=841a30c9111e5424de14f163e7210d9e011eab60

Comment 53

[Andrea Marchesini [:baku]]

Attachment: esr52

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

Comment 54

[Ryan VanderMeulen [:RyanVM]]

Still has one test failure remaining:
https://treeherder.mozilla.org/logviewer.html#?job_id=131948346&repo=try

Comment 55

[Andrea Marchesini [:baku]]

Bug 1361722 must be uplift. I merged bug 1361722 in my patch.

Comment 56

[Andrea Marchesini [:baku]]

Attachment: 9c4f43ce54f1

Comment 57

[Ryan VanderMeulen [:RyanVM]]

Landed and stuck this time :-). Thanks for slogging through that backport, Andrea!

https://hg.mozilla.org/releases/mozilla-esr52/rev/6ff3c82962f0