865076 - Heap-use-after-free in nsAttrAndChildArray::GetAttr

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Abhishek Arya]

Attachment: Testcase

Need the fuzzPriv extension to forceGC

==7961== ERROR: AddressSanitizer: heap-use-after-free on address 0x603800ddc660 at pc 0x7fa52c52e33e bp 0x7fffd07dc730 sp 0x7fffd07dc728
READ of size 8 at 0x603800ddc660 thread T0
    #0 0x7fa52c52e33d in nsAttrAndChildArray::AttrSlotCount() const content/base/src/nsAttrAndChildArray.h:144
    #1 0x7fa52c52d68b in nsAttrAndChildArray::GetAttr(nsIAtom*, int) const content/base/src/nsAttrAndChildArray.cpp:297
    #2 0x7fa52b21200c in mozilla::dom::Element::AttrValueIs(int, nsIAtom*, nsIAtom*, nsCaseTreatment) const ../../../dist/include/mozilla/dom/Element.h:1189
    #3 0x7fa53190bd2e in mozilla::dom::SVGSwitchElement::FindActiveChild() const content/svg/content/src/SVGSwitchElement.cpp:129
    #4 0x7fa53190b82d in mozilla::dom::SVGSwitchElement::MaybeInvalidate() content/svg/content/src/SVGSwitchElement.cpp:59
    #5 0x7fa53190e041 in mozilla::dom::SVGSwitchElement::RemoveChildAt(unsigned int, bool) content/svg/content/src/SVGSwitchElement.cpp:99
    #6 0x7fa52cb68104 in nsINode::Remove() content/base/src/nsINode.cpp:1408
    #7 0x7fa535e8de5a in mozilla::dom::ElementBinding::remove(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Element*, unsigned int, JS::Value*) objdir-ff-asan-sym/dom/bindings/ElementBinding.cpp:2009
    #8 0x7fa535e8d53a in mozilla::dom::ElementBinding::genericMethod(JSContext*, unsigned int, JS::Value*) objdir-ff-asan-sym/dom/bindings/ElementBinding.cpp:2055
    #9 0x7fa53fb7accf in js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/jscntxtinlines.h:337
    #10 0x7fa53fb7accf in js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/jsinterp.cpp:407
    #11 0x7fa53fb541a0 in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode, bool) js/src/jsinterp.cpp:2396
    #12 0x7fa53faff64d in js::RunScript(JSContext*, js::StackFrame*) js/src/jsinterp.cpp:364
    #13 0x7fa53fb7b378 in js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/jsinterp.cpp:421
    #14 0x7fa53f59c836 in js::Invoke(JSContext*, js::InvokeArgsGuard&, js::MaybeConstruct) js/src/jsinterp.h:134
    #15 0x7fa53fb7edd0 in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::Value*) js/src/jsinterp.cpp:454
    #16 0x7fa53f4b3490 in JS_CallFunctionValue(JSContext*, JSObject*, JS::Value, unsigned int, JS::Value*, JS::Value*) js/src/jsapi.cpp:5891
    #17 0x7fa531d9cd87 in nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) js/xpconnect/src/XPCWrappedJSClass.cpp:1445
    #18 0x7fa531d4fc4c in nsXPCWrappedJS::CallMethod(unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) js/xpconnect/src/XPCWrappedJS.cpp:578
    #19 0x7fa5380ca624 in PrepareAndDispatch xpcom/reflect/xptcall/src/md/unix/xptcstubs_x86_64_linux.cpp:122
    #20 0x7fa5380c76da in SharedStub
0x603800ddc660 is located 96 bytes inside of 232-byte region [0x603800ddc600,0x603800ddc6e8)
freed by thread T0 here:
    #0 0x4183f2 in __interceptor_free
    #1 0x7fa5487816ce in moz_free memory/mozalloc/mozalloc.cpp:48
    #2 0x7fa53190e4d9 in operator delete(void*) ../../../../dist/include/mozilla/mozalloc.h:225
    #3 0x7fa53190e4d9 in mozilla::dom::SVGSwitchElement::~SVGSwitchElement() ../../../../dist/include/mozilla/dom/SVGSwitchElement.h:19
    #4 0x7fa52cc319c5 in nsNodeUtils::LastRelease(nsINode*) content/base/src/nsNodeUtils.cpp:258
    #5 0x7fa52cedc2fa in mozilla::dom::FragmentOrElement::Release() content/base/src/FragmentOrElement.cpp:1716
    #6 0x7fa531511d45 in nsSVGElement::Release() content/svg/content/src/nsSVGElement.cpp:228
    #7 0x7fa5317b7905 in mozilla::dom::SVGGraphicsElement::Release() content/svg/content/src/SVGGraphicsElement.cpp:15
    #8 0x7fa53190a075 in mozilla::dom::SVGSwitchElement::Release() content/svg/content/src/SVGSwitchElement.cpp:37
    #9 0x7fa52ca9a106 in mozilla::dom::Element::UnbindFromTree(bool, bool) content/base/src/Element.cpp:1283
    #10 0x7fa52d90caa1 in nsGenericHTMLElement::UnbindFromTree(bool, bool) content/html/content/src/nsGenericHTMLElement.cpp:655
    #11 0x7fa52cb68969 in nsINode::doRemoveChildAt(unsigned int, bool, nsIContent*, nsAttrAndChildArray&) content/base/src/nsINode.cpp:1437
    #12 0x7fa52cec95e5 in mozilla::dom::FragmentOrElement::RemoveChildAt(unsigned int, bool) content/base/src/FragmentOrElement.cpp:924
    #13 0x7fa53190e037 in mozilla::dom::SVGSwitchElement::RemoveChildAt(unsigned int, bool) content/svg/content/src/SVGSwitchElement.cpp:98
    #14 0x7fa52cb68104 in nsINode::Remove() content/base/src/nsINode.cpp:1408
    #15 0x7fa535e8de5a in mozilla::dom::ElementBinding::remove(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Element*, unsigned int, JS::Value*) objdir-ff-asan-sym/dom/bindings/ElementBinding.cpp:2009
    #16 0x7fa535e8d53a in mozilla::dom::ElementBinding::genericMethod(JSContext*, unsigned int, JS::Value*) objdir-ff-asan-sym/dom/bindings/ElementBinding.cpp:2055
    #17 0x7fa53fb7accf in js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/jscntxtinlines.h:337
    #18 0x7fa53fb7accf in js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/jsinterp.cpp:407
    #19 0x7fa53fb541a0 in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode, bool) js/src/jsinterp.cpp:2396
    #20 0x7fa53faff64d in js::RunScript(JSContext*, js::StackFrame*) js/src/jsinterp.cpp:364
    #21 0x7fa53fb7b378 in js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/jsinterp.cpp:421
    #22 0x7fa53f59c836 in js::Invoke(JSContext*, js::InvokeArgsGuard&, js::MaybeConstruct) js/src/jsinterp.h:134
    #23 0x7fa53fb7edd0 in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::Value*) js/src/jsinterp.cpp:454
    #24 0x7fa53f4b3490 in JS_CallFunctionValue(JSContext*, JSObject*, JS::Value, unsigned int, JS::Value*, JS::Value*) js/src/jsapi.cpp:5891
    #25 0x7fa531d9cd87 in nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) js/xpconnect/src/XPCWrappedJSClass.cpp:1445
    #26 0x7fa531d4fc4c in nsXPCWrappedJS::CallMethod(unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) js/xpconnect/src/XPCWrappedJS.cpp:578
    #27 0x7fa5380ca624 in PrepareAndDispatch xpcom/reflect/xptcall/src/md/unix/xptcstubs_x86_64_linux.cpp:122
    #28 0x7fa5380c76da in SharedStub
    #29 0x7fa52d54936c in nsEventListenerManager::HandleEventSubType(nsListenerStruct*, mozilla::dom::CallbackObjectHolder<mozilla::dom::EventListener, nsIDOMEventListener> const&, nsIDOMEvent*, mozilla::dom::EventTarget*, nsCxPusher*) content/events/src/nsEventListenerManager.cpp:944
    #30 0x7fa52d54b91f in nsEventListenerManager::HandleEventInternal(nsPresContext*, nsEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*, nsCxPusher*) content/events/src/nsEventListenerManager.cpp:1015
    #31 0x7fa52d740ceb in nsEventListenerManager::HandleEvent(nsPresContext*, nsEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*, nsCxPusher*) content/events/src/nsEventListenerManager.h:326
previously allocated by thread T0 here:
    #0 0x4184d2 in malloc
    #1 0x7fa548781815 in moz_xmalloc memory/mozalloc/mozalloc.cpp:54
    #2 0x7fa531908360 in operator new(unsigned long) ../../../../dist/include/mozilla/mozalloc.h:201
    #3 0x7fa531908360 in NS_NewSVGSwitchElement(nsIContent**, already_AddRefed<nsINodeInfo>) content/svg/content/src/SVGSwitchElement.cpp:13
    #4 0x7fa5316fb8e8 in CreateSwitchElement(nsIContent**, already_AddRefed<nsINodeInfo>, mozilla::dom::FromParser) content/svg/content/src/SVGTagList.h:85
    #5 0x7fa5316edd78 in NS_NewSVGElement(nsIContent**, already_AddRefed<nsINodeInfo>, mozilla::dom::FromParser) content/svg/content/src/SVGElementFactory.cpp:126
    #6 0x7fa52cc010ee in NS_NewElement(nsIContent**, already_AddRefed<nsINodeInfo>, mozilla::dom::FromParser) content/base/src/nsNameSpaceManager.cpp:203
    #7 0x7fa52c880576 in nsIDocument::CreateElementNS(nsAString_internal const&, nsAString_internal const&, mozilla::ErrorResult&) content/base/src/nsDocument.cpp:4767
    #8 0x7fa535d47756 in mozilla::dom::DocumentBinding::createElementNS(JSContext*, JS::Handle<JSObject*>, nsIDocument*, unsigned int, JS::Value*) objdir-ff-asan-sym/dom/bindings/DocumentBinding.cpp:524
    #9 0x7fa535cae35a in mozilla::dom::DocumentBinding::genericMethod(JSContext*, unsigned int, JS::Value*) objdir-ff-asan-sym/dom/bindings/DocumentBinding.cpp:7645
    #10 0x7fa53fb7accf in js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/jscntxtinlines.h:337
    #11 0x7fa53fb7accf in js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/jsinterp.cpp:407
    #12 0x7fa53fb541a0 in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode, bool) js/src/jsinterp.cpp:2396
    #13 0x7fa53faff64d in js::RunScript(JSContext*, js::StackFrame*) js/src/jsinterp.cpp:364
    #14 0x7fa53fb7b378 in js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/jsinterp.cpp:421
    #15 0x7fa53f59c836 in js::Invoke(JSContext*, js::InvokeArgsGuard&, js::MaybeConstruct) js/src/jsinterp.h:134
    #16 0x7fa53fb7edd0 in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::Value*) js/src/jsinterp.cpp:454
    #17 0x7fa53f4b3490 in JS_CallFunctionValue(JSContext*, JSObject*, JS::Value, unsigned int, JS::Value*, JS::Value*) js/src/jsapi.cpp:5891
    #18 0x7fa531d9cd87 in nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) js/xpconnect/src/XPCWrappedJSClass.cpp:1445
    #19 0x7fa531d4fc4c in nsXPCWrappedJS::CallMethod(unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) js/xpconnect/src/XPCWrappedJS.cpp:578
    #20 0x7fa5380ca624 in PrepareAndDispatch xpcom/reflect/xptcall/src/md/unix/xptcstubs_x86_64_linux.cpp:122
    #21 0x7fa5380c76da in SharedStub
    #22 0x7fa52d54936c in nsEventListenerManager::HandleEventSubType(nsListenerStruct*, mozilla::dom::CallbackObjectHolder<mozilla::dom::EventListener, nsIDOMEventListener> const&, nsIDOMEvent*, mozilla::dom::EventTarget*, nsCxPusher*) content/events/src/nsEventListenerManager.cpp:944
    #23 0x7fa52d54b91f in nsEventListenerManager::HandleEventInternal(nsPresContext*, nsEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*, nsCxPusher*) content/events/src/nsEventListenerManager.cpp:1015
    #24 0x7fa52d740ceb in nsEventListenerManager::HandleEvent(nsPresContext*, nsEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*, nsCxPusher*) content/events/src/nsEventListenerManager.h:326
    #25 0x7fa52d730aa7 in nsEventTargetChainItem::HandleEvent(nsEventChainPostVisitor&, bool, nsCxPusher*) content/events/src/nsEventDispatcher.cpp:200
    #26 0x7fa52d72e46c in nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor&, nsDispatchingCallback*, bool, nsCxPusher*) content/events/src/nsEventDispatcher.cpp:325
    #27 0x7fa52d736131 in nsEventDispatcher::Dispatch(nsISupports*, nsPresContext*, nsEvent*, nsIDOMEvent*, nsEventStatus*, nsDispatchingCallback*, nsCOMArray<mozilla::dom::EventTarget>*) content/events/src/nsEventDispatcher.cpp:631
    #28 0x7fa52d7386dd in nsEventDispatcher::DispatchDOMEvent(nsISupports*, nsEvent*, nsIDOMEvent*, nsPresContext*, nsEventStatus*) content/events/src/nsEventDispatcher.cpp:691
    #29 0x7fa52cb61ca4 in nsINode::DispatchEvent(nsIDOMEvent*, bool*) content/base/src/nsINode.cpp:1136
    #30 0x7fa52c6692db in nsContentUtils::DispatchEvent(nsIDocument*, nsISupports*, nsAString_internal const&, bool, bool, bool, bool*) content/base/src/nsContentUtils.cpp:3620
    #31 0x7fa52c668644 in nsContentUtils::DispatchTrustedEvent(nsIDocument*, nsISupports*, nsAString_internal const&, bool, bool, bool*) content/base/src/nsContentUtils.cpp:3590
Shadow bytes around the buggy address:
  0x0c07801b3870: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c07801b3880: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c07801b3890: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c07801b38a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c07801b38b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x0c07801b38c0: fd fd fd fd fd fd fd fd fd fd fd fd[fd]fd fd fd
  0x0c07801b38d0: fd fd fd fd fd fd fd fd fd fd fd fd fd fa fa fa
  0x0c07801b38e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c07801b38f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c07801b3900: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c07801b3910: 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
  Heap righ 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
  ASan internal:         fe
==7961== ABORTING

Comment 1

[Mats Palmgren (inactive)]

The SVGSwitchElement is deleted.

BTW, here's the add-on I used:
https://www.squarefree.com/extensions/domFuzzLite3.xpi

Comment 2

[Cameron McCormack (:heycam)]

Curious that nsINode::Remove() calls SVGSwitchElement::RemoveChildAt(), when looking at the script, the

  test1.attributes[0].ownerElement.remove();

should be calling remove() on the <url> element.

Just before the remove() call, the document should look like:

  <thead><switch><url><script>...</script></url></switch></thead>

Once the <switch> gets GCed, so should the <url>.  I don't think the "test1" should resolve to the <url> element.  Instead it should throw a ReferenceError.  I wonder if this is an issue with the Global Scope Polluter?

Comment 3

[Cameron McCormack (:heycam)]

Actually the <thead> isn't even in the document, so it shouldn't resolve anyway...

Comment 4

[Cameron McCormack (:heycam)]

Actually, ignore that, and the curiosity in comment 2.

The first time "test1" is referenced -- in the appendChild() call -- causes the GSP property to spring into existence.  So I guess that holds a strong reference to the <url> element.

And of course remove() is equivalent to

  if (this.parentNode) this.parentNode.removeChild(this);

so it almost makes sense that it's calling RemoveChildAt() on the SVGSwitchElement.

So: even though the <switch> is removed from the document, it shouldn't have been GCed, since it should still be accessible through <url>.parentNode.

Comment 5

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

nsINode::Remove is wrong. It must keep the parent alive

Comment 6

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

Attachment: patch

Comment 7

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

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

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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