The default bug view has changed. See this FAQ.

GC hazard in js_ConstructObject from jsobj.c

VERIFIED FIXED in mozilla1.9alpha1

Status

()

Core
JavaScript Engine
P1
critical
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: Igor Bukanov, Assigned: brendan)

Tracking

({verified1.7.13, verified1.8.0.2, verified1.8.1})

Trunk
mozilla1.9alpha1
verified1.7.13, verified1.8.0.2, verified1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.7.13 +
blocking-aviary1.0.8 +
blocking1.8.0.2 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical][rft-dl])

Attachments

(5 attachments, 3 obsolete attachments)

(Reporter)

Description

11 years ago
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.
(Assignee)

Comment 1

11 years ago
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
(Reporter)

Comment 2

11 years ago
(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.
(Reporter)

Comment 3

11 years ago
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('');
Severity: normal → critical
(Reporter)

Comment 4

11 years ago
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('');
(Assignee)

Comment 5

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

/be
Assignee: general → brendan
Flags: blocking1.8.0.2+
Flags: blocking-aviary1.0.8+
Priority: -- → P1
Target Milestone: --- → mozilla1.9alpha
(Reporter)

Comment 6

11 years ago
Created attachment 210408 [details] [diff] [review]
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.
Attachment #210408 - Flags: review?(brendan)
(Assignee)

Comment 7

11 years ago
(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
(Assignee)

Comment 8

11 years ago
Created attachment 210413 [details] [diff] [review]
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
Attachment #210413 - Flags: superreview?(shaver)
Attachment #210413 - Flags: review?(igor.bukanov)
(Assignee)

Comment 9

11 years ago
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
Attachment #210408 - Flags: review?(brendan)
(Reporter)

Comment 10

11 years ago
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?
Attachment #210413 - Flags: review?(igor.bukanov) → review+
(Assignee)

Comment 11

11 years ago
(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
(Assignee)

Comment 12

11 years ago
Created attachment 210425 [details] [diff] [review]
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
Attachment #210425 - Flags: superreview?(shaver)
Attachment #210425 - Flags: review?(igor.bukanov)
(Assignee)

Updated

11 years ago
Attachment #210413 - Attachment is obsolete: true
Attachment #210413 - Flags: superreview?(shaver)
(Reporter)

Updated

11 years ago
Attachment #210425 - Flags: review?(igor.bukanov) → review+
(Assignee)

Comment 13

11 years ago
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
Blocks: 309169
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 14

11 years ago
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.
(Reporter)

Comment 15

11 years ago
Created attachment 210462 [details] [diff] [review]
Removal of js_EnterLocalRootScope

The last patch (with my review+ ) forgot to remove js_EnterLocalRootScope from the JS_InitClass jsapi.c.
Attachment #210462 - Flags: review?(brendan)
(Reporter)

Comment 16

11 years ago
(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.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 17

11 years ago
Created attachment 210464 [details] [diff] [review]
For references: proposed fix, v2 + the last fix

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

Comment 18

11 years ago
I AM TOO STUPID TO LIVE!!!!!

Fixed on trunk, sorry.

/be
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
(Assignee)

Updated

11 years ago
Attachment #210425 - Attachment is obsolete: true
Attachment #210425 - Flags: superreview?(shaver)
(Assignee)

Updated

11 years ago
Attachment #210462 - Flags: review?(brendan) → review+
(Assignee)

Updated

11 years ago
Attachment #210464 - Flags: superreview?(shaver)
Attachment #210464 - Flags: review+
Attachment #210464 - Flags: branch-1.8.1+
Attachment #210464 - Flags: approval1.8.0.2?
Attachment #210464 - Flags: approval1.7.13?
Attachment #210464 - Flags: approval-aviary1.0.8?
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?
Attachment #210464 - Flags: superreview?(shaver) → superreview+
(Assignee)

Comment 20

11 years ago
(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 

Depends on: 325613
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
Attachment #210464 - Flags: approval1.8.0.2?
Attachment #210464 - Flags: approval1.8.0.2+
Attachment #210464 - Flags: approval1.7.13?
Attachment #210464 - Flags: approval1.7.13+
Attachment #210464 - Flags: approval-aviary1.0.8?
Attachment #210464 - Flags: approval-aviary1.0.8+
(Assignee)

Comment 22

11 years ago
Created attachment 211335 [details] [diff] [review]
proposed fix for AVIARY_1_0_1_01252004_BRANCH
Attachment #211335 - Flags: review?(mrbkap)
Attachment #211335 - Flags: approval1.7.13+
Attachment #211335 - Flags: approval-aviary1.0.8+

Updated

11 years ago
Attachment #211335 - Flags: review?(mrbkap) → review+
Flags: blocking1.7.13+

Comment 23

11 years ago
checked in 2006-02-09 by brendan on aviary and 1.7 branches.
Keywords: fixed-aviary1.0.8, fixed1.7.13

Comment 24

11 years ago
Created attachment 211549 [details]
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.

Updated

11 years ago
Flags: testcase+
(Reporter)

Comment 25

11 years ago
(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

11 years ago
(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.
(Reporter)

Comment 28

11 years ago
(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

11 years ago
Created attachment 211812 [details]
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.
Attachment #211549 - Attachment is obsolete: true

Comment 30

11 years ago
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?
Flags: blocking1.8.1?

Comment 31

11 years ago
verified firefox/1.7.13,1.9a1 win/linux/mac, still crashes firefox/1.8.0.1, firefox/1.8.
Status: RESOLVED → VERIFIED
Keywords: fixed-aviary1.0.8 → verified-aviary1.0.8

Comment 32

11 years ago
v moz 1.7.13 windows 20060221
Keywords: fixed1.7.13 → verified1.7.13
(Assignee)

Comment 33

11 years ago
Fixed on the 1.8* branches.

/be
Keywords: fixed1.8.0.2, fixed1.8.1

Updated

11 years ago
Whiteboard: [rft-dl]

Comment 34

11 years ago
v ff 1.8.0.1/1.8/1.9 20060302 win/linux/mac
Keywords: fixed1.8.0.2, fixed1.8.1 → verified1.8.0.2, verified1.8.1
Whiteboard: [rft-dl] → [sg:critical][rft-dl]
Group: security

Comment 35

11 years ago
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

Updated

10 years ago
Flags: blocking1.8.1?
You need to log in before you can comment on or make changes to this bug.