Last Comment Bug 876155 - (CVE-2013-1686) Heap-use-after-free in mozilla::ResetDir
(CVE-2013-1686)
: Heap-use-after-free in mozilla::ResetDir
Status: RESOLVED FIXED
[asan][adv-main22+]
: regression, sec-critical, testcase
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla24
Assigned To: Simon Montagu :smontagu
:
Mentors:
Depends on:
Blocks: DirAuto 900997
  Show dependency treegraph
 
Reported: 2013-05-25 09:58 PDT by Abhishek Arya
Modified: 2014-11-19 19:48 PST (History)
13 users (show)
mats: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
fixed
+
fixed
+
fixed
unaffected
unaffected
unaffected


Attachments
Testcase (853 bytes, text/html)
2013-05-25 09:58 PDT, Abhishek Arya
no flags Details
Patch (1.28 KB, patch)
2013-05-28 20:41 PDT, Simon Montagu :smontagu
ehsan: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
dveditz: sec‑approval+
Details | Diff | Review
Patch part 2 (13.30 KB, patch)
2013-05-30 11:56 PDT, Simon Montagu :smontagu
ehsan: review+
abillings: sec‑approval+
Details | Diff | Review

Description Abhishek Arya 2013-05-25 09:58:58 PDT
Created attachment 754144 [details]
Testcase

Need to install fuzzPriv extension to force gc and need to reload testcase 2-3 times to reproduce.

==14882== ERROR: AddressSanitizer: heap-use-after-free on address 0x60420d3f73ac at pc 0x7fa13585081c bp 0x7fff81577ef0 sp 0x7fff81577ee8
READ of size 4 at 0x60420d3f73ac thread T0
    #0 0x7fa13585081b in nsINode::GetBoolFlag(nsINode::BooleanFlag) const content/base/public/nsINode.h:1359
    #1 0x7fa1392dfbe3 in nsINode::HasTextNodeDirectionalityMap() const ../../../dist/include/nsINode.h:1442
    #2 0x7fa1392de7b9 in mozilla::nsTextNodeDirectionalityMap::RemoveElementFromMap(nsINode*, mozilla::dom::Element*) content/base/src/DirectionalityUtils.cpp:545
    #3 0x7fa1392e5bbf in mozilla::ResetDir(mozilla::dom::Element*) content/base/src/DirectionalityUtils.cpp:996
    #4 0x7fa1398cbf61 in mozilla::dom::Element::UnbindFromTree(bool, bool) content/base/src/Element.cpp:1316
    #5 0x7fa13a750d61 in nsGenericHTMLElement::UnbindFromTree(bool, bool) content/html/content/src/nsGenericHTMLElement.cpp:655
    #6 0x7fa1398cc238 in mozilla::dom::Element::UnbindFromTree(bool, bool) content/base/src/Element.cpp:1329
    #7 0x7fa13a750d61 in nsGenericHTMLElement::UnbindFromTree(bool, bool) content/html/content/src/nsGenericHTMLElement.cpp:655
    #8 0x7fa1398cc238 in mozilla::dom::Element::UnbindFromTree(bool, bool) content/base/src/Element.cpp:1329
    #9 0x7fa13a750d61 in nsGenericHTMLElement::UnbindFromTree(bool, bool) content/html/content/src/nsGenericHTMLElement.cpp:655
    #10 0x7fa13a8ee796 in mozilla::dom::HTMLBodyElement::UnbindFromTree(bool, bool) content/html/content/src/HTMLBodyElement.cpp:357
    #11 0x7fa1398cc238 in mozilla::dom::Element::UnbindFromTree(bool, bool) content/base/src/Element.cpp:1329
    #12 0x7fa13a750d61 in nsGenericHTMLElement::UnbindFromTree(bool, bool) content/html/content/src/nsGenericHTMLElement.cpp:655
    #13 0x7fa13b140125 in mozilla::dom::HTMLSharedElement::UnbindFromTree(bool, bool) content/html/content/src/HTMLSharedElement.cpp:305
    #14 0x7fa13966120f in nsDocument::cycleCollection::UnlinkImpl(void*) content/base/src/nsDocument.cpp:1823
    #15 0x7fa13b5945dc in nsHTMLDocument::cycleCollection::UnlinkImpl(void*) content/html/document/src/nsHTMLDocument.cpp:220
    #16 0x7fa1450dc1be in nsCycleCollector::CollectWhite(nsICycleCollectorListener*) xpcom/base/nsCycleCollector.cpp:2413
    #17 0x7fa1450cc583 in nsCycleCollector::FinishCollection(nsICycleCollectorListener*) xpcom/base/nsCycleCollector.cpp:2876
    #18 0x7fa1450cae49 in nsCycleCollectorRunner::Collect(ccType, nsCycleCollectorResults*, nsICycleCollectorListener*) xpcom/base/nsCycleCollector.cpp:1200
    #19 0x7fa1450e1c80 in nsCycleCollector::Collect(ccType, nsCycleCollectorResults*, nsICycleCollectorListener*) xpcom/base/nsCycleCollector.cpp:2772
    #20 0x7fa1450e7232 in nsCycleCollector_collect(bool, nsCycleCollectorResults*, nsICycleCollectorListener*) xpcom/base/nsCycleCollector.cpp:3088
    #21 0x7fa13c1aff82 in nsJSContext::CycleCollectNow(nsICycleCollectorListener*, int, bool) dom/base/nsJSEnvironment.cpp:2563
    #22 0x7fa13c1ed344 in CCTimerFired(nsITimer*, void*) dom/base/nsJSEnvironment.cpp:2774
    #23 0x7fa145065062 in nsTimerImpl::Fire() xpcom/threads/nsTimerImpl.cpp:547
    #24 0x7fa1450664bd in nsTimerEvent::Run() xpcom/threads/nsTimerImpl.cpp:634
    #25 0x7fa14502bdc7 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:627
    #26 0x7fa144cc9132 in NS_ProcessNextEvent(nsIThread*, bool) objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:238
    #27 0x7fa14139ce24 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:117
    #28 0x7fa1453460eb in MessageLoop::RunInternal() ipc/chromium/src/base/message_loop.cc:219
    #29 0x7fa145345f3e in MessageLoop::RunHandler() ipc/chromium/src/base/message_loop.cc:212
    #30 0x7fa145345e29 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:186
    #31 0x7fa140bf1f8e in nsBaseAppShell::Run() widget/xpwidgets/nsBaseAppShell.cpp:163
    #32 0x7fa13f8be780 in nsAppStartup::Run() toolkit/components/startup/nsAppStartup.cpp:269
    #33 0x7fa13531a0cb in XREMain::XRE_mainRun() toolkit/xre/nsAppRunner.cpp:3872
    #34 0x7fa13531f790 in XREMain::XRE_main(int, char**, nsXREAppData const*) toolkit/xre/nsAppRunner.cpp:3939
    #35 0x7fa135322455 in XRE_main toolkit/xre/nsAppRunner.cpp:4140
    #36 0x42a3f7 in do_main(int, char**, nsIFile*) browser/app/nsBrowserApp.cpp:272
    #37 0x427401 in main browser/app/nsBrowserApp.cpp:632
    #38 0x7fa15748776c in
    #39 0x426b74 in
0x60420d3f73ac is located 44 bytes inside of 120-byte region [0x60420d3f7380,0x60420d3f73f8)
freed by thread T0 here:
    #0 0x41a9e2 in __interceptor_free
    #1 0x7fa1554056de in moz_free memory/mozalloc/mozalloc.cpp:48
    #2 0x7fa139b8b419 in operator delete(void*) ../../../dist/include/mozilla/mozalloc.h:225
    #3 0x7fa139b8b419 in nsTextNode::~nsTextNode() content/base/src/nsTextNode.cpp:94
    #4 0x7fa139a514d5 in nsNodeUtils::LastRelease(nsINode*) content/base/src/nsNodeUtils.cpp:259
    #5 0x7fa13993fb6a in nsGenericDOMDataNode::Release() content/base/src/nsGenericDOMDataNode.cpp:116
    #6 0x7fa139b8c0b5 in nsTextNode::Release() content/base/src/nsTextNode.cpp:97
    #7 0x7fa13ebd6480 in _ZL17DoDeferredReleaseIP11nsISupportsEvR8nsTArrayIT_E js/xpconnect/src/XPCJSRuntime.cpp:616
    #8 0x7fa13ebd5951 in XPCJSRuntime::GCCallback(JSRuntime*, JSGCStatus) js/xpconnect/src/XPCJSRuntime.cpp:816
    #9 0x7fa14ca1161c in Collect(JSRuntime*, bool, long, js::JSGCInvocationKind, JS::gcreason::Reason) js/src/jsgc.cpp:4579
    #10 0x7fa14ca01f14 in js::GC(JSRuntime*, js::JSGCInvocationKind, JS::gcreason::Reason) js/src/jsgc.cpp:4596
    #11 0x7fa14c9761a9 in JS::GCForReason(JSRuntime*, JS::gcreason::Reason) js/src/jsfriendapi.cpp:181
    #12 0x7fa13eae7aae in nsXPCComponents_Utils::ForceGC() js/xpconnect/src/XPCComponents.cpp:4058
    #13 0x7fa14514eb5b in NS_InvokeByIndex xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:162
    #14 0x7fa13ed23d2f in CallMethodHelper::Invoke() js/xpconnect/src/XPCWrappedNative.cpp:2938
    #15 0x7fa13ed23d2f in CallMethodHelper::Call() js/xpconnect/src/XPCWrappedNative.cpp:2273
    #16 0x7fa13ed23d2f in XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) js/xpconnect/src/XPCWrappedNative.cpp:2239
    #17 0x7fa13ed7d1b1 in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1485
    #18 0x7fa14cc5c2df in js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/jscntxtinlines.h:346
    #19 0x7fa14cc5c2df in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/jsinterp.cpp:408
    #20 0x7fa14cc3641f in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode, bool) js/src/jsinterp.cpp:2265
    #21 0x7fa14cbe6843 in js::RunScript(JSContext*, js::StackFrame*) js/src/jsinterp.cpp:365
    #22 0x7fa14cc5c922 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/jsinterp.cpp:421
    #23 0x7fa14cc607ab in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::Value*) js/src/jsinterp.cpp:454
    #24 0x7fa14cf944cd in js::DirectProxyHandler::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) js/src/jsproxy.cpp:479
    #25 0x7fa14d5f4692 in js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) js/src/jswrapper.cpp:445
    #26 0x7fa14cffd93e in js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) js/src/jsproxy.cpp:2611
    #27 0x7fa14d01ae3a in proxy_Call(JSContext*, unsigned int, JS::Value*) js/src/jsproxy.cpp:3175
    #28 0x7fa14cc5bbd2 in js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/jscntxtinlines.h:346
    #29 0x7fa14cc5bbd2 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/jsinterp.cpp:401
    #30 0x7fa14cc3641f in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode, bool) js/src/jsinterp.cpp:2265
    #31 0x7fa14cbe6843 in js::RunScript(JSContext*, js::StackFrame*) js/src/jsinterp.cpp:365
    #32 0x7fa14cc66319 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::AbstractFramePtr, JS::Value*) js/src/jsinterp.cpp:550
    #33 0x7fa14cc67f31 in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) js/src/jsinterp.cpp:589
    #34 0x7fa14c590614 in JS::Evaluate(JSContext*, JS::Handle<JSObject*>, JS::CompileOptions, unsigned short const*, unsigned long, JS::Value*) js/src/jsapi.cpp:5645
previously allocated by thread T0 here:
    #0 0x41aac2 in malloc
    #1 0x7fa155405825 in moz_xmalloc memory/mozalloc/mozalloc.cpp:54
    #2 0x7fa13d54a640 in operator new(unsigned long) ../../dist/include/mozilla/mozalloc.h:201
    #3 0x7fa13d54a640 in nsHtml5TreeOperation::AppendText(unsigned short const*, unsigned int, nsIContent*, nsHtml5TreeOpExecutor*) parser/html/nsHtml5TreeOperation.cpp:167
    #4 0x7fa13d554b88 in nsHtml5TreeOperation::Perform(nsHtml5TreeOpExecutor*, nsIContent**) parser/html/nsHtml5TreeOperation.cpp:459
    #5 0x7fa13d5710f5 in nsHtml5TreeOpExecutor::RunFlushLoop() parser/html/nsHtml5TreeOpExecutor.cpp:557
    #6 0x7fa13d5ab702 in nsHtml5ExecutorFlusher::Run() parser/html/nsHtml5StreamParser.cpp:125
    #7 0x7fa14502bdc7 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:627
    #8 0x7fa144cc9132 in NS_ProcessNextEvent(nsIThread*, bool) objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:238
    #9 0x7fa14139c859 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:82
    #10 0x7fa1453460eb in MessageLoop::RunInternal() ipc/chromium/src/base/message_loop.cc:219
    #11 0x7fa145345f3e in MessageLoop::RunHandler() ipc/chromium/src/base/message_loop.cc:212
    #12 0x7fa145345e29 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:186
    #13 0x7fa140bf1f8e in nsBaseAppShell::Run() widget/xpwidgets/nsBaseAppShell.cpp:163
    #14 0x7fa13f8be780 in nsAppStartup::Run() toolkit/components/startup/nsAppStartup.cpp:269
    #15 0x7fa13531a0cb in XREMain::XRE_mainRun() toolkit/xre/nsAppRunner.cpp:3872
    #16 0x7fa13531f790 in XREMain::XRE_main(int, char**, nsXREAppData const*) toolkit/xre/nsAppRunner.cpp:3939
    #17 0x7fa135322455 in XRE_main toolkit/xre/nsAppRunner.cpp:4140
    #18 0x42a3f7 in do_main(int, char**, nsIFile*) browser/app/nsBrowserApp.cpp:272
    #19 0x427401 in main browser/app/nsBrowserApp.cpp:632
    #20 0x7fa15748776c in
Shadow bytes around the buggy address:
  0x0c08c1a76e20: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c08c1a76e30: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c08c1a76e40: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c08c1a76e50: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c08c1a76e60: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x0c08c1a76e70: fd fd fd fd fd[fd]fd fd fd fd fd fd fd fd fd fa
  0x0c08c1a76e80: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c08c1a76e90: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c08c1a76ea0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c08c1a76eb0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c08c1a76ec0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:     fa
  Heap righ redzone:     fb
  Freed Heap region:     fd
  Stack left redzone:    f1
  Stack mid redzone:     f2
  Stack right redzone:   f3
  Stack partial redzone: f4
  Stack after return:    f5
  Stack use after scope: f8
  Global redzone:        f9
  Global init order:     f6
  Poisoned by user:      f7
  ASan internal:         fe
==14882== ABORTING
Comment 1 Lukas Blakk [:lsblakk] use ?needinfo 2013-05-27 20:43:03 PDT
sec-rating?
Comment 2 :Ehsan Akhgari (busy, don't ask for review please) 2013-05-28 07:14:10 PDT
UAF on an nsINode could mean UAF on its vtable.  => sec-critical.
Comment 3 Simon Montagu :smontagu 2013-05-28 20:41:43 PDT
Created attachment 755167 [details] [diff] [review]
Patch

I'm going to do this in two bites: this patch prevents the UAF and should be backported to branches. A follow-up will stop the assertion happening in this test case.
Comment 4 :Ehsan Akhgari (busy, don't ask for review please) 2013-05-28 21:19:58 PDT
Comment on attachment 755167 [details] [diff] [review]
Patch

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

::: content/base/src/DirectionalityUtils.cpp
@@ +470,5 @@
>  
>    void RemoveEntry(nsINode* aTextNode, Element* aElement)
>    {
> +    MOZ_ASSERT(mElements.Contains(aElement),
> +               "element already removed from map");

Please replace this with NS_ASSERTION, and switch it to MOZ_ASSERT in the other part of the patch.
Comment 6 Ed Morley [:emorley] 2013-05-29 07:36:57 PDT
https://hg.mozilla.org/mozilla-central/rev/ae535a7a3489
Comment 7 Simon Montagu :smontagu 2013-05-30 11:56:12 PDT
Created attachment 756110 [details] [diff] [review]
Patch part 2
Comment 8 :Ehsan Akhgari (busy, don't ask for review please) 2013-05-30 12:08:14 PDT
Comment on attachment 756110 [details] [diff] [review]
Patch part 2

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

::: content/base/src/DirectionalityUtils.cpp
@@ +506,5 @@
> +   * mTextNode the changed text node passed into ResetNodeDirection
> +   * mMustReset when true the dirAutoSetBy property of the element must change;
> +   *            when false it may retain its previous value
> +   */
> +  struct TextNodeForReset {

Make this a MOZ_STACK_CLASS please.
Comment 9 Al Billings [:abillings] 2013-05-30 13:38:53 PDT
The first half of this patch should have had sec-approval+ before landing because it affects branches and is sec-critical rated. See https://wiki.mozilla.org/Security/Bug_Approval_Process. 

Given that this is halfway in now, I'm going to say we should land the second half but I'd like the sec-approval? flag to be set and the form filled out so we can know the implications of this as part of our decision of whether or not to take this on branches (since we evaluate each of those).
Comment 10 Simon Montagu :smontagu 2013-05-30 13:57:52 PDT
Comment on attachment 755167 [details] [diff] [review]
Patch

Sorry, I obviously haven't internalized sec-approval as part of the bug process yet.

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Not at all

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No

Which older supported branches are affected by this flaw? 21, 22, 23

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Yes

How likely is this patch to cause regressions; how much testing does it need? Very unlikely. Existing tests should be sufficient.
Comment 11 Simon Montagu :smontagu 2013-05-30 14:14:27 PDT
Comment on attachment 756110 [details] [diff] [review]
Patch part 2

Not quite sure what to do about the second patch. On the one hand, once the first patch is applied the issue is no longer sec-critical; on the other, since the second patch addresses the original issue much more specifically, the chances of creating an exploit from the patch and checkin comments are much higher.

The chances of regressions are also higher, so I wasn't planning to ask branch approval for the second patch, and maybe it would be better not to check in on trunk until the first patch has been backported and included in release.
Comment 12 Al Billings [:abillings] 2013-05-30 14:17:13 PDT
That's a good question. 

Release management, what do you think?

With the first patch landed, how does the security rating change? What were the effects?
Comment 13 Simon Montagu :smontagu 2013-05-30 14:17:47 PDT
In fact there is a known regression from the second patch in the testcase from bug 815276. The regression is fixed by the w-i-p patch in bug 836925.
Comment 14 Simon Montagu :smontagu 2013-05-30 14:21:49 PDT
(In reply to Al Billings [:abillings] from comment #12)
> With the first patch landed, how does the security rating change? What were
> the effects?

With the first patch landed, the testcase fires the new assertion "element already removed from map", but there is no UAF. (It would be good if someone can confirm this in an asan nightly).
Comment 15 Al Billings [:abillings] 2013-05-30 14:43:56 PDT
Matt can you check this build?
Comment 16 Daniel Veditz [:dveditz] 2013-05-31 11:24:17 PDT
Comment on attachment 755167 [details] [diff] [review]
Patch

This is the one you intend to land on branches, right? sec-approval+ on this one given it's already landed. Hang on to the other one though.
Comment 17 Daniel Veditz [:dveditz] 2013-05-31 11:33:44 PDT
Comment on attachment 756110 [details] [diff] [review]
Patch part 2

That's quite a check-in comment, is it appropriate? It sounds like important explanation that ought to go in the bug or code comments, but as a check-in comment starts pretty invisible (except to check-in watchers, who don't need it) and will eventually fade altogether as the affected lines are touched in the future.

As I understand comments in the bug this patch is intended only for m-c and not branches. Since the comment (whether as a check-in comment or if you move it to the code) points at what's going on we should wait until we're about to ship the minimal fix in Fx22, say June 20 give or take a few days. Please poke us at the security mail address (or IRC) if we've misunderstood the intent and need for this patch and you need to land sooner.
Comment 18 Daniel Veditz [:dveditz] 2013-05-31 11:36:55 PDT
(In reply to Simon Montagu :smontagu from comment #13)
> In fact there is a known regression from the second patch in the testcase
> from bug 815276. The regression is fixed by the w-i-p patch in bug 836925.

So we need to check that one in first, or incorporate that fix into the patch in this bug then?
Comment 19 Matt Wobensmith [:mwobensmith][:matt:] 2013-05-31 14:42:59 PDT
I can repro the UAF in a pre-patch ASan build.

I can confirm the behavior in comment 14 using my own ASan build of today's m-c. I see the assertion, and I cannot generate the UAF that the patch is intended to address.
Comment 20 Al Billings [:abillings] 2013-06-04 10:38:50 PDT
Comment on attachment 756110 [details] [diff] [review]
Patch part 2

sec-approval+ for checkin on trunk after 6/20.
Comment 21 Simon Montagu :smontagu 2013-06-04 23:56:35 PDT
Comment on attachment 755167 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 548206
User impact if declined: exposure to sec-crit vulnerability
Testing completed (on m-c, etc.): baked on m-c since 2013-05-29
Risk to taking this patch (and alternatives if risky): Minimal
String or IDL/UUID changes made by this patch: None
Comment 23 Al Billings [:abillings] 2013-06-06 13:25:39 PDT
Release management, do we want to take part 1 of this on ESR since it is affected?
Comment 24 Simon Montagu :smontagu 2013-06-06 13:48:39 PDT
I think esr17-affected must be a mistake, since dir=auto is only on trunk since ff21.
Comment 25 Al Billings [:abillings] 2013-06-06 13:55:47 PDT
Doh. Good point. Marking.
Comment 26 Mihaela Velimiroviciu (:mihaelav) 2013-06-13 06:28:23 PDT
I tried to verify this bug on latest Firefox 22 beta 5 (non-debug) build, but I could not see any odd behavior when consecutively reloading the testcase attached in bug description, having DOM Fuzzer 2012.07.07 installed. 
Can this bug be verified on non-debug builds? If so, can someone please provide some guidance?
Comment 27 Matt Wobensmith [:mwobensmith][:matt:] 2013-06-17 11:03:13 PDT
Confirmed crash in debug builds Aurora/Beta 2013-05-25.
Confirmed no crash, only assert in ASan debug builds Aurora/Beta 2013-06-14

However, I assume we leave this bug open until the second patch lands.
Comment 28 Matt Wobensmith [:mwobensmith][:matt:] 2013-06-27 13:29:16 PDT
Should we file a new bug for the assert, aka part 2?
Comment 29 Simon Montagu :smontagu 2013-07-01 20:57:58 PDT
Adding dependency to bug 836925, see comment 13
Comment 30 Alex Keybl [:akeybl] 2013-07-23 10:34:59 PDT
Hi Simon, can you prepare a similar patch for ESR17? Thanks!
Comment 31 Simon Montagu :smontagu 2013-07-23 10:47:41 PDT
I don't understand. ESR17 isn't affected by this bug, see comments 23-25. What does "tracking-firefox-esr17: 23+" mean?
Comment 32 Al Billings [:abillings] 2013-07-23 11:09:39 PDT
It means we were expecting this to be fixed in the ESR17 release that goes out at the same time as Firefox 23.

Simon, you reset the flags that I cleared when you made the dependency in comment 29, marking it was ESR affected.
Comment 33 Mats Palmgren (:mats) 2013-07-25 14:19:32 PDT
At this point I think it makes sense to resolve this bug as fixed and create
a new bug for "part 2".  Currently it looks like we have an open sec-critical
when the first patch already fixed that (per comment 11).  Having a separate bug
also helps tracking where and when "part 2" lands.
Comment 34 Mats Palmgren (:mats) 2013-08-02 09:30:57 PDT
I filed bug 900997 for landing "part 2".

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