Last Comment Bug 561592 - TM: rooting bug in json code
: TM: rooting bug in json code
Status: RESOLVED FIXED
[sg:critical] fixed-in-tracemonkey [q...
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: P2 critical (vote)
: ---
Assigned To: Andreas Gal :gal
:
Mentors:
: 553514 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-04-24 17:56 PDT by Andreas Gal :gal
Modified: 2010-06-22 19:50 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta1+
.4+
.4-fixed
.10+
.10-fixed


Attachments
patch (952 bytes, patch)
2010-04-25 00:12 PDT, Andreas Gal :gal
dvander: review+
mbeltzner: approval1.9.2.4+
mbeltzner: approval1.9.1.10+
Details | Diff | Splinter Review

Description Andreas Gal :gal 2010-04-24 17:56:46 PDT
#0  0x0000000104124fe3 in js_HasOwnProperty (cx=0x1053be800, lookup=0xdadadadadadadada, obj=0x106bb8480, id=4407833796, objp=0x7fff5fbfcd20, propp=0x7fff5fbfcd18) at ../../../js/src/jsobj.cpp:1588
#1  0x000000010413bff7 in JO (cx=0x1053be800, vp=0x7fff5fbfcf68, scx=0x7fff5fbfd430) at ../../../js/src/json.cpp:318
#2  0x000000010413b7cb in Str (cx=0x1053be800, id=4365160212, holder=0x106bb8440, scx=0x7fff5fbfd430, vp=0x7fff5fbfcf68, callReplacer=false) at ../../../js/src/json.cpp:506
#3  0x000000010413c2a7 in JO (cx=0x1053be800, vp=0x7fff5fbfd1e8, scx=0x7fff5fbfd430) at ../../../js/src/json.cpp:361
#4  0x000000010413b7cb in Str (cx=0x1053be800, id=4365160148, holder=0x106bb8400, scx=0x7fff5fbfd430, vp=0x7fff5fbfd1e8, callReplacer=false) at ../../../js/src/json.cpp:506
#5  0x000000010413c2a7 in JO (cx=0x1053be800, vp=0x1052b9908, scx=0x7fff5fbfd430) at ../../../js/src/json.cpp:361
#6  0x000000010413b7cb in Str (cx=0x1053be800, id=4398776324, holder=0x106bb8580, scx=0x7fff5fbfd430, vp=0x1052b9908, callReplacer=true) at ../../../js/src/json.cpp:506
#7  0x000000010413ba49 in js_Stringify (cx=0x1053be800, vp=0x1052b9908, replacer=0x0, space=0, cb=@0x7fff5fbfd510) at ../../../js/src/json.cpp:569
#8  0x0000000104077aff in JS_Stringify (cx=0x1053be800, vp=0x1052b9908, replacer=0x0, space=0, callback=0x100a52825 <WriteCallback(unsigned short const*, unsigned int, void*)>, data=0x7fff5fbfd670) at ../../../js/src/jsapi.cpp:5283
#9  0x0000000100a51a7f in nsJSON::EncodeInternal (this=0x104b91cd0, writer=0x7fff5fbfd670) at ../../../../dom/src/json/nsJSON.cpp:267
#10 0x0000000100a539c7 in nsJSON::Encode (this=0x104b91cd0, aJSON=@0x7fff5fbfdd50) at ../../../../dom/src/json/nsJSON.cpp:93
#11 0x00000001013b9055 in NS_InvokeByIndex_P (that=0x104b91cd0, methodIndex=3, paramCount=1, params=0x7fff5fbfd8f0) at ../../../../../../../xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:208
#12 0x00000001000e277f in XPCWrappedNative::CallMethod (ccx=@0x7fff5fbfded0, mode=XPCWrappedNative::CALL_METHOD) at ../../../../../js/src/xpconnect/src/xpcwrappednative.cpp:2750
#13 0x00000001000eabf1 in XPC_WN_CallMethod (cx=0x1053be800, obj=0x106ba0100, argc=1, argv=0x1052b9908, vp=0x7fff5fbfe070) at ../../../../../js/src/xpconnect/src/xpcwrappednativejsops.cpp:1770
#14 0x0000000104115834 in js_Invoke (cx=0x1053be800, argc=1, vp=0x1052b98f8, flags=2) at jsinterp.cpp:834
#15 0x0000000104100879 in js_Interpret (cx=0x1053be800) at jsops.cpp:2237
#16 0x0000000104114a88 in js_Execute () at jsinterp.cpp:1103
#17 0x000000010407a962 in JS_EvaluateUCScriptForPrincipals (cx=0x1053be800, obj=0x106b96840, principals=0x104a2c378, chars=0x106ce3f50, length=16, filename=0x1000126f3 "-e", lineno=1, rval=0x7fff5fbfec98) at ../../../js/src/jsapi.cpp:4862
#18 0x000000010407ac16 in JS_EvaluateScriptForPrincipals (cx=0x1053be800, obj=0x106b96840, principals=0x104a2c378, bytes=0x7fff5fbff3e1 "_execute_test();", nbytes=16, filename=0x1000126f3 "-e", lineno=1, rval=0x7fff5fbfec98) at ../../../js/src/jsapi.cpp:4826
#19 0x000000010000465b in ProcessArgs (cx=0x1053be800, obj=0x106b96840, argv=0x7fff5fbfef70, argc=16) at ../../../../../js/src/xpconnect/shell/xpcshell.cpp:1207
#20 0x00000001000051a1 in main (argc=16, argv=0x7fff5fbfef70, envp=0x7fff5fbfeff8) at ../../../../../js/src/xpconnect/shell/xpcshell.cpp:1885

548	js_Stringify(JSContext *cx, jsval *vp, JSObject *replacer, jsval space,
(gdb) 
549	             JSCharBuffer &cb)
550	{
551	    // XXX stack
552	    JSObject *stack = JS_NewArrayObject(cx, 0, NULL);
553	    if (!stack)
554	        return JS_FALSE;
555	
556	    StringifyContext scx(cx, cb, replacer);
557	    if (!InitializeGap(cx, space, scx.gap))
558	        return JS_FALSE;
(gdb) 
559	
560	    JSObject *obj = NewObject(cx, &js_ObjectClass, NULL, NULL);
561	    if (!obj)
562	        return JS_FALSE;
563	
564	    if (!obj->defineProperty(cx, ATOM_TO_JSID(cx->runtime->atomState.emptyAtom),
565	                             *vp, NULL, NULL, JSPROP_ENUMERATE)) {
566	        return JS_FALSE;
567	    }
568	
(gdb) 
569	    return Str(cx, ATOM_TO_JSID(cx->runtime->atomState.emptyAtom), obj, &scx, vp);
570	}

several rooting issues in this function
Comment 1 Andreas Gal :gal 2010-04-24 18:00:08 PDT
Free memory pointer deref, json can be reached from script, heap spraying would do the rest. We should find out what versions are affected.
Comment 2 Andreas Gal :gal 2010-04-24 22:55:53 PDT
Jesse, this is a bug we really should have found using fuzzing and gczeal. Want to dig into this a bit and find out why we didn't? This is xpcshell, but this can probably be triggered from the regular shell or the browser as well.
Comment 3 Jesse Ruderman 2010-04-24 23:06:54 PDT
I only added JSON functions like JSON.stringify to jsfunfuzz yesterday.
Comment 4 Andreas Gal :gal 2010-04-25 00:12:32 PDT
Created attachment 441337 [details] [diff] [review]
patch

I can't land my patch until this is fixed.
Comment 5 Andreas Gal :gal 2010-04-25 00:34:41 PDT
My iterator patch triggers this crash in the json xpcshell tests. Its not at fault. It just triggers it.
Comment 8 Andreas Gal :gal 2010-04-25 10:30:48 PDT
drivers, this bug is very high risk and the patch is very low risk (no side effects, just ensure a root survives a potential GC). I recommend you take it for the next 1.9.2 dot release even if its late in the cycle.
Comment 9 Mike Beltzner [:beltzner, not reading bugmail] 2010-04-26 09:08:56 PDT
Blocking as per comment 8, approvals coming.
Comment 10 Mike Beltzner [:beltzner, not reading bugmail] 2010-04-26 09:10:40 PDT
Comment on attachment 441337 [details] [diff] [review]
patch

a=beltzner, please land on mozilla-1.9.1 default, mozilla 1.9.2 default AND mozilla-1.9.2 GECKO1924_20100413_RELBRANCH
Comment 12 christian 2010-04-26 15:35:13 PDT
We need this landed on 1.9.1 as well
Comment 13 Al Billings [:abillings] 2010-04-26 17:19:40 PDT
Is there a repro case for this or is it too abstract for such things?
Comment 15 Robert Sayre 2010-04-27 06:05:45 PDT
(In reply to comment #13)
> Is there a repro case for this or is it too abstract for such things?

Not really. In theory, we should be able to make it crash, but the only reproducible test case we have was triggered by a patch Andreas is working on for TM trunk.
Comment 16 Andreas Gal :gal 2010-04-27 12:17:05 PDT
I wasn't able to synthesize a test case for this that actually crashes, but I can try a little harder maybe with a bit fuzzer help. The basic ingredients are JSON.stringify and passing gc() as the replacer function (in the browser substitute gc with a function that allocates a ton of garbage). I will talk to Jesse to see if we can increase the frequencies of those atoms in the fuzzer output and force a test case.
Comment 17 Robert Sayre 2010-05-26 16:52:42 PDT
*** Bug 553514 has been marked as a duplicate of this bug. ***

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