Last Comment Bug 726777 - use after free in ~nsXMLHttpRequest
: use after free in ~nsXMLHttpRequest
Status: VERIFIED FIXED
[sg:critical][qa!]
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 11 Branch
: All All
: -- critical (vote)
: mozilla11
Assigned To: Andrew McCreight [:mccr8]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: 718284
  Show dependency treegraph
 
Reported: 2012-02-13 14:09 PST by Andrew McCreight [:mccr8]
Modified: 2013-04-04 13:53 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
+
verified
+
verified
+
verified
unaffected
unaffected


Attachments
don't dispatch from CloseRequestWithError if we've deleted the object (2.58 KB, patch)
2012-02-13 16:38 PST, Andrew McCreight [:mccr8]
jonas: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
continuation: checkin+
Details | Diff | Splinter Review
rebased for beta (2.87 KB, patch)
2012-02-14 07:18 PST, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
rebased for beta (2.88 KB, patch)
2012-02-14 10:00 PST, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
clear flag on abort (1.14 KB, patch)
2012-02-18 13:37 PST, Andrew McCreight [:mccr8]
jonas: review+
continuation: checkin+
Details | Diff | Splinter Review
folded together for aurora (2.63 KB, patch)
2012-02-20 12:30 PST, Andrew McCreight [:mccr8]
continuation: review+
Details | Diff | Splinter Review
folded, rebased for beta (2.63 KB, patch)
2012-02-20 12:30 PST, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review

Description Andrew McCreight [:mccr8] 2012-02-13 14:09:24 PST

    
Comment 1 Olli Pettay [:smaug] 2012-02-13 14:12:45 PST
Some more information, please. Is this about bug 718284?
Comment 2 Andrew McCreight [:mccr8] 2012-02-13 14:12:55 PST
This seems to be the cause of Bug 718284.

Here's the unfortunate sequence of events when you follow the STR from that bug:

enter the destructor for nsXMLHttpRequest 0x12888dc00
create an event 0x100309900 with type 1.
[...time passes...]
cycle collector runs
0x100309900 has a pointer to 0x12888dc00, which was destroyed before it was created, probably in the destructor itself.  Causes a crash.

It isn't clear to me if this is something weird the addon is doing or what.  It does do some kind of thread magic it probably shouldn't be doing.
Comment 3 Andrew McCreight [:mccr8] 2012-02-13 14:21:08 PST
After we enter the destructor for the nsXMLHttpRequest and create the event, we get ref count traffic for the XHR that looks like this:

<nsXMLHttpRequest> 0x12888dc00 1 AddRef 2
nsDOMEventTargetHelper::QueryInterface(nsID const&, void**)+0x000001AD
nsXHREventTarget::QueryInterface(nsID const&, void**)+0x0000004F
nsXMLHttpRequest::QueryInterface(nsID const&, void**)+0x000000D4
nsEventDispatcher::Dispatch(nsISupports*, nsPresContext*, nsEvent*, nsIDOMEvent*, nsEventStatus*, nsDispatchingCallback*, nsCOMArray<nsIDOMEventTarget>*)+0x000000B2
nsEventDispatcher::DispatchDOMEvent(nsISupports*, nsEvent*, nsIDOMEvent*, nsPresContext*, nsEventStatus*)+0x00000350
nsXMLHttpRequest::ChangeState(unsigned int, bool)+0x000001CA
nsXMLHttpRequest::CloseRequestWithError(nsAString_internal const&, unsigned int)+0x000000E2
nsXMLHttpRequest::~nsXMLHttpRequest()+0x00000735

<nsXMLHttpRequest> 0x12888dc00 1 AddRef 2
nsCOMPtr<nsIDOMEventTarget>::operator=(nsIDOMEventTarget*)+0x00000025
nsEventDispatcher::Dispatch(nsISupports*, nsPresContext*, nsEvent*, nsIDOMEvent*, nsEventStatus*, nsDispatchingCallback*, nsCOMArray<nsIDOMEventTarget>*)+0x00000301
nsEventDispatcher::DispatchDOMEvent(nsISupports*, nsEvent*, nsIDOMEvent*, nsPresContext*, nsEventStatus*)+0x00000350
nsXMLHttpRequest::ChangeState(unsigned int, bool)+0x000001CA
nsXMLHttpRequest::CloseRequestWithError(nsAString_internal const&, unsigned int)+0x000000E2
nsXMLHttpRequest::~nsXMLHttpRequest()+0x00000735

I assume that this is a QI and a store to the newly created event.  The refcount doesn't go up, I assume because we've started destroying the object?

Then there's a bunch more ref count traffic with similar stacks.

jst suggested that we could add a flag to the XHR to indicate that the object is being destroyed, and then not do an Abort() if we're being shut down.  Or something like that.
Comment 4 Andrew McCreight [:mccr8] 2012-02-13 14:22:22 PST
I've been testing on Firefox 13, but I've been able to reproduce the original crash on 11 and 12, too, but not 10.
Comment 5 Olli Pettay [:smaug] 2012-02-13 14:34:26 PST
I wonder how we get into this state. Does the addon change ownership of XHR somehow?
Comment 6 Andrew McCreight [:mccr8] 2012-02-13 16:38:14 PST
Created attachment 596851 [details] [diff] [review]
don't dispatch from CloseRequestWithError if we've deleted the object

jst's suggested work around.  With this patch applied, in some light testing the browser did not crash when searching from the Wikipedia bar with https everywhere.
Comment 7 Andrew McCreight [:mccr8] 2012-02-14 07:18:05 PST
Created attachment 597018 [details] [diff] [review]
rebased for beta

Basically the same thing, except that there's no 1<<18 flag, and we check the flag in Abort(), not in whatever Abort() calls in Aurora/Nightly.  I don't think skipping a flag matters, so I left the one I added as 1<<19 for consistency.
Comment 8 Andrew McCreight [:mccr8] 2012-02-14 07:29:59 PST
It isn't clear how exploitable this really is.  It may just be this addon that does something weird, but I have seen a similar set of crashes on html5test.com (that can be hard to reproduce), so it may be triggerable without it.  With the addon, it is 100% reproducable.  This doesn't always cause a crash in the cycle collector.  Occasionally it causes a crash in a finalizer in the GC.
Comment 9 Andrew McCreight [:mccr8] 2012-02-14 10:00:16 PST
Created attachment 597069 [details] [diff] [review]
rebased for beta

I forgot to return a value in Abort().

This looks good on try for m-c, but for some reason every single Android test is failing on Aurora.  I'm hoping it is just some weird infrastructure issue, and retrying it.
Comment 10 Andrew McCreight [:mccr8] 2012-02-14 10:07:18 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/be845a0d6234
Comment 11 Andrew McCreight [:mccr8] 2012-02-14 10:12:06 PST
Comment on attachment 596851 [details] [diff] [review]
don't dispatch from CloseRequestWithError if we've deleted the object

[Approval Request Comment]
Regression caused by (bug #): unknown, something in 11
User impact if declined: common crash with HTTPS everywhere addon, possible security problems 
Testing completed (on m-c, etc.): try runs, landed on mozilla-inbound just now
Risk to taking this patch (and alternatives if risky): low, I guess?  Should only affect nsXMLHttpRequests that are being destroyed.
String changes made by this patch: none
Comment 12 Andrew McCreight [:mccr8] 2012-02-14 11:23:46 PST
I guess an alternative would be blocklisting HTTPS Everywhere, and/or helping them figure out why they trigger this problem.  That would deal with the top crash, but not fix the underlying issue.
Comment 13 Andrew McCreight [:mccr8] 2012-02-15 09:32:14 PST
https://hg.mozilla.org/mozilla-central/rev/be845a0d6234
Comment 14 Andrew McCreight [:mccr8] 2012-02-16 09:26:54 PST
The first nightly this is in is 2/16.  I'll keep an eye on crash stats.  No crashes in the build from the 16th yet. ;)
Comment 15 Andrew McCreight [:mccr8] 2012-02-17 09:40:50 PST
Two NoteXPCOMChild crashes with the 2/16 build so far, none with the signature of this particular crash.
Comment 16 Alex Keybl [:akeybl] 2012-02-17 16:26:51 PST
(In reply to Andrew McCreight [:mccr8] from comment #11)
> Risk to taking this patch (and alternatives if risky): low, I guess?  Should
> only affect nsXMLHttpRequests that are being destroyed.

How can we gain confidence in this risk evaluation? This patch is highly desirable from both the security and crash angles (suspected cause of bug 469267), so we'd really like to take the fix for Beta 4 if we can get consensus on the low-risk nature of the bug.
Comment 17 Olli Pettay [:smaug] 2012-02-17 16:39:02 PST
The patch looks very safe to me.
Comment 18 Andrew McCreight [:mccr8] 2012-02-17 16:43:05 PST
I don't actually know this code, so it will fall on others to really get a good assessment of that.
Comment 19 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-02-17 19:19:14 PST
The one thing I could think of doing to make it safer would be to also clear the XML_HTTP_REQUEST_SYNCLOOPING flag when bailing. That shouldn't be needed but is the only source of risk that I can see.

So maybe do that and then I think it'd be safe for beta/aurora.
Comment 20 Olli Pettay [:smaug] 2012-02-18 02:52:13 PST
nsCOMPtr<nsIStreamListener> listener should keep XHR alive as long as 
XML_HTTP_REQUEST_SYNCLOOPING is set.
Comment 21 Andrew McCreight [:mccr8] 2012-02-18 13:37:48 PST
Created attachment 598575 [details] [diff] [review]
clear flag on abort

Jonas is this what you mean?
Comment 22 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-02-20 02:22:25 PST
Comment on attachment 598575 [details] [diff] [review]
clear flag on abort

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

Yes. As Olli points out, this really shouldn't be needed, but is safer than not. So for landing on beta/aurora we might as well take this too.
Comment 23 Andrew McCreight [:mccr8] 2012-02-20 12:04:38 PST
landed followup https://hg.mozilla.org/integration/mozilla-inbound/rev/b59f89947b54
Comment 24 Andrew McCreight [:mccr8] 2012-02-20 12:08:22 PST
There have been 5 crashes in NoteXPCOMChild in the 3 builds since this patch landed, and none were in nsDOMEvent.  This is compared to 19 crashes just in the build the day before this patch landed.
Comment 25 Andrew McCreight [:mccr8] 2012-02-20 12:30:28 PST
Created attachment 598942 [details] [diff] [review]
folded together for aurora

Carrying forward sicking's r+.
Comment 26 Andrew McCreight [:mccr8] 2012-02-20 12:30:54 PST
Created attachment 598943 [details] [diff] [review]
folded, rebased for beta
Comment 27 Ed Morley [:emorley] 2012-02-21 08:34:02 PST
(In reply to Andrew McCreight [:mccr8] from comment #23)
> landed followup
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b59f89947b54

https://hg.mozilla.org/mozilla-central/rev/b59f89947b54
Comment 28 Alex Keybl [:akeybl] 2012-02-21 09:19:27 PST
Comment on attachment 596851 [details] [diff] [review]
don't dispatch from CloseRequestWithError if we've deleted the object

[Triage Comment]
Considered very safe and resolves top crasher bug 718284. Approved for Aurora 12 and Beta 11. Please land today to make beta 4 of Firefox 11.
Comment 30 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-02-22 11:26:07 PST
qa+ to verify using bug 718284
Comment 31 Jason Smith [:jsmith] 2012-03-07 17:34:40 PST
Verified FF 11 Beta 6 using reproduction steps from bug 718284.
Comment 32 Jason Smith [:jsmith] 2012-03-07 17:38:33 PST
Verified FF 12 Aurora using reproduction steps from bug 718284.
Comment 33 Jason Smith [:jsmith] 2012-03-07 17:44:10 PST
Verified on Nightly.

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