Last Comment Bug 773207 - (CVE-2012-1973) Heap-use-after-free in nsObjectLoadingContent::LoadObject
(CVE-2012-1973)
: Heap-use-after-free in nsObjectLoadingContent::LoadObject
Status: RESOLVED FIXED
[asan][advisory-tracking+][qa?]
: crash, sec-critical, testcase
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: John Schoenick [:johns]
:
Mentors:
Depends on: 745030
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-12 03:11 PDT by Abhishek Arya
Modified: 2014-07-24 13:43 PDT (History)
13 users (show)
rforbes: sec‑bounty+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
fixed
+
fixed
+
fixed
15+
fixed


Attachments
a guess fix (1.59 KB, patch)
2012-07-12 03:44 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Review
Testcase (crashes after a couple of auto-reloads) (1.62 KB, text/html)
2012-07-12 07:52 PDT, Abhishek Arya
no flags Details
Make mChannel a COMPtr (1.10 KB, patch)
2012-08-07 17:51 PDT, John Schoenick [:johns]
bugs: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑esr10+
Details | Diff | Review

Description Abhishek Arya 2012-07-12 03:11:55 PDT
Reproduces on trunk, testcase coming.

=================================================================
==16419== ERROR: AddressSanitizer heap-use-after-free on address 0x7f51151628d0 at pc 0x7f5137378a53 bp 0x7fff7866bed0 sp 0x7fff7866bec8
READ of size 8 at 0x7f51151628d0 thread T0
    #0 0x7f5137378a53 in nsObjectLoadingContent::LoadObject(nsIURI*, bool, nsCString const&, bool) firefox/src/content/base/src/nsObjectLoadingContent.cpp:1316
    #1 0x7f5137373ece in nsObjectLoadingContent::LoadObject(nsAString_internal const&, bool, nsCString const&, bool) firefox/src/content/base/src/nsObjectLoadingContent.cpp:1250
    #2 0x7f51385729db in nsHTMLSharedObjectElement::StartObjectLoad(bool) firefox/src/content/html/content/src/nsHTMLSharedObjectElement.cpp:482
    #3 0x7f51385772e8 in nsHTMLSharedObjectElement::StartObjectLoad() firefox/src/content/html/content/src/nsHTMLSharedObjectElement.cpp:112
    #4 0x7f5138594c59 in nsRunnableMethodImpl<void (nsHTMLSharedObjectElement::*)(), true>::Run() firefox/src/../../../../dist/include/nsThreadUtils.h:349
    #5 0x7f5136e13965 in nsContentUtils::RemoveScriptBlocker() firefox/src/content/base/src/nsContentUtils.cpp:4888
    #6 0x7f5135c516bd in ~mozAutoDocUpdate firefox/src/../../../../dist/include/mozAutoDocUpdate.h:40
    #7 0x7f5135c1c743 in ~mozAutoDocUpdate firefox/src/../../../../dist/include/mozAutoDocUpdate.h:40
    #8 0x7f51372b2fe8 in nsINode::ReplaceOrInsertBefore(bool, nsINode*, nsINode*) firefox/src/content/base/src/nsINode.cpp:1891
    #9 0x7f51374978f7 in nsINode::ReplaceOrInsertBefore(bool, nsINode*, nsINode*, unsigned int*) firefox/src/../../../../dist/include/nsINode.h:1462
    #10 0x7f51374959d9 in nsINode::InsertBefore(nsINode*, nsINode*, unsigned int*) firefox/src/../../../../dist/include/nsINode.h:494
    #11 0x7f5138073ccb in nsINode::AppendChild(nsINode*, unsigned int*) firefox/src/../../../../dist/include/nsINode.h:504
    #12 0x7f513c101907 in nsIDOMNode_AppendChild(JSContext*, unsigned int, JS::Value*) firefox/src/objdir-ff-asan-sym/js/xpconnect/src/dom_quickstubs.cpp:5400
    #13 0x7f51471ac507 in js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) firefox/src/js/src/jscntxtinlines.h:385
    #14 0x7f5147123006 in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) firefox/src/js/src/jsinterp.cpp:2442
    #15 0x7f51470aa276 in js::RunScript(JSContext*, JSScript*, js::StackFrame*) firefox/src/js/src/jsinterp.cpp:301
    #16 0x7f51471b937d in js::ExecuteKernel(JSContext*, JSScript*, JSObject&, JS::Value const&, js::ExecuteType, js::StackFrame*, JS::Value*) firefox/src/js/src/jsinterp.cpp:487
    #17 0x7f51471bb080 in js::Execute(JSContext*, JSScript*, JSObject&, JS::Value*) firefox/src/js/src/jsinterp.cpp:524
    #18 0x7f51469b1081 in EvaluateUCScriptForPrincipalsCommon(JSContext*, JSObject*, JSPrincipals*, JSPrincipals*, unsigned short const*, unsigned int, char const*, unsigned int, JS::Value*, JSVersion) firefox/src/js/src/jsapi.cpp:5373
    #19 0x7f51469b2fbc in JS_EvaluateUCScriptForPrincipalsVersionOrigin firefox/src/js/src/jsapi.cpp:5410
    #20 0x7f513914479f in nsJSContext::EvaluateString(nsAString_internal const&, JSObject*, nsIPrincipal*, nsIPrincipal*, char const*, unsigned int, JSVersion, nsAString_internal*, bool*) firefox/src/dom/base/nsJSEnvironment.cpp:1466
    #21 0x7f51392f3c9e in nsGlobalWindow::RunTimeoutHandler(nsTimeout*, nsIScriptContext*) firefox/src/dom/base/nsGlobalWindow.cpp:9527
    #22 0x7f51392a9d32 in nsGlobalWindow::RunTimeout(nsTimeout*) firefox/src/dom/base/nsGlobalWindow.cpp:9791
    #23 0x7f51392f1eab in nsGlobalWindow::TimerCallback(nsITimer*, void*) firefox/src/dom/base/nsGlobalWindow.cpp:10063
    #24 0x7f5140707272 in nsTimerImpl::Fire() firefox/src/xpcom/threads/nsTimerImpl.cpp:474
    #25 0x7f5140708eac in nsTimerEvent::Run() firefox/src/xpcom/threads/nsTimerImpl.cpp:558
    #26 0x7f51406cb4fd in nsThread::ProcessNextEvent(bool, bool*) firefox/src/xpcom/threads/nsThread.cpp:625
    #27 0x7f514035a30d in NS_ProcessNextEvent_P(nsIThread*, bool) firefox/src/objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:217
    #28 0x7f513f3fb366 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) firefox/src/ipc/glue/MessagePump.cpp:82
    #29 0x7f514097f46a in MessageLoop::RunInternal() firefox/src/ipc/chromium/src/base/message_loop.cc:209
    #30 0x7f514097f2b3 in MessageLoop::RunHandler() firefox/src/ipc/chromium/src/base/message_loop.cc:202
    #31 0x7f514097f198 in MessageLoop::Run() firefox/src/ipc/chromium/src/base/message_loop.cc:176
    #32 0x7f513e9321ee in nsBaseAppShell::Run() firefox/src/widget/xpwidgets/nsBaseAppShell.cpp:165
    #33 0x7f513d585a28 in nsAppStartup::Run() firefox/src/toolkit/components/startup/nsAppStartup.cpp:257
    #34 0x7f5133d63830 in XREMain::XRE_mainRun() firefox/src/toolkit/xre/nsAppRunner.cpp:3787
    #35 0x7f5133d6a1d2 in XREMain::XRE_main(int, char**, nsXREAppData const*) firefox/src/toolkit/xre/nsAppRunner.cpp:3864
    #36 0x7f5133d6d6a2 in XRE_main firefox/src/toolkit/xre/nsAppRunner.cpp:3940
    #37 0x40c29f in do_main(int, char**) firefox/src/browser/app/nsBrowserApp.cpp:160
    #38 0x409ccd in main firefox/src/browser/app/nsBrowserApp.cpp:298
    #39 0x7f51504e8c4d in ?? ??:0
0x7f51151628d0 is located 80 bytes inside of 1112-byte region [0x7f5115162880,0x7f5115162cd8)
freed by thread T0 here:
    #0 0x4a43a2 in free ??:0
    #1 0x7f514d3745d3 in moz_free firefox/src/memory/mozalloc/mozalloc.cpp:49
    #2 0x7f513470e4f6 in ~nsHttpChannel firefox/src/netwerk/protocol/http/nsHttpChannel.cpp:332
    #3 0x7f5140440702 in nsHashPropertyBag::Release() firefox/src/xpcom/ds/nsHashPropertyBag.cpp:40
    #4 0x7f51346da3d4 in mozilla::net::HttpBaseChannel::Release() firefox/src/netwerk/protocol/http/HttpBaseChannel.cpp:143
    #5 0x7f51347788b4 in mozilla::net::nsHttpChannel::Release() firefox/src/netwerk/protocol/http/nsHttpChannel.cpp:4242
    #6 0x7f51347789ec in non-virtual thunk to mozilla::net::nsHttpChannel::Release() firefox/src/modules/zlib/src/inffast.c:0
    #7 0x7f5133e87001 in nsCOMPtr_base::assign_assuming_AddRef(nsISupports*) firefox/src/../../dist/include/nsCOMPtr.h:440
    #8 0x7f5140310e44 in nsCOMPtr_base::assign_with_AddRef(nsISupports*) firefox/src/objdir-ff-asan-sym/xpcom/build/nsCOMPtr.cpp:50
    #9 0x7f5133e7d563 in nsCOMPtr<nsIChannel>::operator=(nsIChannel*) firefox/src/../../../../dist/include/nsCOMPtr.h:624
    #10 0x7f513470a0f6 in mozilla::net::AutoRedirectVetoNotifier::ReportRedirectResult(bool) firefox/src/netwerk/protocol/http/nsHttpChannel.cpp:178
    #11 0x7f51347c5068 in ~AutoRedirectVetoNotifier firefox/src/netwerk/protocol/http/nsHttpChannel.cpp:162
    #12 0x7f513474a9c3 in ~AutoRedirectVetoNotifier firefox/src/netwerk/protocol/http/nsHttpChannel.cpp:162
    #13 0x7f51347774ff in mozilla::net::nsHttpChannel::ContinueProcessRedirection(unsigned int) firefox/src/netwerk/protocol/http/nsHttpChannel.cpp:4195
    #14 0x7f51347a8e76 in mozilla::net::nsHttpChannel::OnRedirectVerifyCallback(unsigned int) firefox/src/netwerk/protocol/http/nsHttpChannel.cpp:5782
    #15 0x7f51347a9ad2 in non-virtual thunk to mozilla::net::nsHttpChannel::OnRedirectVerifyCallback(unsigned int) firefox/src/modules/zlib/src/inffast.c:0
    #16 0x7f5133e87ca4 in nsAsyncVerifyRedirectCallbackEvent::Run() firefox/src/netwerk/base/src/nsAsyncRedirectVerifyHelper.cpp:44
    #17 0x7f51406cb4fd in nsThread::ProcessNextEvent(bool, bool*) firefox/src/xpcom/threads/nsThread.cpp:625
    #18 0x7f514035a30d in NS_ProcessNextEvent_P(nsIThread*, bool) firefox/src/objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:217
    #19 0x7f513f3fb366 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) firefox/src/ipc/glue/MessagePump.cpp:82
    #20 0x7f514097f46a in MessageLoop::RunInternal() firefox/src/ipc/chromium/src/base/message_loop.cc:209
    #21 0x7f514097f2b3 in MessageLoop::RunHandler() firefox/src/ipc/chromium/src/base/message_loop.cc:202
    #22 0x7f514097f198 in MessageLoop::Run() firefox/src/ipc/chromium/src/base/message_loop.cc:176
    #23 0x7f513e9321ee in nsBaseAppShell::Run() firefox/src/widget/xpwidgets/nsBaseAppShell.cpp:165
    #24 0x7f513d585a28 in nsAppStartup::Run() firefox/src/toolkit/components/startup/nsAppStartup.cpp:257
    #25 0x7f5133d63830 in XREMain::XRE_mainRun() firefox/src/toolkit/xre/nsAppRunner.cpp:3787
    #26 0x7f5133d6a1d2 in XREMain::XRE_main(int, char**, nsXREAppData const*) firefox/src/toolkit/xre/nsAppRunner.cpp:3864
    #27 0x7f5133d6d6a2 in XRE_main firefox/src/toolkit/xre/nsAppRunner.cpp:3940
    #28 0x40c29f in do_main(int, char**) firefox/src/browser/app/nsBrowserApp.cpp:160
    #29 0x409ccd in main firefox/src/browser/app/nsBrowserApp.cpp:298
previously allocated by thread T0 here:
    #0 0x4a4462 in __interceptor_malloc ??:0
    #1 0x7f514d374727 in moz_xmalloc firefox/src/memory/mozalloc/mozalloc.cpp:54
    #2 0x7f51346c8bd9 in nsHttpHandler::NewProxiedChannel(nsIURI*, nsIProxyInfo*, nsIChannel**) firefox/src/netwerk/protocol/http/nsHttpHandler.cpp:1437
    #3 0x7f51346c7acb in nsHttpHandler::NewChannel(nsIURI*, nsIChannel**) firefox/src/netwerk/protocol/http/nsHttpHandler.cpp:1398
    #4 0x7f5133f6c1eb in nsIOService::NewChannelFromURIWithProxyFlags(nsIURI*, nsIURI*, unsigned int, nsIChannel**) firefox/src/netwerk/base/src/nsIOService.cpp:657
    #5 0x7f5133f6ada0 in nsIOService::NewChannelFromURI(nsIURI*, nsIChannel**) firefox/src/netwerk/base/src/nsIOService.cpp:576
    #6 0x7f51347757e2 in mozilla::net::nsHttpChannel::ContinueProcessRedirectionAfterFallback(unsigned int) firefox/src/netwerk/protocol/http/nsHttpChannel.cpp:4120
    #7 0x7f5134723559 in mozilla::net::nsHttpChannel::AsyncProcessRedirection(unsigned int) firefox/src/netwerk/protocol/http/nsHttpChannel.cpp:4052
    #8 0x7f5134734a7c in mozilla::net::nsHttpChannel::ProcessResponse() firefox/src/netwerk/protocol/http/nsHttpChannel.cpp:1264
    #9 0x7f513478d8bc in mozilla::net::nsHttpChannel::OnStartRequest(nsIRequest*, nsISupports*) firefox/src/netwerk/protocol/http/nsHttpChannel.cpp:4802
    #10 0x7f513478ee97 in non-virtual thunk to mozilla::net::nsHttpChannel::OnStartRequest(nsIRequest*, nsISupports*) firefox/src/modules/zlib/src/inffast.c:0
    #11 0x7f5133f3f162 in nsInputStreamPump::OnStateStart() firefox/src/netwerk/base/src/nsInputStreamPump.cpp:416
    #12 0x7f5133f3e30d in nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) firefox/src/netwerk/base/src/nsInputStreamPump.cpp:367
    #13 0x7f5133f41c2f in non-virtual thunk to nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) firefox/src/modules/zlib/src/inffast.c:0
    #14 0x7f51405b72ee in nsInputStreamReadyEvent::Run() firefox/src/xpcom/io/nsStreamUtils.cpp:82
    #15 0x7f51406cb4fd in nsThread::ProcessNextEvent(bool, bool*) firefox/src/xpcom/threads/nsThread.cpp:625
    #16 0x7f514035a30d in NS_ProcessNextEvent_P(nsIThread*, bool) firefox/src/objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:217
    #17 0x7f513f3fb366 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) firefox/src/ipc/glue/MessagePump.cpp:82
    #18 0x7f514097f46a in MessageLoop::RunInternal() firefox/src/ipc/chromium/src/base/message_loop.cc:209
    #19 0x7f514097f2b3 in MessageLoop::RunHandler() firefox/src/ipc/chromium/src/base/message_loop.cc:202
    #20 0x7f514097f198 in MessageLoop::Run() firefox/src/ipc/chromium/src/base/message_loop.cc:176
    #21 0x7f513e9321ee in nsBaseAppShell::Run() firefox/src/widget/xpwidgets/nsBaseAppShell.cpp:165
    #22 0x7f513d585a28 in nsAppStartup::Run() firefox/src/toolkit/components/startup/nsAppStartup.cpp:257
    #23 0x7f5133d63830 in XREMain::XRE_mainRun() firefox/src/toolkit/xre/nsAppRunner.cpp:3787
    #24 0x7f5133d6a1d2 in XREMain::XRE_main(int, char**, nsXREAppData const*) firefox/src/toolkit/xre/nsAppRunner.cpp:3864
==16419== ABORTING
Stats: 340M malloced (374M for red zones) by 855014 calls
Stats: 92M realloced by 60631 calls
Stats: 302M freed by 692995 calls
Stats: 180M really freed by 372279 calls
Stats: 480M (122951 full pages) mmaped in 120 calls
  mmaps   by size class: 8:425958; 9:57337; 10:24570; 11:20470; 12:6144; 13:4608; 14:1792; 15:384; 16:640; 17:160; 18:336; 19:48; 20:16;
  mallocs by size class: 8:671876; 9:92732; 10:38267; 11:31549; 12:8574; 13:6870; 14:2771; 15:720; 16:804; 17:284; 18:490; 19:62; 20:15;
  frees   by size class: 8:533367; 9:79542; 10:33885; 11:27984; 12:7349; 13:6211; 14:2532; 15:641; 16:706; 17:264; 18:443; 19:59; 20:12;
  rfrees  by size class: 8:274158; 9:50191; 10:20177; 11:18120; 12:3448; 13:3069; 14:1711; 15:414; 16:562; 17:161; 18:216; 19:42; 20:10;
Stats: malloc large: 851 small slow: 4152
Shadow byte and word:
  0x1fea22a2c51a: fd
  0x1fea22a2c518: fd fd fd fd fd fd fd fd
More shadow bytes:
  0x1fea22a2c4f8: fa fa fa fa fa fa fa fa
  0x1fea22a2c500: fa fa fa fa fa fa fa fa
  0x1fea22a2c508: fa fa fa fa fa fa fa fa
  0x1fea22a2c510: fd fd fd fd fd fd fd fd
=>0x1fea22a2c518: fd fd fd fd fd fd fd fd
  0x1fea22a2c520: fd fd fd fd fd fd fd fd
  0x1fea22a2c528: fd fd fd fd fd fd fd fd
  0x1fea22a2c530: fd fd fd fd fd fd fd fd
  0x1fea22a2c538: fd fd fd fd fd fd fd fd
Comment 1 Olli Pettay [:smaug] 2012-07-12 03:21:42 PDT
Biesi, why is mChannel a raw ref?
Comment 2 Olli Pettay [:smaug] 2012-07-12 03:32:45 PDT
(Without seeing the testcase)
Is something like this possible: First a load is initiated and
mChannel points to some object. Then another load is initiate so we enter to
mChannel->Cancel(NS_BINDING_ABORTED);
if (mFinalListener) {
  mFinalListener->OnStopRequest(mChannel, nsnull, NS_BINDING_ABORTED);
  mFinalListener = nsnull;
}

Calling mFinalListener->OnStopRequest() releases the last ref to mChannel and ends up calling
LoadObject again. mChannel would point to a deleted object in that case.
Comment 3 Olli Pettay [:smaug] 2012-07-12 03:44:32 PDT
Created attachment 641420 [details] [diff] [review]
a guess fix
Comment 4 Abhishek Arya 2012-07-12 07:52:06 PDT
Created attachment 641463 [details]
Testcase (crashes after a couple of auto-reloads)
Comment 5 Olli Pettay [:smaug] 2012-07-12 08:08:29 PDT
(In reply to Abhishek Arya from comment #4)
> Created attachment 641463 [details]
> Testcase (crashes after a couple of auto-reloads)

Can't reproduce here (with or without the patch).
Comment 6 Abhishek Arya 2012-07-12 09:18:20 PDT
are you testing on an asanified build or with valgrind ? 

I can check your patch later in the evening.
Comment 7 Olli Pettay [:smaug] 2012-07-12 09:29:48 PDT
Oh, I thought it would crash in a normal build (since it really should).
Comment 8 Daniel Veditz [:dveditz] 2012-07-12 13:43:40 PDT
Is this really component Plug-ins if the code fix is in content/base ?
Comment 9 Abhishek Arya 2012-07-12 17:53:50 PDT
Olli, your patch does not fix the Use-after-free. One of the reason i see it is you ref it in nsObjectLoadingContent, whereas the asan free stack does not have any stack frame with nsObjectLoadingContent in it. So, basically looks like you are trying to protect an already freed object. Refptring mChannel in the nsObjectLoadingContent.h header file does fix the crash [same question you asked in c#1), although i don't know if it is the right functional fix (since the comment in header file says explicitly about weak reference and someone needs to see if there are no ref cycles :)

diff -r f9499238bd4b content/base/src/nsObjectLoadingContent.h
--- a/content/base/src/nsObjectLoadingContent.h	Thu Jul 12 17:28:05 2012 +0100
+++ b/content/base/src/nsObjectLoadingContent.h	Thu Jul 12 17:52:42 2012 -0700
@@ -348,7 +348,7 @@
      * The channel that's currently being loaded. This is a weak reference.
      * Non-null between asyncOpen and onStopRequest.
      */
-    nsIChannel*                 mChannel;
+    nsCOMPtr<nsIChannel>                 mChannel;
 
     // The data we were last asked to load
     nsCOMPtr<nsIURI>            mURI;
Comment 10 John Schoenick [:johns] 2012-07-12 18:55:57 PDT
In our refactoring in bug 745030 this is turned into a COMPtr, so that should not be an issue, though why mChannel apparently goes away without OnStopRequest() being called is odd - it seems it is being killed in OnRedirectVerifyCallback -- We may need another listener in this class to know the load was aborted.
Comment 11 Abhishek Arya 2012-07-12 19:21:36 PDT
(In reply to John Schoenick [:johns] from comment #10)
> In our refactoring in bug 745030 this is turned into a COMPtr, so that
> should not be an issue, though why mChannel apparently goes away without
> OnStopRequest() being called is odd - it seems it is being killed in
> OnRedirectVerifyCallback -- We may need another listener in this class to
> know the load was aborted.

Refactorings are usually hard to merge back. Probably just the one-line COMPtr change is easy enough to merge, in case you guys decide to uptake the fix to old branches.
Comment 12 John Schoenick [:johns] 2012-07-12 19:56:11 PDT
(In reply to Abhishek Arya from comment #11)
> Refactorings are usually hard to merge back. Probably just the one-line
> COMPtr change is easy enough to merge, in case you guys decide to uptake the
> fix to old branches.

Agreed, I was just saying that this change is sane as far as the class goes, there is no necessity to the weak pointer.

As for the root of the issue, it looks like we're not checking for OnRedirectVerifyCallback failing in our AsyncOnChannelRedirect hook, which can result in the channel going away. I'm testing a patch right now
Comment 13 Olli Pettay [:smaug] 2012-07-13 02:26:33 PDT
(I need to file a new bug for my patch, since it is fixing, I believe, an sg* bug.)
Comment 14 John Schoenick [:johns] 2012-07-13 17:01:56 PDT
So my patch did not fix this issue either. What is happening is we're aborting the load of the channel at the right moment - after AsyncOnChannelRedirect, but before the *new* channel has reached AsyncOpen() - thus OnStart/StopRequest is *not* guaranteed to be called. The new channel simply goes away after OnRedirect, and we have a dead reference.

As for why the channel goes away, I think it's because we're aborting loading of the page, but we still manage to reach LoadObject

It's not clear to me if this is a bug in the channel handling code (should OnChannelRedirect guarantee the new channel will at least reach open?), or if it is something we need to account for - in which case simply making mChannel a COMPtr resolves it.
Comment 15 Daniel Veditz [:dveditz] 2012-08-02 13:35:58 PDT
going out on a limb and guessing ESR-10 is affected because our channel loading code is mostly old. But there have been more recent perturbations so we'd really need to test.
Comment 16 John Schoenick [:johns] 2012-08-07 17:51:48 PDT
Created attachment 649902 [details] [diff] [review]
Make mChannel a COMPtr

Bug 745030 is on central now and should fix this, but making mChannel a COMPtr on older
branches should be sufficient to fix it there. I also need to file a follow-up on this
channel behavior so we can look into whether its intended
Comment 17 Olli Pettay [:smaug] 2012-08-08 03:38:11 PDT
Comment on attachment 649902 [details] [diff] [review]
Make mChannel a COMPtr

If necko behaves even closely sanely this shouldn't leak :)
Comment 18 John Schoenick [:johns] 2012-08-08 12:07:48 PDT
Comment on attachment 649902 [details] [diff] [review]
Make mChannel a COMPtr

[Approval Request Comment]
User impact if declined: sec-critical issue
Testing completed (on m-c, etc.): Bug 745030 on m-c includes this fix
Risk to taking this patch (and alternatives if risky): Low, this pointer is already carefully managed as a weak ref in current code, making it a COMPtr just catches a rare destruction race.
String or UUID changes made by this patch: None
Comment 19 Andrew McCreight [:mccr8] 2012-08-09 13:10:21 PDT
Marking as fixed because it is fixed on trunk.
Comment 20 John Schoenick [:johns] 2012-08-09 13:29:06 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/9155a5c01de6
Comment 22 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-14 15:24:13 PDT
Does this need a test in-testsuite?
Comment 23 John Schoenick [:johns] 2012-08-14 16:17:01 PDT
I don't believe so, this is caused by a specific case of pointer mishandling, and is not really something that could be otherwise reintroduced or generically tested for (unless olli has any ideas)
Comment 24 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-14 17:11:30 PDT
(In reply to John Schoenick [:johns] from comment #23)
> I don't believe so, this is caused by a specific case of pointer
> mishandling, and is not really something that could be otherwise
> reintroduced or generically tested for (unless olli has any ideas)

Okay, thanks. Will try to verify this manually with the attached testcase.
Comment 25 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-23 15:42:05 PDT
I'm seeing a crash using the attached testcase with decoder's ASan build from here:
http://people.mozilla.org/~choller/firefox/asan/20120823-mozilla-central-linux64-debug-198ca6edd0ae+asan.html

Can someone confirm is this is related or impacts verification?

==5742== ERROR: AddressSanitizer attempting free on address which was not malloc()-ed: 0x7f9543ab4160
    #0 0x430101 (/home/ashughes/Development/asan/17.0a1-20120823/firefox/firefox+0x430101)
    #1 0x7f950fbc0d2e (/usr/lib/jvm/java-6-openjdk-amd64/jre/lib/amd64/IcedTeaPlugin.so+0xdd2e)
Stats: 513M malloced (526M for red zones) by 899694 calls
Stats: 61M realloced by 51761 calls
Stats: 467M freed by 609223 calls
Stats: 327M really freed by 412074 calls
Stats: 656M (168050 full pages) mmaped in 163 calls
  mmaps   by size class: 8:409575; 9:65528; 10:32760; 11:16376; 12:9216; 13:3072; 14:3072; 15:640; 16:512; 17:1248; 18:272; 19:64; 20:20; 21:2; 22:3; 23:1; 
  mallocs by size class: 8:670990; 9:102813; 10:62582; 11:34870; 12:12767; 13:5973; 14:5015; 15:1173; 16:1174; 17:1829; 18:406; 19:76; 20:20; 21:2; 22:3; 23:1; 
  frees   by size class: 8:413727; 9:83230; 10:55667; 11:30950; 12:11030; 13:5518; 14:4696; 15:1098; 16:1037; 17:1809; 18:363; 19:74; 20:19; 21:1; 22:3; 23:1; 
  rfrees  by size class: 8:278209; 9:55703; 10:39451; 11:24967; 12:4845; 13:3275; 14:2215; 15:659; 16:834; 17:1694; 18:143; 19:59; 20:16; 22:3; 23:1; 
Stats: malloc large: 2338 small slow: 5147
Comment 26 John Schoenick [:johns] 2012-08-23 16:03:27 PDT
Does this happen when using oracle's jre6 instead of openjdk/icedtea? I believe openjdk java on linux will always issue a bogus free() on unload, including with --enable-trace-malloc, which is not an issue in our code AFAIK
Comment 27 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-23 16:14:15 PDT
(In reply to John Schoenick [:johns] from comment #26)
> Does this happen when using oracle's jre6 instead of openjdk/icedtea? I
> believe openjdk java on linux will always issue a bogus free() on unload,
> including with --enable-trace-malloc, which is not an issue in our code AFAIK

I'm not sure, and I can't check. For some reason I can't get Oracle JRE to register as a plugin in Firefox.
Comment 28 John Schoenick [:johns] 2012-08-23 17:20:25 PDT
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #27)
> (In reply to John Schoenick [:johns] from comment #26)
> > Does this happen when using oracle's jre6 instead of openjdk/icedtea? I
> > believe openjdk java on linux will always issue a bogus free() on unload,
> > including with --enable-trace-malloc, which is not an issue in our code AFAIK
> 
> I'm not sure, and I can't check. For some reason I can't get Oracle JRE to
> register as a plugin in Firefox.

I just tried this with the ASAN build linked and oracle JRE and don't get this error or the original one, but I can reproduce this exact error with openjdk/icedtea, so I'm fairly confident that this is an unrelated bug in openjdk
Comment 29 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-24 10:34:16 PDT
Purging all JRE from my system I don't get the error reported in comment 25 any longer. However, I'm still unable to reproduce this bug using an ASan build decoder created for me based on http://hg.mozilla.org/try/rev/9f3cc040e41a. I won't be able to verify this fixed until I or someone else can make a build based on a changeset from mozilla-central 2012-07-12.
Comment 30 Daniel Veditz [:dveditz] 2012-10-22 09:38:08 PDT
Created attachment 673897 [details]
Bug Bounty Awarded $3000 [paid]

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