TM: rooting bug in json code

RESOLVED FIXED

Status

()

Core
JavaScript Engine
P2
critical
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: gal, Assigned: gal)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(blocking2.0 beta1+, blocking1.9.2 .4+, status1.9.2 .4-fixed, blocking1.9.1 .10+, status1.9.1 .10-fixed)

Details

(Whiteboard: [sg:critical] fixed-in-tracemonkey [qa-examined-192] [qa-noaction-192][qa-examined-191] [qa-noaction-191])

Attachments

(1 attachment)

(Assignee)

Description

7 years ago
#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
(Assignee)

Updated

7 years ago
Assignee: general → sayrer
Priority: -- → P2
Whiteboard: [sg:critical]
(Assignee)

Comment 1

7 years ago
Free memory pointer deref, json can be reached from script, heap spraying would do the rest. We should find out what versions are affected.
(Assignee)

Comment 2

7 years ago
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

7 years ago
I only added JSON functions like JSON.stringify to jsfunfuzz yesterday.
(Assignee)

Comment 4

7 years ago
Created attachment 441337 [details] [diff] [review]
patch

I can't land my patch until this is fixed.
Assignee: sayrer → gal
Attachment #441337 - Flags: review+
(Assignee)

Comment 5

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

Comment 6

7 years ago
http://hg.mozilla.org/tracemonkey/rev/9ac12950fab3
Whiteboard: [sg:critical] → [sg:critical] fixed-in-tracemonkey

Updated

7 years ago
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
blocking2.0: --- → beta1+

Comment 7

7 years ago
http://hg.mozilla.org/mozilla-central/rev/f83acecde2c2
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Comment 8

7 years ago
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.
Blocking as per comment 8, approvals coming.
blocking1.9.1: ? → .10+
blocking1.9.2: ? → .4+
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
Attachment #441337 - Flags: approval1.9.2.4+
Attachment #441337 - Flags: approval1.9.1.10+

Comment 11

7 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/e3d916b8b8d1
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/6fd51d6e7c80
status1.9.2: --- → .4-fixed

Comment 12

7 years ago
We need this landed on 1.9.1 as well
Is there a repro case for this or is it too abstract for such things?

Comment 14

7 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e38fb9c8ea8e
status1.9.1: --- → .10-fixed

Comment 15

7 years ago
(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.
(Assignee)

Comment 16

7 years ago
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.
Whiteboard: [sg:critical] fixed-in-tracemonkey → [sg:critical] fixed-in-tracemonkey [qa-examined-192]
Whiteboard: [sg:critical] fixed-in-tracemonkey [qa-examined-192] → [sg:critical] fixed-in-tracemonkey [qa-examined-192] [qa-ntd]
Whiteboard: [sg:critical] fixed-in-tracemonkey [qa-examined-192] [qa-ntd] → [sg:critical] fixed-in-tracemonkey [qa-examined-192] [qa-noaction-192]
Whiteboard: [sg:critical] fixed-in-tracemonkey [qa-examined-192] [qa-noaction-192] → [sg:critical] fixed-in-tracemonkey [qa-examined-192] [qa-noaction-192][qa-examined-191] [qa-noaction-191]

Updated

7 years ago
Duplicate of this bug: 553514
Group: core-security
You need to log in before you can comment on or make changes to this bug.