Consider the following example: function prepare_xml() { delete XML.prototype.function::toString; Object.prototype.toString getter = toSource_getter; return new XML("<a>xml_text</a>"); } var counter = 0; function toSource_getter() { // Make sure that lastInternalResult does not contain prepare_xml var tmp = { toSource: function() { return [1,2,3]; } }; uneval(tmp); if (counter++ < 2) return undefined; // run GC onr overwrite the heap if (typeof gc == "function") gc(); var x = 1e-100; for (var i = 0; i != 50000; ++i) var x2 = x / 4; // Return a function that would access this in non-trivial way to // check if prepare_xml() was rooted. return function() { return this.toXMLString(); }; } uneval({ toSource: prepare_xml }); Currently it gives me segmentation fault in the shell since the result of prepare_xml() is accessed after it was GC-ed. The culprit here is js_ValueToSource code that does not root the result of toSource method before passing it to js_ValueToString. This is a GC-hazard since it is possible to trigger a GC and collect the unrooted argument of js_ValueToString before it is accessed using a specially crafted xml object. When js_ValueToString calls OBJ_DEFAULT_VALUE on an unrooted xml object, eventually JS_CallFunctionName calls getMethod on the object to find its "toString" method. That in turn via GetFunction would query the prototype chain. Since it is possible to remove toString from XML objects and define a getter for Object.prototype.toString, it allows to run GC in the getter and collect the object.

My recommendation would be to differ this to 2.0.1 - we need to stop taking changes for FF2...

I am looking at all the places to check that js_ValueToString(cx, v) is always called with a rooted value. It looks like jsstr.c has problematic place in find_replen. That function contains the following lines: /* Lift current frame to include the args and do the call. */ fp = cx->fp; oldsp = fp->sp; fp->sp = sp; ok = js_Invoke(cx, argc, JSINVOKE_INTERNAL); rval = fp->sp[-1]; fp->sp = oldsp; if (ok) { /* * NB: we count on the newborn string root to hold any string * created by this js_ValueToString that would otherwise be GC- * able, until we use rdata->repstr in do_replace. */ repstr = js_ValueToString(cx, rval); Here rval apparently is not rooted when js_ValueToString is called. But testing with GC_MARK_DEBUG revealed that it is rooted through a stack. But how exactly given that the temporary stack is already replaced by the original value?

(In reply to comment #2) > But how exactly > given that the temporary stack is already replaced by the original value? It's protected by the stack segment allocated by js_AllocStack and scanned by the GC (see JSStackHeader, jscntxt.h) until the js_FreeStack at the lambda_out: label. /be

Here is another similar hazard in sort_compare (will Array.sort be ever 100% correct?). The function calls js_ValueToNumber passing as argument a value that is returned by the compare function and which is only rooted through cx->lastResult. As such the following gives the crash: var counter = 0; function prepare_xml() { delete XML.prototype.function::toString; Object.prototype.toString getter = toSource_getter; return new XML("<a>xml_text</a>"); } function toSource_getter() { // Make sure that lastInternalResult does not contain prepare_xml var tmp = { toSource: function() { return [1,2,3]; } }; uneval(tmp); if (counter++ < 2) return undefined; // run GC onr overwrite the heap if (typeof gc == "function") gc(); var x = 1e-100; for (var i = 0; i != 50000; ++i) var x2 = x / 4; // Return a function that would access this in non-trivial way to // check if prepare_xml() was rooted. return function() { print("xxx"); return this.toXMLString(); }; } var a = [1, 2]; a.sort(prepare_xml);

The same story is with js_GetLengthProperty from jsarray.c. It calls js_ValueToECMAUint32 which eventually calls OBJ_DEFAULT_VALUE without rooting the value of length property: var counter = 0; function prepare_xml() { delete XML.prototype.function::toString; Object.prototype.toString getter = toSource_getter; return new XML("<a>xml_text</a>"); } function toSource_getter() { // Make sure that lastInternalResult does not contain prepare_xml var tmp = { toSource: function() { return [1,2,3]; } }; uneval(tmp); if (counter++ < 2) return undefined; // run GC onr overwrite the heap if (typeof gc == "function") gc(); var x = 1e-100; for (var i = 0; i != 50000; ++i) var x2 = x / 4; // Return a function that would access this in non-trivial way to // check if prepare_xml() was rooted. return function() { print("xxx"); return this.toXMLString(); }; } var obj = { }; obj.length getter = prepare_xml; Array.reverse(obj); ~/m/trunk/mozilla/js/src> ./Linux_All_DBG.OBJ/js ~/s/x.js before 9232, after 9232, break 08180000 Segmentation fault

The patch addresses the 3 problems with test cases plus the following: 1. js_HasLengthProperty from jsarray.c has exactly the same issue as js_GetLengthProperty but since the browser or shells do not call it, no script-only test case can be provided. 2. Implementation of Array.protype.toLocalString in array_join_sub contains the following code: if (!js_ValueToObject(cx, v, &obj2) || !js_TryMethod(cx, obj2, cx->runtime->atomState.toLocaleStringAtom, 0, NULL, &v)) { This is GC-safe currently because js_ValueToObject never returns a weakly rooted XML object with weakly meaning "can be unrooted with a script". But such assumptions are extremely fragile like this bug demonstrates. Thus the patch explicitly roots the value.

Comment on attachment 239036 [details] [diff] [review] Fix v1 >+ /* After this point the control must flow through out: */ >+ JS_PUSH_SINGLE_TEMP_ROOT(cx, JSVAL_NULL, &tvr); > id = ATOM_TO_JSID(cx->runtime->atomState.lengthAtom); >- if (!OBJ_GET_PROPERTY(cx, obj, id, &v)) >- return JS_FALSE; >+ ok = OBJ_GET_PROPERTY(cx, obj, id, &tvr.u.value); >+ if (!ok) >+ goto out; > >- /* Short-circuit, because js_ValueToECMAUint32 fails when >- * called during init time. >+ /* >+ * Short-circuit, because js_ValueToECMAUint32 fails when called during >+ * init time. > */ >- if (JSVAL_IS_INT(v)) { >- i = JSVAL_TO_INT(v); >+ if (JSVAL_IS_INT(tvr.u.value)) { >+ i = JSVAL_TO_INT(tvr.u.value); > /* jsuint cast does ToUint32. */ > *lengthp = (jsuint)i; >- return JS_TRUE; >+ ok = JS_TRUE; >+ } else { >+ ok = js_ValueToECMAUint32(cx, tvr.u.value, (uint32 *)lengthp); > } >- return js_ValueToECMAUint32(cx, v, (uint32 *)lengthp); >+ out: >+ JS_POP_TEMP_ROOT(cx, &tvr); >+ return ok; This function is short enough, and enough lines are being changed, that I would avoid goto out and simply nest the bulk of the code inside if (ok) {...} after the OBJ_GET_PROPERTY call. Bonus cleanup point for moving the minor "jsuint cast does ToUint32." comment to the right of the line it applies to, tabbed one stop and with the period at the end removed. It hurts my eyes. >+ * If count is -3, then u.object holds single rooted JSObject*. Is this worth the source code cost in general, and the compiled code cost in jsgc.c, small though the increase is for this new case? OBJECT_TO_JSVAL(obj) passed to JS_PUSH_SINGLE_TEMP_ROOT should be equivalent in compiled code. Looks good otherwise -- totally agree on avoiding fragile "hanging by a thread" rooting. I doubt that rogerl or whoever wrote the toLocaleString method was counting on it, or was aware of the GC hazard -- I obviously was not and I probably reviewed that patch. /be

Pushing to 1.8.1.1...

we should not be passing on gc hazards for releases.

(In reply to comment #9) > we should not be passing on gc hazards for releases. You know that we have a security bug policy and a patch update system, and you know how they have worked together since Firefox 1.5. You know that we have a non-trivial list of to-be-fixed security bugs, and that we will have such a list for the foreseeable future. So why are you making categorical statements about how every such bug ("passing on gc hazards") must be fixed before any particular release occurs? Igor, mrbkap and I at least have been working hard on memory safety, and I'm pretty sure that we don't agree with you that every such bug should slip the release. It's true we are taking a chance that some bad guy might independently discover the flaw. But that's not the only thing to avoid here. Not shipping Firefox 2 this year is also worth a lot, including in security coverage. See bug 352064 comment 19. /be

(In reply to comment #7) > Is this worth the source code cost in general, and the compiled code cost in > jsgc.c, small though the increase is for this new case? I wanted to have a root which address could be passed without ugly casts to a function taking JSObject**. An alternative is to add an object to the union, but stil use -1 as the count assuming that the tagging should do the right job with jsval and assuming a sensible compiler, but that looked unclean. But I guess code bloat is always bad a thing. Thus I will use the union without -3 but with an assert that the sizeof(union) == sizeof(jsval) == sizeof(JSObject *). > I doubt that rogerl or whoever wrote the toLocaleString method was > counting on it, or was aware of the GC hazard -- I obviously was not and I > probably reviewed that patch. When that code was written, it was 100% safe since you need an XML object there. Without jsxml.c js_TryMethod, OBJ_DEFAULT_VALUE etc. are safe against passing unrooted values. This is even true with getter/setters since there "this" still points to to the original object no matter what and as such is rooted. It was jsxml.c that violated that. Also note that without getters/setters almost all reported GC hazards would not exit and probably quite a few fixed places were written before scripted getters and setters were added. So there is pattern: GC-safe-code -> new feature that violates a previous hidden assumption -> GC-hazard. Now the interesting part is to think about close hooks (new feature!) and what hidden assumptions they violate?

(In reply to comment #9) > we should not be passing on gc hazards for releases. These are safe hazards since they require very deliberate code to exploit. As such a probability that they happen in real code is practical 0. Moreover, no tools exists yet that can help to find them, so finding such bugs is rather manual-intensive task. Compare that with, for example, decompiler bugs. They can be and *is* found by an automated tool. Moreover, given the amount of junk that exists on web pages such bugs can be revealed by real-life examples. So it is really important to have a bulletproof decompiler while fixing hazards like this bug can wait for another release.

(In reply to comment #12) > So it is really important to have a bulletproof decompiler while fixing hazards > like this bug can wait for another release. Hence my working night and day recently :-/. Igor, you're right that jsxml.c upset an old assumption, but I bet the assumption was not well understood. I used to think I could see the run-to-completion paths during which GC could not nest, but I know better now. This is like seeing where interrupts need to be blocked in single-processor Unix, or seeing the paths in SP Unix during which no subroutine calls sleep. All such systems violate abstraction, in particular the ability to hook new code in (compositionality). To make the abstractions bullet-proof for GC, we need to track all references. Doing this manually with JSTempValueRooters is hard, but do-able if we keep after it. Conservative GC would be better use of everyone's time, I think. I had occasion to talk to a JVM guru recently, who was convinced that exact GC is fine ("it took Sun six months to switch from conservative to exact"). But Java doesn't have getters and setters, and that makes a big difference. It's not the only or (since JS is evolving) last such difference. Comments? /be

(In reply to comment #13) > (In reply to comment #12) > > So it is really important to have a bulletproof decompiler while fixing hazards > > like this bug can wait for another release. > > Hence my working night and day recently :-/. I wish I could help, but decompiler is a magic box for me :( .... > To make the abstractions bullet-proof for GC, we > need to track all references. Doing this manually with JSTempValueRooters is > hard, but do-able if we keep after it. Conservative GC would be better use of > everyone's time, I think. I think the problem with SM is that newborn array gives really false-sence-of-security. With API that takes only explicit roots like tvr, most of the bugs would not simply exist. Add to that an a tool that check type annotations like one Linux kernel uses, and you get rather safe system. > I had occasion to talk to a JVM guru recently, who was convinced that exact GC > is fine ("it took Sun six months to switch from conservative to exact"). But > Java doesn't have getters and setters, and that makes a big difference. It's > not the only or (since JS is evolving) last such difference. Comments? In Java almost all the code is written in Java itself. Compare that with SM where all host objects are written in C. I wish SM had a high level assembler so at least Array can written using it. How many GC from jsarray.c it would remove? Plus with the right internal and external API it should be easy to enforce everything is rooted model. Just imagine that it would be impossible to pass unrooted jsval* to OBJ_GET_PROPERTY and this is what happens with Java native interface AFAICS.

(In reply to comment #14) > I think the problem with SM is that newborn array gives really > false-sence-of-security. That's true, wherefore bug 313437. > With API that takes only explicit roots like tvr, most > of the bugs would not simply exist. That's not clear to me at all. If you have to take extra effort with macros and balanced PUSH and POP, not everyone will remember to do it. An implicit model would be better. > Add to that an a tool that check type > annotations like one Linux kernel uses, and you get rather safe system. Type system support would be great. > In Java almost all the code is written in Java itself. Compare that with SM > where all host objects are written in C. I wish SM had a high level assembler > so at least Array can written using it. How many GC from jsarray.c it would > remove? Absolutely. But do you really need assembler, or just JS2 features and optimizing runtime? First one, then the other, is one way to do it. In any event, self-hosting more of the standard library is a definite longer-term SpiderMonkey goal for me. > Plus with the right internal and external API it should be easy to enforce > everything is rooted model. Just imagine that it would be impossible to pass > unrooted jsval* to OBJ_GET_PROPERTY and this is what happens with Java native > interface AFAICS. Sure, if you make everyone hold jref or equivalent handle on value. But that is not what I did ten years ago, and we have to break APIs to get there. It also has a cost that isn't obviously lower than conservative GC. Anyway, we should talk about this more in a non-bugzilla forum. But if you would like to take bug 313437 from my list, please do so. /be

The patch besides addressing the nits removes that count == -3 and assumes instead that the trick with tagged jsval works as expected. Another thing that I forgot to mention in the comments to v1 is the declaration of GC_MARK_DEBUG-only js_LiveThingToFind and js_DumpGCHeap in jsgc.c. It helps to avoid writing extra "extren js_LiveThingToFind" when debugging.

Comment on attachment 239084 [details] [diff] [review] Fix v2 >+/* >+ * The following allows to reinterpret JSTempValueUnion.object as jsval using >+ * the tagging property of a generic jsval described bellow. "below" Looks great, thanks for fixing. Yeah, js_LiveThingToFind started out life in the dog-house and only now is escaping. ;-) /be

(In reply to comment #15) > That's not clear to me at all. If you have to take extra effort with macros > and balanced PUSH and POP, not everyone will remember to do it. An implicit > model would be better. Except it would not protect against bug 353 165 as the only implicit model that can stop it is no GC at all during native calls. > > > Add to that an a tool that check type > > annotations like one Linux kernel uses, and you get rather safe system. > > Type system support would be great. I filed bug 353231about it. > Absolutely. But do you really need assembler, or just JS2 features and > optimizing runtime? First one, then the other, is one way to do it. In any > event, self-hosting more of the standard library is a definite longer-term > SpiderMonkey goal for me. See bug 353235. > It > also has a cost that isn't obviously lower than conservative GC. In principle I agree, but GCJ-compiled Eclipse sucks compared with one that runs inside JVM. Yes, its compiler is faster but the rest og GUI is slower. I wonder how much of that slowness comes from conservative GC.

(In reply to comment #18) > (In reply to comment #15) > > That's not clear to me at all. If you have to take extra effort with macros > > and balanced PUSH and POP, not everyone will remember to do it. An implicit > > model would be better. > > Except it would not protect against bug 353165 as the only implicit model > that can stop it is no GC at all during native calls. The other implicit model that can stop such bugs is conservative GC of course ;-). /be

(In reply to comment #19) >> > An implicit > > > model would be better. > > > > Except it would not protect against bug 353165 as the only implicit model > > that can stop it is no GC at all during native calls. > > The other implicit model that can stop such bugs is conservative GC of course Right, so here comes bug 353242.

Patch to commit with spelling-in-comments fixes.

I committed the patch from comment 21 to the trunk: Checking in js.c; /cvsroot/mozilla/js/src/js.c,v <-- js.c new revision: 3.125; previous revision: 3.124 done Checking in jsarray.c; /cvsroot/mozilla/js/src/jsarray.c,v <-- jsarray.c new revision: 3.97; previous revision: 3.96 done Checking in jscntxt.h; /cvsroot/mozilla/js/src/jscntxt.h,v <-- jscntxt.h new revision: 3.119; previous revision: 3.118 done Checking in jsgc.c; /cvsroot/mozilla/js/src/jsgc.c,v <-- jsgc.c new revision: 3.177; previous revision: 3.176 done Checking in jsgc.h; /cvsroot/mozilla/js/src/jsgc.h,v <-- jsgc.h new revision: 3.58; previous revision: 3.57 done Checking in jsstr.c; /cvsroot/mozilla/js/src/jsstr.c,v <-- jsstr.c new revision: 3.133; previous revision: 3.132 done

verified fixed 1.9 20060919 windows/mac*/linux

Restoring lost blocking flag

Comment on attachment 239098 [details] [diff] [review] Fix v2b The patch applies to 1.8.1 as is.

The backport required non-minimal efforts warranting a review.

This is to simplify the review.

Comment on attachment 244623 [details] [diff] [review] Fix v2b for 1.8.0 branch r=me, thanks for the interdiff. /be

Comment on attachment 239098 [details] [diff] [review] Fix v2b Approved for 1.8 branch, a=dveditz for drivers

Comment on attachment 244623 [details] [diff] [review] Fix v2b for 1.8.0 branch Approved for 1.8.0 branch, a=dveditz for drivers

I committed the patch from comment 21 to MOZILLA_1_8_BRANCH: Checking in js.c; /cvsroot/mozilla/js/src/js.c,v <-- js.c new revision: 3.93.2.10; previous revision: 3.93.2.9 done Checking in jsarray.c; /cvsroot/mozilla/js/src/jsarray.c,v <-- jsarray.c new revision: 3.58.2.20; previous revision: 3.58.2.19 done Checking in jscntxt.h; /cvsroot/mozilla/js/src/jscntxt.h,v <-- jscntxt.h new revision: 3.80.4.14; previous revision: 3.80.4.13 done Checking in jsgc.c; /cvsroot/mozilla/js/src/jsgc.c,v <-- jsgc.c new revision: 3.104.2.23; previous revision: 3.104.2.22 done Checking in jsgc.h; /cvsroot/mozilla/js/src/jsgc.h,v <-- jsgc.h new revision: 3.33.4.7; previous revision: 3.33.4.6 done Checking in jsstr.c; /cvsroot/mozilla/js/src/jsstr.c,v <-- jsstr.c new revision: 3.108.2.10; previous revision: 3.108.2.9 done

Comment on attachment 244623 [details] [diff] [review] Fix v2b for 1.8.0 branch The patch does not include the fix for regression from dependant bug 355029. I will include it here.

I took out the previous commit to include into the patch the regression fix from bug 355029.

This is just a the previous patch + the fix from bug 355029 applied on top of it.

This is again the previous version of the patch + the regression fix from bug 355029 applied on top of it.

Comment on attachment 244915 [details] [diff] [review] Fix v2b for 1.8.0 branch + regression fix from bug 355029 approved for 1.8.0 branch, a=dveditz for drivers

Comment on attachment 244914 [details] [diff] [review] For 1.8.1: Fix v2b + regression fix from bug 355029 approved for 1.8 branch, a=dveditz for drivers

I committed the patch from comment 37 to MOZILLA_1_8_BRANCH: Checking in js.c; /cvsroot/mozilla/js/src/js.c,v <-- js.c new revision: 3.93.2.13; previous revision: 3.93.2.12 done Checking in jsarray.c; /cvsroot/mozilla/js/src/jsarray.c,v <-- jsarray.c new revision: 3.58.2.22; previous revision: 3.58.2.21 done Checking in jscntxt.h; /cvsroot/mozilla/js/src/jscntxt.h,v <-- jscntxt.h new revision: 3.80.4.16; previous revision: 3.80.4.15 done Checking in jsgc.c; /cvsroot/mozilla/js/src/jsgc.c,v <-- jsgc.c new revision: 3.104.2.25; previous revision: 3.104.2.24 done Checking in jsgc.h; /cvsroot/mozilla/js/src/jsgc.h,v <-- jsgc.h new revision: 3.33.4.9; previous revision: 3.33.4.8 done Checking in jsstr.c; /cvsroot/mozilla/js/src/jsstr.c,v <-- jsstr.c new revision: 3.108.2.13; previous revision: 3.108.2.12 done

I committed the patch from comment 38 to MOZILLA_1_8_0_BRANCH: Checking in js.c; /cvsroot/mozilla/js/src/js.c,v <-- js.c new revision: 3.93.2.2.2.5; previous revision: 3.93.2.2.2.4 done Checking in jsarray.c; /cvsroot/mozilla/js/src/jsarray.c,v <-- jsarray.c new revision: 3.58.2.10.2.10; previous revision: 3.58.2.10.2.9 done Checking in jscntxt.h; /cvsroot/mozilla/js/src/jscntxt.h,v <-- jscntxt.h new revision: 3.80.4.2.2.9; previous revision: 3.80.4.2.2.8 done Checking in jsgc.c; /cvsroot/mozilla/js/src/jsgc.c,v <-- jsgc.c new revision: 3.104.2.3.2.14; previous revision: 3.104.2.3.2.13 done Checking in jsgc.h; /cvsroot/mozilla/js/src/jsgc.h,v <-- jsgc.h new revision: 3.33.12.3; previous revision: 3.33.12.2 done Checking in jsstr.c; /cvsroot/mozilla/js/src/jsstr.c,v <-- jsstr.c new revision: 3.108.2.5.2.5; previous revision: 3.108.2.5.2.4 done

verified fixed 20061122 1.8.0.9 windows/linux/mac*, 1.8.1.1 windows/linux/mac*, 1.9 windows/linux

/cvsroot/mozilla/js/tests/e4x/extensions/regress-352846-01.js,v <-- regress-352846-01.js /cvsroot/mozilla/js/tests/e4x/extensions/regress-352846-02.js,v <-- regress-352846-02.js /cvsroot/mozilla/js/tests/e4x/extensions/regress-352846-03.js,v <-- regress-352846-03.js moved to extensions/ due to toSource