js_ConstructObject in jsobj.c contain the following code: if (!js_FindConstructor(cx, parent, clasp->name, &cval)) return NULL; ... if (!proto) { if (!OBJ_GET_PROPERTY(cx, ctor, ATOM_TO_JSID(cx->runtime->atomState .classPrototypeAtom), &rval)) { return NULL; } if (JSVAL_IS_OBJECT(rval)) proto = JSVAL_TO_OBJECT(rval); } obj = js_NewObject(cx, clasp, proto, parent); if (!obj) return NULL; if (!js_InternalConstruct(cx, obj, cval, argc, argv, &rval)) goto bad; This can lead to a GC-unrooted access if the following scenario can be realized: 1. script redefine an Object or Array constructor with one that defines a getter for for ".prototype". 2. The getter tries hard to remove the original ctor from object graph so when OBJ_GET_PROPERTY returns, ctor becomes unrooted. 3. js_NewObject triggers GC that collects ctor causing access to freed memory in js_InternalConstruct. At this time I can not prove that this is realizable with a script and in fact can be wrong especially given that it is 23:30 in Norway ;). But it is bette to be 100% sure.

Any getter will be called via js_InternalGetOrSet, which will call js_Invoke, which protects the object on which the getter was defined, namely ctor in js_ConstructObject, via cx->fp->thisp. So this looks safe to me. In general, the |this| parameter, or in native API signatures the 2nd param, obj, after cx, should be safe during invocation. INVALID? I'll let Igor mark it. /be

(In reply to comment #1) > the |this| parameter, or in native API signatures the 2nd param, obj, after cx, > should be safe during invocation. > But what about after the invocation? The script can try to make sure that ctor is no longer belong to live object's graph nor it is found in newborn array. So when the getter returns the following NewObject can GC it.

Here is a test case that generates segfault in js shell compiled with WAY_TOO_MUCH_GC defined to ensure that GC does run in NewObject. var SavedArray = Array; Array = { get prototype() { print("**** GETTER ****"); Array = SavedArray; new Array(); new Array(); new Object(); new Object(); return SavedArray.prototype; } }; new Object(); var y = "test".split('');

Here is a test case that segfaults the shell compiled with standard options without using any _MUCH_GC. It explores the fact that js_ConstructObject query 'prototype' property twice if the first attempt does not return an object. So when the first getter returns and make ctor unrooted the second getter forces GC. var SavedArray = Array; function Redirector() { } Redirector.prototype = 1; Redirector.__defineGetter__('prototype', function() { print("REDIRECTOR"); gc(); return SavedArray.prototype; }); Array = Function('print("Constructor")'); Array.prototype = 1; Array.__defineGetter__('prototype', function() { print("**** GETTER ****"); Array = Redirector; gc(); new Object(); new Object(); return undefined; }); new Object(); var y = "test".split('');

I should sleep more, too. I'll take this bug. /be

So far I was not able to come up with a hack to root jsval in js_ConstructObject without touching too many files. So the idea is to add yet another API for local roots in C functions. The patch adds to JSConfig a linked list with cells allocated on C stack to hold both the rooted value and the link field. In this way initialization/release of such is very cheap compared with LocalRootScope. In addition, since the initialization never fails, the code is slightly simpler. Plus the patch can be trivially back-ported to 1.0.8.

(In reply to comment #6) > Created an attachment (id=210408) [edit] > Something like a fix > > So far I was not able to come up with a hack to root jsval in > js_ConstructObject without touching too many files. So the idea is to add yet > another API for local roots in C functions. Too funny -- I hacked something up very much like this last night. Patch in a minute. I like your thinking, Bukanov! ;-) /be

I did something like what XPConnect does (it has C++ to make this all sweeter with auto-storage-class helpers whose dtors do the cleanup) and allowed for an array of jsvals to be protected by each rooter. There are places in SpiderMonkey crying out for this instead of local root scope (which has its place too) or js_AllocStack abusage. I show one such in jsapi.c here. The affected jsapi.c fix needs to be backported to the aviary-1.0.1 and mozilla-1.7.x branches as well, and backporting is now easier because with this patch, that fix doesn't drag in the local-root-scope stuff. mrbkap, feel free to r+ as well. /be

Comment on attachment 210408 [details] [diff] [review] Something like a fix Obviously I like this patch a lot :-). But I did write the second patch up last night after taking the bug, so I'll finish paying my dues here if that's all right with you. /be

Comment on attachment 210413 [details] [diff] [review] proposed fix It seems most of the cases for JS_TEMP_ROOT would be for one jsval. So perhaps it is better to have separated JS_TEMP_ROOT and JS_TEMP_ROOT_ARRAY?

(In reply to comment #10) > (From update of attachment 210413 [details] [diff] [review] [edit]) > It seems most of the cases for JS_TEMP_ROOT would be for one jsval. So perhaps > it is better to have separated JS_TEMP_ROOT and JS_TEMP_ROOT_ARRAY? Trick would be to fold the storage for the one jsval into the rooter. Could tag the jsval *value pointer. I'll try it out. /be

Thanks to mrbkap for keeping me from being lazy about names and macro factoring and comments and stuff. I'm tired now! /be

Fixed on trunk. I'll nominate/plus for branches when shaver reviews, or shaver: feel free to nominate then for 1.8.0.2 and plus for 1.8.1. /be

This checkin brought SeaMonkey Windows tinderbox "creature" Tp from 191ms to 235ms. I think it did something similar to Firefox Windows tinderbox "pacifica". Also, all 7 DOMWindows created when just opening and closing Firefox now leak (measured with leak-gauge.pl). I'm not sure that started happening at the same time, but it seems like a reasonable guess.

The last patch (with my review+ ) forgot to remove js_EnterLocalRootScope from the JS_InitClass jsapi.c.

(In reply to comment #14) > I'm not sure that started happening at the same > time, but it seems like a reasonable guess. Yes, it must be it: the lines of code that were not removed would prevent from GC almost everything.

This is a combined patch with the extra fix for references.

I AM TOO STUPID TO LIVE!!!!! Fixed on trunk, sorry. /be

Comment on attachment 210464 [details] [diff] [review] For references: proposed fix, v2 + the last fix >+ if (!js_InternalConstruct(cx, obj, cval, argc, argv, &rval)) { >+ cx->newborn[GCX_OBJECT] = NULL; >+ obj = NULL; >+ goto out; >+ } >+ >+ if (!JSVAL_IS_PRIMITIVE(rval)) >+ obj = JSVAL_TO_OBJECT(rval); >+out: >+ JS_POP_TEMP_ROOT(cx, &tvr); >+ return obj; Put the if (!JSVAL_IS_PRIMITIVE(rval)) obj = JSVAL_TO_OBJECT(rval); in the else clause of the InternalConstruct call, preferring if/else over gotos? Could also invert the sense of the test and reorder, so that the else is error-handling, I guess. Either way, or neither, sr=shaver. How much trunk time does this want before we call it branch-worthy?

(In reply to comment #19) > Put the > > if (!JSVAL_IS_PRIMITIVE(rval)) > obj = JSVAL_TO_OBJECT(rval); > > in the else clause of the InternalConstruct call, preferring if/else over > gotos? Could also invert the sense of the test and reorder, so that the else > is error-handling, I guess. I thought a lot about this, but it's less future idiot proof (/me proof) to use the goto out pattern. > Either way, or neither, sr=shaver. I'll let the patch Igor attached stand for branch approval. > How much trunk time does this want before we call it branch-worthy? A week's more than enough, but obviously last night to today was not! Still sleepy, teething baby at home. /be

Comment on attachment 210464 [details] [diff] [review] For references: proposed fix, v2 + the last fix a=dveditz for drivers, please add fixed-aviary1.0.8, fixed1.7.13, and fixed1.8.0.2 keywords when checked in to the branches

checked in 2006-02-09 by brendan on aviary and 1.7 branches.

Note: I couldn't confirm the crash in the browser on winxp with the 2006-02-02 trunk build.

(In reply to comment #24) > > Note: I couldn't confirm the crash in the browser on winxp with the 2006-02-02 > trunk build. The test case assumes that gc() does trigger GC. How is it defined for the browser case?

(In reply to comment #25) function gc() { try { // Thanks to dveditz netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect'); var jsdIDebuggerService = Components.interfaces.jsdIDebuggerService; var service = Components.classes['@mozilla.org/js/jsd/debugger-service;1']. getService(jsdIDebuggerService); service.GC(); } catch(ex) { // Thanks to igor.bukanov@gmail.com for (var i = 0; i != 1 << 15; ++i) { new Object(); } } }

In the browser, I think you can also just open and close a window to trigger a GC.

(In reply to comment #26) > for (var i = 0; i != 1 << 15; ++i) > { > new Object(); > } That explains why you do not see any crashes: the loop here overwrites unrooted object by another valid object. It is better to have something like: var tmp = Math.PI * 1e500, tmp2; for (var i = 0; i != 1 << 15; ++i) { tmp2 = tmp * 1.5; } That populates the heap with double instances that when misread as an object or string would generate a crash unless one extremely unlucky.

Partly was having a problem due to an uncaught recursion error in printStatus. I changed the test to remove the printStatus calls and no longer see the error, but I still don't crash in a 2006-02-02 trunk build on winxp. I made the change to gc() and tested with a profile which allowed the jsd version of gc to be called and another profile where Igor's version was called.

still a problem on 1.8 and 1.8.0.1. Someone care to check attachment 210464 [details] [diff] [review] in on the 1.8 and 1.8.0.1 branches?

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

v moz 1.7.13 windows 20060221

Fixed on the 1.8* branches. /be

v ff 1.8.0.1/1.8/1.9 20060302 win/linux/mac

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