Last Comment Bug 313630 - Unrooted access in js_fun_toString
: Unrooted access in js_fun_toString
Status: VERIFIED FIXED
[sg:critical?]
: js1.6, verified1.7.13, verified1.8
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Linux
: -- critical (vote)
: ---
Assigned To: Brendan Eich [:brendan]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: sbb?
  Show dependency treegraph
 
Reported: 2005-10-24 10:01 PDT by Igor Bukanov
Modified: 2006-06-14 17:06 PDT (History)
5 users (show)
dveditz: blocking1.7.13+
dveditz: blocking‑aviary1.0.8+
brendan: blocking1.8rc1+
dveditz: blocking1.9a1+
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
trivial fix (1.16 KB, patch)
2005-10-24 13:27 PDT, Brendan Eich [:brendan]
igor: review+
shaver: superreview+
mtschrep: approval1.8rc1+
Details | Diff | Splinter Review
js1_5/Regress/regress-313630.js (2.46 KB, text/plain)
2005-10-26 02:57 PDT, Bob Clary [:bc:]
no flags Details

Description Igor Bukanov 2005-10-24 10:01:27 PDT
js_fun_toString from jsfun.x does not root the result of convert call before calling on the supplied argument js_ValueToECMAUint32 and using the unrooted value later. The following code demonstrates the bug with a segmentation fault in js shell.

var f = Function("return 1");
Function("return 2");
var expected_string = f.toSource(0);

var likeFunction = { 
	valueOf: function() {
		var tmp = f;
		f = null;
		return tmp;
	},
	__proto__: Function.prototype
};

var likeNumber = { 
	valueOf: function() {
		gc();
		return 0;
	}
};

var actual = likeFunction.toSource(likeNumber);
print(expected_string === actual);
Comment 1 Daniel Veditz [:dveditz] 2005-10-24 10:45:46 PDT
You're on a roll here... If we wanted to hold 1.5 until you're done finding this class of bug how much longer would that be? Can we speed that process by dividing the remaining code amongst other js engine hackers?
Comment 2 Igor Bukanov 2005-10-24 11:17:06 PDT
(In reply to comment #1)
> You're on a roll here... If we wanted to hold 1.5 until you're done finding
> this class of bug how much longer would that be? 

I wish I could spend more time on this beyond 1-3 hors of ocasinal looking at the source.

> Can we speed that process by
> dividing the remaining code amongst other js engine hackers?

Surely. Initially I thought to simply grep for all places where C code declares jsval locals. But it turned out that it is not enough since JSObject* and especially JSString* locals should also be checked. So at the end I decided to look at all methods that implements ECMA-262 library in js/src/*.c 

As far as I can see jsmath.c, jsarray.c, jsnum.c, jsfun.c, jsstr.c, jsexn.c do not have any obvious holes (any longer :). 

I would say that jsobj.c is safe if there is a guaranty that ALL implementation of the resolve hook do not cause GC for valueOf and toString properties. If there is no such guarantee, then I suggest to add explicit rooting at the beginning of js_DefaultValue/js_TryMethod since few places relies on the fact that it is safe to use the functions with unrooted values. 

I did not look at jsxml.c, jsinterpret.c and jsbool.c. 


Comment 3 Brendan Eich [:brendan] 2005-10-24 13:27:13 PDT
Created attachment 200648 [details] [diff] [review]
trivial fix
Comment 4 Brendan Eich [:brendan] 2005-10-24 13:29:54 PDT
(In reply to comment #2)
> > Can we speed that process by
> > dividing the remaining code amongst other js engine hackers?
> 
> Surely.

Dveditz,

It's important to get fresh eyes on the problem, and good eyes -- Igor's eyes have both virtues.

At this point, I am not concerned about code that Igor has looked at, so the three files he hasn't need scrutiny.

> I would say that jsobj.c is safe if there is a guaranty that ALL implementation
> of the resolve hook do not cause GC for valueOf and toString properties. If
> there is no such guarantee, then I suggest to add explicit rooting at the
> beginning of js_DefaultValue/js_TryMethod since few places relies on the fact
> that it is safe to use the functions with unrooted values. 

I will prepare a patch to do just this and attach it here, unless someone wants a new bug badly.

/be
Comment 5 Brendan Eich [:brendan] 2005-10-24 13:31:32 PDT
I would like to see this fix (at least the jsfun.c one) go in for a respin, since it is a safe one-liner.

/be
Comment 6 Brendan Eich [:brendan] 2005-10-24 13:58:16 PDT
(In reply to comment #4)
> (In reply to comment #2)
> > I would say that jsobj.c is safe if there is a guaranty that ALL implementation
> > of the resolve hook do not cause GC for valueOf and toString properties. If
> > there is no such guarantee, then I suggest to add explicit rooting at the
> > beginning of js_DefaultValue/js_TryMethod since few places relies on the fact
> > that it is safe to use the functions with unrooted values. 

I don't see anything to root in js_DefaultValue.  The |obj| parameter must be rooted or well-connected by callers, per the usual rules.  The v local is used as a result parameter, and could contain a weak ref to a GC-thing liable to be GC'd, but there are no paths that GC without reseting v (or *vp, then returning early before the *vp = v at label out:) after.

What protect *vp after js_DefaultValue (OBJ_DEFAULT_VALUE) returns?  The caller has to make arrangements.

(Just for everyone's info, Igor knows this: Callers protect in and out params in general.  Bugs in following this rule need to be fixed case-by-case if we are not going to try for a general automatic solution.  For 1.8/fx1.5, we are looking for spot fixes.)

js_TryMethod takes an argv and returns a *rval, and those are just locals passed by reference.  So js_TryValueOf, e.g., passings an atom key in argv[0], and it's the atom being pinned that saves it from GC.

The rval that js_TryValueOf receives from JS_ConvertStub is protected by convert's caller, e.g. js_DefaultValue.  But if there are no GC'ing paths in js_DefaultValue after it calls clasp->convert till it returns its result, then the burnen is on its caller.

I hope this helps.  Igor, what am I missing that needs rooting in js_DefaultValue and/or js_TryMethod?

/be
Comment 7 Mike Shaver (:shaver -- probably not reading bugmail closely) 2005-10-24 14:59:07 PDT
Comment on attachment 200648 [details] [diff] [review]
trivial fix

sr=shaver
Comment 8 Igor Bukanov 2005-10-24 15:03:50 PDT
(In reply to comment #6)

I meant to root obj itself as a safeguard against bugs. 

For example, consider js_GetLengthProperty in jsarray.c. It passes to js_DefaultValue via js_ValueToECMAUint32/js_ValueToNumber/OBJ_DEFAULT_VALUE the result of OBJ_GET_PROPERTY. The latter can be made unrooted. Thus the code is safe only if js_DefaultValue is GC-save with unrooted obj argument. 

js_DefaultValue can tolerate unrooted flora if property lookup during search of "toString"/"valueOf" methods is GC safe. That in turn requires that all resolve hooks would not trigger any GC activities regarding these properties. If there is such guarantee, then things are OK. Otherwise one either has to add an extra rooting of obj or patch at least the following places with rooting guards:

jsarray.c:

js_GetLengthProperty
sort_compare -- potentially unrooted bv and rval
array_extra -- potentially unrooted v and rval2

jsstr.c:

find_replen -- unsafe js_ValueToString(cx, rval);

jsregexp.c

regexp_setProperty -- unsafe js_ValueToNumber

Now it would be a good idea to patch those places in any case, but the problem is that the list may not be full.
Comment 9 Brendan Eich [:brendan] 2005-10-24 16:44:42 PDT
Comment on attachment 200648 [details] [diff] [review]
trivial fix

Fixed on the trunk.  This is too easy not to take, but I'll let someone else mark a+.

/be
Comment 10 Bob Clary [:bc:] 2005-10-26 02:57:00 PDT
Created attachment 200849 [details]
js1_5/Regress/regress-313630.js
Comment 11 Bob Clary [:bc:] 2005-11-07 22:31:46 PST
verified firefox 1.5 rc2 linux/win32 2005-11-07
Comment 12 Bob Clary [:bc:] 2005-12-27 10:35:15 PST
testcase+ to get this off my radar. when this is made public, i will check in the test.
Comment 13 Blake Kaplan (:mrbkap) 2006-02-10 19:26:33 PST
Fix checked into the 1.7 branches.
Comment 14 Bob Clary [:bc:] 2006-02-17 03:58:36 PST
verified firefox/1.7.13,1.8.0.1,1.8,1.9a1 win/linux/mac
Comment 15 Bob Clary [:bc:] 2006-02-22 02:57:49 PST
v ff on 1.7.13, 1.8.0.1, 1.8, 1.9a1 on windows/linux/mac 20060221 builds, moz on 1.7.13 windows 20060221 build.
Comment 16 Bob Clary [:bc:] 2006-06-14 17:06:08 PDT
RCS file: /cvsroot/mozilla/js/tests/js1_5/GC/regress-313630.js,v
done
Checking in regress-313630.js;
/cvsroot/mozilla/js/tests/js1_5/GC/regress-313630.js,v  <--  regress-313630.js
initial revision: 1.1
done

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