Closed
Bug 1371657
Opened 7 years ago
Closed 7 years ago
Heap use-after-free in @[nsFrameSelection::cycleCollection::TraverseNative(void*, nsCycleCollectionTraversalCallback&) /home/worker/workspace/build/src/layout/generic/nsSelection.cpp:613]
Categories
(Core :: DOM: Editor, defect)
Tracking
()
People
(Reporter: jkratzer, Assigned: smaug)
References
(Blocks 1 open bug)
Details
(4 keywords, Whiteboard: [adv-main56+][adv-esr52.4+][post-critsmash-triage])
Attachments
(4 files, 5 obsolete files)
1.23 KB,
text/html
|
Details | |
7.20 KB,
patch
|
jcristau
:
approval-mozilla-esr52+
|
Details | Diff | Splinter Review |
7.88 KB,
patch
|
Details | Diff | Splinter Review | |
7.62 KB,
patch
|
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Found while fuzzing mozilla-central rev 20170602-aeb3d0ca558f. Will update with a testcase shortly. ==10192==ERROR: AddressSanitizer: heap-use-after-free on address 0x622000807108 at pc 0x7fd02eab9ba1 bp 0x7ffde84db860 sp 0x7ffde84db858 READ of size 8 at 0x622000807108 thread T0 #0 0x7fd02eab9ba0 in get /home/worker/workspace/build/src/obj-firefox/dist/include/nsCOMPtr.h:742:48 #1 0x7fd02eab9ba0 in operator nsIDocument * /home/worker/workspace/build/src/obj-firefox/dist/include/nsCOMPtr.h:750 #2 0x7fd02eab9ba0 in GetDocument /home/worker/workspace/build/src/obj-firefox/dist/include/nsIPresShell.h:265 #3 0x7fd02eab9ba0 in nsFrameSelection::cycleCollection::TraverseNative(void*, nsCycleCollectionTraversalCallback&) /home/worker/workspace/build/src/layout/generic/nsSelection.cpp:613 #4 0x7fd027c5af1f in TraverseNativeAndJS /home/worker/workspace/build/src/obj-firefox/dist/include/nsCycleCollectionParticipant.h:128:19 #5 0x7fd027c5af1f in CCGraphBuilder::BuildGraph(js::SliceBudget&) /home/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:2269 #6 0x7fd027c620c7 in nsCycleCollector::MarkRoots(js::SliceBudget&) /home/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:2865:33 #7 0x7fd027c680ab in nsCycleCollector::Collect(ccType, js::SliceBudget&, nsICycleCollectorListener*, bool) /home/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:3654:9 #8 0x7fd027c6be60 in nsCycleCollector_collect(nsICycleCollectorListener*) /home/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:4187:21 #9 0x7fd02a958520 in nsJSContext::CycleCollectNow(nsICycleCollectorListener*, int) /home/worker/workspace/build/src/dom/base/nsJSEnvironment.cpp:1448:3 #10 0x7fd02a957b61 in nsJSEnvironmentObserver::Observe(nsISupports*, char const*, char16_t const*) /home/worker/workspace/build/src/dom/base/nsJSEnvironment.cpp:344:7 #11 0x7fd027ccdecc in nsObserverList::NotifyObservers(nsISupports*, char const*, char16_t const*) /home/worker/workspace/build/src/xpcom/ds/nsObserverList.cpp:112:19 #12 0x7fd027cd1644 in nsObserverService::NotifyObservers(nsISupports*, char const*, char16_t const*) /home/worker/workspace/build/src/xpcom/ds/nsObserverService.cpp:281:19 #13 0x7fd027dda9b1 in NS_InvokeByIndex /home/worker/workspace/build/src/xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_x86_64_unix.S:129 #14 0x7fd02964521b in Invoke /home/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNative.cpp:1996:12 #15 0x7fd02964521b in Call /home/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNative.cpp:1315 #16 0x7fd02964521b in XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) /home/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNative.cpp:1282 #17 0x7fd02964c2ff in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) /home/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:982:12 #18 0x7fd031cce423 in CallJSNative /home/worker/workspace/build/src/js/src/jscntxtinlines.h:293:15 #19 0x7fd031cce423 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:470 #20 0x7fd031cb6f6b in CallFromStack /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:521:12 #21 0x7fd031cb6f6b in Interpret(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3028 #22 0x7fd031c9d198 in js::RunScript(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:410:12 #23 0x7fd031cce5a8 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:488:15 #24 0x7fd031ccedd2 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:534:10 #25 0x7fd032634973 in JS_CallFunctionValue(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/jsapi.cpp:2832:12 #26 0x7fd02956d49b in xpc::FunctionForwarder(JSContext*, unsigned int, JS::Value*) /home/worker/workspace/build/src/js/xpconnect/src/ExportHelpers.cpp:319:18 #27 0x7fd031cce423 in CallJSNative /home/worker/workspace/build/src/js/src/jscntxtinlines.h:293:15 #28 0x7fd031cce423 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:470 #29 0x7fd031ef1c2d in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/jit/BaselineIC.cpp:2453:14 #30 0x7fcfd6f13382 (<unknown module>) 0x622000807108 is located 8 bytes inside of 5776-byte region [0x622000807100,0x622000808790) freed by thread T0 here: #0 0x4bb62b in __interceptor_free /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:47:3 #1 0x7fd02e69fdec in Release /home/worker/workspace/build/src/layout/base/PresShell.cpp:892:1 #2 0x7fd02e69fdec in non-virtual thunk to mozilla::PresShell::Release() /home/worker/workspace/build/src/layout/base/PresShell.cpp:892 #3 0x7fd02e0a4c15 in ~nsCOMPtr_base /home/worker/workspace/build/src/obj-firefox/dist/include/nsCOMPtr.h:294:7 #4 0x7fd02e0a4c15 in nsTextServicesDocument::~nsTextServicesDocument() /home/worker/workspace/build/src/editor/txtsvc/nsTextServicesDocument.cpp:96 #5 0x7fd02e0a500d in nsTextServicesDocument::~nsTextServicesDocument() /home/worker/workspace/build/src/editor/txtsvc/nsTextServicesDocument.cpp:94:1 #6 0x7fd027c61b57 in SnowWhiteKiller::~SnowWhiteKiller() /home/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:2651:25 #7 0x7fd027c68b3b in FreeSnowWhite /home/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:2826:3 #8 0x7fd027c68b3b in nsCycleCollector::BeginCollection(ccType, nsICycleCollectorListener*) /home/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:3829 #9 0x7fd027c68053 in nsCycleCollector::Collect(ccType, js::SliceBudget&, nsICycleCollectorListener*, bool) /home/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:3650:9 #10 0x7fd027c6be60 in nsCycleCollector_collect(nsICycleCollectorListener*) /home/worker/workspace/build/src/xpcom/base/nsCycleCollector.cpp:4187:21 #11 0x7fd02a958520 in nsJSContext::CycleCollectNow(nsICycleCollectorListener*, int) /home/worker/workspace/build/src/dom/base/nsJSEnvironment.cpp:1448:3 #12 0x7fd02a957b61 in nsJSEnvironmentObserver::Observe(nsISupports*, char const*, char16_t const*) /home/worker/workspace/build/src/dom/base/nsJSEnvironment.cpp:344:7 #13 0x7fd027ccdecc in nsObserverList::NotifyObservers(nsISupports*, char const*, char16_t const*) /home/worker/workspace/build/src/xpcom/ds/nsObserverList.cpp:112:19 #14 0x7fd027cd1644 in nsObserverService::NotifyObservers(nsISupports*, char const*, char16_t const*) /home/worker/workspace/build/src/xpcom/ds/nsObserverService.cpp:281:19 #15 0x7fd027dda9b1 in NS_InvokeByIndex /home/worker/workspace/build/src/xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_x86_64_unix.S:129 #16 0x7fd02964521b in Invoke /home/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNative.cpp:1996:12 #17 0x7fd02964521b in Call /home/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNative.cpp:1315 #18 0x7fd02964521b in XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) /home/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNative.cpp:1282 #19 0x7fd02964c2ff in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) /home/worker/workspace/build/src/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:982:12 #20 0x7fd031cce423 in CallJSNative /home/worker/workspace/build/src/js/src/jscntxtinlines.h:293:15 #21 0x7fd031cce423 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:470 #22 0x7fd031cb6f6b in CallFromStack /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:521:12 #23 0x7fd031cb6f6b in Interpret(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3028 #24 0x7fd031c9d198 in js::RunScript(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:410:12 #25 0x7fd031cce5a8 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:488:15 #26 0x7fd031ccedd2 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:534:10 #27 0x7fd032634973 in JS_CallFunctionValue(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/jsapi.cpp:2832:12 #28 0x7fd02956d49b in xpc::FunctionForwarder(JSContext*, unsigned int, JS::Value*) /home/worker/workspace/build/src/js/xpconnect/src/ExportHelpers.cpp:319:18 #29 0x7fd031cce423 in CallJSNative /home/worker/workspace/build/src/js/src/jscntxtinlines.h:293:15 #30 0x7fd031cce423 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:470 #31 0x7fd031ef1c2d in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/jit/BaselineIC.cpp:2453:14 #32 0x7fcfd6f13382 (<unknown module>) #33 0x6210020905ff (<unknown module>) #34 0x7fcfd6f088a5 (<unknown module>) #35 0x7fd031f19812 in EnterBaseline(JSContext*, js::jit::EnterJitData&) /home/worker/workspace/build/src/js/src/jit/BaselineJIT.cpp:160:9 #36 0x7fd031f1a9a5 in js::jit::EnterBaselineAtBranch(JSContext*, js::InterpreterFrame*, unsigned char*) /home/worker/workspace/build/src/js/src/jit/BaselineJIT.cpp:268:28 #37 0x7fd031cc0c7f in Interpret(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:1979:28 previously allocated by thread T0 here: #0 0x4bb97c in malloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:64:3 #1 0x4ece9d in moz_xmalloc /home/worker/workspace/build/src/memory/mozalloc/mozalloc.cpp:83:17 #2 0x7fd02a83ed26 in operator new /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/mozalloc.h:194:12 #3 0x7fd02a83ed26 in nsDocument::CreateShell(nsPresContext*, nsViewManager*, mozilla::StyleSetHandle) /home/worker/workspace/build/src/dom/base/nsDocument.cpp:3872 #4 0x7fd02e791d27 in nsDocumentViewer::InitPresentationStuff(bool) /home/worker/workspace/build/src/layout/base/nsDocumentViewer.cpp:706:27 #5 0x7fd02e791609 in nsDocumentViewer::InitInternal(nsIWidget*, nsISupports*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, bool, bool, bool) /home/worker/workspace/build/src/layout/base/nsDocumentViewer.cpp:964:10 #6 0x7fd02e790817 in nsDocumentViewer::Init(nsIWidget*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) /home/worker/workspace/build/src/layout/base/nsDocumentViewer.cpp:690:10 #7 0x7fd030d3f43b in nsDocShell::SetupNewViewer(nsIContentViewer*) /home/worker/workspace/build/src/docshell/base/nsDocShell.cpp:9469:7 #8 0x7fd030d3dcb6 in nsDocShell::Embed(nsIContentViewer*, char const*, nsISupports*) /home/worker/workspace/build/src/docshell/base/nsDocShell.cpp:7294:17 #9 0x7fd030cd4841 in nsDocShell::CreateContentViewer(nsACString const&, nsIRequest*, nsIStreamListener**) /home/worker/workspace/build/src/docshell/base/nsDocShell.cpp:9274:3 #10 0x7fd030cd171b in nsDSURIContentListener::DoContent(nsACString const&, bool, nsIRequest*, nsIStreamListener**, bool*) /home/worker/workspace/build/src/docshell/base/nsDSURIContentListener.cpp:129:21 #11 0x7fd029849186 in nsDocumentOpenInfo::TryContentListener(nsIURIContentListener*, nsIChannel*) /home/worker/workspace/build/src/uriloader/base/nsURILoader.cpp:756:28 #12 0x7fd02984635c in nsDocumentOpenInfo::DispatchContent(nsIRequest*, nsISupports*) /home/worker/workspace/build/src/uriloader/base/nsURILoader.cpp:434:30 #13 0x7fd029844b73 in nsDocumentOpenInfo::OnStartRequest(nsIRequest*, nsISupports*) /home/worker/workspace/build/src/uriloader/base/nsURILoader.cpp:296:8 #14 0x7fd027f23877 in nsBaseChannel::OnStartRequest(nsIRequest*, nsISupports*) /home/worker/workspace/build/src/netwerk/base/nsBaseChannel.cpp:849:25 #15 0x7fd027f6fcb0 in nsInputStreamPump::OnStateStart() /home/worker/workspace/build/src/netwerk/base/nsInputStreamPump.cpp:539:25 #16 0x7fd027f6f1fe in nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) /home/worker/workspace/build/src/netwerk/base/nsInputStreamPump.cpp:441:25 #17 0x7fd027d5babd in nsInputStreamReadyEvent::Run() /home/worker/workspace/build/src/xpcom/io/nsStreamUtils.cpp:96:20 #18 0x7fd027dbdd0e in nsThread::ProcessNextEvent(bool, bool*) /home/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1321:14 #19 0x7fd027dca158 in NS_ProcessNextEvent(nsIThread*, bool) /home/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:472:10 #20 0x7fd028b940f1 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /home/worker/workspace/build/src/ipc/glue/MessagePump.cpp:96:21 #21 0x7fd028af1660 in RunInternal /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:238:10 #22 0x7fd028af1660 in RunHandler /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:231 #23 0x7fd028af1660 in MessageLoop::Run() /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:211 #24 0x7fd02dfacbdf in nsBaseAppShell::Run() /home/worker/workspace/build/src/widget/nsBaseAppShell.cpp:156:27 #25 0x7fd03163e8c1 in nsAppStartup::Run() /home/worker/workspace/build/src/toolkit/components/startup/nsAppStartup.cpp:283:30 #26 0x7fd03180da54 in XREMain::XRE_mainRun() /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4571:22 #27 0x7fd03180f5dc in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4751:8 #28 0x7fd031810931 in XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4846:21 #29 0x4eb5a3 in do_main /home/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:236:22 #30 0x4eb5a3 in main /home/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:309 #31 0x7fd043ac982f in __libc_start_main /build/glibc-Qz8a69/glibc-2.23/csu/../csu/libc-start.c:291 SUMMARY: AddressSanitizer: heap-use-after-free /home/worker/workspace/build/src/obj-firefox/dist/include/nsCOMPtr.h:742:48 in get Shadow bytes around the buggy address: 0x0c44800f8dd0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c44800f8de0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c44800f8df0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c44800f8e00: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c44800f8e10: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa =>0x0c44800f8e20: fd[fd]fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x0c44800f8e30: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x0c44800f8e40: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x0c44800f8e50: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x0c44800f8e60: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x0c44800f8e70: 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 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 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb ==10192==ABORTING
Reporter | ||
Comment 1•7 years ago
|
||
Reporter | ||
Updated•7 years ago
|
Flags: in-qa-testsuite?
Keywords: testcase
Summary: Intermittent heap use-after-free in @[nsFrameSelection::cycleCollection::TraverseNative(void*, nsCycleCollectionTraversalCallback&) /home/worker/workspace/build/src/layout/generic/nsSelection.cpp:613] → Heap use-after-free in @[nsFrameSelection::cycleCollection::TraverseNative(void*, nsCycleCollectionTraversalCallback&) /home/worker/workspace/build/src/layout/generic/nsSelection.cpp:613]
Comment 2•7 years ago
|
||
Can be a bit of a PITA to reproduce, requiring some refreshing and/or letting it sit for a bit before eventually crashing. INFO: Last good revision: e7d613b3bcfe1e865378bfac37de64560d1234ec (2015-09-17) INFO: First bad revision: 11dc79e232110ba6de5179e46dfbda77b52a88c3 (2015-09-18) INFO: Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e7d613b3bcfe1e865378bfac37de64560d1234ec&tochange=11dc79e232110ba6de5179e46dfbda77b52a88c3 Too old to get a more narrow regression range via mozregression, unfortunately :(. Hopefully someone else sees a culprit in that range.
Group: core-security → layout-core-security
status-firefox54:
--- → wontfix
status-firefox55:
--- → affected
status-firefox56:
--- → affected
status-firefox-esr52:
--- → affected
tracking-firefox55:
--- → ?
tracking-firefox56:
--- → ?
tracking-firefox-esr52:
--- → ?
Flags: in-qa-testsuite? → in-testsuite?
Version: unspecified → 43 Branch
Comment 3•7 years ago
|
||
I don't see anything suspicious in this range given the testcase... We may need to do bisect build in this range to know exactly what's happening.
Updated•7 years ago
|
Keywords: regressionwindow-wanted
Comment 5•7 years ago
|
||
Cornel, can you help narrow down the regression window? I cc-d you since I'm not sure who else from QE might be checking sec bugs.
Flags: needinfo?(cornel.ionce)
Comment 6•7 years ago
|
||
Adding Cristian in the loop, he will try to investigate this and provide the required information.
Flags: needinfo?(cornel.ionce) → needinfo?(cristian.comorasu)
Comment 7•7 years ago
|
||
Hi everyone. Although I`ve updated mozregression to the latest version I could not check the versions between last-good and first bad. However I could reproduce this issue on some earlier builds. The regression I found is: Last good: 20150913030204 First bad: 20150914030205 Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=68718290640b0ca195344fa5cc4c55fbc260e19e&tochange=9ed17db42e3e46f1c712e4dffd62d54e915e0fac Crash reports: https://crash-stats.mozilla.com/report/index/1ef0fb50-6ffc-4129-9c4e-9e5b00170731 https://crash-stats.mozilla.com/report/index/d8c2bb91-16e4-4181-b31c-542b80170731 https://crash-stats.mozilla.com/report/index/7abdc21f-a3c0-4eb1-a0ff-8b6920170731 If there is a workaround with the mozregression issue or I missed something please let me know. Cheers!
Flags: needinfo?(cristian.comorasu)
Comment 8•7 years ago
|
||
It looks like this is a UAF of a document, so I'm guessing this is happening here: if (tmp->mShell && tmp->mShell->GetDocument() && nsCCUncollectableMarker::InGeneration(cb, tmp->mShell->GetDocument()-> GetMarkedCCGeneration())) { It looks like mShell is weak. I'm not sure how the lifetime of this is supposed to work.
Assignee | ||
Comment 9•7 years ago
|
||
Attachment #8892151 -
Flags: review?(masayuki)
Assignee | ||
Comment 10•7 years ago
|
||
That is a quick and dirty patch which should be safe for branches too, and even if some other approach would be taken too, I definitely want to keep that patch. We really must unbind native anonymous elements editor has created.
Comment 11•7 years ago
|
||
Comment on attachment 8892151 [details] [diff] [review] quick and dirty Hmm... Editor may be destroyed without calling PreDestroy()???
Attachment #8892151 -
Flags: review?(masayuki) → review+
Assignee | ||
Comment 12•7 years ago
|
||
Comment on attachment 8892151 [details] [diff] [review] quick and dirty [Security approval request comment] How easily could an exploit be constructed based on the patch? I'd say not very easily. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Commit message could be "Bug 1371657, ensure editing UI is hidden when disabling editing, r=masayuki" 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? The patch should apply everywhere How likely is this patch to cause regressions; how much testing does it need? Should be quite safe. We're normally calling the method in PreDestroy.
Attachment #8892151 -
Flags: sec-approval?
Attachment #8892151 -
Flags: approval-mozilla-esr52?
Attachment #8892151 -
Flags: approval-mozilla-beta?
Comment 13•7 years ago
|
||
Comment on attachment 8892151 [details] [diff] [review] quick and dirty Sec-approval+ for trunk. Ritu will approve elsewhere.
Attachment #8892151 -
Flags: sec-approval? → sec-approval+
Al pinged me about this one. Let's take this in 55.0 RC2 and ESR52.3
Comment on attachment 8892151 [details] [diff] [review] quick and dirty Let's uplift to Beta56, Release55 (for RC), ESR52.3
Attachment #8892151 -
Flags: approval-mozilla-release+
Attachment #8892151 -
Flags: approval-mozilla-esr52?
Attachment #8892151 -
Flags: approval-mozilla-esr52+
Attachment #8892151 -
Flags: approval-mozilla-beta?
Attachment #8892151 -
Flags: approval-mozilla-beta+
status-firefox57:
--- → affected
Comment 16•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb6f73d12931075601f54683113b4c0a166ec941
Comment 17•7 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-release/rev/78278c4dc7304598cb366e31e80ab6521586dc9d https://hg.mozilla.org/releases/mozilla-esr52/rev/20a1a6ad46d5a4ec83d9800614fc288bf79e14a8 Will land on Beta56 once all the post-merge bustage is cleaned up.
Updated•7 years ago
|
Whiteboard: [adv-main55+][adv-esr52.3+]
https://hg.mozilla.org/mozilla-central/rev/eb6f73d12931
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 19•7 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/ffbcc905a80a
Updated•7 years ago
|
Group: layout-core-security → core-security-release
Comment 20•7 years ago
|
||
Hi everyone, I could still reproduce this crash using build 20170803100352 . ( https://crash-stats.mozilla.com/report/index/846f6969-4a16-4251-9c2f-befa70170803 ) I tried again with build 20170806100257 and could still reproduce it: - https://crash-stats.mozilla.com/report/index/3847dd5e-cf9b-4a15-90a9-2d2050170807 - https://crash-stats.mozilla.com/report/index/5d9df8b4-9acb-4ba3-963a-baa900170807 - https://crash-stats.mozilla.com/report/index/f9b6295d-143b-4382-a5c2-634a80170807 Am I missing something?
Flags: needinfo?(jkratzer)
Reporter | ||
Comment 21•7 years ago
|
||
(In reply to Cristian Comorasu [:CristiComo] from comment #20) > Hi everyone, > I could still reproduce this crash using build 20170803100352 . ( > https://crash-stats.mozilla.com/report/index/846f6969-4a16-4251-9c2f- > befa70170803 ) > I tried again with build 20170806100257 and could still reproduce it: > - > https://crash-stats.mozilla.com/report/index/3847dd5e-cf9b-4a15-90a9- > 2d2050170807 > - > https://crash-stats.mozilla.com/report/index/5d9df8b4-9acb-4ba3-963a- > baa900170807 > - > https://crash-stats.mozilla.com/report/index/f9b6295d-143b-4382-a5c2- > 634a80170807 > > Am I missing something? I just tested with mc-asan-opt rev 20170807-47248637eafa and can confirm that this issue still reproduces.
Flags: needinfo?(jkratzer)
Updated•7 years ago
|
Flags: needinfo?(bugs)
Updated•7 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla57 → ---
Assignee | ||
Comment 22•7 years ago
|
||
Whaat, odd, I'm getting now assertions I definitely didn't get earlier. Looking.
Assignee | ||
Comment 23•7 years ago
|
||
er, idiot me. The patch was for bug 1380284.
Flags: needinfo?(bugs)
Updated•7 years ago
|
Attachment #8892151 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Component: Layout: HTML Frames → Editor
Comment 24•7 years ago
|
||
Pretty unlikely we'll ship this in Fx55/ESR52.3 at this point.
Updated•7 years ago
|
Whiteboard: [adv-main55+][adv-esr52.3+]
Assignee | ||
Comment 25•7 years ago
|
||
Assignee | ||
Comment 26•7 years ago
|
||
Comment on attachment 8895058 [details] [diff] [review] WIP The issue here is that nsContentUtils::AddScriptRunner is used, and while replaceChild is called, there is first a removal and then insertion and the runner runs only afterwards, yet selection listeners etc are notified, and editor clearly can't deal with the case when GetBody returns something bogus. I'm not quite happy with this. We had similar-ish issue in bug 767169 which added the scriptrunner usage. Couldn't think of anything simpler, and this doesn't regress bug 545775. But feel free to suggest something else :)
Attachment #8895058 -
Flags: review?(masayuki)
Comment 27•7 years ago
|
||
Comment on attachment 8895058 [details] [diff] [review] WIP Although, I don't know if this approach can fix this bug, how about to stop caching mRootElement at first call of HTMLEditor::GetRootElement()? I think that mRootElement should be actual root element every time. So, in EditorBase::Init(), it should be initialized properly: https://searchfox.org/mozilla-central/rev/c329d562fb6c6218bdb79290faaf015467ef89e2/editor/libeditor/EditorBase.cpp#257-258 # FYI: If it's an HTMLEditor, aRoot is nullptr Then, HTMLEditor::ContentRemoved() should clear mRootElement and HTMLEditor::DoContentInserted() set new root element. Finally, HTMLEditor::ResetRootElementAndEventTarget() should do nothing about mRootElement. If HTMLEditor::DoContentInserted() puts off to set mRootElement until HTMLEditor::ResetRootElementAndEventTarget(), then, the behavior should be same as your patch even without the new bool member.
Assignee | ||
Comment 28•7 years ago
|
||
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #27) > Comment on attachment 8895058 [details] [diff] [review] > WIP > > Although, I don't know if this approach can fix this bug, how about to stop > caching mRootElement at first call of HTMLEditor::GetRootElement()? That would be probably a bit too slow. > Then, HTMLEditor::ContentRemoved() should clear mRootElement and > HTMLEditor::DoContentInserted() set new root element. Finally, > HTMLEditor::ResetRootElementAndEventTarget() should do nothing about > mRootElement. > > If HTMLEditor::DoContentInserted() puts off to set mRootElement until > HTMLEditor::ResetRootElementAndEventTarget(), then, the behavior should be > same as your patch even without the new bool member. Ah, this could work.
Assignee | ||
Comment 29•7 years ago
|
||
Except that calling GetRoot() before ResetRootElementAndEventTarget would reget the root element.
Assignee | ||
Comment 30•7 years ago
|
||
And that would lead to odd setup. We would have root element pointing to something, but there would be pending ResetRootElementAndEventTarget in script runner queue. I think I prefer my patch.
Comment 31•7 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #29) > Except that calling GetRoot() before ResetRootElementAndEventTarget would > reget the root element. I meant that when GetRootElement() shouldn't modify the mRootElement anymore. mRootElement should be set by: EditorBase::Init(), HTMLEditor::DoContentInserted() and HTMLEditor::ContentRemoved(). So, my idea makes mRootElement always exactly same (actual) root element any time. I.e., GetRootElement(), GetRoot() should just return mRootElement even when it's nullptr.
Comment 32•7 years ago
|
||
FYI: I'll be offline tomorrow due to Japanese national holiday.
Assignee | ||
Comment 33•7 years ago
|
||
oh, now I see what you mean. yeah, that should work the same was as my patch.
Assignee | ||
Comment 34•7 years ago
|
||
Hmm, what if body is the root element and then that it removed. html element should become the root. /me thinks
Assignee | ||
Comment 35•7 years ago
|
||
This way then? I couldn't see reason for RemoveEventListeners/InstallEventListeners calls.
Attachment #8895964 -
Flags: review?(masayuki)
Assignee | ||
Updated•7 years ago
|
Attachment #8895058 -
Flags: review?(masayuki)
Comment 36•7 years ago
|
||
Comment on attachment 8895964 [details] [diff] [review] v2 > void >-HTMLEditor::ResetRootElementAndEventTarget() >+HTMLEditor::NotifyRootChanged() > { >+ if (!mRootElement) { >+ return; >+ } >+ > nsCOMPtr<nsIMutationObserver> kungFuDeathGrip(this); > >- // Need to remove the event listeners first because BeginningOfDocument >- // could set a new root (and event target is set by InstallEventListeners()) >- // and we won't be able to remove them from the old event target then. >- RemoveEventListeners(); Yeah, all event listeners are added to the document node. So, we don't need to reinstall them when its root is replaced. On the other hand, RemoveEventListener() commits composition: https://searchfox.org/mozilla-central/rev/c329d562fb6c6218bdb79290faaf015467ef89e2/editor/libeditor/EditorBase.cpp#379,385,388 > EditorBase::RemoveEventListeners() > { > if (!IsInitialized() || !mEventListener) { > return; > } > reinterpret_cast<EditorEventListener*>(mEventListener.get())->Disconnect(); > if (mComposition) { > // Even if this is called, don't release mComposition because this is > // may be reused after reframing. > mComposition->EndHandlingComposition(this); This *might* be necessary but I should check this with actual IME and would fix this in a followup bug if necessary.
Attachment #8895964 -
Flags: review?(masayuki) → review+
Assignee | ||
Comment 37•7 years ago
|
||
Ah, hrm, better to keep remove/installEventListeners then, for now.
Assignee | ||
Comment 38•7 years ago
|
||
Better to reduce risk for regression in security bugs.
Attachment #8895964 -
Attachment is obsolete: true
Attachment #8896409 -
Flags: review?(masayuki)
Comment 39•7 years ago
|
||
Comment on attachment 8896409 [details] [diff] [review] v3 Oops, sorry for the delay. I marked the request email "as read" unexpectedly.
Attachment #8896409 -
Flags: review?(masayuki) → review+
Assignee | ||
Comment 40•7 years ago
|
||
Comment on attachment 8896409 [details] [diff] [review] v3 [Security approval request comment] How easily could an exploit be constructed based on the patch? I'd say not easily. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Commit message could be -m "Bug 1371657, ensure we use the right root element in editor, r=masayuki" Which older supported branches are affected by this flaw? all If not all supported branches, which bug introduced the flaw? old code Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? The patch seems to apply to beta at least. esr52 would need a new patch. Quite some merging issues there. How likely is this patch to cause regressions; how much testing does it need? This affects relatively rarely used replace-the-root-element case. Testing various text editing frameworks implemented in web technologies would be still good.
Attachment #8896409 -
Flags: sec-approval?
Assignee | ||
Comment 41•7 years ago
|
||
Hmm, the patch doesn't work in esr52, I mean even after merging there is a crash.
Assignee | ||
Updated•7 years ago
|
Attachment #8896409 -
Flags: sec-approval?
Sec-high, tracked for 57.
Assignee | ||
Comment 43•7 years ago
|
||
Sorry, still one. Need to mark root null in ContentRemoved because bug 1361268 and bug 1364180 aren't in esr52 and I want to keep trunk and esr52 patches as close to each others as possible.
Attachment #8896409 -
Attachment is obsolete: true
Attachment #8898458 -
Flags: review?(masayuki)
Assignee | ||
Comment 44•7 years ago
|
||
Comment 45•7 years ago
|
||
Comment on attachment 8898458 [details] [diff] [review] editor_root_v4.diff But could you also change: Make EditorBase::GetRoot() won't call GetRootElement(nsIDOMElement**) and make it inline? (i.e., moving the implementation into EditorBase.h?)
Attachment #8898458 -
Flags: review?(masayuki) → review+
Assignee | ||
Comment 46•7 years ago
|
||
I'm trying to keep security fixes as small as possible, so such inlining should be done in a separate bug.
Assignee | ||
Comment 47•7 years ago
|
||
Comment on attachment 8898458 [details] [diff] [review] editor_root_v4.diff [Security approval request comment] How easily could an exploit be constructed based on the patch? I'd say not easily. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Commit message could be -m "Bug 1371657, ensure we use the right root element in editor, r=masayuki" Which older supported branches are affected by this flaw? all If not all supported branches, which bug introduced the flaw? old code Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? The patch seems to apply to beta at least. there is a separate patch for esr52. How likely is this patch to cause regressions; how much testing does it need? This affects relatively rarely used replace-the-root-element case. Testing various text editing frameworks implemented in web technologies would be still good.
Attachment #8898458 -
Flags: sec-approval?
Comment 48•7 years ago
|
||
sec-approval for checked on August 28, three weeks into the current cycle. We'll want beta and esr52 patches made and nominated as well to land afterwards.
Whiteboard: [checkin on 8/28]
Updated•7 years ago
|
Attachment #8898458 -
Flags: sec-approval? → sec-approval+
Updated•7 years ago
|
Attachment #8895058 -
Attachment is obsolete: true
Comment 49•7 years ago
|
||
This patch needs rebasing for trunk. Looks like Beta will need a rebased patch as well.
Flags: needinfo?(bugs)
Whiteboard: [checkin on 8/28]
Assignee | ||
Comment 50•7 years ago
|
||
This is for m-i (the patch did apply with some fuzz). Another one for beta coming.
Attachment #8898458 -
Attachment is obsolete: true
Flags: needinfo?(bugs)
Assignee | ||
Comment 51•7 years ago
|
||
for beta
Comment 52•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/08c89ceda404930d9e58e7c015496255ef1d42b5
Comment 53•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/08c89ceda404 Please nominate the Beta/ESR52 patches for approval when you get a chance.
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Flags: needinfo?(bugs)
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Assignee | ||
Comment 54•7 years ago
|
||
Comment on attachment 8901943 [details] [diff] [review] editor_root_v5_beta.diff Approval Request Comment See comment 47 [String changes made/needed]: NA
Flags: needinfo?(bugs)
Attachment #8901943 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 55•7 years ago
|
||
Comment on attachment 8898459 [details] [diff] [review] for esr52 Approval Request Comment See comment 47 [String changes made/needed]: NA
Attachment #8898459 -
Flags: approval-mozilla-esr52?
Comment 56•7 years ago
|
||
Comment on attachment 8901943 [details] [diff] [review] editor_root_v5_beta.diff Fix for a sec-high issue, let's land this for the beta 8 build tomorrow morning.
Attachment #8901943 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 57•7 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/3b007282412b
Comment 58•7 years ago
|
||
Comment on attachment 8898459 [details] [diff] [review] for esr52 sec-high fix for editor, esr52.4+
Attachment #8898459 -
Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Comment 59•7 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-esr52/rev/ae110cf77596
Updated•7 years ago
|
Whiteboard: [adv-main56+][adv-esr52.4+]
Updated•7 years ago
|
Flags: qe-verify-
Whiteboard: [adv-main56+][adv-esr52.4+] → [adv-main56+][adv-esr52.4+][post-critsmash-triage]
Updated•6 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•