Last Comment Bug 777806 - XHR use-after-free of JS
: XHR use-after-free of JS
Status: VERIFIED FIXED
[advisory-tracking+]
: crash, sec-critical
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Andrew McCreight [:mccr8]
: Anthony Hughes (:ashughes) [GFX][QA][Mentor]
Mentors:
Depends on:
Blocks: 778179
  Show dependency treegraph
 
Reported: 2012-07-26 11:16 PDT by John Schoenick [:johns]
Modified: 2012-10-21 22:15 PDT (History)
12 users (show)
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
verified
+
verified
+
verified
15+
verified


Attachments
Test change that triggers the crash (4.41 KB, patch)
2012-07-26 11:16 PDT, John Schoenick [:johns]
no flags Details | Diff | Splinter Review
slightly reduced (5.75 KB, patch)
2012-07-29 18:40 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
root moar, bz's idea (1.01 KB, patch)
2012-07-31 13:53 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
part 1: root in CreateResponseParsedJSON (892 bytes, patch)
2012-08-02 11:09 PDT, Andrew McCreight [:mccr8]
bent.mozilla: review+
Details | Diff | Splinter Review
test case (2.46 KB, patch)
2012-08-02 11:11 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
part 2: fix the name of the rooting function (2.96 KB, patch)
2012-08-03 13:33 PDT, Andrew McCreight [:mccr8]
bzbarsky: review+
Details | Diff | Splinter Review
part 1+2 folded for backporting (2.90 KB, patch)
2012-08-05 08:46 PDT, Andrew McCreight [:mccr8]
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑esr10+
Details | Diff | Splinter Review
non-mochitest test, file 1 (14 bytes, text/plain)
2012-08-28 11:07 PDT, Andrew McCreight [:mccr8]
no flags Details
non-mochitest test, file 2 (1.37 KB, text/html)
2012-08-28 11:09 PDT, Andrew McCreight [:mccr8]
no flags Details

Description John Schoenick [:johns] 2012-07-26 11:16:27 PDT
Created attachment 646222 [details] [diff] [review]
Test change that triggers the crash

When we landed the attached tweak to a test in 771666, this random orange started occuring - it doesn't seem to happen in ASAN builds, but seems to be 100% reproducible if you run the test *twice* in one session with EXTRA_TEST_ARGS="--repeat=1"

Assertion failure: allocated(), at ../../../dist/include/gc/Heap.h:497

Program received signal SIGSEGV, Segmentation fault.
js::gc::ArenaHeader::getAllocKind (this=0x7fffc645c000) at ../../../dist/include/gc/Heap.h:497
497             JS_ASSERT(allocated());
(gdb) bt
#0  js::gc::ArenaHeader::getAllocKind (this=0x7fffc645c000) at ../../../dist/include/gc/Heap.h:497
#1  0x00007ffff4cbc40d in js::gc::Cell::getAllocKind (this=0x7fffc645c020) at /home/johns/moz/moz-git/js/src/gc/Heap.h:935
#2  0x00007ffff4d415ad in js::gc::GetGCThingTraceKind (thing=0x7fffc645c020) at /home/johns/moz/moz-git/js/src/jsgcinlines.h:30
#3  0x00007ffff4d538d5 in js_GetGCThingTraceKind (thing=0x7fffc645c020) at /home/johns/moz/moz-git/js/src/jsgc.cpp:1760
#4  0x00007ffff36905b5 in xpc_GCThingIsGrayCCThing (thing=0x7fffc645c020)
    at /home/johns/moz/moz-git/js/xpconnect/src/nsXPConnect.cpp:574
#5  0x00007ffff41f551f in ChildFinder::NoteJSChild (this=0x7fffffffaeb0, child=0x7fffc645c020)
    at /home/johns/moz/moz-git/xpcom/base/nsCycleCollector.cpp:1991
#6  0x00007ffff414b8b8 in nsScriptObjectTracer::NoteJSChild (aScriptThing=0x7fffc645c020, name=0x7ffff52f0967 "mResultJSON", 
    aClosure=0x7fffffffaeb0) at /home/johns/moz/ff-dbg/xpcom/build/nsCycleCollectionParticipant.cpp:16
#7  0x00007ffff29ef110 in nsXMLHttpRequest::cycleCollection::TraceImpl(void*, void (*)(void*, char const*, void*), void*) ()
   from /home/johns/moz/ff-dbg/dist/bin/libxul.so
#8  0x00007ffff2b8c91f in nsDOMEventTargetHelper::cycleCollection::TraverseImpl(nsDOMEventTargetHelper::cycleCollection*, void*, nsCycleCollectionTraversalCallback&) () from /home/johns/moz/ff-dbg/dist/bin/libxul.so
#9  0x00007ffff29eb853 in nsXHREventTarget::cycleCollection::TraverseImpl(nsXHREventTarget::cycleCollection*, void*, nsCycleCollectionTraversalCallback&) () from /home/johns/moz/ff-dbg/dist/bin/libxul.so
#10 0x00007ffff29ee4d3 in nsXMLHttpRequest::cycleCollection::TraverseImpl(nsXMLHttpRequest::cycleCollection*, void*, nsCycleCollectionTraversalCallback&) () from /home/johns/moz/ff-dbg/dist/bin/libxul.so
#11 0x00007ffff28a4afe in nsCycleCollectionParticipant::Traverse (this=0x7ffff626e7f8, p=0x7fffc133dc00, cb=...)
    at ../../../dist/include/nsCycleCollectionParticipant.h:262
#12 0x00007ffff41f56cb in MayHaveChild (o=0x7fffc133dc00, cp=0x7ffff626e7f8)
    at /home/johns/moz/moz-git/xpcom/base/nsCycleCollector.cpp:2022
#13 0x00007ffff41f5625 in nsPurpleBuffer::RemoveSkippable (this=0x7fffe91b0090, removeChildlessNodes=true)
    at /home/johns/moz/moz-git/xpcom/base/nsCycleCollector.cpp:2042
#14 0x00007ffff41f57d0 in nsCycleCollector::ForgetSkippable (this=0x7fffe91b0000, removeChildlessNodes=true)
    at /home/johns/moz/moz-git/xpcom/base/nsCycleCollector.cpp:2066
#15 0x00007ffff41f818e in nsCycleCollector_forgetSkippable (aRemoveChildlessNodes=true)
    at /home/johns/moz/moz-git/xpcom/base/nsCycleCollector.cpp:3217
#16 0x00007ffff2ee544a in FireForgetSkippable (aSuspected=496, aRemoveChildless=true)
    at /home/johns/moz/moz-git/dom/base/nsJSEnvironment.cpp:3033
#17 0x00007ffff2ee5b5b in CCTimerFired (aTimer=0x7fffc6296dc0, aClosure=0x0)
    at /home/johns/moz/moz-git/dom/base/nsJSEnvironment.cpp:3268
Comment 1 Andrew McCreight [:mccr8] 2012-07-26 11:47:52 PDT
Sounds like XHR isn't rooting some JS thing it holds onto properly, similar to bug 762280.
Comment 2 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-07-26 12:22:11 PDT
Do we end up tracing an uninitialized jsval or something?
Comment 3 Andrew McCreight [:mccr8] 2012-07-26 14:57:45 PDT
Bill said allocated() either means it has been freed already or hasn't been initialized.
Comment 4 Andrew McCreight [:mccr8] 2012-07-29 15:01:08 PDT
I can reproduce on OSX too.
Comment 5 Andrew McCreight [:mccr8] 2012-07-29 18:40:34 PDT
Created attachment 647037 [details] [diff] [review]
slightly reduced

This applies on top of the other patch.  I just removed a few of the tests.  It is a bit flakey, but you don't have to do the rerun thing any more.
Comment 6 Boris Zbarsky [:bz] 2012-07-31 10:37:48 PDT
> Sounds like XHR isn't rooting some JS thing it holds onto properly, similar to bug
> 762280.

That seems plausible, yes.  We call RootResultArrayBuffer() for cases when we set mResultArrayBuffer, but we have nothing along those lines for the case when we set mResultJSON.

Does calling RootResultArrayBuffer (yes, its name is silly at the moment) right before the CreateResponseParsedJSON help?
Comment 7 Andrew McCreight [:mccr8] 2012-07-31 13:53:20 PDT
Created attachment 647667 [details] [diff] [review]
root moar, bz's idea

I think this is what bz suggested (though maybe it would make more sense right inside CreateResponseParsedJSON).  I can't reproduce the crash with this patch.  John, if you want to try it out too that would be good.
Comment 8 Andrew McCreight [:mccr8] 2012-07-31 14:01:04 PDT
If this is actually the fix, it looks like a regression from bug 732377.
Comment 9 Boris Zbarsky [:bz] 2012-07-31 14:09:43 PDT
I don't think the code flow here changed in bug 732377.
Comment 10 John Schoenick [:johns] 2012-07-31 15:19:53 PDT
The patch appears to fix this for me - without the patch I crash within 2-3 iterations of the test with high certainty, without I just ran ten iterations of the test twice without issue.

I'm seeing if I can reproduce this at all on esr10 to see if a fix is needed there
Comment 11 John Schoenick [:johns] 2012-07-31 16:39:26 PDT
Backporting this test isn't a simple matter, and I can't seem to make a smaller test case to easily reproduce this, so I'm not sure I can verify if this affects esr10 or not
Comment 12 Andrew McCreight [:mccr8] 2012-08-02 11:09:06 PDT
Created attachment 648394 [details] [diff] [review]
part 1: root in CreateResponseParsedJSON

I moved this into the actual function that is writing to mResultJSON as that seems to make more sense.
Comment 13 Andrew McCreight [:mccr8] 2012-08-02 11:11:03 PDT
Created attachment 648396 [details] [diff] [review]
test case

From the patch, the test case was pretty easy to construct.  Which is a little worrying from the perspective of release.  But anywho, this test crashes without the patch and is okay with it.
Comment 14 Andrew McCreight [:mccr8] 2012-08-02 11:21:52 PDT
On ESR-10, this test case produces this assertion failure:
Assertion failure: !hasLazyType(), at /Users/amccreight/mz/esr10/js/src/jsobj.h:911

Which doesn't show up on 17, but between that and looking at the code, I think everything is affected.
Comment 15 Andrew McCreight [:mccr8] 2012-08-02 11:23:19 PDT
Comment on attachment 648394 [details] [diff] [review]
part 1: root in CreateResponseParsedJSON

I'm not sure what a good landing strategy for this is, given how obvious it seems to at least make crash.
Comment 16 Andrew McCreight [:mccr8] 2012-08-02 11:59:42 PDT
It looks like this goes back to the initial landing of the JSON response type in Firefox 9. bug 655727
Comment 17 Andrew McCreight [:mccr8] 2012-08-02 15:25:03 PDT
Jesse, this test case could be an interesting fuzzing pattern.

1. access some property
2. don't touch that object
3. access the property again

I'm not sure how common this pattern is, where the first access creates some value and saves away a reference, and further references just look at that existing value. As we see in this particular instance, if the object doesn't root itself properly, there can be badness.
Comment 18 Bill McCloskey (:billm) 2012-08-03 08:01:05 PDT
I managed to catch bug 778179 in gdb and it looks like a dupe of this. Here's the stack.

#0  0x00007ffff1af1912 in js::gc::ArenaHeader::getAllocKind (this=0x7fffb2d0c000)
    at ../../../dist/include/gc/Heap.h:506
#1  0x00007ffff3d7b9aa in js::gc::Cell::getAllocKind (this=0x7fffb2d0c040)
    at /home/billm/mozilla/in0/js/src/gc/Heap.h:982
#2  0x00007ffff3e0c1d1 in js::gc::GetGCThingTraceKind (thing=0x7fffb2d0c040)
    at /home/billm/mozilla/in0/js/src/jsgcinlines.h:30
#3  0x00007ffff3e1fe24 in js_GetGCThingTraceKind (thing=0x7fffb2d0c040)
    at /home/billm/mozilla/in0/js/src/jsgc.cpp:1832
#4  0x00007ffff2a8627c in xpc_GCThingIsGrayCCThing (thing=0x7fffb2d0c040)
    at /home/billm/mozilla/in0/js/xpconnect/src/nsXPConnect.cpp:574
#5  0x00007ffff33c6e76 in GCGraphBuilder::NoteJSChild (this=0x7fffe43d4ba0, child=0x7fffb2d0c040)
    at /home/billm/mozilla/in0/xpcom/base/nsCycleCollector.cpp:1890
#6  0x00007ffff333fac5 in nsScriptObjectTracer::NoteJSChild (aScriptThing=0x7fffb2d0c040, 
    name=0x7ffff43d3e3e "mResultJSON", aClosure=0x7fffe43d4ba0)
    at /home/billm/mozilla/in0/objdir-ff-dbg/xpcom/build/nsCycleCollectionParticipant.cpp:16
#7  0x00007ffff2121c35 in nsXMLHttpRequest::cycleCollection::TraceImpl (p=0x1b7f830, 
    aCallback=0x7ffff333fa54 <nsScriptObjectTracer::NoteJSChild(void*, char const*, void*)>, 
    aClosure=0x7fffe43d4ba0) at /home/billm/mozilla/in0/content/base/src/nsXMLHttpRequest.cpp:680
#8  0x00007ffff22607ae in nsDOMEventTargetHelper::cycleCollection::TraverseImpl (
    that=0x7ffff55b38a0, p=0x1b7f830, cb=...)
    at /home/billm/mozilla/in0/content/events/src/nsDOMEventTargetHelper.cpp:59
#9  0x00007ffff211f43c in nsXHREventTarget::cycleCollection::TraverseImpl (that=0x7ffff55b38a0, 
    p=0x1b7f830, cb=...) at /home/billm/mozilla/in0/content/base/src/nsXMLHttpRequest.cpp:287

We should get this landed soon, because that bug has really exploded on inbound.
Comment 19 Andrew McCreight [:mccr8] 2012-08-03 09:05:00 PDT
(In reply to Bill McCloskey (:billm) from comment #18)
> I managed to catch bug 778179 in gdb and it looks like a dupe of this.
> Here's the stack.

Thank for looking into that!

> We should get this landed soon, because that bug has really exploded on
> inbound.

The proximate cause of that was apparently johns landing bug 771666, which was just a test change that initially found this bug.  My suggested hack around to avoid this crash was apparently ineffective. :)
Comment 20 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-08-03 09:22:01 PDT
Comment on attachment 648394 [details] [diff] [review]
part 1: root in CreateResponseParsedJSON

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

This looks ok, mainly because nsContentUtils::PreserveWrapper guards against double-preserving.
Comment 21 Andrew McCreight [:mccr8] 2012-08-03 09:24:28 PDT
Comment on attachment 648394 [details] [diff] [review]
part 1: root in CreateResponseParsedJSON

Thanks!
Comment 22 Andrew McCreight [:mccr8] 2012-08-03 09:33:10 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/7eb691b3ef12
Comment 23 Boris Zbarsky [:bz] 2012-08-03 12:14:00 PDT
Can we please give the rooting function a better name, since the name just started being a lie?
Comment 24 Andrew McCreight [:mccr8] 2012-08-03 12:28:52 PDT
(In reply to Boris Zbarsky (:bz) [In and out Aug 1 - 10, out Aug 11-20] from comment #23)
> Can we please give the rooting function a better name, since the name just
> started being a lie?

Good point. I was thinking about that but didn't manage to put that into action. How about "RootResultBuffers"?  Or maybe "RootJSResultBuffers".
Comment 25 Boris Zbarsky [:bz] 2012-08-03 12:38:51 PDT
RootJSResultObjects?
Comment 26 Andrew McCreight [:mccr8] 2012-08-03 13:33:25 PDT
Created attachment 648829 [details] [diff] [review]
part 2: fix the name of the rooting function

SGTM
Comment 27 Ed Morley [:emorley] 2012-08-04 11:25:37 PDT
https://hg.mozilla.org/mozilla-central/rev/7eb691b3ef12
Comment 28 Boris Zbarsky [:bz] 2012-08-04 22:52:07 PDT
Comment on attachment 648829 [details] [diff] [review]
part 2: fix the name of the rooting function

r=me
Comment 29 Andrew McCreight [:mccr8] 2012-08-05 08:43:59 PDT
Comment on attachment 648829 [details] [diff] [review]
part 2: fix the name of the rooting function

Thanks.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a2d271ce54ff
Comment 30 Andrew McCreight [:mccr8] 2012-08-05 08:46:58 PDT
Created attachment 649103 [details] [diff] [review]
part 1+2 folded for backporting
Comment 31 Ryan VanderMeulen [:RyanVM] 2012-08-05 17:41:52 PDT
(In reply to Andrew McCreight [:mccr8] from comment #29)
> Comment on attachment 648829 [details] [diff] [review]
> part 2: fix the name of the rooting function
> 
> Thanks.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a2d271ce54ff

https://hg.mozilla.org/mozilla-central/rev/a2d271ce54ff
Comment 32 Andrew McCreight [:mccr8] 2012-08-06 13:34:04 PDT
Comment on attachment 649103 [details] [diff] [review]
part 1+2 folded for backporting

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 655727
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: possible security problems, crash is easy to reproduce
Testing completed (on m-c, etc.): it has been on m-c for a few days
Fix Landed on Version: 17
Risk to taking this patch (and alternatives if risky): Low. This patch just adds a call to PreserveWrapper (via RootJSResultObjects), which can be safely called whenever. It mainly just can cause things to live slightly longer. The rest of the patch is just renaming a function.
String or UUID changes made by this patch: none
Comment 34 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-14 15:30:09 PDT
What needs to be done to satisfy in-testsuite? Does it just need Andrew's testcase patch checked in?
Comment 35 Andrew McCreight [:mccr8] 2012-08-14 16:58:21 PDT
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #34)
> What needs to be done to satisfy in-testsuite? Does it just need Andrew's
> testcase patch checked in?

Yup.  I'm waiting until 15 is released or maybe until this bug is made public.  The test shows the problem pretty explicitly...
Comment 36 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-14 17:02:51 PDT
Okay, so this needs manual verification before Firefox 15 is released? I would assume verification is applying the patch locally and running with mochitest?
Comment 37 Andrew McCreight [:mccr8] 2012-08-14 17:18:56 PDT
Yes. It is probably better to test in a debug build.
Comment 38 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-24 16:22:17 PDT
(In reply to Andrew McCreight [:mccr8] from comment #37)
> Yes. It is probably better to test in a debug build.

Can I get some guidance on how to test this? I know that the test is contained in the patch but I'm not sure which changesets to apply the patch to and how to run the test. Thanks in advance.
Comment 39 Andrew McCreight [:mccr8] 2012-08-28 11:07:03 PDT
Created attachment 656107 [details]
non-mochitest test, file 1
Comment 40 Andrew McCreight [:mccr8] 2012-08-28 11:09:26 PDT
Created attachment 656109 [details]
non-mochitest test, file 2

Here's a new version of the test that should be easier to run.  Download both files and put them in the same directory.  Install Jesse's fuzzpriv addon from here: https://www.squarefree.com/extensions/domFuzzLite3.xpi   Load the html file.  It should crash after a few seconds.  I think a debug build is better.
Comment 41 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-09-11 15:44:51 PDT
Thanks Andrews, that does reproduce with the 2012-07-26 debug Nightly.

Verified fixed with:
 * 2012-08-25 Firefox 15.0 Release Debug
 * 2012-08-28 Firefox 16.0 Beta Debug
 * 2012-08-28 Firefox 17.0 Aurora Debug
 * 2012-08-25 Firefox 10.0.7esrpre Debug

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