Last Comment Bug 353165 - GC hazard with xml_getMethod
: GC hazard with xml_getMethod
Status: VERIFIED FIXED
[sg:critical?]
: verified1.8.0.8, verified1.8.1
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Igor Bukanov
:
Mentors:
Depends on:
Blocks: 353365 354145 js1.7src
  Show dependency treegraph
 
Reported: 2006-09-18 08:29 PDT by Igor Bukanov
Modified: 2007-05-26 01:39 PDT (History)
2 users (show)
dveditz: blocking‑aviary1.0.9-
dveditz: blocking1.8.0.8+
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix (4.50 KB, patch)
2006-09-19 02:57 PDT, Igor Bukanov
brendan: review+
Details | Diff | Splinter Review
Fix b (4.49 KB, patch)
2006-09-19 13:19 PDT, Igor Bukanov
mtschrep: approval1.8.1+
Details | Diff | Splinter Review
e4x/GC/regress-353165.js (3.52 KB, text/plain)
2006-09-21 01:39 PDT, Bob Clary [:bc:]
no flags Details
Fix b for 1.8.0 branch (4.49 KB, patch)
2006-10-09 14:43 PDT, Igor Bukanov
dveditz: approval1.8.0.8+
Details | Diff | Splinter Review

Description Igor Bukanov 2006-09-18 08:29:07 PDT
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 2006-09-19 02:49:59 PDT
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 2006-09-19 02:57:01 PDT
Created attachment 239145 [details] [diff] [review]
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] 2006-09-19 12:13:28 PDT
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] 2006-09-19 12:14:19 PDT
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 2006-09-19 13:00:01 PDT
(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 2006-09-19 13:19:50 PDT
Created attachment 239229 [details] [diff] [review]
Fix b

Patch to commit that uses anonymous enum instead of preprocessor defines.
Comment 7 Igor Bukanov 2006-09-19 14:15:44 PDT
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] 2006-09-19 16:03:05 PDT
Restoring lost blocking flag
Comment 9 Brendan Eich [:brendan] 2006-09-19 22:31:03 PDT
(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:] 2006-09-21 01:39:28 PDT
Created attachment 239482 [details]
e4x/GC/regress-353165.js
Comment 11 Bob Clary [:bc:] 2006-09-21 16:25:07 PDT
verified fixed 1.9 20060921 windows/mac*/linux
Comment 12 Igor Bukanov 2006-10-09 03:10:34 PDT
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 2006-10-09 05:32:53 PDT
Fix b apply to branch?  If so please nom..
Comment 14 Igor Bukanov 2006-10-09 07:54:33 PDT
Comment on attachment 239229 [details] [diff] [review]
Fix b

This is a fix for exploitable GC hazard.
Comment 15 Mike Schroepfer 2006-10-09 08:12:21 PDT
Comment on attachment 239229 [details] [diff] [review]
Fix b

Approved for RC2
Comment 16 Mike Schroepfer 2006-10-09 08:12:27 PDT
Comment on attachment 239229 [details] [diff] [review]
Fix b

Approved for RC3
Comment 17 Igor Bukanov 2006-10-09 08:28:20 PDT
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] 2006-10-09 10:53:37 PDT
Comment on attachment 239229 [details] [diff] [review]
Fix b

approved for 1.8.0 branch, a=dveditz for drivers
Comment 19 Igor Bukanov 2006-10-09 14:43:13 PDT
Created attachment 241750 [details] [diff] [review]
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] 2006-10-09 16:03:43 PDT
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 2006-10-09 16:30:51 PDT
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:] 2006-10-11 02:26:52 PDT
verified fixed 1.8 20061010 windows/mac*/linux
Comment 23 alice nodelman [:alice] [:anode] 2006-10-12 11:20:46 PDT
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] 2006-11-01 17:41:43 PST
Bug not relevant to aviary/moz1.7 branches
Comment 25 Bob Clary [:bc:] 2007-02-08 16:29:14 PST
/cvsroot/mozilla/js/tests/e4x/extensions/regress-353165.js,v  <--  regress-353165.js

moved to extensions/ due to getter

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