The default bug view has changed. See this FAQ.

GC hazard with xml_getMethod

VERIFIED FIXED

Status

()

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

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

({verified1.8.0.8, verified1.8.1})

Trunk
verified1.8.0.8, verified1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking-aviary1.0.9 -
blocking1.8.0.8 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?])

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

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

Updated

11 years ago
Assignee: general → igor.bukanov
(Assignee)

Updated

11 years ago
Severity: normal → critical
Flags: blocking1.8.1.1?
Flags: blocking1.8.0.8?
(Assignee)

Comment 1

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

Comment 2

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

Updated

11 years ago
Blocks: 353365
(Assignee)

Comment 5

11 years ago
(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>.
No longer blocks: 353365
(Assignee)

Updated

11 years ago
Blocks: 353365
(Assignee)

Comment 6

11 years ago
Created attachment 239229 [details] [diff] [review]
Fix b

Patch to commit that uses anonymous enum instead of preprocessor defines.
Attachment #239145 - Attachment is obsolete: true
(Assignee)

Comment 7

11 years ago
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
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Restoring lost blocking flag
Flags: blocking1.8.0.9?
(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

11 years ago
Created attachment 239482 [details]
e4x/GC/regress-353165.js

Updated

11 years ago
Flags: in-testsuite+

Comment 11

11 years ago
verified fixed 1.9 20060921 windows/mac*/linux
Status: RESOLVED → VERIFIED
(Assignee)

Comment 12

11 years ago
This is also an exploitable GC hazard in XML. The patch from 1.8.1 bug 354145 depends on this bug fixed.
Blocks: 354145
Flags: blocking1.8.1?
Flags: blocking1.8.1.1?
Flags: blocking1.8.0.9?
Flags: blocking1.8.0.8?
(Assignee)

Updated

11 years ago
Blocks: 355044

Comment 13

11 years ago
Fix b apply to branch?  If so please nom..
(Assignee)

Comment 14

11 years ago
Comment on attachment 239229 [details] [diff] [review]
Fix b

This is a fix for exploitable GC hazard.
Attachment #239229 - Flags: approval1.8.1?

Comment 15

11 years ago
Comment on attachment 239229 [details] [diff] [review]
Fix b

Approved for RC2
Attachment #239229 - Flags: approval1.8.1? → approval1.8.1+

Comment 16

11 years ago
Comment on attachment 239229 [details] [diff] [review]
Fix b

Approved for RC3
(Assignee)

Comment 17

11 years ago
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
Keywords: fixed1.8.1
Flags: blocking1.8.0.8? → blocking1.8.0.8+
Comment on attachment 239229 [details] [diff] [review]
Fix b

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #239229 - Flags: approval1.8.0.8+
(Assignee)

Comment 19

11 years ago
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.
Attachment #241750 - Flags: approval1.8.0.8?
Comment on attachment 241750 [details] [diff] [review]
Fix b for 1.8.0 branch

moving approval to the correct branch patch
Attachment #241750 - Flags: approval1.8.0.8? → approval1.8.0.8+
Attachment #239229 - Flags: approval1.8.0.8+
(Assignee)

Comment 21

11 years ago
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
Keywords: fixed1.8.0.8
Whiteboard: [sg:critical?]

Comment 22

11 years ago
verified fixed 1.8 20061010 windows/mac*/linux
Keywords: fixed1.8.1 → verified1.8.1
verified fixed 1.8.0.8 on mac/win/linux
firefox_1.8.0.8pre_2006101114_dbg
firefox_1.8.0.8pre_2006101114_opt
Keywords: fixed1.8.0.8 → verified1.8.0.8
Bug not relevant to aviary/moz1.7 branches
Flags: blocking1.7.14-
Flags: blocking-aviary1.0.9-
Group: security

Comment 25

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

moved to extensions/ due to getter
(Assignee)

Updated

10 years ago
Flags: blocking1.7.14-
(Assignee)

Updated

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