561592 - TM: rooting bug in json code

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Andreas Gal :gal]

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

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]

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]

I only added JSON functions like JSON.stringify to jsfunfuzz yesterday.

Comment 4

[Andreas Gal :gal]

Attachment: patch

I can't land my patch until this is fixed.

Comment 5

[Andreas Gal :gal]

My iterator patch triggers this crash in the json xpcshell tests. Its not at fault. It just triggers it.

Comment 6

[Robert Sayre]

http://hg.mozilla.org/tracemonkey/rev/9ac12950fab3

Comment 7

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/f83acecde2c2

Comment 8

[Andreas Gal :gal]

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

Blocking as per comment 8, approvals coming.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

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 11

[Benjamin Smedberg]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/e3d916b8b8d1
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/6fd51d6e7c80

Comment 12

[LegNeato]

We need this landed on 1.9.1 as well

Comment 13

[Al Billings [:abillings - ex-MoCo]]

Is there a repro case for this or is it too abstract for such things?

Comment 14

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e38fb9c8ea8e

Comment 15

[Robert Sayre]

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

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.