Last Comment Bug 313500 - Unrooted access to "prototype" property
: Unrooted access to "prototype" property
Status: VERIFIED FIXED
[sg:critical?]
: js1.6, verified1.8
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: P1 critical (vote)
: mozilla1.8rc1
Assigned To: Brendan Eich [:brendan]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-10-23 10:29 PDT by Igor Bukanov
Modified: 2011-08-05 21:32 PDT (History)
5 users (show)
dveditz: blocking1.7.14?
dveditz: blocking‑aviary1.0.9?
brendan: blocking1.8rc1+
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proposed fix (5.56 KB, patch)
2005-10-23 14:15 PDT, Brendan Eich [:brendan]
igor: review+
shaver: superreview+
dveditz: approval1.8rc1+
Details | Diff | Splinter Review
js1_5/Regress/regress-313500.js (2.28 KB, text/plain)
2005-10-23 21:48 PDT, Bob Clary [:bc:]
no flags Details

Description Igor Bukanov 2005-10-23 10:29:27 PDT
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] 2005-10-23 11:32:42 PDT
(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] 2005-10-23 14:02:58 PDT
(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 2005-10-23 14:05:16 PDT
(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] 2005-10-23 14:15:45 PDT
Created attachment 200549 [details] [diff] [review]
proposed fix

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

/be
Comment 5 Brendan Eich [:brendan] 2005-10-23 14:42:38 PDT
Comment on attachment 200549 [details] [diff] [review]
proposed fix

Checked into trunk.

/be
Comment 6 Mike Shaver (:shaver -- probably not reading bugmail closely) 2005-10-23 15:33:10 PDT
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] 2005-10-23 20:44:13 PDT
Comment on attachment 200549 [details] [diff] [review]
proposed fix

a=dveditz
Comment 8 Brendan Eich [:brendan] 2005-10-23 21:07:10 PDT
Fixed on branch and trunk.

/be
Comment 9 Bob Clary [:bc:] 2005-10-23 21:48:12 PDT
Created attachment 200579 [details]
js1_5/Regress/regress-313500.js
Comment 10 Bob Clary [:bc:] 2005-11-07 22:32:07 PST
verified firefox 1.5 rc2 linux/win32 2005-11-07
Comment 11 Bob Clary [:bc:] 2005-12-27 10:34:57 PST
testcase+ to get this off my radar. when this is made public, i will check in the test.
Comment 12 Daniel Veditz [:dveditz] 2006-04-03 10:25:28 PDT
This should have gone on the old branches
Comment 13 Alexander Sack 2006-07-28 09:09:41 PDT
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] 2006-07-29 10:38:50 PDT
Yes, this was part of MFSA 2006-10 but wasn't explicitly mentioned. I'll go update that advisory.
Comment 15 Bob Clary [:bc:] 2006-08-14 08:35:47 PDT
verified fixed 1.9 windows/mac(ppc|tel)/linux 20060812
Comment 16 Bob Clary [:bc:] 2007-02-08 18:22:47 PST
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-313500.js,v  <--  regress-313500.js

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