777806 - XHR use-after-free of JS

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[John Schoenick [:johns]]

Attachment: 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]]

Sounds like XHR isn't rooting some JS thing it holds onto properly, similar to bug 762280.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Do we end up tracing an uninitialized jsval or something?

Comment 3

[Andrew McCreight [:mccr8]]

Bill said allocated() either means it has been freed already or hasn't been initialized.

Comment 4

[Andrew McCreight [:mccr8]]

I can reproduce on OSX too.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: 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 [:bzbarsky]]

> 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]]

Attachment: 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]]

If this is actually the fix, it looks like a regression from bug 732377.

Comment 9

[Boris Zbarsky [:bzbarsky]]

I don't think the code flow here changed in bug 732377.

Comment 10

[John Schoenick [:johns]]

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]]

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]]

Attachment: 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]]

Attachment: 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]]

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]]

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]]

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]]

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 [inactive unless it's an emergency] (:billm)]

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]]

(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!)]

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]]

Comment on attachment 648394 [details] [diff] [review]
part 1: root in CreateResponseParsedJSON

Thanks!

Comment 22

[Andrew McCreight [:mccr8]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/7eb691b3ef12

Comment 23

[Boris Zbarsky [:bzbarsky]]

Can we please give the rooting function a better name, since the name just started being a lie?

Comment 24

[Andrew McCreight [:mccr8]]

(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 [:bzbarsky]]

RootJSResultObjects?

Comment 26

[Andrew McCreight [:mccr8]]

Attachment: part 2: fix the name of the rooting function

SGTM

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7eb691b3ef12

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 648829 [details] [diff] [review]
part 2: fix the name of the rooting function

r=me

Comment 29

[Andrew McCreight [:mccr8]]

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]]

Attachment: part 1+2 folded for backporting

Comment 31

[Ryan VanderMeulen [:RyanVM]]

(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]]

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 33

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d68436b2cf7d
https://hg.mozilla.org/releases/mozilla-beta/rev/bc9dfaebe970
https://hg.mozilla.org/releases/mozilla-esr10/rev/736543cdf409

Comment 34

[u279076]

What needs to be done to satisfy in-testsuite? Does it just need Andrew's testcase patch checked in?

Comment 35

[Andrew McCreight [:mccr8]]

(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

[u279076]

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]]

Yes. It is probably better to test in a debug build.

Comment 38

[u279076]

(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]]

Attachment: non-mochitest test, file 1

Comment 40

[Andrew McCreight [:mccr8]]

Attachment: 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

[u279076]

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