1397811 - heap-use-after-free in [@ nsINode::DeleteProperty]

Status: RESOLVED FIXED

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

Description

[Tyson Smith [:tsmith]]

I don't have a working test case at the moment but I will attach one when available.

==8484==ERROR: AddressSanitizer: heap-use-after-free on address 0x6110003d01e0 at pc 0x7f4d52a753dc bp 0x7fff7ff022d0 sp 0x7fff7ff022c8
READ of size 8 at 0x6110003d01e0 thread T0 (file:// Content)
    #0 0x7f4d52a753db in get /src/obj-firefox/dist/include/mozilla/RefPtr.h:287:27
    #1 0x7f4d52a753db in operator-> /src/obj-firefox/dist/include/mozilla/RefPtr.h:319
    #2 0x7f4d52a753db in OwnerDoc /src/dom/base/nsINode.h:530
    #3 0x7f4d52a753db in nsINode::DeleteProperty(unsigned short, nsIAtom*) /src/dom/base/nsINode.cpp:195
    #4 0x7f4d5728e513 in DeleteProperty /src/dom/base/nsINode.h:853:5
    #5 0x7f4d5728e513 in mozilla::LayerActivityTracker::NotifyExpired(mozilla::LayerActivity*) /src/layout/painting/ActiveLayerTracker.cpp:176
    #6 0x7f4d573c9775 in ExpirationTrackerImpl<mozilla::LayerActivity, 4u, detail::PlaceholderLock, detail::PlaceholderAutoLock>::AgeOneGenerationLocked(detail::PlaceholderAutoLock const&) /src/obj-firefox/dist/include/nsExpirationTracker.h:258:7
    #7 0x7f4d573c930c in ExpirationTrackerImpl<mozilla::LayerActivity, 4u, detail::PlaceholderLock, detail::PlaceholderAutoLock>::HandleTimeout() /src/obj-firefox/dist/include/nsExpirationTracker.h:426:7
    #8 0x7f4d4fe12296 in nsTimerImpl::Fire(int) /src/xpcom/threads/nsTimerImpl.cpp:514:7
    #9 0x7f4d4fe11a74 in nsTimerEvent::Run() /src/xpcom/threads/TimerThread.cpp:286:11
    #10 0x7f4d4fdf8190 in mozilla::SchedulerGroup::Runnable::Run() /src/xpcom/threads/SchedulerGroup.cpp:396:25
    #11 0x7f4d4fe21dad in nsThread::ProcessNextEvent(bool, bool*) /src/xpcom/threads/nsThread.cpp:1039:14
    #12 0x7f4d4fe273e8 in NS_ProcessNextEvent(nsIThread*, bool) /src/xpcom/threads/nsThreadUtils.cpp:521:10
    #13 0x7f4d50bc76c1 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /src/ipc/glue/MessagePump.cpp:97:21
    #14 0x7f4d50b279ab in RunInternal /src/ipc/chromium/src/base/message_loop.cc:326:10
    #15 0x7f4d50b279ab in RunHandler /src/ipc/chromium/src/base/message_loop.cc:319
    #16 0x7f4d50b279ab in MessageLoop::Run() /src/ipc/chromium/src/base/message_loop.cc:299
    #17 0x7f4d562c7adf in nsBaseAppShell::Run() /src/widget/nsBaseAppShell.cpp:158:27
    #18 0x7f4d5a5f2797 in XRE_RunAppShell() /src/toolkit/xre/nsEmbedFunctions.cpp:866:22
    #19 0x7f4d50b279ab in RunInternal /src/ipc/chromium/src/base/message_loop.cc:326:10
    #20 0x7f4d50b279ab in RunHandler /src/ipc/chromium/src/base/message_loop.cc:319
    #21 0x7f4d50b279ab in MessageLoop::Run() /src/ipc/chromium/src/base/message_loop.cc:299
    #22 0x7f4d5a5f2200 in XRE_InitChildProcess(int, char**, XREChildData const*) /src/toolkit/xre/nsEmbedFunctions.cpp:691:34
    #23 0x4eb873 in content_process_main /src/browser/app/../../ipc/contentproc/plugin-container.cpp:63:30
    #24 0x4eb873 in main /src/browser/app/nsBrowserApp.cpp:285
    #25 0x7f4d6d7db82f in __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:291
    #26 0x41d1c8 in _start (/home/ubuntu/firefox/firefox+0x41d1c8)

0x6110003d01e0 is located 32 bytes inside of 248-byte region [0x6110003d01c0,0x6110003d02b8)
freed by thread T0 (file:// Content) here:
    #0 0x4bb6fb in __interceptor_free /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:47:3
    #1 0x7f4d4fcba8d7 in SnowWhiteKiller::~SnowWhiteKiller() /src/xpcom/base/nsCycleCollector.cpp:2695:25
    #2 0x7f4d4fcc1f9b in FreeSnowWhite /src/xpcom/base/nsCycleCollector.cpp:2883:3
    #3 0x7f4d4fcc1f9b in nsCycleCollector::BeginCollection(ccType, nsICycleCollectorListener*) /src/xpcom/base/nsCycleCollector.cpp:3899
    #4 0x7f4d4fcc14b3 in nsCycleCollector::Collect(ccType, js::SliceBudget&, nsICycleCollectorListener*, bool) /src/xpcom/base/nsCycleCollector.cpp:3720:9
    #5 0x7f4d4fcc5300 in nsCycleCollector_collect(nsICycleCollectorListener*) /src/xpcom/base/nsCycleCollector.cpp:4289:21
    #6 0x7f4d52a96cad in nsJSContext::CycleCollectNow(nsICycleCollectorListener*) /src/dom/base/nsJSEnvironment.cpp:1482:3
    #7 0x7f4d525d766b in nsDOMWindowUtils::CycleCollect(nsICycleCollectorListener*) /src/dom/base/nsDOMWindowUtils.cpp:1434:3
    #8 0x7f4d4fe47631 in NS_InvokeByIndex /src/xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_x86_64_unix.S:129
    #9 0x7f4d515fe670 in Invoke /src/js/xpconnect/src/XPCWrappedNative.cpp:1996:12
    #10 0x7f4d515fe670 in Call /src/js/xpconnect/src/XPCWrappedNative.cpp:1315
    #11 0x7f4d515fe670 in XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) /src/js/xpconnect/src/XPCWrappedNative.cpp:1282
    #12 0x7f4d5160563f in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) /src/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:928:12
    #13 0x7f4d5aad9cf4 in CallJSNative /src/js/src/jscntxtinlines.h:293:15
    #14 0x7f4d5aad9cf4 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /src/js/src/vm/Interpreter.cpp:494
    #15 0x7f4d5aac3602 in CallFromStack /src/js/src/vm/Interpreter.cpp:545:12
    #16 0x7f4d5aac3602 in Interpret(JSContext*, js::RunState&) /src/js/src/vm/Interpreter.cpp:3093
    #17 0x7f4d5aaaaecb in js::RunScript(JSContext*, js::RunState&) /src/js/src/vm/Interpreter.cpp:434:12
    #18 0x7f4d5aad9e8c in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /src/js/src/vm/Interpreter.cpp:512:15
    #19 0x7f4d5aada7e2 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /src/js/src/vm/Interpreter.cpp:558:10
    #20 0x7f4d5b529253 in JS_CallFunctionValue(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /src/js/src/jsapi.cpp:2878:12
    #21 0x7f4d5151c8db in xpc::FunctionForwarder(JSContext*, unsigned int, JS::Value*) /src/js/xpconnect/src/ExportHelpers.cpp:315:18
    #22 0x7f4d5aad9cf4 in CallJSNative /src/js/src/jscntxtinlines.h:293:15
    #23 0x7f4d5aad9cf4 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /src/js/src/vm/Interpreter.cpp:494
    #24 0x7f4d5aac3602 in CallFromStack /src/js/src/vm/Interpreter.cpp:545:12
    #25 0x7f4d5aac3602 in Interpret(JSContext*, js::RunState&) /src/js/src/vm/Interpreter.cpp:3093
    #26 0x7f4d5aaaaecb in js::RunScript(JSContext*, js::RunState&) /src/js/src/vm/Interpreter.cpp:434:12
    #27 0x7f4d5aad9e8c in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /src/js/src/vm/Interpreter.cpp:512:15
    #28 0x7f4d5aada7e2 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /src/js/src/vm/Interpreter.cpp:558:10
    #29 0x7f4d5b52b0db in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /src/js/src/jsapi.cpp:2937:12
    #30 0x7f4d53ebfb37 in mozilla::dom::EventListener::HandleEvent(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, mozilla::ErrorResult&) /src/obj-firefox/dom/bindings/EventListenerBinding.cpp:47:8
    #31 0x7f4d54845f7f in HandleEvent<mozilla::dom::EventTarget *> /src/obj-firefox/dist/include/mozilla/dom/EventListenerBinding.h:65:12
    #32 0x7f4d54845f7f in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) /src/dom/events/EventListenerManager.cpp:1109
    #33 0x7f4d54848080 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) /src/dom/events/EventListenerManager.cpp:1283:20
    #34 0x7f4d54827e71 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) /src/dom/events/EventDispatcher.cpp:462:16
    #35 0x7f4d5482b342 in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) /src/dom/events/EventDispatcher.cpp:822:9
    #36 0x7f4d547f9a4a in mozilla::EventDispatcher::DispatchDOMEvent(nsISupports*, mozilla::WidgetEvent*, nsIDOMEvent*, nsPresContext*, nsEventStatus*) /src/dom/events/EventDispatcher.cpp:891:12
    #37 0x7f4d52a7d5a1 in nsINode::DispatchEvent(nsIDOMEvent*, bool*) /src/dom/base/nsINode.cpp:1341:5

previously allocated by thread T0 (file:// Content) here:
    #0 0x4bba4c in malloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:64:3
    #1 0x4ecf6d in moz_xmalloc /src/memory/mozalloc/mozalloc.cpp:84:17
    #2 0x7f4d54a4f743 in operator new /src/obj-firefox/dist/include/mozilla/mozalloc.h:206:12
    #3 0x7f4d54a4f743 in NS_NewHTMLCanvasElement(already_AddRefed<mozilla::dom::NodeInfo>&&, mozilla::dom::FromParser) /src/dom/html/HTMLCanvasElement.cpp:54
    #4 0x7f4d519b4eca in nsHtml5TreeOperation::CreateHTMLElement(nsIAtom*, nsHtml5HtmlAttributes*, mozilla::dom::FromParser, nsNodeInfoManager*, nsHtml5DocumentBuilder*, nsGenericHTMLElement* (*)(already_AddRefed<mozilla::dom::NodeInfo>&&, mozilla::dom::FromParser)) /src/parser/html/nsHtml5TreeOperation.cpp:353:39
    #5 0x7f4d519c37a7 in nsHtml5TreeOperation::Perform(nsHtml5TreeOpExecutor*, nsIContent**, bool*) /src/parser/html/nsHtml5TreeOperation.cpp:810:17
    #6 0x7f4d519c0152 in nsHtml5TreeOpExecutor::RunFlushLoop() /src/parser/html/nsHtml5TreeOpExecutor.cpp:466:27
    #7 0x7f4d519cb0df in nsHtml5ExecutorReflusher::Run() /src/parser/html/nsHtml5TreeOpExecutor.cpp:60:18
    #8 0x7f4d4fdf8190 in mozilla::SchedulerGroup::Runnable::Run() /src/xpcom/threads/SchedulerGroup.cpp:396:25
    #9 0x7f4d4fe21dad in nsThread::ProcessNextEvent(bool, bool*) /src/xpcom/threads/nsThread.cpp:1039:14
    #10 0x7f4d4fe273e8 in NS_ProcessNextEvent(nsIThread*, bool) /src/xpcom/threads/nsThreadUtils.cpp:521:10
    #11 0x7f4d50bc76c1 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /src/ipc/glue/MessagePump.cpp:97:21
    #12 0x7f4d50b279ab in RunInternal /src/ipc/chromium/src/base/message_loop.cc:326:10
    #13 0x7f4d50b279ab in RunHandler /src/ipc/chromium/src/base/message_loop.cc:319
    #14 0x7f4d50b279ab in MessageLoop::Run() /src/ipc/chromium/src/base/message_loop.cc:299
    #15 0x7f4d562c7adf in nsBaseAppShell::Run() /src/widget/nsBaseAppShell.cpp:158:27
    #16 0x7f4d5a5f2797 in XRE_RunAppShell() /src/toolkit/xre/nsEmbedFunctions.cpp:866:22
    #17 0x7f4d50b279ab in RunInternal /src/ipc/chromium/src/base/message_loop.cc:326:10
    #18 0x7f4d50b279ab in RunHandler /src/ipc/chromium/src/base/message_loop.cc:319
    #19 0x7f4d50b279ab in MessageLoop::Run() /src/ipc/chromium/src/base/message_loop.cc:299
    #20 0x7f4d5a5f2200 in XRE_InitChildProcess(int, char**, XREChildData const*) /src/toolkit/xre/nsEmbedFunctions.cpp:691:34
    #21 0x4eb873 in content_process_main /src/browser/app/../../ipc/contentproc/plugin-container.cpp:63:30
    #22 0x4eb873 in main /src/browser/app/nsBrowserApp.cpp:285
    #23 0x7f4d6d7db82f in __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:291

SUMMARY: AddressSanitizer: heap-use-after-free /src/obj-firefox/dist/include/mozilla/RefPtr.h:287:27 in get
Shadow bytes around the buggy address:
  0x0c2280071fe0: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
  0x0c2280071ff0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c2280072000: 00 00 00 00 fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2280072010: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2280072020: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x0c2280072030: fa fa fa fa fa fa fa fa fd fd fd fd[fd]fd fd fd
  0x0c2280072040: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2280072050: fd fd fd fd fd fd fd fa fa fa fa fa fa fa fa fa
  0x0c2280072060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c2280072070: 00 00 00 00 00 00 00 00 00 00 00 00 fa fa fa fa
  0x0c2280072080: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
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

Comment 1

[Tyson Smith [:tsmith]]

Found with m-c 20170906-d8e238b811d3

Comment 2

[Stephanie Ouillon [:arroway] (needinfo me)]

Olli, you seem to have been involved with the code in OwnerDoc, could you take a look?

Comment 3

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

This looks more like LayerActivityTracker issue. I assume 
http://searchfox.org/mozilla-central/rev/3dbb47302e114219c53e99ebaf50c5cb727358ab/layout/painting/ActiveLayerTracker.cpp#160 has pointer to deleted nsIContent object
http://searchfox.org/mozilla-central/rev/3dbb47302e114219c53e99ebaf50c5cb727358ab/layout/painting/ActiveLayerTracker.cpp#91
Raw pointer as a member variable bite us again.

Comment 4

[Markus Stange [:mstange]]

Interesting. mContent is only supposed to be non-null while LayerActivity is stored in a node property of mContent.

>  aContent->SetProperty(nsGkAtoms::LayerActivity, layerActivity,
>                        nsINode::DeleteProperty<LayerActivity>, true);

My assumption was that, when mContent gets destroyed, it will delete the LayerActivity object so that there never exists a LayerActivity object whose mContent points to a destroyed node.

Once we have a testcase, this should be easy to debug.

Comment 5

[Jesse Schwartzentruber (:truber)]

Attachment: testcase.html

This is still intermittent and not very reduced, but it is very fragile. This reproduces for me in m-c rev 20171010-a0488ecc201c.

Requires the domFuzzLite3 extension per below:
 1. clone https://github.com/MozillaSecurity/domfuzz/
 2. run "make" in the dom/extension directory
 3. set prefs to allow you to install that
extension:
       xpinstall.signatures.required = false
       extensions.legacy.enabled = true
 4. load the domFuzzLite3.xpi extension (browse via file:/// or drag-n-drop)

Comment 6

[Boris Zbarsky [:bzbarsky]]

I just tried reproducing on m-c rev a0488ecc201c with an opt Linux ASAN build and the fuzzpriv extension, but it's not reproducing for me.  :(

Comment 7

[Timothy Nikkel (:tnikkel)]

This reproduces easily on OS X for me.

It looks like we transfer a canvas element with a LayerActivity property back and forth between two documents, because aTransfer was true when we set the property we also transfer the property. Let's say the final transfer is to document B. Following that I see nsIDocument::DeleteAllPropertiesFor called on the canvas but in document A. So it seems the property failed to make the journey with the canvas to document A? Or somehow otherwise they got out of sync.

Someone in content want to take a look?

Comment 8

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

Ahaa, this sounds familiar after all. Something wrong with adopt code. Let me see if I can reproduce.
I assume there is JS recursion error happening somewhere during adopt... building a debug build.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Hmm.  So we're supposed to handle the "adoption failed" case.  Adoption collects up the set of nodes with properties, then if adoption failed in any way deletes all properties for those nodes, in the _old_ document (which is where they live so far).  If adoption succeeds, then we transfer-or-adopt all the properties for each node in the list.

Timothy, if you can reproduce, do you end up in any of the error cases in nsIDocument::AdoptNode?  Specifically, do you end up in the block at http://searchfox.org/mozilla-central/rev/dca019c94bf3a840ed7ff50261483410cfece24f/dom/base/nsDocument.cpp#8123-8132 ?

Comment 10

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

Reproduces quite reliably on debug linux

Comment 11

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

FWIW, there is warning WARNING: 'aError.Failed()', file /home/smaug/mozilla/hg/vsync/dom/base/nsNodeUtils.cpp, line 640
so adopting clearly fails.

Comment 12

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

And that fails because reparenting of some node fails.

Comment 13

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

(but I don't still understand nsGkAtoms::LayerActivity property lifetime management)

Comment 14

[Timothy Nikkel (:tnikkel)]

Thanks for the pointers.

We fail at

http://searchfox.org/mozilla-central/source/dom/base/nsNodeUtils.cpp#614

reparenting the wrapper because we hit the js recursion limit. And so we exit early and never hit the code at the end of CloneAndAdopt that puts us in aNodesWithProperties. So the code in nsIDocument::AdoptNode designed to handle a failure in nsNodeUtils::AdoptNode runs but it has no nodes to delete properties for. I wrote a hacky patch to make sure the node gets put into aNodesWithProperties and that fixes the testcase for me.

We can't just simply move the aNodesWithProperties code to the start of CloneAndAdopt because we need access to the clone we might create later. Someone more familiar with this code want to take this over?

Comment 15

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

yeah, I can take this, since this reproduces on my machine.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> And so we exit early and never hit the code at the end of CloneAndAdopt that puts us in aNodesWithProperties.

OK, but also we have not moved the node to the new document, right?  That's what http://searchfox.org/mozilla-central/rev/dca019c94bf3a840ed7ff50261483410cfece24f/dom/base/nsNodeUtils.cpp#618 is about.

So we don't delete the properties for the node, but that should be OK: it's still in the document it started in, its properties are in the same document, things should be good.

So what actually goes wrong after that?

Comment 17

[Boris Zbarsky [:bzbarsky]]

That is, the invariant as far as I can tell is that after nsNodeUtils::CloneAndAdopt the set of nodes in aNodesWithProperties are the nodes that had properties and were adopted into the new document.  So not placing a node in there if ReparentWrapper fails seems correct.

Comment 18

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

Isn't the issue that we do move some nodes, before recursion limit, to the new document, yet we don't move the properties of those ones to the new document, since nsNodeUtils::Adopt fails.
And we don't put those moved nodes to the aNodesWithProperties, since CloneAndAdopt failed on their child nodes http://searchfox.org/mozilla-central/rev/dca019c94bf3a840ed7ff50261483410cfece24f/dom/base/nsNodeUtils.cpp#637,641,691 and aNodesWithProperties->AppendObject(aNode); happens later.
So we have some elements in new document, their properties still in the old one, yet aNodesWithProperties doesn't contain those nodes.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> And we don't put those moved nodes to the aNodesWithProperties, since CloneAndAdopt failed on their child nodes

Aha!  _That_ sounds like an all too plausible explanation, there, and the bit I was missing.  We should be adding to aNodesWithProperties once we have committed to adopting the node into the new document, so right after the aReparentScope block.

Comment 20

[Timothy Nikkel (:tnikkel)]

(In reply to Olli Pettay [:smaug] (work week Oct 16-20) from comment #18)
> Isn't the issue that we do move some nodes, before recursion limit, to the
> new document, yet we don't move the properties of those ones to the new
> document, since nsNodeUtils::Adopt fails.
> And we don't put those moved nodes to the aNodesWithProperties, since
> CloneAndAdopt failed on their child nodes
> http://searchfox.org/mozilla-central/rev/
> dca019c94bf3a840ed7ff50261483410cfece24f/dom/base/nsNodeUtils.cpp#637,641,
> 691 and aNodesWithProperties->AppendObject(aNode); happens later.
> So we have some elements in new document, their properties still in the old
> one, yet aNodesWithProperties doesn't contain those nodes.

Yes, we fail one of the recursive CloneAndAdopt calls on a child. Sorry, this code is not my usual hang out.

Comment 21

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

Attachment: adopt_crash.diff

And yes, exactly, we need to ensure the adopted nodes are in the list.
I added MOZ_RELEASE_ASSERT to make is clear we want to crash there, though looks like even if nsCOMArray API looks like a fallible one, it is internally infallible.

Comment 22

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

And since you want reasonable commit message, I'm trying to find a vague enough such for this security bug. Maybe

-m "Bug 1397811, in order to not leak properties, ensure all the properties of adopted nodes are removed in case the adopt call fails, r=bz"

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8920078 [details] [diff] [review]
adopt_crash.diff

r=me, with the proposed commit message.

Comment 24

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

Comment on attachment 8920078 [details] [diff] [review]
adopt_crash.diff

[Approval Request Comment]
[Security approval request comment]
How easily could an exploit be constructed based on the patch?
I'd say not too easily. One needs to run JS close to the recursion limit and then call something which causes node adoption to happen.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
-m "Bug 1397811, in order to not leak properties, ensure all the properties of adopted nodes are removed in case the adopt call fails, r=bz"
is trying to hide some of the badness here, and focus on the possible temporary leaks we also get because of this issue

Which older supported branches are affected by this flaw?
All. This is old code.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
esr52 needs a new patch, since the old code uses nsresult, and new one ErrorResult, but the change is trivial

How likely is this patch to cause regressions; how much testing does it need?
Highly unlikely to cause regression. This is fixing an issue which shows up only when we're close to the js recursion limit.

Comment 25

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

Sec-approval+ for trunk. 
I'm not sure why there is no beta nomination as it isn't going to go into ESR52 if we don't take it on Beta (57) during this cycle. I'm on the fence about taking it at this late hour close to 57 release though. I'll ni? release management folks for an opinion.

Comment 26

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

oops, I forgot beta nomination.

Comment 27

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

The same patch applies to beta. I'll upload esr52 patch.

Comment 28

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

Attachment: adopt_crash_esr52.diff

For 52.
-m "Bug 1397811, in order to not leak properties, ensure all the properties of adopted nodes are removed in case the adopt call fails, r=bz"

Comment 29

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

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

Comment 30

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8920078 [details] [diff] [review]
adopt_crash.diff

Fix for sec-high issue, let's take this for beta 11 next week & for ESR.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f609508b197d
https://hg.mozilla.org/releases/mozilla-esr52/rev/d6f008f95598

Comment 32

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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