432915 - Analyze SpiderMonkey Push/pop for tvr & lock/unlock/etc

Status: RESOLVED WONTFIX

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[(dormant account)]

Will get a list of functions from Igor that are function-local

Comment 1

[(dormant account)]

try to inline InitTempNSArray, etc to see if gcc will show it as part of the cfg.

Comment 2

[(dormant account)]

Igor, just ran this on actionmonkey's C++ spidermonkey(which is a little out of date) and didn't find anything wrong. 
The functions I used were
/^JS_PUSH/ -> "JS_POP_TEMP_ROOT" (these defines were turned into static functions for analysis purposes
/^InitTempNSArray$/ -> "FinishTempNSArray"


What are the other functions that act on stack variables that you'd like to check?

Comment 3

[(dormant account)]

Attachment: Treeehydra script for finding mismatched constructor/destructor type things

Comment 4

[Jason Orendorff [:jorendorff]]

actionmonkey's SpiderMonkey has deleted most TempValueRooters, as they're not needed anymore with the conservative stack-scanning GC.

Comment 5

[Igor Bukanov]

(In reply to comment #2)
...
> /^InitTempNSArray$/ -> "FinishTempNSArray"
> 
> 
> What are the other functions that act on stack variables that you'd like to
> check?

Another famous-for-bugs pair is OBJ_LOOKUP_PROPERTY(cx,obj,id,objp,propp) and OBJ_DROP_PROPERTY(cx,obj,prop) macros.

But the rules here requires heavy control flow analysis since OBJ_DROP_PROPERTY must be called only and only if OBJ_LOOKUP_PROPERTY returns true and sets prop to non-null value. In addition in few places OBJ_LOOKUP_PROPERTY is hidden besides helper functions. If this is not feasible, we could at least annotate code with OBJ_MUST_DROP_PROPERTY(obj, prop) right after the checks for non-null prop. That can be combined with the requirement that OBJ_MUST_DROP_PROPERTY must have preceding OBJ_MUST_DROP_PROPERTY. 

Plus there is that JS_LOCK/JS_UNLOCK. But it definitely requires propagation from the function return. I guess this implies that the functions that returns with object locked must be annotated if we want to stay with a function-local analysis.

Comment 6

[(dormant account)]

(In reply to comment #5)
> (In reply to comment #2)
> ...
> > /^InitTempNSArray$/ -> "FinishTempNSArray"
> > 
> > 
> > What are the other functions that act on stack variables that you'd like to
> > check?
> 
> Another famous-for-bugs pair is OBJ_LOOKUP_PROPERTY(cx,obj,id,objp,propp) and
> OBJ_DROP_PROPERTY(cx,obj,prop) macros.
> 
> But the rules here requires heavy control flow analysis since OBJ_DROP_PROPERTY
> must be called only and only if OBJ_LOOKUP_PROPERTY returns true and sets prop
> to non-null value. In addition in few places OBJ_LOOKUP_PROPERTY is hidden
> besides helper functions. If this is not feasible, we could at least annotate
> code with OBJ_MUST_DROP_PROPERTY(obj, prop) right after the checks for non-null

So lets treat the helper functions same as OBJ_LOOKUP_PROPERTY?
Also we can detect null/non-null in most cases. However we do have to annotate the functions to indicate that they don't leak the pointer when it's assigned through an outparam.

> prop. That can be combined with the requirement that OBJ_MUST_DROP_PROPERTY
> must have preceding OBJ_MUST_DROP_PROPERTY. 

> 
> Plus there is that JS_LOCK/JS_UNLOCK. But it definitely requires propagation
> from the function return. I guess this implies that the functions that returns
> with object locked must be annotated if we want to stay with a function-local
> analysis.

Actually looks like JS_LOCK/JS_UNLOCK are function local. perhaps other lock/unlock functions aren't.

Comment 7

[(dormant account)]

I think i found a rooting bug
http://mxr.mozilla.org/mozilla/source/js/src/jsinterp.c#935?

Comment 8

[(dormant account)]

I guess I should mention that I'm checking push/pop on tvr with JS_PUSH_SINGLE_TEMP_ROOT in that method

Comment 9

[Brendan Eich [:brendan]]

Line 935 has shifted recently -- which rev are you using?

/be

Comment 10

[(dormant account)]

i'm using some old rev, but i'm assuming mxr is current and there shouldn't be a return JS_FALSE there in js_OnUnknownMethod(JSContext *cx, jsval *vp) as that doesn't flow through out

Comment 11

[Brendan Eich [:brendan]]

Oh I see! Yeah, that's a bug. Igor, can you file and fix?

/be

Comment 12

[(dormant account)]

For my notes:
jst mentions

nsI[Thread]JSContextStack::Push/Pop would be another one that'd be good to pay attention to.

example:
http://mxr.mozilla.org/mozilla-central/source/dom/src/base/nsJSEnvironment.cpp#1327

Comment 13

[Michael Kohler [:mkohler]]

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 14

[Jeff Walden [:Waldo]]

Property holding/dropping is gone these days.  We use RAII for tvr push/popping.  All is right with the world.

(...okay, maybe I shouldn't have gone that far.  :-) )