Closed Bug 772282 Opened 12 years ago Closed 12 years ago

InsertElementTxn crash

Categories

(Core :: DOM: Editor, defect)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla16
Tracking Status
firefox14 + verified
firefox15 + verified
firefox16 + verified
firefox-esr10 14+ verified

People

(Reporter: jruderman, Assigned: ayg)

References

Details

(4 keywords)

Crash Data

Attachments

(5 files)

      No description provided.
Attached file stack trace (debug)
Attached file stack trace (opt)
Breakpad doesn't catch this (bug 717758) :(
Aryeh, can you please have a look at this?  Thanks!
On Windows: bp-80367dcd-fbde-4174-9576-cf2822120710.
Crash Signature: [@ InsertElementTxn::DoTransaction] [@ InsertElementTxn::Init] → [@ InsertElementTxn::DoTransaction] [@ InsertElementTxn::Init] [@ msvcr100.dll@0x8af06 ]
OS: Mac OS X → All
Hardware: x86_64 → All
Attached patch PatchSplinter Review
I'm surprised that such an elaborate test-case is even needed to hit this bug.  nsEditor::MoveNode takes nsIDOMNode* as a parameter, and calls DeleteNode on it followed by InsertNode.  But it holds no reference, so if the only reference was held by the parent, the object gets destroyed after the DeleteNode.  This doesn't seem to be a regression in MoveNode, looking at the blame.  I guess there are just lots of ways for references to be held to nodes, and maybe most of MoveNode's callers hold a reference, so it just doesn't come up easily.

Try: https://tbpl.mozilla.org/?tree=Try&rev=3a2be29c4645
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Attachment #640531 - Flags: review?(ehsan)
Attachment #640531 - Flags: review?(ehsan) → review+
I also filed bug 772601 to get a static analysis for this kind of bug.
This is hit in my fuzzing as well. Shouldn't this be marked security or is it just trunk churn ?
Jesse said he didn't realize this was use-after-free when he filed it -- he didn't realize that "pure virtual function call" can mean using a dangling pointer.  Maybe you're familiar with why it can, but for Jesse and me, this page was helpful: <http://www.artima.com/cppsource/pure_virtual.html>.  I guess it should have been marked security.

The underlying bug is very old, incidentally -- it was probably just a recent change that exposed it.  Nodes tend to have large reference counts anyway, so missing nsCOMPtrs will often work, until they don't.  It might have been exploitable before in a different way.

https://hg.mozilla.org/integration/mozilla-inbound/rev/26b9d20710ea
Flags: in-testsuite+
Target Milestone: --- → mozilla16
Group: core-security
Keywords: sec-critical
=================================================================
==11180== ERROR: AddressSanitizer heap-use-after-free on address 0x7f139ac97df0 at pc 0x7f13d411d094 bp 0x7fff6101d650 sp 0x7fff6101d648
READ of size 8 at 0x7f139ac97df0 thread T0
    #0 0x7f13d411d094 in nsQueryInterface::operator()(nsID const&, void**) const firefox/src/objdir-ff-asan-sym/xpcom/build/nsCOMPtr.cpp:14
    #1 0x7f13d411dc03 in nsCOMPtr_base::assign_from_qi(nsQueryInterface, nsID const&) firefox/src/objdir-ff-asan-sym/xpcom/build/nsCOMPtr.cpp:56
    #2 0x7f13c931bf40 in nsCOMPtr<nsIDOMNode>::operator=(nsQueryInterface) firefox/src/../../../dist/include/nsCOMPtr.h:641
    #3 0x7f13cdf7c3e1 in InsertElementTxn::Init(nsIDOMNode*, nsIDOMNode*, int, nsIEditor*) firefox/src/editor/libeditor/base/InsertElementTxn.cpp:48
    #4 0x7f13cdea9330 in nsEditor::CreateTxnForInsertElement(nsIDOMNode*, nsIDOMNode*, int, InsertElementTxn**) firefox/src/editor/libeditor/base/nsEditor.cpp:4549
    #5 0x7f13cde49166 in nsEditor::InsertNode(nsIDOMNode*, nsIDOMNode*, int) firefox/src/editor/libeditor/base/nsEditor.cpp:1361
    #6 0x7f13cde5c8da in nsEditor::MoveNode(nsIDOMNode*, nsIDOMNode*, int) firefox/src/editor/libeditor/base/nsEditor.cpp:1736
    #7 0x7f13ce456d92 in nsHTMLEditor::RelativeFontChangeOnNode(int, nsINode*) firefox/src/editor/libeditor/html/nsHTMLEditorStyle.cpp:1818
    #8 0x7f13ce45720a in nsHTMLEditor::RelativeFontChangeOnNode(int, nsINode*) firefox/src/editor/libeditor/html/nsHTMLEditorStyle.cpp:1832
    #9 0x7f13ce4500c7 in nsHTMLEditor::RelativeFontChange(int) firefox/src/editor/libeditor/html/nsHTMLEditorStyle.cpp:1632
    #10 0x7f13ce453328 in nsHTMLEditor::DecreaseFontSize() firefox/src/editor/libeditor/html/nsHTMLEditorStyle.cpp:1508
    #11 0x7f13ce45338c in non-virtual thunk to nsHTMLEditor::DecreaseFontSize() firefox/src/modules/zlib/src/inffast.c:0
    #12 0x7f13d1946940 in nsDecreaseFontSizeCommand::DoCommand(char const*, nsISupports*) firefox/src/editor/composer/src/nsComposerCommands.cpp:1267
    #13 0x7f13d11ba86d in nsControllerCommandTable::DoCommand(char const*, nsISupports*) firefox/src/embedding/components/commandhandler/src/nsControllerCommandTable.cpp:158
    #14 0x7f13d118d174 in nsBaseCommandController::DoCommand(char const*) firefox/src/embedding/components/commandhandler/src/nsBaseCommandController.cpp:137
    #15 0x7f13d11a55b1 in nsCommandManager::DoCommand(char const*, nsICommandParams*, nsIDOMWindow*) firefox/src/embedding/components/commandhandler/src/nsCommandManager.cpp:238
    #16 0x7f13cc7a4b7a in nsHTMLDocument::ExecCommand(nsAString_internal const&, bool, nsAString_internal const&, bool*) firefox/src/content/html/document/src/nsHTMLDocument.cpp:3200
    #17 0x7f13cc7a74ee in non-virtual thunk to nsHTMLDocument::ExecCommand(nsAString_internal const&, bool, nsAString_internal const&, bool*) firefox/src/modules/zlib/src/inffast.c:0
    #18 0x7f13d45d900a in NS_InvokeByIndex_P firefox/src/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:161
    #19 0x7f13cfd4a26b in CallMethodHelper::Call() firefox/src/js/xpconnect/src/XPCWrappedNative.cpp:2405
    #20 0x7f13cfdb14d4 in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) firefox/src/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1474
    #21 0x7f13dafbc077 in js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) firefox/src/js/src/jscntxtinlines.h:400
    #22 0x7f13daf322f9 in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) firefox/src/js/src/jsinterp.cpp:2442
    #23 0x7f13daeb7656 in js::RunScript(JSContext*, JSScript*, js::StackFrame*) firefox/src/js/src/jsinterp.cpp:301
    #24 0x7f13dafbc499 in js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) firefox/src/js/src/jsinterp.cpp:355
    #25 0x7f13da91b080 in js::Invoke(JSContext*, js::InvokeArgsGuard&, js::MaybeConstruct) firefox/src/js/src/jsinterp.h:119
    #26 0x7f13dafc163d in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::Value*) firefox/src/js/src/jsinterp.cpp:387
    #27 0x7f13da7c6759 in JS_CallFunctionValue firefox/src/js/src/jsapi.cpp:5500
    #28 0x7f13cfcfa00e in nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) firefox/src/js/xpconnect/src/XPCWrappedJSClass.cpp:1436
    #29 0x7f13cfcabda8 in nsXPCWrappedJS::CallMethod(unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) firefox/src/js/xpconnect/src/XPCWrappedJS.cpp:580
    #30 0x7f13d45debc0 in PrepareAndDispatch firefox/src/xpcom/reflect/xptcall/src/md/unix/xptcstubs_x86_64_linux.cpp:121
    #31 0x7f13d45dc357 in SharedStub firefox/src/xpcom/reflect/xptcall/src/md/unix/xptcstubs_x86_64_linux.cpp:0
    #32 0x7f13cbb65a33 in nsEventListenerManager::HandleEventSubType(nsListenerStruct*, nsIDOMEventListener*, nsIDOMEvent*, nsIDOMEventTarget*, unsigned int, nsCxPusher*) firefox/src/content/events/src/nsEventListenerManager.cpp:818
    #33 0x7f13cbb66efa in nsEventListenerManager::HandleEventInternal(nsPresContext*, nsEvent*, nsIDOMEvent**, nsIDOMEventTarget*, unsigned int, nsEventStatus*, nsCxPusher*) firefox/src/content/events/src/nsEventListenerManager.cpp:891
    #34 0x7f13cbd115e7 in nsEventListenerManager::HandleEvent(nsPresContext*, nsEvent*, nsIDOMEvent**, nsIDOMEventTarget*, unsigned int, nsEventStatus*, nsCxPusher*) firefox/src/content/events/src/nsEventListenerManager.h:144
    #35 0x7f13cbd00286 in nsEventTargetChainItem::HandleEvent(nsEventChainPostVisitor&, unsigned int, bool, nsCxPusher*) firefox/src/content/events/src/nsEventDispatcher.cpp:185
    #36 0x7f13cbcfddec in nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor&, unsigned int, nsDispatchingCallback*, bool, nsCxPusher*) firefox/src/content/events/src/nsEventDispatcher.cpp:313
    #37 0x7f13cbd03900 in nsEventDispatcher::Dispatch(nsISupports*, nsPresContext*, nsEvent*, nsIDOMEvent*, nsEventStatus*, nsDispatchingCallback*, nsCOMArray<nsIDOMEventTarget>*) firefox/src/content/events/src/nsEventDispatcher.cpp:634
    #38 0x7f13cbd07e94 in nsEventDispatcher::DispatchDOMEvent(nsISupports*, nsEvent*, nsIDOMEvent*, nsPresContext*, nsEventStatus*) firefox/src/content/events/src/nsEventDispatcher.cpp:697
    #39 0x7f13cb0b3ef3 in nsINode::DispatchEvent(nsIDOMEvent*, bool*) firefox/src/content/base/src/nsINode.cpp:1079
    #40 0x7f13cabf90d2 in nsContentUtils::DispatchEvent(nsIDocument*, nsISupports*, nsAString_internal const&, bool, bool, bool, bool*) firefox/src/content/base/src/nsContentUtils.cpp:3421
    #41 0x7f13cabf8566 in nsContentUtils::DispatchTrustedEvent(nsIDocument*, nsISupports*, nsAString_internal const&, bool, bool, bool*) firefox/src/content/base/src/nsContentUtils.cpp:3391
    #42 0x7f13cadff8c6 in nsDocument::DispatchContentLoadedEvents() firefox/src/content/base/src/nsDocument.cpp:4106
    #43 0x7f13caec0129 in nsRunnableMethodImpl<void (nsDocument::*)(), true>::Run() firefox/src/../../../dist/include/nsThreadUtils.h:349
    #44 0x7f13d44d806d in nsThread::ProcessNextEvent(bool, bool*) firefox/src/xpcom/threads/nsThread.cpp:625
    #45 0x7f13d4166e7d in NS_ProcessNextEvent_P(nsIThread*, bool) firefox/src/objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:217
    #46 0x7f13d3208086 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) firefox/src/ipc/glue/MessagePump.cpp:82
    #47 0x7f13d478bfda in MessageLoop::RunInternal() firefox/src/ipc/chromium/src/base/message_loop.cc:209
    #48 0x7f13d478be23 in MessageLoop::RunHandler() firefox/src/ipc/chromium/src/base/message_loop.cc:202
    #49 0x7f13d478bd08 in MessageLoop::Run() firefox/src/ipc/chromium/src/base/message_loop.cc:176
    #50 0x7f13d273eeee in nsBaseAppShell::Run() firefox/src/widget/xpwidgets/nsBaseAppShell.cpp:165
    #51 0x7f13d138fb38 in nsAppStartup::Run() firefox/src/toolkit/components/startup/nsAppStartup.cpp:257
    #52 0x7f13c7b71960 in XREMain::XRE_mainRun() firefox/src/toolkit/xre/nsAppRunner.cpp:3787
    #53 0x7f13c7b78302 in XREMain::XRE_main(int, char**, nsXREAppData const*) firefox/src/toolkit/xre/nsAppRunner.cpp:3864
    #54 0x7f13c7b7b7d2 in XRE_main firefox/src/toolkit/xre/nsAppRunner.cpp:3940
    #55 0x40c29f in do_main(int, char**) firefox/src/browser/app/nsBrowserApp.cpp:160
    #56 0x409ccd in main firefox/src/browser/app/nsBrowserApp.cpp:298
    #57 0x7f13e42dec4d in ?? ??:0
0x7f139ac97df0 is located 112 bytes inside of 120-byte region [0x7f139ac97d80,0x7f139ac97df8)
freed by thread T0 here:
    #0 0x4a43a2 in free ??:0
    #1 0x7f13e116a5d3 in moz_free firefox/src/memory/mozalloc/mozalloc.cpp:49
    #2 0x7f13cb280f46 in ~nsTextNode firefox/src/content/base/src/nsTextNode.cpp:121
    #3 0x7f13cb15442d in nsNodeUtils::LastRelease(nsINode*) firefox/src/content/base/src/nsNodeUtils.cpp:252
    #4 0x7f13cafba23f in nsGenericDOMDataNode::Release() firefox/src/content/base/src/nsGenericDOMDataNode.cpp:113
    #5 0x7f13cb281374 in nsTextNode::Release() firefox/src/content/base/src/nsTextNode.cpp:124
    #6 0x7f13c7b3b930 in ~nsCOMPtr_base firefox/src/../../dist/include/nsCOMPtr.h:408
    #7 0x7f13c9069336 in nsCOMPtr<nsIContent>::~nsCOMPtr() firefox/src/../../dist/include/nsCOMPtr.h:447
    #8 0x7f13c9069003 in nsCOMPtr<nsIContent>::~nsCOMPtr() firefox/src/../../dist/include/nsCOMPtr.h:447
    #9 0x7f13ce3c9e2b in nsHTMLEditor::DeleteNode(nsIDOMNode*) firefox/src/editor/libeditor/html/nsHTMLEditor.cpp:3225a
    #10 0x7f13cde5c621 in nsEditor::MoveNode(nsIDOMNode*, nsIDOMNode*, int) firefox/src/editor/libeditor/base/nsEditor.cpp:1734
    #11 0x7f13ce456d92 in nsHTMLEditor::RelativeFontChangeOnNode(int, nsINode*) firefox/src/editor/libeditor/html/nsHTMLEditorStyle.cpp:1818
    #12 0x7f13ce45720a in nsHTMLEditor::RelativeFontChangeOnNode(int, nsINode*) firefox/src/editor/libeditor/html/nsHTMLEditorStyle.cpp:1832
    #13 0x7f13ce4500c7 in nsHTMLEditor::RelativeFontChange(int) firefox/src/editor/libeditor/html/nsHTMLEditorStyle.cpp:1632
    #14 0x7f13ce453328 in nsHTMLEditor::DecreaseFontSize() firefox/src/editor/libeditor/html/nsHTMLEditorStyle.cpp:1508
    #15 0x7f13ce45338c in non-virtual thunk to nsHTMLEditor::DecreaseFontSize() firefox/src/modules/zlib/src/inffast.c:0
    #16 0x7f13d1946940 in nsDecreaseFontSizeCommand::DoCommand(char const*, nsISupports*) firefox/src/editor/composer/src/nsComposerCommands.cpp:1267
    #17 0x7f13d11ba86d in nsControllerCommandTable::DoCommand(char const*, nsISupports*) firefox/src/embedding/components/commandhandler/src/nsControllerCommandTable.cpp:158
    #18 0x7f13d118d174 in nsBaseCommandController::DoCommand(char const*) firefox/src/embedding/components/commandhandler/src/nsBaseCommandController.cpp:137
    #19 0x7f13d11a55b1 in nsCommandManager::DoCommand(char const*, nsICommandParams*, nsIDOMWindow*) firefox/src/embedding/components/commandhandler/src/nsCommandManager.cpp:238
    #20 0x7f13cc7a4b7a in nsHTMLDocument::ExecCommand(nsAString_internal const&, bool, nsAString_internal const&, bool*) firefox/src/content/html/document/src/nsHTMLDocument.cpp:3200
    #21 0x7f13cc7a74ee in non-virtual thunk to nsHTMLDocument::ExecCommand(nsAString_internal const&, bool, nsAString_internal const&, bool*) firefox/src/modules/zlib/src/inffast.c:0
    #22 0x7f13d45d900a in NS_InvokeByIndex_P firefox/src/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:161
    #23 0x7f13cfd4a26b in CallMethodHelper::Call() firefox/src/js/xpconnect/src/XPCWrappedNative.cpp:2405
    #24 0x7f13cfdb14d4 in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) firefox/src/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1474
    #25 0x7f13dafbc077 in js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) firefox/src/js/src/jscntxtinlines.h:400
    #26 0x7f13daf322f9 in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) firefox/src/js/src/jsinterp.cpp:2442
    #27 0x7f13daeb7656 in js::RunScript(JSContext*, JSScript*, js::StackFrame*) firefox/src/js/src/jsinterp.cpp:301
    #28 0x7f13dafbc499 in js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) firefox/src/js/src/jsinterp.cpp:355
    #29 0x7f13da91b080 in js::Invoke(JSContext*, js::InvokeArgsGuard&, js::MaybeConstruct) firefox/src/js/src/jsinterp.h:119
previously allocated by thread T0 here:
    #0 0x4a4462 in __interceptor_malloc ??:0
    #1 0x7f13e116a727 in moz_xmalloc firefox/src/memory/mozalloc/mozalloc.cpp:54
    #2 0x7f13cb282a55 in nsTextNode::CloneDataNode(nsINodeInfo*, bool) const firefox/src/content/base/src/nsTextNode.cpp:146
    #3 0x7f13cab04bb4 in nsGenericDOMDataNode::Clone(nsINodeInfo*, nsINode**) const firefox/src/content/xml/content/src/../../../base/src/nsGenericDOMDataNode.h:216
    #4 0x7f13cb159a5a in nsNodeUtils::CloneAndAdopt(nsINode*, bool, bool, nsNodeInfoManager*, JSContext*, JSObject*, nsCOMArray<nsINode>&, nsINode*, nsINode**) firefox/src/content/base/src/nsNodeUtils.cpp:438
    #5 0x7f13caefa9ca in nsNodeUtils::CloneAndAdopt(nsINode*, bool, bool, nsNodeInfoManager*, JSContext*, JSObject*, nsCOMArray<nsINode>&, nsIDOMNode**) firefox/src/../../../../dist/include/nsNodeUtils.h:272
    #6 0x7f13cae13705 in nsNodeUtils::Clone(nsINode*, bool, nsNodeInfoManager*, nsCOMArray<nsINode>&, nsIDOMNode**) firefox/src/../../../../dist/include/nsNodeUtils.h:145
    #7 0x7f13cb1578d6 in nsNodeUtils::CloneNodeImpl(nsINode*, bool, bool, nsIDOMNode**) firefox/src/content/base/src/nsNodeUtils.cpp:360
    #8 0x7f13cab102bf in nsGenericDOMDataNode::CloneNode(bool, unsigned char, nsIDOMNode**) firefox/src/content/xml/content/src/../../../base/src/nsGenericDOMDataNode.h:119
    #9 0x7f13cb289bdf in nsTextNode::CloneNode(bool, unsigned char, nsIDOMNode**) firefox/src/content/base/src/nsTextNode.h:27
    #10 0x7f13cb28db4f in non-virtual thunk to nsTextNode::CloneNode(bool, unsigned char, nsIDOMNode**) firefox/src/modules/zlib/src/inffast.c:0
    #11 0x7f13cdf929a4 in SplitElementTxn::DoTransaction() firefox/src/editor/libeditor/base/SplitElementTxn.cpp:69
    #12 0x7f13d131f889 in nsTransactionItem::DoTransaction() firefox/src/editor/txmgr/src/nsTransactionItem.cpp:178
    #13 0x7f13d134425e in nsTransactionManager::BeginTransaction(nsITransaction*) firefox/src/editor/txmgr/src/nsTransactionManager.cpp:729
    #14 0x7f13d13321f5 in nsTransactionManager::DoTransaction(nsITransaction*) firefox/src/editor/txmgr/src/nsTransactionManager.cpp:74
    #15 0x7f13cde336f5 in nsEditor::DoTransaction(nsITransaction*) firefox/src/editor/libeditor/base/nsEditor.cpp:676
    #16 0x7f13cde4a9bc in nsEditor::SplitNode(nsIDOMNode*, int, nsIDOMNode**) firefox/src/editor/libeditor/base/nsEditor.cpp:1390
    #17 0x7f13cdea4ae0 in nsEditor::DeleteSelectionAndPrepareToCreateNode() firefox/src/editor/libeditor/base/nsEditor.cpp:4428
    #18 0x7f13ce2f6143 in nsHTMLEditor::DoInsertHTMLWithContext(nsAString_internal const&, nsAString_internal const&, nsAString_internal const&, nsAString_internal const&, nsIDOMDocument*, nsIDOMNode*, int, bool, bool) firefox/src/editor/libeditor/html/nsHTMLDataTransfer.cpp:365
    #19 0x7f13ce2f2ea3 in nsHTMLEditor::InsertHTMLWithContext(nsAString_internal const&, nsAString_internal const&, nsAString_internal const&, nsAString_internal const&, nsIDOMDocument*, nsIDOMNode*, int, bool) firefox/src/editor/libeditor/html/nsHTMLDataTransfer.cpp:229
    #20 0x7f13ce2f27cd in nsHTMLEditor::InsertHTML(nsAString_internal const&) firefox/src/editor/libeditor/html/nsHTMLDataTransfer.cpp:214
    #21 0x7f13ce2f288f in non-virtual thunk to nsHTMLEditor::InsertHTML(nsAString_internal const&) firefox/src/modules/zlib/src/inffast.c:0
    #22 0x7f13d1948703 in nsInsertHTMLCommand::DoCommandParams(char const*, nsICommandParams*, nsISupports*) firefox/src/editor/composer/src/nsComposerCommands.cpp:1329
    #23 0x7f13d11bb11c in nsControllerCommandTable::DoCommandParams(char const*, nsICommandParams*, nsISupports*) firefox/src/embedding/components/commandhandler/src/nsControllerCommandTable.cpp:175
    #24 0x7f13d118d9df in nsBaseCommandController::DoCommandWithParams(char const*, nsICommandParams*) firefox/src/embedding/components/commandhandler/src/nsBaseCommandController.cpp:153
==11180== ABORTING
Stats: 152M malloced (186M for red zones) by 456381 calls
Stats: 41M realloced by 19471 calls
Stats: 110M freed by 225958 calls
Stats: 0M really freed by 0 calls
Stats: 368M (94257 full pages) mmaped in 92 calls
  mmaps   by size class: 8:376809; 9:49146; 10:20475; 11:18423; 12:3072; 13:2048; 14:1536; 15:384; 16:576; 17:128; 18:160; 19:48; 20:16;
  mallocs by size class: 8:370354; 9:46345; 10:16034; 11:16866; 12:2347; 13:1830; 14:1405; 15:319; 16:552; 17:115; 18:160; 19:41; 20:13;
  frees   by size class: 8:158858; 9:36208; 10:12653; 11:13632; 12:1505; 13:945; 14:1220; 15:264; 16:467; 17:100; 18:58; 19:38; 20:10;
  rfrees  by size class:
Stats: malloc large: 329 small slow: 2041
Shadow byte and word:
  0x1fe273592fbe: fd
  0x1fe273592fb8: fd fd fd fd fd fd fd fd
More shadow bytes:
  0x1fe273592f98: fd fd fd fd fd fd fd fd
  0x1fe273592fa0: fa fa fa fa fa fa fa fa
  0x1fe273592fa8: fa fa fa fa fa fa fa fa
  0x1fe273592fb0: fd fd fd fd fd fd fd fd
=>0x1fe273592fb8: fd fd fd fd fd fd fd fd
  0x1fe273592fc0: fa fa fa fa fa fa fa fa
  0x1fe273592fc8: fa fa fa fa fa fa fa fa
  0x1fe273592fd0: fd fd fd fd fd fd fd fd
  0x1fe273592fd8: fd fd fd fd fd fd fd fd
(In reply to :Aryeh Gregor from comment #8)
> Jesse said he didn't realize this was use-after-free when he filed it -- he
> didn't realize that "pure virtual function call" can mean using a dangling
> pointer.  Maybe you're familiar with why it can, but for Jesse and me, this
> page was helpful: <http://www.artima.com/cppsource/pure_virtual.html>.  I
> guess it should have been marked security.

In our case, we look upon "pure virtual function call" cases with suspicion and always manually verify them in debugger. Then ASAN came along and removed the need for manual verification. Valgrind should catch this too, but can be very slow and annoying at times.

> 
> The underlying bug is very old, incidentally -- it was probably just a
> recent change that exposed it.  Nodes tend to have large reference counts
> anyway, so missing nsCOMPtrs will often work, until they don't.  It might
> have been exploitable before in a different way.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/26b9d20710ea
https://hg.mozilla.org/mozilla-central/rev/26b9d20710ea

(Fixed modulo testing on aurora/beta/esr)
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment on attachment 640531 [details] [diff] [review]
Patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version:
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

This is pretty bad, it's a security bug which was open when the patch landed, and the test case is now in m-c... We should take this on all branches, especially since the fix is very simple and low-risk.
Attachment #640531 - Flags: approval-mozilla-esr10?
Attachment #640531 - Flags: approval-mozilla-beta?
Attachment #640531 - Flags: approval-mozilla-aurora?
(In reply to Ehsan Akhgari [:ehsan] from comment #12)
> This is pretty bad, it's a security bug which was open when the patch
> landed, and the test case is now in m-c... We should take this on all
> branches, especially since the fix is very simple and low-risk.

Ehsan - is this low enough risk that you would unleash it on our release audience without testing on the Beta channel specifically? We would still get testing on Aurora 15. I'm trying to conserve resources for the final builds of FF14 instead of devoting them to an out-of-band beta.

CC'ing Dan and Al to get their take, because my preference of course would be to not take this patch at all on FF14.
This is about the simplest and safest possible security fix. For anyone who's been studying our security patches it screams "look for use-after-free here", and the testcase means they don't even have to look to find it. We should take it.
(In reply to Alex Keybl [:akeybl] from comment #13)
> (In reply to Ehsan Akhgari [:ehsan] from comment #12)
> > This is pretty bad, it's a security bug which was open when the patch
> > landed, and the test case is now in m-c... We should take this on all
> > branches, especially since the fix is very simple and low-risk.
> 
> Ehsan - is this low enough risk that you would unleash it on our release
> audience without testing on the Beta channel specifically?

Absolutely.  I'm 100% certain that this patch is safe enough to be taken on all of our branches even without the usual bake time.

> We would still
> get testing on Aurora 15. I'm trying to conserve resources for the final
> builds of FF14 instead of devoting them to an out-of-band beta.

Understood, and I think we should take it.

Also, I'm very sorry about neglecting the potential severity of this bug until it was too late.  I'll try to make sure I won't make this mistake again.  :/
Comment on attachment 640531 [details] [diff] [review]
Patch

[Triage Comment]
Let's land on all branches given Ehsan's risk assessment.
Attachment #640531 - Flags: approval-mozilla-esr10?
Attachment #640531 - Flags: approval-mozilla-esr10+
Attachment #640531 - Flags: approval-mozilla-beta?
Attachment #640531 - Flags: approval-mozilla-beta+
Attachment #640531 - Flags: approval-mozilla-aurora?
Attachment #640531 - Flags: approval-mozilla-aurora+
Is there any point in keeping this bug hidden?  The patch already landed on m-c with the exploit as a crashtest.
(In reply to :Aryeh Gregor from comment #17)
> Is there any point in keeping this bug hidden?  The patch already landed on
> m-c with the exploit as a crashtest.

Not sure, I'll let dveditz decide.
(In reply to :Ms2ger from comment #19)
> https://hg.mozilla.org/releases/mozilla-beta/rev/0d510b2d60bb

Ouch this includes the testcase :(
(In reply to Ehsan Akhgari [:ehsan] from comment #20)
> (In reply to :Ms2ger from comment #19)
> > https://hg.mozilla.org/releases/mozilla-beta/rev/0d510b2d60bb
> 
> Ouch this includes the testcase :(

Yes, and so did the trunk landing. Next time you want me to land something else than the attached patch, please tell me in advance.
(In reply to :Ms2ger from comment #22)
> (In reply to Ehsan Akhgari [:ehsan] from comment #20)
> > (In reply to :Ms2ger from comment #19)
> > > https://hg.mozilla.org/releases/mozilla-beta/rev/0d510b2d60bb
> > 
> > Ouch this includes the testcase :(
> 
> Yes, and so did the trunk landing. Next time you want me to land something
> else than the attached patch, please tell me in advance.

That is because the bug was not completely understood in time and it was landed when the bug was public.  Please see comment 8.

As a general policy, we do not land test cases for security sensitive bugs until they're opened up.
Was 13.0.1 not affected by this? The testcase doesn't crash it.
(In reply to Al Billings [:abillings] from comment #24)
> Was 13.0.1 not affected by this? The testcase doesn't crash it.

The bug did exist there, yes.  Triggering it however might not exactly be possible using this same testcase, but other similar test cases should still trigger it.
Verified fixed with against all versions with nightly debug builds from 2012-07-13.
Group: core-security

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Keywords: regression

Restrict comments?

Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(gijskruitbosch+bugs)
Restrict Comments: true
You need to log in before you can comment on or make changes to this bug.