325269 - GC hazard in js_ConstructObject from jsobj.c

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Igor Bukanov]

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.

Comment 1

[Brendan Eich [:brendan]]

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

Comment 2

[Igor Bukanov]

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

Comment 3

[Igor Bukanov]

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('');

Comment 4

[Igor Bukanov]

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('');

Comment 5

[Brendan Eich [:brendan]]

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

/be

Comment 6

[Igor Bukanov]

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

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.

Comment 7

[Brendan Eich [:brendan]]

(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

Comment 8

[Brendan Eich [:brendan]]

Attachment: proposed fix

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 9

[Brendan Eich [:brendan]]

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 10

[Igor Bukanov]

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?

Comment 11

[Brendan Eich [:brendan]]

(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

Comment 12

[Brendan Eich [:brendan]]

Attachment: proposed fix, v2

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

/be

Comment 13

[Brendan Eich [:brendan]]

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

Comment 14

[Jesse Ruderman]

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.

Comment 15

[Igor Bukanov]

Attachment: Removal of js_EnterLocalRootScope

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

Comment 16

[Igor Bukanov]

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

Comment 17

[Igor Bukanov]

Attachment: For references: proposed fix, v2 + the last fix

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

Comment 18

[Brendan Eich [:brendan]]

I AM TOO STUPID TO LIVE!!!!!

Fixed on trunk, sorry.

/be

Comment 19

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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?

Comment 20

[Brendan Eich [:brendan]]

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

[Daniel Veditz [:dveditz]]

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

Comment 22

[Brendan Eich [:brendan]]

Attachment: proposed fix for AVIARY_1_0_1_01252004_BRANCH

Comment 23

[Bob Clary [:bc] (inactive)]

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

Comment 24

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/Regress/regress-325269.js

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

Comment 25

[Igor Bukanov]

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

Comment 26

[Bob Clary [:bc] (inactive)]

(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 [e-mail]
    for (var i = 0; i != 1 << 15; ++i) 
    {
      new Object();
    }
  }
}

Comment 27

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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

Comment 28

[Igor Bukanov]

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

Comment 29

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/Regress/regress-325269.js

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.

Comment 30

[Bob Clary [:bc] (inactive)]

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?

Comment 31

[Bob Clary [:bc] (inactive)]

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

Comment 32

[Bob Clary [:bc] (inactive)]

v moz 1.7.13 windows 20060221

Comment 33

[Brendan Eich [:brendan]]

Fixed on the 1.8* branches.

/be

Comment 34

[Bob Clary [:bc] (inactive)]

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

Comment 35

[Bob Clary [:bc] (inactive)]

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