353165 - GC hazard with xml_getMethod

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

Consider the following example:

var set = new XML('<a><b>text</b></a>').children('b');
var counter = 0;
Object.prototype.unrooter getter = function() {
  ++counter;
  if (counter == 5) {
    set[0] = new XML('<c/>');
    if (typeof gc == "function") { 
      gc();
      var tmp = Math.sqrt(2), tmp2;
      for (i = 0; i != 50000; ++i)
        tmp2 = tmp / 2;
    }
  }
  return undefined;
}

set.unrooter();

Currently it crashes when accessing GC-ed xml object. This happens because xml_getMethod does not root the first child of XML-list when it searches for a method there. The script exploits it defining a getter in Object.prototype that at the right moment removes the child from the list cutting all its roots. Thus the following forced GC collects the child leading to a crash during toString conversion of the child.

Comment 1

[Igor Bukanov]

The previous test case on Linux may not crash as Math.sqrt(2) when reinterpreted as JSObject could produce a valid map. So here is a better one:

var expected = "SOME RANDOM TEXT";

var set = <a><b>{expected}</b></a>.children('b');
var counter = 0;

function unrooter_impl()
{
  return String(this);
}

Object.prototype.unrooter getter = function() {
  ++counter;
  if (counter == 7)
    return unrooter_impl;
  if (counter == 5) {
    set[0] = new XML('<c/>');
    if (typeof gc == "function") { 
      gc();
      var tmp = 1e500, tmp2;
      for (i = 0; i != 50000; ++i)
        tmp2 = tmp / 1.1;
    }
  }
  return undefined;
}

var actual = set.unrooter();
print(actual === expected);

Comment 2

[Igor Bukanov]

Attachment: Fix

The fix tries to be proactive to be new-feature-proof. But it also demonstrates that getMethod is API with bad signature. It should really demand rooted JSObject ** as argument to place the result.

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 239145 [details] [diff] [review]
Fix

> static JSObject *
> xml_getMethod(JSContext *cx, JSObject *obj, jsid id, jsval *vp)

Please file a bug on changing the getMethod signature.  We should bite the bullet and always use out parameters, and for type safety do something even stronger than what you proposed (special attributes checked by a separate tool): require a "rooter" or "holder" type to be passed by its address.

In my Unix kernel hacking days, with good example code from Sun and 4.4BSD, SGI moved toward a uniform pattern for all out parameters: always return error status via the return value (errno in the kernel case), with out parameters for result pointers.  In Mozilla, with XPCOM, this has led to bloat for useless status return values and extra memory bandwidth for reference-counted pointers via out parameters, and it costs in source and compiled code and in cycles.  Many of the abuses of XPCOM should not even add a reference, rather they can return a reference guaranteed to live as long as the object whose method was being called.

With exact GC, things are different, and I'm in favor of moving to out parameters.

> {
>     JSXML *xml;
>-    jsval fval;
>+    JSTempValueRooter tvr;
>+    jsval roots[2];
>+#define FUN_ROOT 0
>+#define OBJ_ROOT   1

Nit: line up the values.

/be

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 239145 [details] [diff] [review]
Fix


>+    jsval roots[2];
>+#define FUN_ROOT 0
>+#define OBJ_ROOT   1

Or even nicer, to avoid #undef unsightliness, use an anonymous enum.

/be

Comment 5

[Igor Bukanov]

(In reply to comment #3)
> (From update of attachment 239145 [details] [diff] [review] [edit])
> > static JSObject *
> > xml_getMethod(JSContext *cx, JSObject *obj, jsid id, jsval *vp)
> 
> Please file a bug on changing the getMethod signature.  

See bug 353365. In fact this how I started the patch but then it grew too big to qualify for a security-only fix :(

> With exact GC, things are different, and I'm in favor of moving to out
> parameters.

IMO this has nothing to do with the way to pass parameters. Rather it is about giving the callee an access to a rooted location that the caller has to have in any case. In C this translates to passing the address. In C++ one can still use return values but hide the rooting besides a wrapper class similar to auto_ptr<T>.

Comment 6

[Igor Bukanov]

Attachment: Fix b

Patch to commit that uses anonymous enum instead of preprocessor defines.

Comment 7

[Igor Bukanov]

I committed the patch from comment 6 to the trunk:

Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.126; previous revision: 3.125
done

Comment 8

[Daniel Veditz [:dveditz]]

Restoring lost blocking flag

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #5)
> > With exact GC, things are different, and I'm in favor of moving to out
> > parameters.
> 
> IMO this has nothing to do with the way to pass parameters. Rather it is about
> giving the callee an access to a rooted location that the caller has to have in
> any case. In C this translates to passing the address.

We're talking about C code, so it does have to do with the way to pass parameters in this codebase.

> In C++ one can still use
> return values but hide the rooting besides a wrapper class similar to
> auto_ptr<T>.

Sure.  Should we switch to a sane dialect of C++?  I'm not against it by nature.  SpiderMonkey has lots of consumers (some unknown to us), but in 2006 one can finally count on most of the ISO C++ standard to work on GCC and MSVC-modern or VS.whatever.  I'd want to have a sane plan before committing any changes, and run it by the m.d.t.js-engine newsgroup and as many embedders as we can reach first.

/be 

Comment 10

[Bob Clary [:bc] (inactive)]

Attachment: e4x/GC/regress-353165.js

Comment 11

[Bob Clary [:bc] (inactive)]

verified fixed 1.9 20060921 windows/mac*/linux

Comment 12

[Igor Bukanov]

This is also an exploitable GC hazard in XML. The patch from 1.8.1 bug 354145 depends on this bug fixed.

Comment 13

[Mike Schroepfer]

Fix b apply to branch?  If so please nom..

Comment 14

[Igor Bukanov]

Comment on attachment 239229 [details] [diff] [review]
Fix b

This is a fix for exploitable GC hazard.

Comment 15

[Mike Schroepfer]

Comment on attachment 239229 [details] [diff] [review]
Fix b

Approved for RC2

Comment 16

[Mike Schroepfer]

Comment on attachment 239229 [details] [diff] [review]
Fix b

Approved for RC3

Comment 17

[Igor Bukanov]

I committed the patch from comment 6 to MOZILLA_1_8_BRANCH:

Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.50.2.50; previous revision: 3.50.2.49
done

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 239229 [details] [diff] [review]
Fix b

approved for 1.8.0 branch, a=dveditz for drivers

Comment 19

[Igor Bukanov]

Attachment: Fix b for 1.8.0 branch

The patch for trunk/1.8.1 does not apply to 1.8.0 as it lacks JS_ARRAY_LENGTH macro and there AFAICS whitespace issues with GetFinctiom. So here is an update.

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 241750 [details] [diff] [review]
Fix b for 1.8.0 branch

moving approval to the correct branch patch

Comment 21

[Igor Bukanov]

I committed the patch from comment 19 to MOZILLA_1_8_0_BRANCH:

Checking in jsxml.c;
/cvsroot/mozilla/js/src/jsxml.c,v  <--  jsxml.c
new revision: 3.50.2.15.2.20; previous revision: 3.50.2.15.2.19
done

Comment 22

[Bob Clary [:bc] (inactive)]

verified fixed 1.8 20061010 windows/mac*/linux

Comment 23

[alice nodelman [:alice] [:anode]]

verified fixed 1.8.0.8 on mac/win/linux
firefox_1.8.0.8pre_2006101114_dbg
firefox_1.8.0.8pre_2006101114_opt

Comment 24

[Daniel Veditz [:dveditz]]

Bug not relevant to aviary/moz1.7 branches

Comment 25

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/e4x/extensions/regress-353165.js,v  <--  regress-353165.js

moved to extensions/ due to getter