600853 - last ditch GC and OOM reporting with title lock held

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

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.

Comment 1

[Igor Bukanov]

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.

Comment 2

[Igor Bukanov]

Attachment: 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.

Comment 3

[Robert Sayre]

is this ready for review?

Comment 4

[Igor Bukanov]

(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.

Comment 5

[Igor Bukanov]

Attachment: 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.

Comment 6

[Johnny Stenback (:jst)]

Brendan, this is a beta8 blocker and a security bug, can you do this review?

Comment 7

[Jeff Walden [:Waldo]]

We killed titles within the last two weeks, so is this even a bug any more?

Comment 8

[Brendan Eich [:brendan]]

(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

Comment 9

[Johnny Stenback (:jst)]

Igor, is the attached patch good for older branches as well? If not, please upload a branch version.

Comment 10

[Johnny Stenback (:jst)]

Igor, ping?

Comment 11

[Igor Bukanov]

(In reply to comment #10)
> Igor, ping?

We need a new patch. I should have it this week.

Comment 12

[Johnny Stenback (:jst)]

Excellent, thanks Igor!

Comment 13

[Igor Bukanov]

Attachment: 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 14

[Igor Bukanov]

Comment on attachment 495512 [details] [diff] [review]
patch for 192 v1

The patch passes testing.

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 495512 [details] [diff] [review]
patch for 192 v1

Looks good -- any reason not to make HAS_TITLES_TO_SHARE be an inline hasTitlesToShare method?

/be

Comment 16

[Igor Bukanov]

(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.

Comment 17

[Brendan Eich [:brendan]]

Agh, you reminded me of the nameless horror that was L***C*****t!

/be

Comment 18

[Igor Bukanov]

Attachment: 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 19

[Daniel Veditz [:dveditz]]

Comment on attachment 495512 [details] [diff] [review]
patch for 192 v1

Approved for 1.9.2.14, a=dveditz for release-drivers

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 497823 [details] [diff] [review]
patch for 191

Approved for 1.9.1.17, a=dveditz for release-drivers

Comment 21

[Robert Sayre]

Igor, could you land this on the branch?

Comment 22

[Igor Bukanov]

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

Comment 23

[Bob Clary [:bc] (inactive)]

(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

Comment 24

[Igor Bukanov]

(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?

Comment 25

[Bob Clary [:bc] (inactive)]

1.9.1 only.

Comment 26

[Igor Bukanov]

The failing test cases revealed a bug in landed patches. So I clear the status the status and will post the regression fixes soon.

Comment 27

[Igor Bukanov]

Attachment: 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.

Comment 28

[Igor Bukanov]

Brendan - do you have time to look to the regression fix today? If not I will have to back out the landed patches.

Comment 29

[Brendan Eich [:brendan]]

Comment on attachment 504450 [details] [diff] [review]
regression fix

Sorry for the tardy r+ -- obvious fix now that you point it out.

/be

Comment 30

[Igor Bukanov]

Comment on attachment 504450 [details] [diff] [review]
regression fix

asking for regression fix approval

Comment 31

[LegNeato]

Comment on attachment 504450 [details] [diff] [review]
regression fix

Approving regression fix

Comment 32

[Igor Bukanov]

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

Comment 33

[Daniel Veditz [:dveditz]]

Since 2.0 is marked "unaffected" and the branches are patched this bug is FIXED, right?

Comment 34

[LegNeato]

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)

Comment 35

[Ludovic Hirlimann [:Usul]]

Shall this be reopened then ?