js_fun_toString from jsfun.x does not root the result of convert call before calling on the supplied argument js_ValueToECMAUint32 and using the unrooted value later. The following code demonstrates the bug with a segmentation fault in js shell. var f = Function("return 1"); Function("return 2"); var expected_string = f.toSource(0); var likeFunction = { valueOf: function() { var tmp = f; f = null; return tmp; }, __proto__: Function.prototype }; var likeNumber = { valueOf: function() { gc(); return 0; } }; var actual = likeFunction.toSource(likeNumber); print(expected_string === actual);

You're on a roll here... If we wanted to hold 1.5 until you're done finding this class of bug how much longer would that be? Can we speed that process by dividing the remaining code amongst other js engine hackers?

(In reply to comment #1) > You're on a roll here... If we wanted to hold 1.5 until you're done finding > this class of bug how much longer would that be? I wish I could spend more time on this beyond 1-3 hors of ocasinal looking at the source. > Can we speed that process by > dividing the remaining code amongst other js engine hackers? Surely. Initially I thought to simply grep for all places where C code declares jsval locals. But it turned out that it is not enough since JSObject* and especially JSString* locals should also be checked. So at the end I decided to look at all methods that implements ECMA-262 library in js/src/*.c As far as I can see jsmath.c, jsarray.c, jsnum.c, jsfun.c, jsstr.c, jsexn.c do not have any obvious holes (any longer :). I would say that jsobj.c is safe if there is a guaranty that ALL implementation of the resolve hook do not cause GC for valueOf and toString properties. If there is no such guarantee, then I suggest to add explicit rooting at the beginning of js_DefaultValue/js_TryMethod since few places relies on the fact that it is safe to use the functions with unrooted values. I did not look at jsxml.c, jsinterpret.c and jsbool.c.

(In reply to comment #2) > > Can we speed that process by > > dividing the remaining code amongst other js engine hackers? > > Surely. Dveditz, It's important to get fresh eyes on the problem, and good eyes -- Igor's eyes have both virtues. At this point, I am not concerned about code that Igor has looked at, so the three files he hasn't need scrutiny. > I would say that jsobj.c is safe if there is a guaranty that ALL implementation > of the resolve hook do not cause GC for valueOf and toString properties. If > there is no such guarantee, then I suggest to add explicit rooting at the > beginning of js_DefaultValue/js_TryMethod since few places relies on the fact > that it is safe to use the functions with unrooted values. I will prepare a patch to do just this and attach it here, unless someone wants a new bug badly. /be

I would like to see this fix (at least the jsfun.c one) go in for a respin, since it is a safe one-liner. /be

(In reply to comment #4) > (In reply to comment #2) > > I would say that jsobj.c is safe if there is a guaranty that ALL implementation > > of the resolve hook do not cause GC for valueOf and toString properties. If > > there is no such guarantee, then I suggest to add explicit rooting at the > > beginning of js_DefaultValue/js_TryMethod since few places relies on the fact > > that it is safe to use the functions with unrooted values. I don't see anything to root in js_DefaultValue. The |obj| parameter must be rooted or well-connected by callers, per the usual rules. The v local is used as a result parameter, and could contain a weak ref to a GC-thing liable to be GC'd, but there are no paths that GC without reseting v (or *vp, then returning early before the *vp = v at label out:) after. What protect *vp after js_DefaultValue (OBJ_DEFAULT_VALUE) returns? The caller has to make arrangements. (Just for everyone's info, Igor knows this: Callers protect in and out params in general. Bugs in following this rule need to be fixed case-by-case if we are not going to try for a general automatic solution. For 1.8/fx1.5, we are looking for spot fixes.) js_TryMethod takes an argv and returns a *rval, and those are just locals passed by reference. So js_TryValueOf, e.g., passings an atom key in argv[0], and it's the atom being pinned that saves it from GC. The rval that js_TryValueOf receives from JS_ConvertStub is protected by convert's caller, e.g. js_DefaultValue. But if there are no GC'ing paths in js_DefaultValue after it calls clasp->convert till it returns its result, then the burnen is on its caller. I hope this helps. Igor, what am I missing that needs rooting in js_DefaultValue and/or js_TryMethod? /be

Comment on attachment 200648 [details] [diff] [review] trivial fix sr=shaver

(In reply to comment #6) I meant to root obj itself as a safeguard against bugs. For example, consider js_GetLengthProperty in jsarray.c. It passes to js_DefaultValue via js_ValueToECMAUint32/js_ValueToNumber/OBJ_DEFAULT_VALUE the result of OBJ_GET_PROPERTY. The latter can be made unrooted. Thus the code is safe only if js_DefaultValue is GC-save with unrooted obj argument. js_DefaultValue can tolerate unrooted flora if property lookup during search of "toString"/"valueOf" methods is GC safe. That in turn requires that all resolve hooks would not trigger any GC activities regarding these properties. If there is such guarantee, then things are OK. Otherwise one either has to add an extra rooting of obj or patch at least the following places with rooting guards: jsarray.c: js_GetLengthProperty sort_compare -- potentially unrooted bv and rval array_extra -- potentially unrooted v and rval2 jsstr.c: find_replen -- unsafe js_ValueToString(cx, rval); jsregexp.c regexp_setProperty -- unsafe js_ValueToNumber Now it would be a good idea to patch those places in any case, but the problem is that the list may not be full.

Comment on attachment 200648 [details] [diff] [review] trivial fix Fixed on the trunk. This is too easy not to take, but I'll let someone else mark a+. /be

verified firefox 1.5 rc2 linux/win32 2005-11-07

testcase+ to get this off my radar. when this is made public, i will check in the test.

Fix checked into the 1.7 branches.

verified firefox/1.7.13,1.8.0.1,1.8,1.9a1 win/linux/mac

v ff on 1.7.13, 1.8.0.1, 1.8, 1.9a1 on windows/linux/mac 20060221 builds, moz on 1.7.13 windows 20060221 build.

RCS file: /cvsroot/mozilla/js/tests/js1_5/GC/regress-313630.js,v done Checking in regress-313630.js; /cvsroot/mozilla/js/tests/js1_5/GC/regress-313630.js,v <-- regress-313630.js initial revision: 1.1 done