313500 - Unrooted access to "prototype" property

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Igor Bukanov]

js_Interpret in jsinterp.c stores the result of accessing "prototype" property of function objects in unrooted "rval" local variable. This is GC-unsafe since there is object allocation before the result of accessing "prototype" property is rooted.

If one recompiles JS shell with TOO_MUCH_GC defined then the following example gives segmentation fault:

function F() { }

var prepared = new Object();

F.prototype = {};
F.__defineGetter__('prototype', function() {
	var tmp = prepared;
	prepared = null;
	return tmp;
});

new F();

There is a similar pattern is 2 places in jsobj.c with problematic access of "prototype" property. I suspect that there is a problem in fun_resolve from jsfun.c as well, but it may be actually GC-safe. On the other hand accessing the property in Exception from jsexn.c is GC safe since a script can not define a getter for "prototype" for Error object as it already defined as read-only property.

Comment 1

[Brendan Eich [:brendan]]

(In reply to comment #0)
> There is a similar pattern is 2 places in jsobj.c with problematic access of
> "prototype" property. I suspect that there is a problem in fun_resolve from
> jsfun.c as well, but it may be actually GC-safe.

No, those are both hazardous -- thanks for finding them.  Are you reviewing all OBJ_GET_PROPERTY usage, or looking for classPrototypeAtom usage, or both?

> On the other hand accessing
> the property in Exception from jsexn.c is GC safe since a script can not define
> a getter for "prototype" for Error object as it already defined as read-only
> property.

Yes, a saving grace of ECMA-262!

Patch soon, thanks again.

/be

Comment 2

[Brendan Eich [:brendan]]

(In reply to comment #1)
> (In reply to comment #0)
> > On the other hand accessing
> > the property in Exception from jsexn.c is GC safe since a script can not define
> > a getter for "prototype" for Error object as it already defined as read-only
> > property.
> 
> Yes, a saving grace of ECMA-262!

Although it is the permanent (ECMA DontDelete) attribute that saves us here.  A read-only property can be deleted, and then its id rebound to a getter.  So only the permanent attribute prevents an exploit such as this bug's testcase.

/be

Comment 3

[Igor Bukanov]

(In reply to comment #1)
> Are you reviewing all
> OBJ_GET_PROPERTY usage, or looking for classPrototypeAtom usage, or both?

I try to go through all cases of OBJ_GET_PROPERTY/ValueToString/ValueToSource in js/src/*.c as time permits. The case of classPrototypeAtom was just a common pattern easy to spot and sufficiently unique to justify a separated bug.

Comment 4

[Brendan Eich [:brendan]]

Attachment: proposed fix

mrbkap, please have a look too.  The comments tell the tale.

/be

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 200549 [details] [diff] [review]
proposed fix

Checked into trunk.

/be

Comment 6

[Mike Shaver (:shaver emeritus)]

Comment on attachment 200549 [details] [diff] [review]
proposed fix

sr=shaver.  Almost into XXX territory with the newborn-root hacking!

Can you add a comment before GetClassPrototype stating the 'delegate-or-get-class' invariant that it expects its callers to uphold?

Comment 7

[Daniel Veditz [:dveditz]]

Comment on attachment 200549 [details] [diff] [review]
proposed fix

a=dveditz

Comment 8

[Brendan Eich [:brendan]]

Fixed on branch and trunk.

/be

Comment 9

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/Regress/regress-313500.js

Comment 10

[Bob Clary [:bc] (inactive)]

verified firefox 1.5 rc2 linux/win32 2005-11-07

Comment 11

[Bob Clary [:bc] (inactive)]

testcase+ to get this off my radar. when this is made public, i will check in the test.

Comment 12

[Daniel Veditz [:dveditz]]

This should have gone on the old branches

Comment 13

[Alexander Sack]

Dan, has there already been an advisory for this, so I can release the patch in 'pseudo 1.7.15/1.0.10' ?

How to procede with this one?

Comment 14

[Daniel Veditz [:dveditz]]

Yes, this was part of MFSA 2006-10 but wasn't explicitly mentioned. I'll go update that advisory.

Comment 15

[Bob Clary [:bc] (inactive)]

verified fixed 1.9 windows/mac(ppc|tel)/linux 20060812

Comment 16

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-313500.js,v  <--  regress-313500.js