Last Comment Bug 778428 - (CVE-2012-1972) Heap-use-after-free in nsHTMLEditor::CollapseAdjacentTextNodes
(CVE-2012-1972)
: Heap-use-after-free in nsHTMLEditor::CollapseAdjacentTextNodes
Status: VERIFIED FIXED
[asan][advisory-tracking+][qa-]
: crash, reproducible, sec-critical, testcase
Product: Core
Classification: Components
Component: Editor (show other bugs)
: unspecified
: All All
: -- critical (vote)
: mozilla17
Assigned To: Aryeh Gregor (:ayg) (working until September 2)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-28 08:07 PDT by Abhishek Arya
Modified: 2014-07-24 13:43 PDT (History)
10 users (show)
rforbes: sec‑bounty+
ayg: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
fixed
+
verified
+
verified
15+
fixed


Attachments
Testcase (1.07 KB, text/html)
2012-07-28 08:07 PDT, Abhishek Arya
no flags Details
stack (17.96 KB, text/plain)
2012-07-28 09:51 PDT, Mats Palmgren (:mats)
no flags Details
Patch, nsTArray -> nsTArray<nsCOMPtr> (3.69 KB, patch)
2012-07-31 03:09 PDT, Aryeh Gregor (:ayg) (working until September 2)
ehsan: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑esr10+
Details | Diff | Splinter Review

Description Abhishek Arya 2012-07-28 08:07:53 PDT
Created attachment 646852 [details]
Testcase

Reproduces on trunk
20120728051930
http://hg.mozilla.org/mozilla-central/rev/08428deb1e89

=================================================================
==12811== ERROR: AddressSanitizer heap-use-after-free on address 0x7f81c5127cf0 at pc 0x7f81e8acb7f9 bp 0x7fff2669f0d0 sp 0x7fff2669f0c8
READ of size 8 at 0x7f81c5127cf0 thread T0
    #0 0x7f81e8acb7f9 in nsHTMLEditor::CollapseAdjacentTextNodes(nsIDOMRange*) src/editor/libeditor/html/nsHTMLEditor.cpp:3810
    #1 0x7f81e8b5aa3e in nsHTMLEditRules::AfterEditInner(nsEditor::OperationID, short) src/editor/libeditor/html/nsHTMLEditRules.cpp:451
    #2 0x7f81e8b587f1 in nsHTMLEditRules::AfterEdit(nsEditor::OperationID, short) src/editor/libeditor/html/nsHTMLEditRules.cpp:376
    #3 0x7f81e8ac34c5 in nsHTMLEditor::EndOperation() src/editor/libeditor/html/nsHTMLEditor.cpp:3505
    #4 0x7f81e84e6554 in ~nsAutoRules src/editor/libeditor/base/nsEditorUtils.h:95
    #5 0x7f81e84c4373 in ~nsAutoRules src/editor/libeditor/base/nsEditorUtils.h:97
    #6 0x7f81e8a99cb7 in nsHTMLEditor::Indent(nsAString_internal const&) src/editor/libeditor/html/nsHTMLEditor.cpp:2229
    #7 0x7f81e8a9b01f in non-virtual thunk to nsHTMLEditor::Indent(nsAString_internal const&) asn1cmn.c:0
    #8 0x7f81ec071575 in nsOutdentCommand::DoCommand(char const*, nsISupports*) src/editor/composer/src/nsComposerCommands.cpp:519
    #9 0x7f81eb8fbf9d in nsControllerCommandTable::DoCommand(char const*, nsISupports*) src/embedding/components/commandhandler/src/nsControllerCommandTable.cpp:158
    #10 0x7f81eb8ce894 in nsBaseCommandController::DoCommand(char const*) src/embedding/components/commandhandler/src/nsBaseCommandController.cpp:137
    #11 0x7f81eb8e6ce1 in nsCommandManager::DoCommand(char const*, nsICommandParams*, nsIDOMWindow*) src/embedding/components/commandhandler/src/nsCommandManager.cpp:238
    #12 0x7f81e6e54cea in nsHTMLDocument::ExecCommand(nsAString_internal const&, bool, nsAString_internal const&, bool*) src/content/html/document/src/nsHTMLDocument.cpp:3201
    #13 0x7f81e6e5765e in non-virtual thunk to nsHTMLDocument::ExecCommand(nsAString_internal const&, bool, nsAString_internal const&, bool*) asn1cmn.c:0
    #14 0x7f81eee55fcb in NS_InvokeByIndex_P src/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:162
    #15 0x7f81ea44923b in CallMethodHelper::Call() src/js/xpconnect/src/XPCWrappedNative.cpp:2416
    #16 0x7f81ea4b05c4 in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) src/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1474
    #17 0x7f81f5ae9677 in js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) src/js/src/jscntxtinlines.h:387
    #18 0x7f81f5a5dbe8 in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) src/js/src/jsinterp.cpp:2405
    #19 0x7f81f59def35 in js::RunScript(JSContext*, JSScript*, js::StackFrame*) src/js/src/jsinterp.cpp:302
    #20 0x7f81f5ae9a99 in js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) src/js/src/jsinterp.cpp:356
    #21 0x7f81f540ff50 in js::Invoke(JSContext*, js::InvokeArgsGuard&, js::MaybeConstruct) src/js/src/jsinterp.h:119
    #22 0x7f81f5aeebfd in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::Value*) src/js/src/jsinterp.cpp:388
    #23 0x7f81f52b5bb9 in JS_CallFunctionValue src/js/src/jsapi.cpp:5698
    #24 0x7f81ea3f860e in nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) src/js/xpconnect/src/XPCWrappedJSClass.cpp:1436
    #25 0x7f81ea39e508 in nsXPCWrappedJS::CallMethod(unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) src/js/xpconnect/src/XPCWrappedJS.cpp:580
    #26 0x7f81eee5b7d0 in PrepareAndDispatch src/xpcom/reflect/xptcall/src/md/unix/xptcstubs_x86_64_linux.cpp:121
    #27 0x7f81eee58f67 in SharedStub src/xpcom/reflect/xptcall/src/md/unix/xptcstubs_x86_64_linux.cpp:0
    #28 0x7f81e6256563 in nsEventListenerManager::HandleEventSubType(nsListenerStruct*, nsIDOMEventListener*, nsIDOMEvent*, nsIDOMEventTarget*, unsigned int, nsCxPusher*) src/content/events/src/nsEventListenerManager.cpp:794
    #29 0x7f81e6257a2a in nsEventListenerManager::HandleEventInternal(nsPresContext*, nsEvent*, nsIDOMEvent**, nsIDOMEventTarget*, unsigned int, nsEventStatus*, nsCxPusher*) src/content/events/src/nsEventListenerManager.cpp:867
    #30 0x7f81e63e6f77 in nsEventListenerManager::HandleEvent(nsPresContext*, nsEvent*, nsIDOMEvent**, nsIDOMEventTarget*, unsigned int, nsEventStatus*, nsCxPusher*) src/content/events/src/nsEventListenerManager.h:144
    #31 0x7f81e63d5c16 in nsEventTargetChainItem::HandleEvent(nsEventChainPostVisitor&, unsigned int, bool, nsCxPusher*) src/content/events/src/nsEventDispatcher.cpp:184
    #32 0x7f81e63d377c in nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor&, unsigned int, nsDispatchingCallback*, bool, nsCxPusher*) src/content/events/src/nsEventDispatcher.cpp:312
    #33 0x7f81e63d9290 in nsEventDispatcher::Dispatch(nsISupports*, nsPresContext*, nsEvent*, nsIDOMEvent*, nsEventStatus*, nsDispatchingCallback*, nsCOMArray<nsIDOMEventTarget>*) src/content/events/src/nsEventDispatcher.cpp:633
    #34 0x7f81e63dd824 in nsEventDispatcher::DispatchDOMEvent(nsISupports*, nsEvent*, nsIDOMEvent*, nsPresContext*, nsEventStatus*) src/content/events/src/nsEventDispatcher.cpp:696
    #35 0x7f81e57a21b3 in nsINode::DispatchEvent(nsIDOMEvent*, bool*) src/content/base/src/nsINode.cpp:1079
    #36 0x7f81e52e4972 in nsContentUtils::DispatchEvent(nsIDocument*, nsISupports*, nsAString_internal const&, bool, bool, bool, bool*) src/content/base/src/nsContentUtils.cpp:3428
    #37 0x7f81e52e3e06 in nsContentUtils::DispatchTrustedEvent(nsIDocument*, nsISupports*, nsAString_internal const&, bool, bool, bool*) src/content/base/src/nsContentUtils.cpp:3398
    #38 0x7f81e54eca26 in nsDocument::DispatchContentLoadedEvents() src/content/base/src/nsDocument.cpp:4131
    #39 0x7f81e55ad459 in nsRunnableMethodImpl<void (nsDocument::*)(), true>::Run() src/../../../dist/include/nsThreadUtils.h:349
    #40 0x7f81eed56e0d in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:625
    #41 0x7f81ee9e595d in NS_ProcessNextEvent_P(nsIThread*, bool) src/objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:220
    #42 0x7f81ed97f886 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:82
    #43 0x7f81ef00a76a in MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:209
    #44 0x7f81ef00a5b3 in MessageLoop::RunHandler() src/ipc/chromium/src/base/message_loop.cc:202
    #45 0x7f81ef00a498 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:176
    #46 0x7f81ece8568e in nsBaseAppShell::Run() src/widget/xpwidgets/nsBaseAppShell.cpp:165
    #47 0x7f81ebad3448 in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:271
    #48 0x7f81e2210820 in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:3798
    #49 0x7f81e22171c2 in XREMain::XRE_main(int, char**, nsXREAppData const*) src/toolkit/xre/nsAppRunner.cpp:3875
    #50 0x7f81e221a692 in XRE_main src/toolkit/xre/nsAppRunner.cpp:3951
    #51 0x40c28f in do_main(int, char**) src/browser/app/nsBrowserApp.cpp:174
    #52 0x409cbd in main src/browser/app/nsBrowserApp.cpp:312
    #53 0x7f81ff037c4d in ?? ??:0
0x7f81c5127cf0 is located 112 bytes inside of 120-byte region [0x7f81c5127c80,0x7f81c5127cf8)
freed by thread T0 here:
    #0 0x4a4392 in free ??:0
    #1 0x7f81fbec35c3 in moz_free src/memory/mozalloc/mozalloc.cpp:49
    #2 0x7f81e5971366 in ~nsTextNode src/content/base/src/nsTextNode.cpp:121
    #3 0x7f81e584274d in nsNodeUtils::LastRelease(nsINode*) src/content/base/src/nsNodeUtils.cpp:252
    #4 0x7f81e56a878f in nsGenericDOMDataNode::Release() src/content/base/src/nsGenericDOMDataNode.cpp:113
    #5 0x7f81e5971794 in nsTextNode::Release() src/content/base/src/nsTextNode.cpp:124
    #6 0x7f81e21da640 in ~nsCOMPtr_base src/../../dist/include/nsCOMPtr.h:408
    #7 0x7f81e37264b6 in nsCOMPtr<nsIContent>::~nsCOMPtr() src/../../dist/include/nsCOMPtr.h:447
    #8 0x7f81e3726183 in nsCOMPtr<nsIContent>::~nsCOMPtr() src/../../dist/include/nsCOMPtr.h:447
    #9 0x7f81e5705272 in nsGenericElement::RemoveChildAt(unsigned int, bool) src/content/base/src/nsGenericElement.cpp:2629
    #10 0x7f81e652dd23 in nsGenericHTMLElement::SetInnerHTML(nsAString_internal const&) src/content/html/content/src/nsGenericHTMLElement.cpp:1338
    #11 0x7f81e666de38 in nsHTMLBodyElement::SetInnerHTML(nsAString_internal const&) src/content/html/content/src/nsHTMLBodyElement.cpp:71
    #12 0x7f81ead19acd in nsIDOMHTMLElement_SetInnerHTML(JSContext*, JS::Handle<JSObject*>, JS::Handle<long>, int, JS::Value*) src/objdir-ff-asan-sym/js/xpconnect/src/dom_quickstubs.cpp:14166
    #13 0x7f81f5d61d60 in js::CallJSPropertyOpSetter(JSContext*, int (*)(JSContext*, JS::Handle<JSObject*>, JS::Handle<long>, int, JS::Value*), JS::Handle<JSObject*>, JS::Handle<long>, int, JS::Value*) src/js/src/jscntxtinlines.h:463
    #14 0x7f81f5d93bc7 in js::baseops::SetPropertyHelper(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<long>, unsigned int, JS::Value*, int) src/js/src/jsobj.cpp:4906
    #15 0x7f81f5b30601 in js::SetPropertyOperation(JSContext*, unsigned char*, JS::Value const&, JS::Value const&) src/js/src/jsinterpinlines.h:350
    #16 0x7f81f5a5266f in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) src/js/src/jsinterp.cpp:2319
    #17 0x7f81f59def35 in js::RunScript(JSContext*, JSScript*, js::StackFrame*) src/js/src/jsinterp.cpp:302
    #18 0x7f81f5ae9a99 in js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) src/js/src/jsinterp.cpp:356
    #19 0x7f81f540ff50 in js::Invoke(JSContext*, js::InvokeArgsGuard&, js::MaybeConstruct) src/js/src/jsinterp.h:119
    #20 0x7f81f5aeebfd in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::Value*) src/js/src/jsinterp.cpp:388
    #21 0x7f81f52b5bb9 in JS_CallFunctionValue src/js/src/jsapi.cpp:5698
    #22 0x7f81ea3f860e in nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) src/js/xpconnect/src/XPCWrappedJSClass.cpp:1436
    #23 0x7f81ea39e508 in nsXPCWrappedJS::CallMethod(unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) src/js/xpconnect/src/XPCWrappedJS.cpp:580
    #24 0x7f81eee5b7d0 in PrepareAndDispatch src/xpcom/reflect/xptcall/src/md/unix/xptcstubs_x86_64_linux.cpp:121
    #25 0x7f81eee58f67 in SharedStub src/xpcom/reflect/xptcall/src/md/unix/xptcstubs_x86_64_linux.cpp:0
    #26 0x7f81e6256563 in nsEventListenerManager::HandleEventSubType(nsListenerStruct*, nsIDOMEventListener*, nsIDOMEvent*, nsIDOMEventTarget*, unsigned int, nsCxPusher*) src/content/events/src/nsEventListenerManager.cpp:794
    #27 0x7f81e6257a2a in nsEventListenerManager::HandleEventInternal(nsPresContext*, nsEvent*, nsIDOMEvent**, nsIDOMEventTarget*, unsigned int, nsEventStatus*, nsCxPusher*) src/content/events/src/nsEventListenerManager.cpp:867
    #28 0x7f81e63e6f77 in nsEventListenerManager::HandleEvent(nsPresContext*, nsEvent*, nsIDOMEvent**, nsIDOMEventTarget*, unsigned int, nsEventStatus*, nsCxPusher*) src/content/events/src/nsEventListenerManager.h:144
    #29 0x7f81e63d5c16 in nsEventTargetChainItem::HandleEvent(nsEventChainPostVisitor&, unsigned int, bool, nsCxPusher*) src/content/events/src/nsEventDispatcher.cpp:184
previously allocated by thread T0 here:
    #0 0x4a4452 in __interceptor_malloc ??:0
    #1 0x7f81fbec3717 in moz_xmalloc src/memory/mozalloc/mozalloc.cpp:54
    #2 0x7f81e5972e75 in nsTextNode::CloneDataNode(nsINodeInfo*, bool) const src/content/base/src/nsTextNode.cpp:146
    #3 0x7f81e51f0364 in nsGenericDOMDataNode::Clone(nsINodeInfo*, nsINode**) const src/content/xml/content/src/../../../base/src/nsGenericDOMDataNode.h:216
    #4 0x7f81e5847d53 in nsNodeUtils::CloneAndAdopt(nsINode*, bool, bool, nsNodeInfoManager*, JSContext*, JSObject*, nsCOMArray<nsINode>&, nsINode*, nsINode**) src/content/base/src/nsNodeUtils.cpp:438
    #5 0x7f81e55e92fa in nsNodeUtils::CloneAndAdopt(nsINode*, bool, bool, nsNodeInfoManager*, JSContext*, JSObject*, nsCOMArray<nsINode>&, nsIDOMNode**) src/../../../../dist/include/nsNodeUtils.h:272
    #6 0x7f81e5500755 in nsNodeUtils::Clone(nsINode*, bool, nsNodeInfoManager*, nsCOMArray<nsINode>&, nsIDOMNode**) src/../../../../dist/include/nsNodeUtils.h:145
    #7 0x7f81e5845bf6 in nsNodeUtils::CloneNodeImpl(nsINode*, bool, bool, nsIDOMNode**) src/content/base/src/nsNodeUtils.cpp:360
    #8 0x7f81e51fba6f in nsGenericDOMDataNode::CloneNode(bool, unsigned char, nsIDOMNode**) src/content/xml/content/src/../../../base/src/nsGenericDOMDataNode.h:119
    #9 0x7f81e5979fff in nsTextNode::CloneNode(bool, unsigned char, nsIDOMNode**) src/content/base/src/nsTextNode.h:27
    #10 0x7f81e597df6f in non-virtual thunk to nsTextNode::CloneNode(bool, unsigned char, nsIDOMNode**) asn1cmn.c:0
    #11 0x7f81e58e6539 in nsRange::CloneContents(nsIDOMDocumentFragment**) src/content/base/src/nsRange.cpp:1974
    #12 0x7f81ea6eee9b in nsIDOMRange_CloneContents(JSContext*, unsigned int, JS::Value*) src/objdir-ff-asan-sym/js/xpconnect/src/dom_quickstubs.cpp:18372
    #13 0x7f81f5ae9677 in js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) src/js/src/jscntxtinlines.h:387
    #14 0x7f81f5a5dbe8 in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) src/js/src/jsinterp.cpp:2405
    #15 0x7f81f59def35 in js::RunScript(JSContext*, JSScript*, js::StackFrame*) src/js/src/jsinterp.cpp:302
    #16 0x7f81f5ae9a99 in js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) src/js/src/jsinterp.cpp:356
    #17 0x7f81f540ff50 in js::Invoke(JSContext*, js::InvokeArgsGuard&, js::MaybeConstruct) src/js/src/jsinterp.h:119
    #18 0x7f81f5aeebfd in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::Value*) src/js/src/jsinterp.cpp:388
    #19 0x7f81f52b5bb9 in JS_CallFunctionValue src/js/src/jsapi.cpp:5698
    #20 0x7f81ea3f860e in nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) src/js/xpconnect/src/XPCWrappedJSClass.cpp:1436
    #21 0x7f81ea39e508 in nsXPCWrappedJS::CallMethod(unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) src/js/xpconnect/src/XPCWrappedJS.cpp:580
    #22 0x7f81eee5b7d0 in PrepareAndDispatch src/xpcom/reflect/xptcall/src/md/unix/xptcstubs_x86_64_linux.cpp:121
    #23 0x7f81eee58f67 in SharedStub src/xpcom/reflect/xptcall/src/md/unix/xptcstubs_x86_64_linux.cpp:0
    #24 0x7f81e6256563 in nsEventListenerManager::HandleEventSubType(nsListenerStruct*, nsIDOMEventListener*, nsIDOMEvent*, nsIDOMEventTarget*, unsigned int, nsCxPusher*) src/content/events/src/nsEventListenerManager.cpp:794
==12811== ABORTING
Stats: 220M malloced (239M for red zones) by 552524 calls
Stats: 82M realloced by 23243 calls
Stats: 178M freed by 294094 calls
Stats: 47M really freed by 74448 calls
Stats: 448M (114755 full pages) mmaped in 112 calls
  mmaps   by size class: 8:425958; 9:57337; 10:16380; 11:16376; 12:3072; 13:2048; 14:1536; 15:384; 16:576; 17:128; 18:224; 19:112; 20:40;
  mallocs by size class: 8:454989; 9:54790; 10:18129; 11:16960; 12:2544; 13:2096; 14:1545; 15:345; 16:590; 17:145; 18:241; 19:112; 20:38;
  frees   by size class: 8:215604; 9:44215; 10:14411; 11:13853; 12:1685; 13:1785; 14:1296; 15:278; 16:518; 17:116; 18:197; 19:102; 20:34;
  rfrees  by size class: 8:64335; 9:3355; 10:2380; 11:3507; 12:193; 13:231; 14:145; 15:39; 16:164; 17:27; 18:21; 19:29; 20:22;
Stats: malloc large: 536 small slow: 2323
Shadow byte and word:
  0x1ff038a24f9e: fd
  0x1ff038a24f98: fd fd fd fd fd fd fd fd
More shadow bytes:
  0x1ff038a24f78: fd fd fd fd fd fd fd fd
  0x1ff038a24f80: fa fa fa fa fa fa fa fa
  0x1ff038a24f88: fa fa fa fa fa fa fa fa
  0x1ff038a24f90: fd fd fd fd fd fd fd fd
=>0x1ff038a24f98: fd fd fd fd fd fd fd fd
  0x1ff038a24fa0: fa fa fa fa fa fa fa fa
  0x1ff038a24fa8: fa fa fa fa fa fa fa fa
  0x1ff038a24fb0: fd fd fd fd fd fd fd fd
  0x1ff038a24fb8: fd fd fd fd fd fd fd fd
Comment 1 Mats Palmgren (:mats) 2012-07-28 09:51:33 PDT
Created attachment 646871 [details]
stack

CollapseAdjacentTextNodes stores non-refcounted nsIDOMNode* in
the local 'textNodes' array.  This is the stack when one of
those nodes are deleted.

Adding a nsAutoScriptBlocker in CollapseAdjacentTextNodes
fixes the crash, but I'm not sure if that's sufficient in
general.  We should also analyze the code in the enclosing
methods, nsHTMLEditRules::AfterEditInner etc, and make
sure they can deal with script running.

We crash at:

  result =
    rightTextNode->GetPreviousSibling(getter_AddRefs(prevSibOfRightNode));

with 'rightTextNode' deleted.  GetPreviousSibling is virtual
so this seems exploitable.
Comment 2 Mats Palmgren (:mats) 2012-07-28 15:32:03 PDT
Same crash occurs on esr10 branch with
  s/execCommand("Outdent")/execCommand("Outdent",false,null)/
so it probably affects all branches.
Comment 3 :Ehsan Akhgari 2012-07-30 10:12:33 PDT
$ git grep 'nsTArray<nsI' editor/
editor/libeditor/html/nsHTMLCSSUtils.cpp:840:nsHTMLCSSUtils::BuildCSSDeclarations(nsTArray<nsIAtom*> & aPropertyArray,
editor/libeditor/html/nsHTMLCSSUtils.cpp:887:                                                     nsTArray<nsIAtom*>& cssPropertyArray,
editor/libeditor/html/nsHTMLCSSUtils.cpp:1002:  nsTArray<nsIAtom*> cssPropertyArray;
editor/libeditor/html/nsHTMLCSSUtils.cpp:1036:  nsTArray<nsIAtom*> cssPropertyArray;
editor/libeditor/html/nsHTMLCSSUtils.cpp:1076:  nsTArray<nsIAtom*> cssPropertyArray;
editor/libeditor/html/nsHTMLCSSUtils.h:337:  void      BuildCSSDeclarations(nsTArray<nsIAtom*> & aPropertyArray,
editor/libeditor/html/nsHTMLCSSUtils.h:360:                                                 nsTArray<nsIAtom*>& aPropertyArray,
editor/libeditor/html/nsHTMLEditRules.cpp:7652:  nsTArray<nsINode*> skipList;
editor/libeditor/html/nsHTMLEditor.cpp:3772:  nsTArray<nsIDOMNode*> textNodes;
editor/libeditor/html/nsTableEditor.cpp:1309:  nsTArray<nsIDOMElement*> spanCellList;
editor/libeditor/html/nsTableEditor.cpp:2147:    nsTArray<nsIDOMElement*> deleteList;

The proper fix here is to store comptr's in the arrays for the last four cases.
Comment 4 :Ehsan Akhgari 2012-07-30 10:15:32 PDT
*Also*, we should add a script blocker in nsHTMLEditor::EndOperation I think (and verify that it doesn't break anything.)  Nothing that gets called from that function expects scripts to run. :(
Comment 5 Aryeh Gregor (:ayg) (working until September 2) 2012-07-31 03:09:47 PDT
Created attachment 647480 [details] [diff] [review]
Patch, nsTArray -> nsTArray<nsCOMPtr>

(In reply to Ehsan Akhgari [:ehsan] from comment #3)
> The proper fix here is to store comptr's in the arrays for the last four
> cases.

This patch does that, but I can't reproduce the crash on m-c even without this patch.  Does the crash still happen on tip?

(In reply to Ehsan Akhgari [:ehsan] from comment #4)
> *Also*, we should add a script blocker in nsHTMLEditor::EndOperation I think
> (and verify that it doesn't break anything.)  Nothing that gets called from
> that function expects scripts to run. :(

Should that be in this bug?

Try: https://tbpl.mozilla.org/?tree=Try&rev=6b59d9707fc9
Comment 6 :Ehsan Akhgari 2012-08-07 13:57:21 PDT
(In reply to :Aryeh Gregor from comment #5)
> Created attachment 647480 [details] [diff] [review]
> Patch, nsTArray -> nsTArray<nsCOMPtr>
> 
> (In reply to Ehsan Akhgari [:ehsan] from comment #3)
> > The proper fix here is to store comptr's in the arrays for the last four
> > cases.
> 
> This patch does that, but I can't reproduce the crash on m-c even without
> this patch.  Does the crash still happen on tip?

That worries me.  Jesse, do you test this stuff in your fuzzer?

> (In reply to Ehsan Akhgari [:ehsan] from comment #4)
> > *Also*, we should add a script blocker in nsHTMLEditor::EndOperation I think
> > (and verify that it doesn't break anything.)  Nothing that gets called from
> > that function expects scripts to run. :(
> 
> Should that be in this bug?

Yes.
Comment 7 Aryeh Gregor (:ayg) (working until September 2) 2012-08-08 02:28:34 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/4683a522952d
Comment 8 Aryeh Gregor (:ayg) (working until September 2) 2012-08-08 03:53:37 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #4)
> *Also*, we should add a script blocker in nsHTMLEditor::EndOperation I think
> (and verify that it doesn't break anything.)  Nothing that gets called from
> that function expects scripts to run. :(

This does break stuff.  editor/libeditor/html/crashtests/682650-1.html asserts because it tries to fire mutation events but it's not safe.  nsHTMLEditor::EndOperation() calls nsHTMLEditRules::AfterEdit() which calls nsHTMLEditRules::AfterEditInner() which calls nsHTMLEditor::CollapseAdjacentTextNodes(), which of course wants to do various DOM mutations.  Tell me if you want me to pursue this, and if so how.
Comment 9 Ed Morley [:emorley] 2012-08-08 10:35:42 PDT
https://hg.mozilla.org/mozilla-central/rev/4683a522952d
Comment 10 :Ehsan Akhgari 2012-08-08 11:04:59 PDT
(In reply to :Aryeh Gregor from comment #8)
> (In reply to Ehsan Akhgari [:ehsan] from comment #4)
> > *Also*, we should add a script blocker in nsHTMLEditor::EndOperation I think
> > (and verify that it doesn't break anything.)  Nothing that gets called from
> > that function expects scripts to run. :(
> 
> This does break stuff.  editor/libeditor/html/crashtests/682650-1.html
> asserts because it tries to fire mutation events but it's not safe. 
> nsHTMLEditor::EndOperation() calls nsHTMLEditRules::AfterEdit() which calls
> nsHTMLEditRules::AfterEditInner() which calls
> nsHTMLEditor::CollapseAdjacentTextNodes(), which of course wants to do
> various DOM mutations.  Tell me if you want me to pursue this, and if so how.

Sigh... No, that's probably not worth it then.  Let's leave this bug as is for now.
Comment 11 Al Billings [:abillings] 2012-08-08 16:35:28 PDT
This is sec-critical. Have we looked at uplifting this to Aurora and Beta?
Comment 12 :Ehsan Akhgari 2012-08-09 12:44:45 PDT
Comment on attachment 647480 [details] [diff] [review]
Patch, nsTArray -> nsTArray<nsCOMPtr>

(In reply to Al Billings [:abillings] from comment #11)
> This is sec-critical. Have we looked at uplifting this to Aurora and Beta?

We should.  The risk here is very low.
Comment 14 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-16 16:53:13 PDT
This is also tracking for ESR10 - does the current fix land cleanly?  Please nominate if so or let us know if an alternate patch is forthcoming.
Comment 15 Aryeh Gregor (:ayg) (working until September 2) 2012-08-19 01:23:20 PDT
Comment on attachment 647480 [details] [diff] [review]
Patch, nsTArray -> nsTArray<nsCOMPtr>

Yes, the same basic patch applies to ESR10, although one chunk doesn't apply cleanly and needs to be manually re-applied (but it's the exact same change).
Comment 16 Aryeh Gregor (:ayg) (working until September 2) 2012-08-21 04:37:56 PDT
Someone else please push to Aurora -- I don't have time to watch the tree.
Comment 17 Aryeh Gregor (:ayg) (working until September 2) 2012-08-21 04:38:07 PDT
Er, ESR10, not Aurora.
Comment 19 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-23 15:50:47 PDT
Confirmed testcase reproducible with try-server ASan build from decoder with changeset 9f3cc040e41a.

Verified testcase not reproducible with: 
 * 17.0a1: 198ca6edd0ae (debug) built on 20120823 by decoder
 * 16.0a2: 805e936380ab (debug) built on 20120823 by decoder

qa- for Firefox 15 and ESR15 as builds are not available.

Note You need to log in before you can comment on or make changes to this bug.