Last Comment Bug 795804 - (CVE-2012-4214) Heap-use-after-free in nsTextEditorState::PrepareEditor
(CVE-2012-4214)
: Heap-use-after-free in nsTextEditorState::PrepareEditor
Status: RESOLVED FIXED
[adv-track-main17+][adv-track-esr17+]...
: csectype-uaf, sec-critical, testcase
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: x86_64 All
: -- normal (vote)
: mozilla19
Assigned To: :Aryeh Gregor (away until August 15)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-30 22:22 PDT by Abhishek Arya
Modified: 2014-07-24 13:42 PDT (History)
13 users (show)
dveditz: sec‑bounty+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed
+
fixed
17+
fixed


Attachments
Testcase (868 bytes, text/html)
2012-09-30 22:22 PDT, Abhishek Arya
no flags Details
Patch (3.02 KB, patch)
2012-10-09 10:52 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
akeybl: approval‑mozilla‑aurora+
abillings: sec‑approval+
Details | Diff | Review
Patch backported to beta (11.45 KB, patch)
2012-10-10 00:37 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
akeybl: approval‑mozilla‑beta+
akeybl: approval‑mozilla‑esr10+
Details | Diff | Review
Patch backported to ESR10 (14.54 KB, patch)
2012-10-14 01:11 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
akeybl: approval‑mozilla‑esr10+
Details | Diff | Review

Description Abhishek Arya 2012-09-30 22:22:11 PDT
Created attachment 666435 [details]
Testcase

Reproduces on trunk. Run a few firefox instances simultaneously for reproducing reliabily.

=================================================================
==19969== ERROR: AddressSanitizer heap-use-after-free on address 0x7f310f2083b0 at pc 0x7f313ddafc7c bp 0x7fffb5d04130 sp 0x7fffb5d04128
READ of size 8 at 0x7f310f2083b0 thread T0
    #0 0x7f313ddafc7b in nsTextEditorState::PrepareEditor(nsAString_internal const*) src/content/html/content/src/nsTextEditorState.cpp:1125
    #1 0x7f313ddcd603 in PrepareEditorEvent::Run() src/content/html/content/src/nsTextEditorState.cpp:1026
    #2 0x7f313cad7c4c in nsContentUtils::RemoveScriptBlocker() src/content/base/src/nsContentUtils.cpp:5021
    #3 0x7f313b04835e in ~nsAutoScriptBlocker src/../../../../dist/include/nsContentUtils.h:2275
    #4 0x7f313b0320a6 in ~nsAutoScriptBlocker src/../../../../dist/include/nsContentUtils.h:2274
    #5 0x7f313b43fc91 in PresShell::FlushPendingNotifications(mozFlushType) src/layout/base/nsPresShell.cpp:3834
    #6 0x7f313bb77232 in nsHideViewer::Run() src/layout/generic/nsSubDocumentFrame.cpp:782
    #7 0x7f313cad7c4c in nsContentUtils::RemoveScriptBlocker() src/content/base/src/nsContentUtils.cpp:5021
    #8 0x7f313b04835e in ~nsAutoScriptBlocker src/../../../../dist/include/nsContentUtils.h:2275
    #9 0x7f313b0320a6 in ~nsAutoScriptBlocker src/../../../../dist/include/nsContentUtils.h:2274
    #10 0x7f313ccf2af5 in nsDocument::AdoptNode(nsIDOMNode*, nsIDOMNode**) src/content/base/src/nsDocument.cpp:6371
    #11 0x7f313e668b5c in nsHTMLDocument::AdoptNode(nsIDOMNode*, nsIDOMNode**) src/content/html/document/src/nsHTMLDocument.h:87
    #12 0x7f3141f73cd6 in nsIDOMDocument_AdoptNode(JSContext*, unsigned int, JS::Value*) src/objdir-ff-asan-sym/js/xpconnect/src/dom_quickstubs.cpp:3452
    #13 0x7f314d3dcd9f in js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) src/js/src/jscntxtinlines.h:370
    #14 0x7f314d37eb79 in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) src/js/src/jsinterp.cpp:2460
    #15 0x7f314d2caaee in js::RunScript(JSContext*, JS::Handle<JSScript*>, js::StackFrame*) src/js/src/jsinterp.cpp:324
    #16 0x7f314d3ea566 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::StackFrame*, JS::Value*) src/js/src/jsinterp.cpp:509
    #17 0x7f314d3ec2fb in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) src/js/src/jsinterp.cpp:546
    #18 0x7f314caff289 in JS::Evaluate(JSContext*, JS::Handle<JSObject*>, JS::CompileOptions, unsigned short const*, unsigned long, JS::Value*) src/js/src/jsapi.cpp:5679
    #19 0x7f313ee16dee in nsJSContext::EvaluateString(nsAString_internal const&, JSObject*, nsIPrincipal*, nsIPrincipal*, char const*, unsigned int, JSVersion, nsAString_internal*, bool*) src/dom/base/nsJSEnvironment.cpp:1506
    #20 0x7f313efcfb76 in nsGlobalWindow::RunTimeoutHandler(nsTimeout*, nsIScriptContext*) src/dom/base/nsGlobalWindow.cpp:9621
    #21 0x7f313ef876c4 in nsGlobalWindow::RunTimeout(nsTimeout*) src/dom/base/nsGlobalWindow.cpp:9880
    #22 0x7f313efcda28 in nsGlobalWindow::TimerCallback(nsITimer*, void*) src/dom/base/nsGlobalWindow.cpp:10147
    #23 0x7f3146b44972 in nsTimerImpl::Fire() src/xpcom/threads/nsTimerImpl.cpp:473
    #24 0x7f3146b45e7a in nsTimerEvent::Run() src/xpcom/threads/nsTimerImpl.cpp:556
    #25 0x7f3146b09580 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:612
    #26 0x7f314679becb in NS_ProcessNextEvent_P(nsIThread*, bool) src/objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:220
    #27 0x7f31451e53b6 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:82
    #28 0x7f3146dc1e11 in MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:208
    #29 0x7f3146dc1c46 in MessageLoop::RunHandler() src/ipc/chromium/src/base/message_loop.cc:201
    #30 0x7f3146dc1b2b in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:175
    #31 0x7f314468cdda in nsBaseAppShell::Run() src/widget/xpwidgets/nsBaseAppShell.cpp:163
    #32 0x7f31432bf9b4 in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:290
    #33 0x7f3139932a4d in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:3782
    #34 0x7f31399388c5 in XREMain::XRE_main(int, char**, nsXREAppData const*) src/toolkit/xre/nsAppRunner.cpp:3848
    #35 0x7f313993b774 in XRE_main src/toolkit/xre/nsAppRunner.cpp:3923
    #36 0x40d013 in do_main(int, char**) src/browser/app/nsBrowserApp.cpp:174
    #37 0x40a755 in main src/browser/app/nsBrowserApp.cpp:279
    #38 0x7f3157830c4c in ?? ??:0
0x7f310f2083b0 is located 48 bytes inside of 120-byte region [0x7f310f208380,0x7f310f2083f8)
freed by thread T0 here:
    #0 0x4c4af0 in free ??:0
    #1 0x7f31546bc586 in moz_free src/memory/mozalloc/mozalloc.cpp:51
    #2 0x7f313e05e4a6 in operator delete(void*) src/../../../../dist/include/mozilla/mozalloc.h:224
    #3 0x7f313e06b9b9 in nsHTMLInputElement::HandleTypeChange(unsigned char) src/content/html/content/src/nsHTMLInputElement.cpp:2621
    #4 0x7f313e09a2f4 in nsHTMLInputElement::ParseAttribute(int, nsIAtom*, nsAString_internal const&, nsAttrValue&) src/content/html/content/src/nsHTMLInputElement.cpp:2751
    #5 0x7f313cece403 in nsGenericElement::SetAttr(int, nsIAtom*, nsIAtom*, nsAString_internal const&, bool) src/content/base/src/nsGenericElement.cpp:1952
    #6 0x7f313dd28b8d in nsGenericHTMLElement::SetAttr(int, nsIAtom*, nsIAtom*, nsAString_internal const&, bool) src/content/html/content/src/nsGenericHTMLElement.cpp:1994
    #7 0x7f313dd0c5a3 in nsGenericHTMLElement::SetAttr(int, nsIAtom*, nsAString_internal const&, bool) src/content/html/document/src/../../content/src/nsGenericHTMLElement.h:175
    #8 0x7f313dd0e4b7 in nsGenericHTMLElement::SetAttrHelper(nsIAtom*, nsAString_internal const&) src/content/html/content/src/nsGenericHTMLElement.cpp:2863
    #9 0x7f313e0772d0 in nsHTMLInputElement::SetType(nsAString_internal const&) src/content/html/content/src/nsHTMLInputElement.cpp:906
    #10 0x7f313e07733e in non-virtual thunk to nsHTMLInputElement::SetType(nsAString_internal const&) src/gfx/cairo/cairo/src/cairo-surface-subsurface.c:0
    #11 0x7f3142620241 in nsIDOMHTMLInputElement_SetType(JSContext*, JS::Handle<JSObject*>, JS::Handle<long>, int, JS::MutableHandle<JS::Value>) src/objdir-ff-asan-sym/js/xpconnect/src/dom_quickstubs.cpp:15518
    #12 0x7f314d6bf785 in js::CallJSPropertyOpSetter(JSContext*, int (*)(JSContext*, JS::Handle<JSObject*>, JS::Handle<long>, int, JS::MutableHandle<JS::Value>), JS::Handle<JSObject*>, JS::Handle<long>, int, JS::MutableHandle<JS::Value>) src/js/src/jscntxtinlines.h:456
    #13 0x7f314d6f6056 in js::baseops::SetPropertyHelper(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<long>, unsigned int, JS::MutableHandle<JS::Value>, int) src/js/src/jsobj.cpp:4592
    #14 0x7f314d423bf6 in js::SetPropertyOperation(JSContext*, unsigned char*, JS::Handle<JS::Value>, JS::Handle<JS::Value>) src/js/src/jsinterpinlines.h:355
    #15 0x7f314d36ed8e in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) src/js/src/jsinterp.cpp:2370
    #16 0x7f314d2caaee in js::RunScript(JSContext*, JS::Handle<JSScript*>, js::StackFrame*) src/js/src/jsinterp.cpp:324
    #17 0x7f314d3dd590 in js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) src/js/src/jsinterp.cpp:378
    #18 0x7f314cc39f44 in js::Invoke(JSContext*, js::InvokeArgsGuard&, js::MaybeConstruct) src/js/src/jsinterp.h:109
    #19 0x7f314d3e29e0 in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::Value*) src/js/src/jsinterp.cpp:411
    #20 0x7f314cb0c356 in JS_CallFunctionValue src/js/src/jsapi.cpp:5860
    #21 0x7f313ee25356 in nsJSContext::CallEventHandler(nsISupports*, JSObject*, JSObject*, nsIArray*, nsIVariant**) src/dom/base/nsJSEnvironment.cpp:1907
    #22 0x7f313f68fc68 in nsJSEventListener::HandleEvent(nsIDOMEvent*) src/dom/src/events/nsJSEventListener.cpp:212
    #23 0x7f313da2fecb in nsEventListenerManager::HandleEventSubType(nsListenerStruct*, nsIDOMEventListener*, nsIDOMEvent*, nsIDOMEventTarget*, unsigned int, nsCxPusher*) src/content/events/src/nsEventListenerManager.cpp:845
    #24 0x7f313da31648 in nsEventListenerManager::HandleEventInternal(nsPresContext*, nsEvent*, nsIDOMEvent**, nsIDOMEventTarget*, unsigned int, nsEventStatus*, nsCxPusher*) src/content/events/src/nsEventListenerManager.cpp:918
    #25 0x7f313dbc773f in nsEventListenerManager::HandleEvent(nsPresContext*, nsEvent*, nsIDOMEvent**, nsIDOMEventTarget*, unsigned int, nsEventStatus*, nsCxPusher*) src/content/events/src/nsEventListenerManager.h:143
    #26 0x7f313dbb72de in nsEventTargetChainItem::HandleEvent(nsEventChainPostVisitor&, unsigned int, bool, nsCxPusher*) src/content/events/src/nsEventDispatcher.cpp:182
    #27 0x7f313dbb5215 in nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor&, unsigned int, nsDispatchingCallback*, bool, nsCxPusher*) src/content/events/src/nsEventDispatcher.cpp:313
    #28 0x7f313dbbb569 in nsEventDispatcher::Dispatch(nsISupports*, nsPresContext*, nsEvent*, nsIDOMEvent*, nsEventStatus*, nsDispatchingCallback*, nsCOMArray<nsIDOMEventTarget>*) src/content/events/src/nsEventDispatcher.cpp:629
    #29 0x7f313b255602 in DocumentViewerImpl::LoadComplete(tag_nsresult) src/layout/base/nsDocumentViewer.cpp:1024
previously allocated by thread T0 here:
    #0 0x4c4bb0 in __interceptor_malloc ??:0
    #1 0x7f31546bc6da in moz_xmalloc src/memory/mozalloc/mozalloc.cpp:57
    #2 0x7f313e05d2ca in operator new(unsigned long) src/../../../../dist/include/mozilla/mozalloc.h:200
    #3 0x7f313e05c040 in NS_NewHTMLInputElement(already_AddRefed<nsINodeInfo>, mozilla::dom::FromParser) src/content/html/content/src/nsHTMLInputElement.cpp:537
    #4 0x7f313e5e54af in CreateHTMLElement(unsigned int, already_AddRefed<nsINodeInfo>, mozilla::dom::FromParser) src/content/html/document/src/nsHTMLContentSink.cpp:497
    #5 0x7f313e5e5d0a in NS_NewHTMLElement(nsIContent**, already_AddRefed<nsINodeInfo>, mozilla::dom::FromParser) src/content/html/document/src/nsHTMLContentSink.cpp:480
    #6 0x7f313cfa66b1 in NS_NewElement(nsIContent**, already_AddRefed<nsINodeInfo>, mozilla::dom::FromParser) src/content/base/src/nsNameSpaceManager.cpp:201
    #7 0x7f314017055a in nsHtml5TreeOperation::Perform(nsHtml5TreeOpExecutor*, nsIContent**) src/parser/html/nsHtml5TreeOperation.cpp:340
    #8 0x7f314018fb75 in nsHtml5TreeOpExecutor::RunFlushLoop() src/parser/html/nsHtml5TreeOpExecutor.cpp:564
    #9 0x7f31401cc259 in nsHtml5ExecutorFlusher::Run() src/parser/html/nsHtml5StreamParser.cpp:127
    #10 0x7f3146b09580 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:612
    #11 0x7f314679becb in NS_ProcessNextEvent_P(nsIThread*, bool) src/objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:220
    #12 0x7f31451e53b6 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:82
    #13 0x7f3146dc1e11 in MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:208
    #14 0x7f3146dc1c46 in MessageLoop::RunHandler() src/ipc/chromium/src/base/message_loop.cc:201
    #15 0x7f3146dc1b2b in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:175
    #16 0x7f314468cdda in nsBaseAppShell::Run() src/widget/xpwidgets/nsBaseAppShell.cpp:163
    #17 0x7f31432bf9b4 in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:290
    #18 0x7f3139932a4d in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:3782
    #19 0x7f31399388c5 in XREMain::XRE_main(int, char**, nsXREAppData const*) src/toolkit/xre/nsAppRunner.cpp:3848
    #20 0x7f313993b774 in XRE_main src/toolkit/xre/nsAppRunner.cpp:3923
    #21 0x40d013 in do_main(int, char**) src/browser/app/nsBrowserApp.cpp:174
    #22 0x40a755 in main src/browser/app/nsBrowserApp.cpp:279
Shadow byte and word:
  0x1fe621e41076: fd
  0x1fe621e41070: fd fd fd fd fd fd fd fd
More shadow bytes:
  0x1fe621e41050: 00 00 00 00 00 00 00 00
  0x1fe621e41058: fb fb fb fb fb fb fb fb
  0x1fe621e41060: fa fa fa fa fa fa fa fa
  0x1fe621e41068: fa fa fa fa fa fa fa fa
=>0x1fe621e41070: fd fd fd fd fd fd fd fd
  0x1fe621e41078: fd fd fd fd fd fd fd fd
  0x1fe621e41080: fa fa fa fa fa fa fa fa
  0x1fe621e41088: fa fa fa fa fa fa fa fa
  0x1fe621e41090: 00 00 00 fb fb fb fb fb
Stats: 254M malloced (296M for red zones) by 516614 calls
Stats: 42M realloced by 24355 calls
Stats: 221M freed by 290089 calls
Stats: 87M really freed by 207706 calls
Stats: 484M (123993 full pages) mmaped in 121 calls
  mmaps   by size class: 8:311277; 9:32764; 10:8190; 11:14329; 12:2048; 13:1536; 14:1280; 15:384; 16:1024; 17:1248; 18:144; 19:40; 20:28;
  mallocs by size class: 8:448277; 9:33777; 10:8842; 11:17027; 12:2365; 13:1758; 14:1483; 15:342; 16:1199; 17:1317; 18:159; 19:42; 20:26;
  frees   by size class: 8:238380; 9:24927; 10:5659; 11:13922; 12:1543; 13:1467; 14:1292; 15:301; 16:1126; 17:1300; 18:110; 19:39; 20:23;
  rfrees  by size class: 8:184498; 9:8844; 10:2132; 11:9156; 12:663; 13:494; 14:464; 15:181; 16:934; 17:311; 18:24; 19:4; 20:1;
Stats: malloc large: 1544 small slow: 2450
==19969== ABORTING
Comment 1 :Ehsan Akhgari (busy, don't ask for review please) 2012-10-01 10:31:38 PDT
Aryeh, can you please take a stab at this?  Thanks!
Comment 3 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-10-07 11:21:28 PDT
Can't reproduce on linux. I didn't use ASAN build, but added some manual checks.
Anyhow, nsTextEditorState &mState; is a guaranteed security problem in a runnable.
Comment 4 Abhishek Arya 2012-10-07 20:31:30 PDT
(In reply to Olli Pettay [:smaug] from comment #3)
> Can't reproduce on linux. I didn't use ASAN build, but added some manual
> checks.
> Anyhow, nsTextEditorState &mState; is a guaranteed security problem in a
> runnable.

ASAN build is broken again :(, so can't test on trunk. ccing Christian. I tested using 
20121005051921
http://hg.mozilla.org/mozilla-central/rev/3b458f4e0f42
and it reproduces nicely with multiple simultaneous firefox instances (like 10) started at once with the testcase path on command line.
Comment 5 Abhishek Arya 2012-10-08 00:21:03 PDT
I was able to compile ASAN trunk by disabling all warnings. Then i checked the testcase and it still reliably crashes for me with multiple firefox instances started simultaneously.
Comment 6 :Aryeh Gregor (away until August 15) 2012-10-09 02:01:49 PDT
I can't reproduce a crash on current trunk with a bunch of refreshing (Linux 32-bit).  But it looks pretty simple -- nsTextEditorState is not reference-counted, and it gets deleted while PrepareEditorEvent holds a reference.  I guess the right way to do this is make it reference-counted.  Ehsan, should I do this similarly to bug 771994?  I can't currently test that it fixes the problem, because I can't reproduce with a local build.
Comment 7 Abhishek Arya 2012-10-09 05:39:30 PDT
This reproduces a day ago easily with a python script like this and using a nightly build from https://people.mozilla.com/~choller/firefox/asan/

import os
import time

thread_num = 10

for i in range(0, thread_num):
  profile_dir = '/tmp/firefox_profile_%d' % i
  os.system('mkdir %s' % profile_dir)
  os.system('touch %s/prefs.js')
  cmd = '/path_to_firefox/firefox-bin -no-remote -profile "%s" "test.html" 2>&1 | tee /tmp/%d.log &' % (profile_dir, i)
  os.system(cmd)
  time.sleep(0.3)
Comment 8 :Ehsan Akhgari (busy, don't ask for review please) 2012-10-09 10:14:00 PDT
(In reply to :Aryeh Gregor from comment #6)
> I can't reproduce a crash on current trunk with a bunch of refreshing (Linux
> 32-bit).  But it looks pretty simple -- nsTextEditorState is not
> reference-counted, and it gets deleted while PrepareEditorEvent holds a
> reference.  I guess the right way to do this is make it reference-counted. 
> Ehsan, should I do this similarly to bug 771994?  I can't currently test
> that it fixes the problem, because I can't reproduce with a local build.

So we don't really need to make nsTextEditorState refcounted, since it's not really supposed to be owned by multiple things.  What we should do here is to use a WeakPtr to nsTextEditorState in PrepareEditorEvent and just check to make sure it's alive before using it (since the stuff that PrepareEditorEvent does are not important if something has destroyed the nsTextEditorState object.
Comment 9 :Ehsan Akhgari (busy, don't ask for review please) 2012-10-09 10:14:38 PDT
(You can use this weakptr implementation: http://mxr.mozilla.org/mozilla-central/source/mfbt/WeakPtr.h.  We can backport it to branches.)
Comment 10 :Aryeh Gregor (away until August 15) 2012-10-09 10:52:54 PDT
Created attachment 669620 [details] [diff] [review]
Patch

Compiles locally.  I can't verify it fixes the issue because I couldn't reproduce it on my own compiled build -- I didn't try the ASAN builds because it wouldn't help me verify that I fixed it.  But in theory it should.

Would you like a separate patch for beta that backports WeakPtr?  It was only added on Thursday, but that means it should already be in Aurora.
Comment 11 :Ehsan Akhgari (busy, don't ask for review please) 2012-10-09 11:49:48 PDT
Comment on attachment 669620 [details] [diff] [review]
Patch

Review of attachment 669620 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, 100% sure this fixes it.  Thanks!
Comment 12 :Ehsan Akhgari (busy, don't ask for review please) 2012-10-09 11:50:12 PDT
(In reply to :Aryeh Gregor from comment #10)
> Would you like a separate patch for beta that backports WeakPtr?  It was
> only added on Thursday, but that means it should already be in Aurora.

That would be great, yes.
Comment 13 :Ehsan Akhgari (busy, don't ask for review please) 2012-10-09 11:52:07 PDT
Comment on attachment 669620 [details] [diff] [review]
Patch

[Security approval request comment]
How easily can the security issue be deduced from the patch? It's perfectly evident that this fix prevents against a pointer to a deleted object to be used.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?  It shouldn't be immediately obvious on how to trigger this security bug.  The patch does not accompany tests.

Which older supported branches are affected by this flaw? All of them.

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? The backport should be easy to create.  We should just import the WeakPtr implementation.

How likely is this patch to cause regressions; how much testing does it need? It is very unlikely to cause regressions.  The fix is very small and well understood.
Comment 14 :Aryeh Gregor (away until August 15) 2012-10-10 00:37:56 PDT
Created attachment 669874 [details] [diff] [review]
Patch backported to beta

This backports WeakPtr.h, and NullPtr.h as well because WeakPtr.h depends on it.  Fortunately, this was easy because of how self-contained mfbt is.  It compiles locally, but I didn't push to try.  Again, I didn't test it because I couldn't reproduce locally anyway.
Comment 15 :Ehsan Akhgari (busy, don't ask for review please) 2012-10-10 05:38:52 PDT
Should we request branch approval before or after getting a sec-approval+?
Comment 16 Daniel Veditz [:dveditz] 2012-10-10 11:57:57 PDT
Presumably this affects ESR-10 as well? Hard to test or verify because we've never gotten ASan to build on that branch.
Comment 17 [On PTO until 6/29] 2012-10-10 11:58:42 PDT
Ehsan, you should ask sec-approval first since we should land on trunk before going elsewhere.
Comment 18 :Ehsan Akhgari (busy, don't ask for review please) 2012-10-10 12:21:00 PDT
(In reply to Daniel Veditz [:dveditz] from comment #16)
> Presumably this affects ESR-10 as well? Hard to test or verify because we've
> never gotten ASan to build on that branch.

The buggy code exists there so I have all reasons to believe that ESR is affected as well.
Comment 19 :Ehsan Akhgari (busy, don't ask for review please) 2012-10-10 12:23:07 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea3394d0280c
Comment 20 :Ehsan Akhgari (busy, don't ask for review please) 2012-10-10 12:26:02 PDT
Comment on attachment 669620 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 534785 (the first time this code got into the tree)
User impact if declined: sec-critical
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): minimal
String or UUID changes made by this patch: none
Comment 21 Ed Morley [:emorley] 2012-10-11 07:13:00 PDT
https://hg.mozilla.org/mozilla-central/rev/ea3394d0280c
Comment 22 Alex Keybl [:akeybl] 2012-10-11 16:12:10 PDT
Comment on attachment 669620 [details] [diff] [review]
Patch

Low risk sg:crit fix, approving for all branches.
Comment 24 :Ehsan Akhgari (busy, don't ask for review please) 2012-10-12 19:53:07 PDT
Backed out of ESR for build failures: https://tbpl.mozilla.org/?tree=Mozilla-Esr10&rev=7797a9612159

https://hg.mozilla.org/releases/mozilla-esr10/rev/d61843854f33

Aryeh, can you please look into why the build failed?  My suspicion is that the mfbt setup in ESR is very different, and we need to expose the added headers some other way.  Waldo might know how.
Comment 25 :Aryeh Gregor (away until August 15) 2012-10-14 01:11:48 PDT
Created attachment 671190 [details] [diff] [review]
Patch backported to ESR10

Um, how did the patch even apply to ESR10?  There's no file mfbt/exported_headers.mk, so it should have failed.  mfbt/Makefile.in was created only in bug 711775, last December.  Before that, it looks like js/src/Makefile.in is what you need to change.  I also had to backport TypeTraits.h for ESR10, and remove the #include "mozilla/Assertions.h" (since MOZ_ASSERT was in mozilla/Util.h at that point).  I also opted to remove a use of MOZ_STATIC_ASSERT instead of backporting the function.  After that it still didn't compile, but Ms2ger was kind enough to suggest adding "using namespace mozilla;", and then it compiled.  I didn't test beyond building the tree.
Comment 26 :Ehsan Akhgari (busy, don't ask for review please) 2012-10-14 08:16:37 PDT
Comment on attachment 671190 [details] [diff] [review]
Patch backported to ESR10

Review of attachment 671190 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!  ESR is showing that it's dated... :-)
Comment 27 Jeff Walden [:Waldo] (remove +bmo to email) 2012-10-18 16:26:01 PDT
The esr10 mfbt-addition details look fine in the newer patch...modulo mfbt/tests likely not existing there, and the test file itself likely not compiling (due to MOZ_ASSERT use) there.  I'd have omitted the test file, but whatever.  It's certainly not worth backporting the mfbt test hookup stuff to esr10.  :-)
Comment 28 :Ehsan Akhgari (busy, don't ask for review please) 2012-10-18 16:56:52 PDT
Landed the ESR patch without the mfbt test:

https://hg.mozilla.org/releases/mozilla-esr10/rev/a0ae1918be12
Comment 29 Tracy Walker [:tracy] 2014-01-10 10:41:43 PST
mass remove verifyme requests greater than 4 months old

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