Bugzilla@Mozilla – Bug 600853
last ditch GC and OOM reporting with title lock held
Last modified: 2011-03-29 19:26:08 PDT
Summon comment box
obj_toSource contains: ok = obj->lookupProperty(cx, id, &obj2, &prop); ... idstr = js_ValueToString(cx, IdToValue(id)); ... JS_UNLOCK_OBJ(cx, obj2); Here js_ValueToString allocates a string when id is an integer. This implies that the last-ditch GC and OOM reporting will run with title lock held. Similarly JS_SetWatchPoint contains: if (!js_LookupProperty(cx, obj, propid, &pobj, &prop)) ... watcher = js_WrapWatchedSetter(cx, propid, shape->attributes(), shape->setter()); ... JS_UNLOCK_OBJ(cx, obj); Here js_WrapWatchedSetter allocates at least new Function leading to the same bug. Marking as security-sensitive as this is related to the bug 569162 and in case other places contains the similar pattern but when an arbitrary JS code can run.
The bugs in the comment 0 are a real hazards if the corresponding code is run against thread-shared objects. If another thread is waiting for the current thread to share the objects, then, as a consequence of changes in the big 477627, the last-ditch GC shares the object. That allows that thread to run until it suspends its the request. That other thread can then delete the shapes and let the GC to collect them before the first thread accesses them.
Created attachment 481920 [details] [review] v1 There are a lot of places where the the GC allocation happens when the object is locked. It is rather non-trivial and often very error-prone to change the corresponding code. So the patch takes simpler approach of just disabling the last-ditch GC when the current thread has any titles to share.
is this ready for review?
(In reply to comment #3) > is this ready for review? It seems the try server gives more orange than usual for this patch. I am investigating that.
Created attachment 482832 [details] [review] v2 here is a rebased patch. It passes the tryserver and the oranges that I have seen comes from unrelated patch. To allow for simpler detection of waiting titles for the current thread the patch moves the title sharing list into JSThread.
Brendan, this is a beta8 blocker and a security bug, can you do this review?
We killed titles within the last two weeks, so is this even a bug any more?
(In reply to comment #7) > We killed titles within the last two weeks, so is this even a bug any more? For the branches. /be
Igor, is the attached patch good for older branches as well? If not, please upload a branch version.
Igor, ping?
(In reply to comment #10) > Igor, ping? We need a new patch. I should have it this week.
Excellent, thanks Igor!
Created attachment 495512 [details] [review] patch for 192 v1 Here is a minimal patch. It makes sure that the GC allocation paths do not invoke the last ditch GC if the current thread has titles to share. I will ask for a review after more testing.
Comment on attachment 495512 [details] [review] patch for 192 v1 The patch passes testing.
Comment on attachment 495512 [details] [review] patch for 192 v1 Looks good -- any reason not to make HAS_TITLES_TO_SHARE be an inline hasTitlesToShare method? /be
(In reply to comment #15) > any reason not to make HAS_TITLES_TO_SHARE be an inline > hasTitlesToShare method? The reason is the patch minimality to avoid extra #ifdef C++ as LiveConnect is alive and kicking on 1.9.2.
Agh, you reminded me of the nameless horror that was L***C*****t! /be
Created attachment 497823 [details] [review] patch for 191 191 version required a minimal backporting from 192. Here is a plain diff between 192 and 191 patches: > @@ -1807,8 +1809,9 @@ IsGCThresholdReached(JSRuntime *rt) 129c122 < - rt->gcBytes >= rt->gcTriggerBytes; --- > - rt->gcBytes / rt->gcTriggerFactor >= rt->gcLastBytes / 100; 131c124,125 < + rt->gcBytes >= rt->gcTriggerBytes) && !HAS_TITLES_TO_SHARE(cx); --- > + rt->gcBytes / rt->gcTriggerFactor >= rt->gcLastBytes / 100) && > + !HAS_TITLES_TO_SHARE(cx); 134,135c128,129 < template <class T> static JS_INLINE T* < @@ -1840,7 +1842,7 @@ NewGCThing(JSContext *cx, uintN flags) --- > @@ -2298,9 +2301,10 @@ js_AddAsGCBytes(JSContext *cx, size_t sz > JSRuntime *rt; > > rt = cx->runtime; > - if (rt->gcBytes >= rt->gcMaxBytes || > - sz > (size_t) (rt->gcMaxBytes - rt->gcBytes) || > - IsGCThresholdReached(rt)) { > + if ((rt->gcBytes >= rt->gcMaxBytes || > + sz > (size_t) (rt->gcMaxBytes - rt->gcBytes) || > + IsGCThresholdReached(cx)) && > + !HAS_TITLES_TO_SHARE(cx)) { > if (JS_ON_TRACE(cx)) { > /* > * If we can't leave the trace, signal OOM condition, otherwise Here 1.9.1-only js_AddAsGCBytes required an extra change to disable GC when HAS_TITLES_TO_SHARE is true.
Comment on attachment 495512 [details] [review] patch for 192 v1 Approved for 1.9.2.14, a=dveditz for release-drivers
Comment on attachment 497823 [details] [review] patch for 191 Approved for 1.9.1.17, a=dveditz for release-drivers
Igor, could you land this on the branch?
landed on branches: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3b97e967b6cc http://hg.mozilla.org/releases/mozilla-1.9.1/rev/81e4317eb7f8
(In reply to comment #22) > http://hg.mozilla.org/releases/mozilla-1.9.1/rev/81e4317eb7f8 FYI, I think this is related to an ./shell.js:701 out of memory error (jit, shell only) I started seeing with 2 tests on 1.9.1 linux 32bit with 4G (the only jstests I still run regularly). It seems this would be expected behavior from this patch in some cases and probably is not of concern. I thought I would mention it though. The tests in question are: js1_5/extensions/regress-350531.js and js1_6/extensions/regress-456826.js
(In reply to comment #23) э> FYI, I think this is related to an ./shell.js:701 out of memory error (jit, > shell only) I started seeing with 2 tests on 1.9.1 linux 32bit with 4G (the > only jstests I still run regularly). Is this only n 1.9.1 or 1.9.2 is also affected?
1.9.1 only.
The failing test cases revealed a bug in landed patches. So I clear the status the status and will post the regression fixes soon.
Created attachment 504450 [details] [review] regression fix When NewArena returns NULL, it signals to to the GC if possible or report OOM. The essence of the landed patch was to prevent that GC form happening when the current thread has titles to share. So the idea was to ignore the heap size limit in NewArena with to-be-shared-titles and report OOM when when when NewArena failed to get new GC arenas via mmap. Needless to say, the landed patch did exactly the opposite - it would ignore the limit when there were no waiting titles and would enforce the limit where there are titles. This patch fixes the typo. It applies to both ranches as-is.
Brendan - do you have time to look to the regression fix today? If not I will have to back out the landed patches.
Comment on attachment 504450 [details] [review] regression fix Sorry for the tardy r+ -- obvious fix now that you point it out. /be
Comment on attachment 504450 [details] [review] regression fix asking for regression fix approval
Comment on attachment 504450 [details] [review] regression fix Approving regression fix
I landed regression fixes: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/e19b68b68daf http://hg.mozilla.org/releases/mozilla-1.9.1/rev/237c9fe03984
Since 2.0 is marked "unaffected" and the branches are patched this bug is FIXED, right?
I backed this out of 1.9.2 (default and 1.9.2.14 relbranch): http://hg.mozilla.org/releases/mozilla-1.9.2/rev/844ae0377afa http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a50a49f952c0 (see bug 633869)
Shall this be reopened then ?