Last Comment Bug 421934 - Implement GC-safety checks for spidermonkey
: Implement GC-safety checks for spidermonkey
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Rewriting and Analysis (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal with 1 vote (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
: Michael Layzell [:mystor] [:mrl]
Mentors:
Depends on: 432507 432546
Blocks: analyses
  Show dependency treegraph
 
Reported: 2008-03-10 11:40 PDT by (dormant account)
Modified: 2015-04-02 10:35 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments

Description (dormant account) 2008-03-10 11:40:00 PDT
This is still applicable for 1.9. Should investigate if this is easy to do with either gcc dehydra(ideal) or the new typedef stuff in pork.

http://wiki.mozilla.org/GC_SafetySpec
Comment 1 (dormant account) 2008-03-25 16:48:46 PDT
Igor,
I'm trying to start on my analysis and I already ran into a case I'm not too sure about.

In JS_CallFunctionName in jsapi.c, what rule am I supposed to follow for jsval fval;
Comment 2 Igor Bukanov 2008-03-26 02:24:00 PDT
(In reply to comment #1)
> Igor,
> I'm trying to start on my analysis and I already ran into a case I'm not too
> sure about.
> 
> In JS_CallFunctionName in jsapi.c, what rule am I supposed to follow for jsval
> fval;

The code relies on the fact that js_InternalCall stores a copy of fval into a rooted location before calling anything that trigger GC. It is well known how fragile is that so you can treat it as a bug.
Comment 3 (dormant account) 2008-05-06 11:38:17 PDT
Note to self, JS_PUSH_SINGLE_TEMP_ROOT is a value of rooted rooting...So I think the rule should say that one can not dereference anything below the value of the root

So if jsval is rooted..then turning that into jsval* is forbidden, if jsval* is rooted, then doing jsval** is forbidden..etc.
Comment 4 (dormant account) 2008-05-06 11:47:14 PDT
Above is not quite correct

Code:
JSTempValueRooter tvr;
jsval v;
JS_PUSH_SINGLE_TEMP_ROOT(cx, v, &tvr)

Result:
tvr is rooted
v is copy-of-rooted

&v a violation
&tvr.u.value is safe
Comment 5 (dormant account) 2008-05-06 11:50:53 PDT
functions marked with JS_PUBLIC_API are unsafe, thus parameters should be rooted
Comment 6 Igor Bukanov 2008-05-06 12:01:06 PDT
(In reply to comment #4)
> Above is not quite correct
> 
> Code:
> JSTempValueRooter tvr;
> jsval v;
> JS_PUSH_SINGLE_TEMP_ROOT(cx, v, &tvr)
> 
> Result:
> tvr is rooted
> v is copy-of-rooted

Note this code is bogus: jsval is not initialized! What happens with public API is the following code pattern:

void public_api(..., jsval v)
{
    JSTempValueRooter tvr;
    JS_PUSH_SINGLE_TEMP_ROOT(cx, v, &tvr)
    ....

}

Here according to the generic rule v is already copy-of-rooted. So taking its address is prohibited. I guess the only thing this example shows that the jsval argument to JS_PUSH_SINGLE_TEMP_ROOT must be a rooted or copy-of-rooted jsval. But this is just a generic rule for any function call.    
Comment 7 (dormant account) 2008-05-06 12:47:07 PDT
To clarify comment 6, v becomes copy-of-rooted IFF JS_PUBLIC_API...In all other cases the value argument to JS_PUSH* should be copy-of-rooted or a constant
Comment 8 (dormant account) 2008-05-06 15:58:50 PDT
Lefty rule: No part of a struct is ever registered(such that we dont need to do shape analysis), so when checking for rootedness of a.nested.member, only the leftmost part needs to be checked for rootedness.
Comment 9 (dormant account) 2008-05-08 12:45:28 PDT
in order to allow outparams

foo(&tvr.u.value) we must assume that no parameters ever escape the stack...if they do they shall be annotated as ESCAPES
Comment 10 (dormant account) 2008-05-08 13:57:53 PDT
So turns out, we don't just manage jsvals, but all other parameters entering js functions.

* One can not pass part of a struct to a function, unless it is explicitly registered.
* Currently only jsvals have a way to get registered. Something will be developed for structs too

* Due to expansion of the spec, we are now aiming for June completion of the analysis.
Comment 11 (dormant account) 2009-06-26 19:27:48 PDT
So last time I tried this I had to give up because inferring rootedness turned to be too painful due to jsvals found in structs, etc.

I think the only realistic approach is to mark the code explicitly.

A template wrapper might be ok so it's enforced at the language level. If that's too ugly we can use a typedef(but that involves making new typedefs for jsval, JSObject, etc).


Basically I want easy to spot transitions like:
JSRooted<jsval> foo = make_rooted(v);
Now foo is easy to track and it's easy to tell that v is not rooted. This becomes especially useful for struct members, as without annotation it's really hard to determine when things are rooted in there(due to transitive rooting).

Is this kind of API-change acceptable? I think we'd have to go with typedefs to stay compatible with existing spidermonkey API.
Comment 12 Brendan Eich [:brendan] 2009-06-30 15:44:06 PDT
Explicit template type is fine. We need to vouch that a pointer (usually a parameter, sometimes an intentional weak ref that's safe for a well-inspected critical section, or safe because it's scanned by an owning object, e.g. dslots in JSObject) is already rooted. Otherwise if you are allocating a jsval you probably need to root it.

Igor, any comments?

/be
Comment 13 (dormant account) 2009-06-30 15:46:41 PDT
(In reply to comment #12)
> Explicit template type is fine. We need to vouch that a pointer (usually a
> parameter, sometimes an intentional weak ref that's safe for a well-inspected
> critical section, or safe because it's scanned by an owning object, e.g. dslots
> in JSObject) is already rooted. Otherwise if you are allocating a jsval you
> probably need to root it.

Actually I'd like to mark parameters too.
Comment 14 Brendan Eich [:brendan] 2009-06-30 16:15:14 PDT
Yes, "mark" as in annotate or (if we can use C++) apply the template type -- but this vouches without adding extra rooting. For in parameters the caller must root so this is the manually annotated edge in the transitive closure graph that helps the analysis. It's not automation that causes a redundant root. Right?

/be
Comment 15 Igor Bukanov 2009-07-01 01:26:11 PDT
(In reply to comment #11)
> Basically I want easy to spot transitions like:
> JSRooted<jsval> foo = make_rooted(v);
> Now foo is easy to track and it's easy to tell that v is not rooted. This
> becomes especially useful for struct members, as without annotation it's really
> hard to determine when things are rooted in there(due to transitive rooting).

Do you mean that JSRooted<> should annotate any rooted thing including structs with jsvals? I suspect that this would be rather invasive. And I am not sure how this would help with structs.
Comment 16 Michael Kohler [:mkohler] 2010-05-13 10:07:18 PDT
This is a mass change. Every comment has "assigned-to-new" in it.

I didn't look through the bugs, so I'm sorry if I change a bug which shouldn't be changed. But I guess these bugs are just bugs that were once assigned and people forgot to change the Status back when unassigning.
Comment 17 Steve Fink [:sfink] [:s:] 2015-04-02 10:35:18 PDT
AFAICT, this has now all been implemented using Rooted and Handle types, and in conjunction with the sixgill-based static rooting analysis.

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