494235 - prevent calls to upvar-referring closures with incorrect JSContext.display

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Igor Bukanov]

The bug 489255 shows that JSDBG API can leak upvar-referencing closures and allows to call them with unsuitable JSContext.display setup. Ideally this should be detected and such closures should not be leaked from the getters of Call, Arguments and Block objects.

Comment 1

[Igor Bukanov]

Attachment: v1 - work in progress

The patch censor any leakage of upvars. This is wrong as it is OK to leak closures with dvars (flat closures?). That is for v2.

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 378941 [details] [diff] [review]
v1 - work in progress

Leaks could happen only via debugger or similar API calls, right? If we could put the check in the API layer we'd avoid a perf hit.

VisibleFrame seems the wrong term. Isn't it checking for optimized closure escapes (escape is better than leak, which is overloaded)?

/be

Comment 3

[Igor Bukanov]

(In reply to comment #2)
> (From update of attachment 378941 [details] [diff] [review])
> Leaks could happen only via debugger or similar API calls, right?

The JSDG API exposes the Call object and a debugger may call JS_GetProperty on it instead of calling JS_GetPropertyDescArray. 

> If we could
> put the check in the API layer we'd avoid a perf hit.

The performance hit indicates that the upvar optimization does not do its job ;) forcing the code to access the name through the Call object. When escaping closures would be properly supported that hit will be removed.

> VisibleFrame seems the wrong term. Isn't it checking for optimized closure
> escapes (escape is better than leak, which is overloaded)?

Yes, it should check for any closure that uses JSContext.display. Perhaps js_CanEscapeFromFrame would be a better name?

Comment 4

[Brendan Eich [:brendan]]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 378941 [details] [diff] [review] [details])
> > Leaks could happen only via debugger or similar API calls, right?
> 
> The JSDG API exposes the Call object and a debugger may call JS_GetProperty on
> it instead of calling JS_GetPropertyDescArray.

Sure -- "or similar API calls", I wrote. Or am I missing something?

> > If we could
> > put the check in the API layer we'd avoid a perf hit.
> 
> The performance hit indicates that the upvar optimization does not do its job
> ;) forcing the code to access the name through the Call object.

Not while you're checking for flat closures accessed via Call objects. This can happen when there are no upvars unoptimized, due to with and eval.

To avoid that, test FUN_FLAT_CLOSURE(fun) and allow those to escape.

> When escaping
> closures would be properly supported that hit will be removed.

Reduced, maybe. Not removed. At some limit there's no alternative but to have a Call object or record that's heap-allocated and whose lifetime can be extended beyond the activation that created it, however you name or represent it.

> > VisibleFrame seems the wrong term. Isn't it checking for optimized closure
> > escapes (escape is better than leak, which is overloaded)?
> 
> Yes, it should check for any closure that uses JSContext.display. Perhaps
> js_CanEscapeFromFrame would be a better name?

"Frame" does not seem to be relevant (no JSStackFrame * parameter, no use of cx->fp indicated by JS_REQUIRES_STACK or similar). js_UnsafeEscapeCheck or js_CheckForUnsafeEscape?

/be

Comment 5

[Brendan Eich [:brendan]]

(In reply to comment #4)
> At some limit there's no alternative but to have a
> Call object or record that's heap-allocated and whose lifetime can be extended
> beyond the activation that created it, however you name or represent it.

An alternative I should have mentioned is what Chez Scheme does with assignment analysis and conversion: find all upvars that are mutated or otherwise can't be captured by copying, and move them to heap cells, converting all uses of them to references indirecting to the heap cell.

This seems too costly for JS, though, compared to Call objects. We'd need a box type *and* a ref type.

/be

Comment 6

[Igor Bukanov]

(In reply to comment #4)
> > When escaping
> > closures would be properly supported that hit will be removed.
> 
> Reduced, maybe. Not removed. At some limit there's no alternative but to have a
> Call object or record that's heap-allocated and whose lifetime can be extended
> beyond the activation that created it, however you name or represent it.

The issue here is that the current upvar code accesses fp.args and fp.slots directly for the upper frame via JSContext.display link. This means that the closures can only be invoked at very specific places and from very specific frame violating compatibility with the debugger.

If instead of using JSContext.display the code would use something like JSStackFrame.staticParent where staticParent can be a pointer to either the lexical parent frame or a Call object, then there would be no problem making initial non-escaping closure an escaping one for the debugger later and the debugger can freely access the closure with upvar references.

Comment 7

[Igor Bukanov]

(In reply to comment #4)
> To avoid that, test FUN_FLAT_CLOSURE(fun) and allow those to escape.

A flat closure does not have any (GET|CALL)UPVAR, right?

Comment 8

[Brendan Eich [:brendan]]

> (In reply to comment #4)
> > > When escaping
> > > closures would be properly supported that hit will be removed.
> > 
> > Reduced, maybe. Not removed. At some limit there's no alternative but to have a
> > Call object or record that's heap-allocated and whose lifetime can be extended
> > beyond the activation that created it, however you name or represent it.
> 
> The issue here is that the current upvar code accesses fp.args and fp.slots
> directly for the upper frame via JSContext.display link. This means that the
> closures can only be invoked at very specific places and from very specific
> frame violating compatibility with the debugger.

This is true only for non-escaping closures, which are like Algol procedures. Flat closures escape (more below) and do not JSOP_GETUPVAR.

> If instead of using JSContext.display the code would use something like
> JSStackFrame.staticParent where staticParent can be a pointer to either the
> lexical parent frame or a Call object,

There is a static parent *chain*, and it would have to be searched to find the right level. The display is simply an O(1) optimization to avoid that O(n) search cost.

> then there would be no problem making
> initial non-escaping closure an escaping one for the debugger later and the
> debugger can freely access the closure with upvar references.

That defeats the point of optimizing non-escaping closures.

(In reply to comment #7)
> (In reply to comment #4)
> > To avoid that, test FUN_FLAT_CLOSURE(fun) and allow those to escape.
> 
> A flat closure does not have any (GET|CALL)UPVAR, right?

That's right -- flat closures capture copies of the invariant values of their upvars (safe because the analysis proves the initialization or initializing assignment of each upvar dominates the closure's expression). The opcodes instead are JSOP_{GET,CALL}DSLOT.(In reply to comment #6)

/be

Comment 9

[Brendan Eich [:brendan]]

The debugger can slow down debugged code, or all code compiled while debugging, but otherwise the optimizations should not be compromised by O(n) costs just in case of a late debugger activation.

/be

Comment 10

[Igor Bukanov]

(In reply to comment #8)
> > If instead of using JSContext.display the code would use something like
> > JSStackFrame.staticParent where staticParent can be a pointer to either the
> > lexical parent frame or a Call object,
> 
> There is a static parent *chain*, and it would have to be searched to find the
> right level. The display is simply an O(1) optimization to avoid that O(n)
> search cost.

I guess I am using the wrong terminology here. By "static parent link" I meant "a link to lexical parent". Since the display adds a level of indirection, it means that just following that lexical link for the most common case of one or two upvar levels would be just as fast.

> 
> > then there would be no problem making
> > initial non-escaping closure an escaping one for the debugger later

> That defeats the point of optimizing non-escaping closures.

Well, if GETUPVAR can be made to continue work after the closure escapes without any slowdown before that point compared with nthe current code, then indeed, the would be no pint to optimize non-escaping closures.

Comment 11

[Igor Bukanov]

(In reply to comment #8)
> > A flat closure does not have any (GET|CALL)UPVAR, right?
> 
> That's right -- flat closures capture copies of the invariant values of their
> upvars (safe because the analysis proves the initialization or initializing
> assignment of each upvar dominates the closure's expression).

Thanks for explanation, this provides a simple way to detect a function that contains GETUPVAR and CALLUPVAR bytecode. The check is 

  script->script->upvarsOffset != 0 && !FUN_FLAT_CLOSURE(fun)

Comment 12

[Brendan Eich [:brendan]]

It depends on how many links. But losing the display due to a debugger-enabled call that the compiler did not see has the same problem: the static link is to a frame that holds the upvars, and that's not the same as the caller in such a debugger-enabled call. It could be many frames back along the dynamic link (fp->down -- gah, must rename that to match "up" sense).

Sorry, should have mentioned that check:

  script->script->upvarsOffset != 0 && !FUN_FLAT_CLOSURE(fun)

There is a comment in jsfun.h that lays all this out.

/be

Comment 13

[Igor Bukanov]

(In reply to comment #12)
>   script->script->upvarsOffset != 0 && !FUN_FLAT_CLOSURE(fun)
> 
> There is a comment in jsfun.h that lays all this out.

One more question: can non-escaping non-flat closure be stored in an argument? I.e. does the compiler optimizes cases like one below?

function f(x, y)
{
    ++x;
    y = function() { return x; }
    return y();
}

If it is not optimized, then in the patch I would not check the arguments slots. Instead I just add an assert that this should not happen.

Comment 14

[Brendan Eich [:brendan]]

Can't optimize that case yet -- we do not consider ++ and -- as initializing assignments. This particular case is easy to analyze but in general, to make a flat closure the upvar must be an initialized var, const, let, or function; or a binding form followed in the same block by an assignment to initialize that var, const, or let.

/be

$ cat /tmp/igor.js
function f(x, y)
{
    ++x;
    y = function() { return x; }
    dis(y);
    return y();
}
dis(f);
print();
f();
$ ./Darwin_DBG.OBJ/js /tmp/igor.js
flags: HEAVYWEIGHT
main:
00000:  incarg 0
00003:  pop
00004:  lambda (function () {return x;})
00007:  setarg 1
00010:  pop
00011:  callname "dis"
00014:  getarg 1
00017:  call 1
00020:  pop
00021:  callarg 1
00024:  call 0
00027:  return
00028:  stop

Source notes:
  0:     0 [   0] newline 
  1:     0 [   0] newline 
  2:     4 [   4] newline 
  3:    11 [   7] newline 
  4:    17 [   6] pcbase   offset 6
  6:    21 [   4] newline 
  7:    24 [   3] pcbase   offset 3
  9:    28 [   4] setline  lineno 1

flags: LAMBDA
main:
00000:  name "x"
00003:  return
00004:  stop

Source notes:

/be

Comment 15

[Igor Bukanov]

I was wrong when I though that just checking for closure exposure in few places would be enough. The debugger can also access the closure via direct debugging like setting a breakpoint inside it. Moreover, an access to the JSContext.dsiplay referring closure is not harmful on its own. What is problematic is calling such closure from the wrong place. So I morph this bug to prevent exactly this. 

I consider if a simple runtime check would be feasible at the moment of closure invocation. If not, the plan B is to add something like JSOP_DISPLAY_CALL, a version of JSOP_CALL for calls to display-referring closures. Then the check would be to disallow such closure calls using JSOP_CALL.

Comment 16

[Brendan Eich [:brendan]]

(In reply to comment #15)
> I consider if a simple runtime check would be feasible at the moment of closure
> invocation. If not, the plan B is to add something like JSOP_DISPLAY_CALL, a
> version of JSOP_CALL for calls to display-referring closures. Then the check
> would be to disallow such closure calls using JSOP_CALL.

This is a great idea -- it should let us confine display update to calls and returns within the graph of non-escaping functions. The API entry points to call a function, including friends such as js_Invoke, will need conditional display update as well.

Suggest JSOP_CALL_DISPLAY as the name (some precedent for JSOP_<name>_<suffix> already).

The JSOP_CALL check can be done at recording time on trace, since we specialize to callee (function) identity when inlining. This should speed up interpretation of JSOP_CALL by eliminating display update.

/be

Comment 17

[John J. Barton]

(In reply to comment #15)
> ...The debugger can also access the closure via direct debugging
> like setting a breakpoint inside it. 

I'm curious: how can the debugger set a breakpoint inside a closure? As far as I know only jsdIScript objects (functions) can have breakpoints. (Consequently by the way, another way to prevent debuggers from breakpointing into optimized code is to prevent the jsdIScript from being created).

Comment 18

[Brendan Eich [:brendan]]

JJB: object has-a function for function objects; function has-a script. Debugger gets notified for all scripts.

/be

Comment 19

[Brendan Eich [:brendan]]

And object is-a function for compiler-created function objects; closure is a cloned function object such that the closure object has-a function, but !is-a. In any case, once you get to the function from the object, you can get to the script (object = JSObject, function = JSFunction, script = JSScript).

/be

Comment 20

[John J. Barton]

(In reply to comment #19)
> ... the closure object has-a function,
>...once you get to the function from the object, ...

Ok, but how to get the closure object from a script (using only jsdIDebuggerService). Can you get to it from jsdIScript? or the scope on a jsdIStackFrame when halted?

Comment 21

[Brendan Eich [:brendan]]

jsdbgapi.h:JS_GetFrameCalleeObject(JSContext *cx, JSStackFrame *fp);

is one way to get the running closure. It's in fp->argv[-2].

/be

Comment 22

[Brendan Eich [:brendan]]

But this is a job for the engine to fix, not the jsd user. So I'm not sure if you need to get the callee. As jsd does not use JS_GetFrameCalleeObject, you'd have to use raw jsdbgapi.h.

/be

Comment 23

[John J. Barton]

(In reply to comment #22)
> But this is a job for the engine to fix, not the jsd user.
 
Ah, so you are saying that while comment 15 does not affect jsd users, there are other debuggers on other APIs which could get the closure.

Comment 24

[Brendan Eich [:brendan]]

Yes, we must stop leaks via the API. The scripted counterpart to this native-code leak path bug is bug 489255, where the debugger evaluates a call to a name bound in the scope chain (in a Call object, to be sure) but out of site of the compiler that judged the callee as non-escaping.

/be

Comment 25

[Igor Bukanov]

> (In reply to comment #19)
> > ... the closure object has-a function,
> >...once you get to the function from the object, ...
> 
> Ok, but how to get the closure object from a script (using only
> jsdIDebuggerService). Can you get to it from jsdIScript? or the scope on a
> jsdIStackFrame when halted?

See jsdIStackFrame.callee and jsdIStackFrame.closure, http://mxr.mozilla.org/mozilla-central/source/js/jsd/idl/jsdIDebuggerService.idl#834

Comment 26

[Igor Bukanov]

(In reply to comment #25)
> See jsdIStackFrame.callee and jsdIStackFrame.closure,

I meant jsdIStackFrame.callee and jsdIStackFrame.scope.

Comment 27

[Igor Bukanov]

(In reply to comment #16)
> Suggest JSOP_CALL_DISPLAY as the name (some precedent for JSOP_<name>_<suffix>
> already).

The reason I prefer JSOP_CALL_DISPLAY is that JSOP_CALLNAME, JSOP_CALLPROP etc. means to push function and this in preparation for the call, not the calls itself.

Comment 28

[Brendan Eich [:brendan]]

Ok, if you mean JSOP_DISPLAY_CALL then the convention is to run together the name after JSOP_ -- no underscores: JSOP_DISPLAYCALL. Works for me.

/be

Comment 29

[Igor Bukanov]

(In reply to comment #28)
> Ok, if you mean JSOP_DISPLAY_CALL then the convention is to run together the
> name after JSOP_ -- no underscores: JSOP_DISPLAYCALL. Works for me.

Or JSOP_INNERCALL? It is not that precise as JSOP_DISPLAYCALL but the latter is little bit long for all-caps names without underscores.

Comment 30

[Brendan Eich [:brendan]]

INNER does not convey non-escaping. Idiomatic DISPLAY is better, and not overlong when you consider LOOKUPSWITCH etc.

/be

Comment 31

[Brendan Eich [:brendan]]

DISPCALL is plausible as a shorter name.

/be

Comment 32

[Igor Bukanov]

From bug 494645 comment #3:
> Another problem from that example from the comment 0 is that the nested
> function h is also marked as a null closure despite that it includes a closure
> that refers to the variable from the parent of h.

This problem, that a nested function containing closures that refer to function's parent upvar can be marked marked as a null or flat closure, prevents effective check for the proper display chain. In particular, even with JSOP_DISPCALL it would be necessary to check that not only the closure itself is called using JSOP_DISPCALL but also all its lexical parents.

Comment 33

[Igor Bukanov]

(In reply to comment #16)
> it should let us confine display update to calls and
> returns within the graph of non-escaping functions.

JSOP_DISPCALL would not help this since cx->display has to be updated when staticDepth is zero while for this bug it would be helpful to have a marker that detects an upvar closure call starting from staticLevel == 1.

Comment 34

[Brendan Eich [:brendan]]

(In reply to comment #33)
> (In reply to comment #16)
> > it should let us confine display update to calls and
> > returns within the graph of non-escaping functions.
> 
> JSOP_DISPCALL would not help this since cx->display has to be updated when
> staticDepth is zero

staticLevel 0 is top level code, either global or new Function. These can afford to update the display. The savings is for escaping calls above level 0.

/be

Comment 35

[Igor Bukanov]

(In reply to comment #34)
> The savings is for escaping calls above level 0.

Due to that debugger-facilitated closure escape what has to be protected is the call to a closure which itself or via one of its nested functions accesses upvars from the upper frames while the display has to be updated for frames where the upvars come from. Thus JSOP_DISPCALL cannot in general help to avoid useless display updates.

But this all pretty moot as JSOP_DISPCALL cannot provide optimized protection I was hoping to get. The problem is that a debugger-injected direct eval can change a reference to one upvar closure with another with different requirements for the JSContext.display. Thus a call to the replaced closure will still go through JSOP_DISPCALL and, as such, will not be a subject of any checks.

Due to this problem I do not see how to do any better than to check that during a call to a closure the display chain is a valid one for any entry the closure may refer to. An alternative to that is just to check in JSOP_GETUPVAR that upvar index does not point outside frme.slots. But that adds an extra check for each and every upvar access.

Comment 36

[Brendan Eich [:brendan]]

Reconstructing the display is possible if the needed frames are active, as they can be identified by walking the dynamic frame chain matching static level.

By induction if the debugger nests another eval then the same procedure can reconstruct a nested display.

All modified display entries must be restored when leaving debugger-eval, of course.

/be

Comment 37

[Igor Bukanov]

(In reply to comment #36)
> All modified display entries must be restored when leaving debugger-eval, of
> course.

That does not work since debugger-injected eval can just store a closure in a variable. Then a call to that closure can happen outside that eval and the proper display chain.

Comment 38

[Brendan Eich [:brendan]]

(In reply to comment #37)
> (In reply to comment #36)
> > All modified display entries must be restored when leaving debugger-eval, of
> > course.
> 
> That does not work since debugger-injected eval can just store a closure in a
> variable. Then a call to that closure can happen outside that eval and the
> proper display chain.

I know, but let's divide and conquer. The proximate bug is the one where you call an optimized closure while stopped in its parent, from a debugger eval dialog. That's what comment 36 talks about fixing.

A barrier for escaping closures that throws seems like the other bug you filed.

/be

Comment 39

[Igor Bukanov]

(In reply to comment #38)
> I know, but let's divide and conquer.

I see no point spending efforts fixing this bug one-halve. Partial fix still mean an entry in release notes that one has to be careful when using debugger extensions to Firefox. In particular, debugging untrusted code may put the user at risk of arbitrary code execution.

> A barrier for escaping closures that throws seems like the other bug you filed.

That bug is just this bug with the older name. That barrier does not sound as a simple solution for at least the following reasons. First, since the debugger allows to access the Call objects and its properties for any frame, it means quite a few entries to protect. Second, the debug-induced eval must be completely deoptimized to make sure that no new ways for closures to escape are added. Third, some non-trivial changes has to be added to the emitter so it would be possible to check quickly if a closure or one of its nested functions accesses an upvar from ancestor frames.

Given that my preference goes to adding an extra check to js_GetUpvar to make sure that the function never accesses out-of-bounds slots for any frame.

Comment 40

[Brendan Eich [:brendan]]

(In reply to comment #39)
> (In reply to comment #38)
> > I know, but let's divide and conquer.
> 
> I see no point spending efforts fixing this bug one-halve.

Maybe the figure of speech was unclear: "divide and conquer" means fix *both* bugs. Nothing left unfixed, all conquered.

I'm going to object to anything that penalizes js_GetUpvar. We are optimizing more and more over time. The debugger will have to deoptimize. We shouldn't slow down the fast paths for it.

Even if hacking js_GetUpvar were acceptable, out of bound slot is not the only problem. The wrong value including some inner window that should not be accessible could be accessed. This assumes that null-checking display elements is enough too.

/be

Comment 41

[Igor Bukanov]

(In reply to comment #40)
> Even if hacking js_GetUpvar were acceptable, out of bound slot is not the only
> problem. The wrong value including some inner window that should not be
> accessible could be accessed. This assumes that null-checking display elements
> is enough too.

I was thinking to initialize display elements with a pointers to a statically allocated frame to avoid the null check. But you are right, the bound check is not enough due to that exposure of display elements from unrelated frames. So the check is the same bad partial solution.

Comment 42

[Igor Bukanov]

To verify that JSContext.display is the right one I considered storing JSScript instances for upper frames for upvar closures. The idea was to verify at the moment of a call to the closure or in js_GetUpvar (whatever is more optimal) that the script stored in the frame from JSContext.display matches the script associated with a closure.

But that again would be a partial solution. The problem with matching the script is that via a debugger exposure a closure from one invocation of the function can be transferred into unrelated invocation of the same function. That may still allow for the closure to see the state that it should not access in the first place.

Comment 43

[Igor Bukanov]

It seems that to properly verify that the display is the right one it is necessary to store the pointer to the lexical frame in the closure itself so the frame from the display can be matched against the stored one.

That is, the idea is to extend the upvar-referencing closure with the link to its immediate lexical parent frame and a linkage field. The latter would be used to null the pointer to the parent frame in the closure when the parent frame exits. Then during a call to the closure the interpreter would simply walk the lexical scope chain for the closure and copy it into JSContext.display. If any of the links would be null, an error would be reported. This way JSContext.display would always be the right one.

Comment 44

[Brendan Eich [:brendan]]

We should not be burning space or cycles in closures for the debugger. Bug 471214 is likely to be fixed and it avoids cloning closure function objects in common cases, so there won't be a closure to mutate.

Let me try an idea today, report back. As we optimize more and more, we will end up in a world of debugger-mode (gcc -g) being required, even if we can automate recompilation. This is life in the big JIT city.

/be

Comment 45

[Brendan Eich [:brendan]]

Will attach a write-up tonight.

/be

Comment 46

[Brendan Eich [:brendan]]

Attachment: write-up of proposed solution

Igor, mrbkap, anyone: please poke holes in this. Thanks,

/be

Comment 47

[Igor Bukanov]

(In reply to comment #46)
> Created an attachment (id=379837) [details]

Two quick observations:

1. The debugger can access the Call object or the Callee for an upvar closure via setting a breakpoint into it and using the debug API to get these two value. These upvar exposing paths should also be protected.

2. It is not sufficient to check for closures with JSScript.upvarOffset != 0. The code must also protect a closure that contains nested functions with upvars that access the lexical parent or higher for the closure.

Comment 48

[Igor Bukanov]

(In reply to comment #47)
> Two quick observations:

One more thing: _FC bytecodes that call js_NewFlatClosure must also be protected. The functions does js_GetUpvar for all upvars in the closure and those upvars may refer not only to the immediate lexical parent (which is always safe to access) but also to variables from upper lexical parents. So if the function containing this flat closure is exposed via the debugger, it will bring the same problems as with JSOP_GETUPVAR.

Comment 49

[Brendan Eich [:brendan]]

(In reply to comment #47)
> (In reply to comment #46)
> > Created an attachment (id=379837) [details] [details]
> 
> Two quick observations:
> 
> 1. The debugger can access the Call object or the Callee for an upvar closure
> via setting a breakpoint into it and using the debug API to get these two
> value. These upvar exposing paths should also be protected.

Agreed, seems do-able in debugger-only codepaths.

> 2. It is not sufficient to check for closures with JSScript.upvarOffset != 0.
> The code must also protect a closure that contains nested functions with upvars
> that access the lexical parent or higher for the closure.

Right, I forgot that -- that was why I was toying with wrapper automation yesterday. But wrappers break identity (===) and are more complex than brute force conversion (even if done lazily). To use brute force conversion lazily we would need to monitor calls. Or perhaps we should just eagerly walk the tree of potentially-JSOP_*UPVAR-using functions when any function is called from debugger code.

Same goes for nested _FC ops as you point out in coment 48. Again brute force from first debugger call to any non-debugger-compiled function might be ok.

/be

Comment 50

[Brendan Eich [:brendan]]

We could store, in each function in a tree of nested function expressions and definitions, the maximum skip from that function's static level its upvars, or any of its nested functions upvars adjusted to reduce their skips by their static levels. 0 means no upvars directly or in nested functions.

/be

Comment 51

[Brendan Eich [:brendan]]

The debug API eval in frame entry point, JS_EvaluateUCInStackFrame, does not compile its script with a usable static level -- it passes JS_DISPLAY_SIZE. This should leave intact the display, whatever its contents. If the frame being eval'ed in is top-most then that's the correct display.

If not, then arguably there's a bug in this debug API entry point, and it should reconstruct the display for its JSStackFrame *fp parameter.

/be

Comment 52

[Brendan Eich [:brendan]]

Reconstructing the display can be done by walking the frame chain, storing the youngest frame's address at its staticLevel index in cx->display (if in bounds), but for non-top frames, the displaySave members of frames from the top down to the target could be restored.

/be

Comment 53

[Igor Bukanov]

(In reply to comment #51)
> The debug API eval in frame entry point, JS_EvaluateUCInStackFrame, does not
> compile its script with a usable static level -- it passes JS_DISPLAY_SIZE.
> This should leave intact the display, whatever its contents.

But what about the debugger-injected eval not doing any calls but rather just storing an upvar-closure in some variable like doing Array = closure_refrence? That closure can be called even after the debugger is disabled.

Comment 54

[Brendan Eich [:brendan]]

(In reply to comment #53)
> (In reply to comment #51)
> > The debug API eval in frame entry point, JS_EvaluateUCInStackFrame, does not
> > compile its script with a usable static level -- it passes JS_DISPLAY_SIZE.
> > This should leave intact the display, whatever its contents.
> 
> But what about the debugger-injected eval not doing any calls but rather just
> storing an upvar-closure in some variable like doing Array = closure_refrence?

It can only do this by getting closure_reference from its scope chain, which means from a Call or Block object -- I just realized that the Arguments object is not an issue. If the eval is injected into a function frame and one of its arguments was a funarg, that funarg cannot have been compiled to use JSOP_*UPVAR.

> That closure can be called even after the debugger is disabled.

That closure cannot use JSOP_*UPVAR. If it contains closures that do, they can be converted eagerly or lazily. Eager seems better to put all cost on the debugger, not on the invocation paths.

/be

Comment 55

[Igor Bukanov]

(In reply to comment #46)
> Created an attachment (id=379837) [details]
> write-up of proposed solution

Here is yet another issue. Consider the following:

function f(a, b, c)
{
    a = (function() { return function() { return b; }; })();
    
    return c.a;
}

Suppose that a debugger is stopped on the "return c.a" line and a uses evaluates at that point "c = arguments" and then resume a normal execution of the function. This will leak upvar-referencing closure stored in "a" when doing c.a. But at that point there would be no debug frames on the stack.

This suggests to do that opcode -> opcode_dbg transformation on any attempt to access arguments for a light-weight frame.

Comment 56

[Igor Bukanov]

(In reply to comment #54)
> That closure cannot use JSOP_*UPVAR. If it contains closures that do, they can
> be converted eagerly or lazily.
> Eager seems better to put all cost on the
> debugger, not on the invocation paths.

Indeed: "lazily" means to figure out at the call or flat nested closure construction site that the display chain is wrong. And if that can be done cheaply, there would be no need for opcode_DBG - just throwing an exception would be OK.

Comment 57

[Igor Bukanov]

Regarding the cost of extra checks in Block and Call objects to censor upvar leaks:

It seems that for a lightweight function js_GetCallObject and js_GetScopeChain can only be called either to implement the indirect eval or to provide debugging facilities. Moreover, AFAICS asking for js_GetScopeChain for indirect eval is seems like a bug as that eval must never have access to the internals of a lightweight function. 

Fixing this bug would mean that for lightweight function only a debugger can ask for its Call object or scope chain. Which suggests to provide a custom version of Call and Block objects that would do the upvar check and those customized objects will be created for any light-weight function.

Comment 58

[Brendan Eich [:brendan]]

Paging mrbkap to the ER: did we really break indirect eval compatibility to match ES4-5 by making it global eval, never local (as if direct) eval? If not, this bug is not debugger-only.

/be

Comment 59

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Then can we get a test case?  Everything in this bug so far has indicated that you need the debugger API, which is different from a severity perspective IMO.

Comment 65

[Robert Sayre]

cf758561-e80a-4126-b12c-2361c2090525, an example of our js_MonitorLoopEdge top crash, looks related to this.

Comment 66

[Brendan Eich [:brendan]]

(In reply to comment #55)
> (In reply to comment #46)
> > Created an attachment (id=379837) [details] [details]
> > write-up of proposed solution
> 
> Here is yet another issue. Consider the following:
> 
> function f(a, b, c)
> {
>     a = (function() { return function() { return b; }; })();
> 
>     return c.a;
> }

The function immediately applied is a null closure with no upvars,b ut the function it returns is a flat closure formed when the display (once we fix JS_EvaluateUCInStackFrame) is correct. It is not a procedure, it's an "upward funarg".

> Suppose that a debugger is stopped on the "return c.a" line and a uses
> evaluates at that point "c = arguments" and then resume a normal execution of
> the function. This will leak upvar-referencing closure stored in "a" when doing
> c.a. But at that point there would be no debug frames on the stack.

There's no upvar referencing at that point. The immediately applied lambda called on the right-hand side of the assignment to a gets the b upvar and copies its value into a dslot for the upward funarg, which returns the value of that dslot.

> This suggests to do that opcode -> opcode_dbg transformation on any attempt to
> access arguments for a light-weight frame.

I claim again (comment 54) that Arguments objects are not relevant. You can't "get" a procedure to escape except via the scope chain, specifically from a Call object in which its non-escaping name was bound.

/be

Comment 67

[Igor Bukanov]

(In reply to comment #66)
> There's no upvar referencing at that point. The immediately applied lambda
> called on the right-hand side of the assignment to a gets the b upvar and
> copies its value into a dslot for the upward funarg, which returns the value of
> that dslot.

Yes, my reasoning about example is wrong. AFAICS any assignment to an argument can not put there a function that will access upvar later. The current escape analysis does not cover this case so this is safe.

> I claim again (comment 54) that Arguments objects are not relevant. You can't
> "get" a procedure to escape except via the scope chain, specifically from a
> Call object in which its non-escaping name was bound.

If a debug-injected eval does something like "global_variable = arguments" in a upvar-referencing closure, then the closure escapes via global_variable.callee. That does not reference any Call object.

Comment 68

[Brendan Eich [:brendan]]

(In reply to comment #67)
> If a debug-injected eval does something like "global_variable = arguments" in a
> upvar-referencing closure, then the closure escapes via global_variable.callee.
> That does not reference any Call object.

Indeed, this is why arguments usage (any, not just arguments.callee since arguments[callee_as_string] works too) defeats the procedure optimization.

So Call and Arguments require special handling.

Block does not, by the inductive proof. You can't get a procedure ref from a let binding unless you captured it by assignment, which defeats the procedure optimization and contradicts the hypothesis.

/be

Comment 69

[Brendan Eich [:brendan]]

Attachment: fix JS_EvaluateUCInStackFrame to recompute and restore display for non-topmost fp

Comment 70

[Brendan Eich [:brendan]]

Attachment: last patch, plus throw on arguments.callee leak of procedure

Comment 71

[Andreas Gal :gal]

Attachment: wrapper skeleton

Comment 72

[Andreas Gal :gal]

Attachment: wrap callee before handing it out via the debugger interface

Completely untested patch and I have really no idea what I am doing here. Feedback appreciated.

Comment 73

[Andreas Gal :gal]

One problem I don't know how to solve:

I had to introduce a new class (WrapperClass) so I can set a wrappedObject hook which is used by the strict equality check. As a result the object is no longer of FunctionClass. I am not sure whether thats bad or not.

Comment 74

[Igor Bukanov]

(In reply to comment #72)
> Created an attachment (id=380362) [details]
> wrap callee before handing it out via the debugger interface

Due to the bug 495325 this bug is no longer debug-only. As such for semantic correctness it would not be possible to fill the reconstructed display entry with values like (void) 0. So code patching may be in due. On the other hand the wrappers allows to limit the patched script to them avoiding deoptimizing the original function forever.

(In reply to comment #73)
> I had to introduce a new class (WrapperClass) so I can set a wrappedObject hook
> which is used by the strict equality check. As a result the object is no longer
> of FunctionClass. 

This is bad since this will lead to incorrect semantics in many cases. So what about making the function to implement the wrapper hook?

Comment 75

[Andreas Gal :gal]

Probably not a bad idea igor. We could make FunctionClass do all of this itself in call() for scripted functions since most the time we don't use the call path anyway so we can take a small check there whether the display is in good condition.

Comment 76

[Brendan Eich [:brendan]]

I've got a patch in progress here -- shall I take the bug? Igor, do you have a patch going?

/be

Comment 77

[Igor Bukanov]

(In reply to comment #76)
> I've got a patch in progress here -- shall I take the bug? Igor, do you have a
> patch going?

No: I spent some time on a patch with wrappers (making the Function to be extended class with a custom wrapper) and recompilation, but then I realized that, although it solves the indirect eval problem, it would not help the debugger. To get a source to compile the patch would need to use the decompiler, but that would not preserve the line numbers. So wrappers with code patching (a wrapper allow to skip any code locking) is seems way to go. 

Plus I still now idea how to check efficiently that a function or one of its nested functions uses upvars that accesses upper frames.

Comment 78

[Brendan Eich [:brendan]]

Attachment: proposed fix, v1

Lightly tested, writing tests now -- please help by trying this out, especially you Firebug and Venkman users. Thanks,

/be

Comment 79

[Brendan Eich [:brendan]]

Comment on attachment 380953 [details] [diff] [review]
proposed fix, v1

I'm no longer sure JS_EvaluateUCInStackFrame (who named that, anyway?) needs to fix up the display.

I think I see a hole in skipmin computation, though: need to include funargs (escaping functions) that become flat closures, as Igor pointed out in this bug in a comment.

/be

Comment 80

[Brendan Eich [:brendan]]

Attachment: proposed fix, v2

Comment 81

[Brendan Eich [:brendan]]

Comment on attachment 380985 [details] [diff] [review]
proposed fix, v2

FindFunArgs really needs to iterate over all upvars to compute skipmin, even if the function is already a funarg, or is flagged as one mid-iteration.

This version has more comments and seems clearer to me. Comments welcome (jorendorff too if he's watching).

/be

Comment 82

[Rob Campbell [:rc] (:robcee)]

running a 191 build with this applied now.

I errored out with the following:

/Volumes/Data/MOZILLA-191/mozilla-1.9.1/js/src/liveconnect/jsj.c:853: warning: redeclaration of ‘JSJ_ConvertJavaObjectToJSValue’ with different visibility (old visibility preserved)
/Volumes/Data/MOZILLA-191/mozilla-1.9.1/js/src/liveconnect/jsjava.h:306: warning: previous declaration of ‘JSJ_ConvertJavaObjectToJSValue’ was here
In file included from ../../../dist/include/js/jscntxt.h:53,
                 from /Volumes/Data/MOZILLA-191/mozilla-1.9.1/js/src/liveconnect/jsj_JSObject.c:56:
../../../dist/include/js/jsinterp.h:555: error: syntax error before ‘&’ token
make[4]: *** [jsj_JSObject.o] Error 1
make[4]: *** Waiting for unfinished jobs....
make[3]: *** [libs_tier_gecko] Error 2
make[2]: *** [tier_gecko] Error 2
make[1]: *** [default] Error 2
make: *** [build] Error 2

Comment 83

[Rob Campbell [:rc] (:robcee)]

build ran through after #ifdef'ing around the above syntax error in jsinterp.h

e.g.,

#ifdef __cplusplus
/* Notoriously fallible. */
extern JS_REQUIRES_STACK JSBool
js_LooselyEqual(JSContext *cx, jsval& lval, jsval& rval, JSBool *bp);
#endif

Browser runs and made it through most of the test suite before I killed it. Firebug UI starts up but I didn't get anything in my error console. Should've run a debug build.

Comment 84

[Rob Campbell [:rc] (:robcee)]

lastly, an exchange between bz and myself in irc:

21:10 <@bz> robcee: JavaScript error: chrome://firebug/content/bindings.xml, line 822: 
            Firebug.CommandLine is undefined
21:11 <@bz> JavaScript strict warning: chrome://firebug/content/bindings.xml, line 822: reference 
            to undefined property Firebug.CommandLine
21:11 <@bz> JavaScript error: chrome://firebug/content/lib.js, line 628: cannot access optimized 
            closure
21:11 < robcee> is that showing up in the error console?
21:11 <@bz> That last when clicking firebug icon
21:11 <@bz> yes, these are all in error console
21:11 <@bz> and on my stdout, of course
21:11 *** bz does have a debug build
21:11 < robcee> yeah
21:11 <@bz> is lib.js a JS component?
21:12 < robcee> it's a firebug module
21:12 < robcee> not in components, no
21:12 <@bz> ok
21:12 <@bz> I still get those with jit off

Comment 85

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

(In reply to comment #83)
> Firebug UI starts up but I didn't get anything in my error console.

"Firebug UI doesn't fully come up", I think.

Thanks for the testing help!

Comment 86

[Brendan Eich [:brendan]]

Attachment: proposed fix, v3

Hoping robcee can test and have good news.

/be

Comment 87

[Brendan Eich [:brendan]]

The patches wrap on every get. Also, the wrappers are not transparent with respect to mutation. These could be fixed but first order of business is to fix the unsafe closure leak.

/be

Comment 88

[Brendan Eich [:brendan]]

Attachment: proposed fix, v4

Firebug works. Bad bug fixed. Any arguments reification for a frame whose fun is a null closure with non-zero u.i.skipmin will fail. That's a "can't happen" kind of situation, except via a debugger.

/be

Comment 89

[Igor Bukanov]

(In reply to comment #88)
> Created an attachment (id=381010) [details]
> proposed fix, v4

(I do not have match time today - I am taking a day off to join a friend on an excursion, so here is an impression from a quick look):

I do not see that the patch does anything special for flat closure opcodes. AFAICS any _FC opcode must also be duplicated (or at least NewFlatClosure must check what kind of caller invoked the operation) since a flat closure creation code calls js_GetUpvar. That may access upframes outside the frame that creates the closure.

Comment 91

[Brendan Eich [:brendan]]

Attachment: proposed fix, v5

Comment 92

[Brendan Eich [:brendan]]

Attachment: v4-v5 interdiff

Bugzilla interdiff fails again.

/be

Comment 93

[Rob Campbell [:rc] (:robcee)]

v4 worked, bumping to new patch (4 + interdiff) to try the new hotness.

Comment 94

[Rob Campbell [:rc] (:robcee)]

verifies running above example interactively on the console. Probably be good to try this on a full set of unittests at this point.

Comment 95

[Brendan Eich [:brendan]]

Comment on attachment 381070 [details] [diff] [review]
proposed fix, v5

Hrm, at least the idiff on top of v4 is good.

/be

Comment 96

[Brendan Eich [:brendan]]

Attachment: proposed fix, v6

Comment 97

[Brendan Eich [:brendan]]

v4-v6 bugzilla interdiff works great.

/be

Comment 99

[Brendan Eich [:brendan]]

To make these wrappers fully transparent, we need at least:

* fun_addProperty, to put the property being defined on a wrapper on the wrapper's proto (the wrapped funobj).

* fun_resolve must handle __proto__ to hide the extra layer of delegation.

* fun_resolve must check the wrapped funobj in the proto slot for standard properties such as 'prototype'. Don't want "sub-classing" automation whereby a wrapper.prototype.__proto__ == wrapped.prototype. Do want wrapper.prototype === wrapped.prototype, I think (see below for more).

* fun_getProperty needs to skip to the wrapped object in proto, unless I'm missing something.

* fun_setProperty must store to the wrapped object property in general (the one created by fun_addProperty on the wrapped funobj, if it didn't exist before the addProperty call).

* Object.prototype.hasOwnProperty must lie for wrappers, to unify their "own" properties with their wrapped funobj's.

Blake knows more than I do about wrappers. What is missing from this list?

On the rewrap-on-every-get property of this patch: to fix it, we need to map wrapped funobj (JSObject * -- not JSFunction *fun!) in the scope of a particular Call object to wrapper funobj. This means a weak reference table (really, an Ephemeron table -- google it, popular topic in Ecma TC39 lately :->) optionally associated with every Call object.

That seems like a lot of work, so I'd like to split that out to a later bug. In fact I'd like to split all fully-transparent wrapper work out to post-1.9.1 bugs. Sometimes, though, user requirements push back and require more work in the end of the release cycle. So some analysis buddying would be appreciated.

/be

Comment 100

[John J. Barton]

(In reply to comment #99)
...
> That seems like a lot of work, so I'd like to split that out to a later bug. In
> fact I'd like to split all fully-transparent wrapper work out to post-1.9.1
> bugs. Sometimes, though, user requirements push back and require more work in
> the end of the release cycle. So some analysis buddying would be appreciated.

Is the "let the debugger turn off opt" still an option? As an expedience, using jsd.isOn to control opt would probably work if the failure mode is short of crashing.  Chromebug turns jsd on and leaves it on; Firebug turns it on once the user decides to use Firebug, then pauses/unPauses. So I don't expect a mix of opt and non-opt code for current versions of Firebug.

Comment 101

[Brendan Eich [:brendan]]

(In reply to comment #100)
> (In reply to comment #99)
> ...
> > That seems like a lot of work, so I'd like to split that out to a later bug. In
> > fact I'd like to split all fully-transparent wrapper work out to post-1.9.1
> > bugs. Sometimes, though, user requirements push back and require more work in
> > the end of the release cycle. So some analysis buddying would be appreciated.
> 
> Is the "let the debugger turn off opt" still an option?

That's really about a different question than the one of how transparent the wrappers are, unless the lack of transparency makes a debugger user wish to turn off optimizations. But it's possible the debugger user will feel more pain from an "optimized closure leak" exception. Either could motivate a "disable upvar optimizations" choice.

> As an expedience, using
> jsd.isOn to control opt would probably work if the failure mode is short of
> crashing.  Chromebug turns jsd on and leaves it on; Firebug turns it on once
> the user decides to use Firebug, then pauses/unPauses. So I don't expect a mix
> of opt and non-opt code for current versions of Firebug.

This bug 489255 work seems worth doing for either of the above reasons (wrappers break abstractions, wrapping can't be done because a closure escapes too far and you get the "leak" exception).

/be

Comment 102

[Rob Campbell [:rc] (:robcee)]

Brendan: I think for the scope of this bug, the additional wrapper implementation you're suggesting above sounds risky. I'm willing to treat optimization as a black-box at this point, and defer to a JIT-disabled view for debugging purposes. I'll continue following along in bug 489255 for those details.

Certainly, down-the-line, a more transparent optimizer would be ideal.

Comment 103

[Brendan Eich [:brendan]]

Rob: this bug is a blocker, we lose memory safety without it fixed.

So the only question for debuggers is whether to deoptimize when the debugger starts up, which will probably make some developers file bogus perf bugs, or do what I understand Visual Studio does sometimes: let the user see an exception and turn off opt and recompile.

/be

Comment 104

[Brendan Eich [:brendan]]

Attachment: proposed fix, v7

Blake reminded me how JSExtendedClass.equality works -- it's called only if tags compare equal (I reminded him that this lets null in along with objects via the jsval parameter :-P). So I took out js_LooselyEqual. I'll put it in another bug.

This is much reduced both in size and risk.

/be

Comment 105

[Brendan Eich [:brendan]]

Filed bug 496052 on js_LooselyEqual.

Aggregate diffstat win:
v6: 21 files changed, 568 insertions(+), 183 deletions(-)
v7: 19 files changed, 453 insertions(+), 99 deletions(-)

/be

Comment 106

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 381199 [details] [diff] [review]
proposed fix, v7

r=me with JSCLASS_IS_EXTENDED on js_FunctionClass.

Comment 107

[Brendan Eich [:brendan]]

Attachment: proposed fix, v7a

Fixed that JSCLASS_IS_EXTENDED oversight -- I was getting around to it! ;-)

/be

Comment 108

[Damon Sicore (:damons)]

I just heard that Igor is out for a while on a trip?  Can Shaver review this instead?

Comment 109

[Brendan Eich [:brendan]]

Comment on attachment 381209 [details] [diff] [review]
proposed fix, v7a

Shaver's comments are welcome, Jason is already looking I think.

/be

Comment 111

[Igor Bukanov]

The reason for the regression with the patch from the comment 107 in ecma_2/String/split-002.js is that now == mistests a regular expression object and the following throws "Bug": 

if ("".constructor == RegExp)
    throw "Bug";

Comment 112

[Brendan Eich [:brendan]]

Great to have Igor back in action after a day. Will fix today. Thanks,

/be

Comment 113

[Igor Bukanov]

Comment on attachment 381209 [details] [diff] [review]
proposed fix, v7a

>@@ -4324,7 +4320,7 @@ JS_PUBLIC_API(JSObject *)
> JS_CloneFunctionObject(JSContext *cx, JSObject *funobj, JSObject *parent)
> {
>     CHECK_REQUEST(cx);
>-    if (OBJ_GET_CLASS(cx, funobj) != &js_FunctionClass) {
>+    if (OBJ_GET_CLASS(cx, funobj) != &js_FunctionClass.base) 

This should be HAS_FUNCTION_CLASS(funobj). But perhaps to make the patch small js_FuncionClass should be renamed into something like js_FunctionExtendedClass with 

#define js_FuncionClass (js_FunctionExtendedClass.base) ?

Comment 114

[Igor Bukanov]

The patch from the comment 107 throws "an optimized closure leak" for the following test case:

function f(a, b) {
    var x = 0;
    ++x;

    function g() { return x; }

    a(b);
    return g();
}

dis("-r", f);


f(eval, "leak=function() { return g; }");

print(leak()());

The exception comes from the checks in js_GetCallVar that wraps the object only when the corresponding Call object is not put.

Comment 115

[Jason Orendorff [:jorendorff]]

Phew. There is a lot I still don't understand about this patch.

I think it's worth a comment explaining why we don't have to worry about JSOP_TRAP when memcpying the script code; and another one briefly explaining why it's safe to save cx->display and then restore it in JS_EvaluateUCInStackFrame.

Comment 116

[Igor Bukanov]

Given the ever increasing complexity of the patch to properly support all the issues with indirect eval (there more issues to consider) and relative lack of test coverage for the changes IMO we should really consider making the indirect eval a global one in FF 3.5. This is especially true given that indirect eval in 3.5 is semantically different from the indirect eval in 3.0 and turning it into global after FF 3.5 would mean yet another change. 

If this breaks some addons, then a promise to work with authors to fix their code could a viable alternative then spending efforts to fix the indirect eval and inevitable fallout from it. 

As regarding the issue with debugger compatibility, just making sure that the originally reported case works as expected and the debugger does not crash without full semantic preservation of the indirect eval would be an adequate solution.

Comment 117

[Brendan Eich [:brendan]]

Attachment: proposed fix, v8

This fixes flat closures that capture upvars across an escaping null closure, and of course the broken == bug (comment 112).

I also reduced the patch size using Igor's good suggestion from comment 113.

I don't care about the exception throwing reported in comment 114 at this point.

Igor won't review this before tomorrow, but I wonder if it can be landed tonight to get some testing from a larger group of debugger users and others who can help.

/be

Comment 118

[Brendan Eich [:brendan]]

Attachment: proposed fix, v8 (corrected patch)

Comment 119

[Brendan Eich [:brendan]]

Attachment: proposed fix, v8 (refreshed)

Let's try that again... mq tends to bring back the bad penny.

/be

Comment 120

[Igor Bukanov]

(In reply to comment #117)
> I don't care about the exception throwing reported in comment 114 at this
> point.

But why then care about preserving == and === and for the wrapped functions? I.e. indirect eval behaves very strangely already including, for example, flat closures, that do not see updates to local variables that indirect eval can do.

Comment 121

[Igor Bukanov]

Here for reference an example demonstrating that a flat closure does not see updates to a local variable caused by the indirect eval:

var e = eval;

function f()
{
    var x = 0;
    var g = function() { return x; }
    e('++x');
    return g;
}

print(f()());

The example prints 0 instead of expected 1.

Comment 122

[Igor Bukanov]

Here is example demonstrating that reconstructing the Call object chain only for upvar-referencing escaping closures is not enough:

var e = eval;

function f()
{
    var x = 0;
    return function() {
        return e('x');
    };
}

print(f()());

When executed (with the patch from the comment 119 applied), the example throws an exception instead of printing 0:

  ReferenceError: x is not defined

Given this issues I think the patch should not bother with reconstructing the Call chain for null closures. And since a chance that something like above exists on web IMO is bigger than the problem of wrappers that does not compare equal with each other, the patch should not bother with making special support for == and === in functions.

Comment 123

[Brendan Eich [:brendan]]

(In reply to comment #122)
> When executed (with the patch from the comment 119 applied), the example throws
> an exception instead of printing 0:
> 
>   ReferenceError: x is not defined

The goal is to make things work when the outer functions are still active. We can't reconstruct Call objects after their frames have popped, at least not at all easily!
 
> Given this issues I think the patch should not bother with reconstructing the
> Call chain for null closures.

Then we have problems in js_NewDebuggableFlatClosure. It at least needs to be prepared not to find outer variables.

> And since a chance that something like above
> exists on web IMO is bigger than the problem of wrappers that does not compare
> equal with each other, the patch should not bother with making special support
> for == and === in functions.

I buy that. Wrappers that aren't fully transparent shouldn't try to be in just one or two aspects.

/be

Comment 124

[Brendan Eich [:brendan]]

(In reply to comment #123)
> > And since a chance that something like above
> > exists on web IMO is bigger

This big, up to "bigger", I do not buy. We don't know who uses indirect eval in escaping funargs to look at null-closure upvars. It's unlikely and I hope we can break it -- doing so is strictly less incompatible a change than breaking all indirect eval.

But the real goal, again, is debugging in an active call stack. That seems worth supporting with Call object reification in WrapEscapingClosure. Is that code in the latest patch correct, though?

/be

Comment 125

[Igor Bukanov]

(In reply to comment #123)
> The goal is to make things work when the outer functions are still active.

I do not see why this should be a goal. So far the only report we have is that with the debugger a particular injected eval does not work. For this reason the patch should just make sure that that reported case works in a resonable way and that the code does not crash with an evil indirect eval. Anything beyond that should wait the real issues with addons or web sites using indirect eval.

> Then we have problems in js_NewDebuggableFlatClosure. It at least needs to be
> prepared not to find outer variables.

So be it - the lesson I have learn from the cache bugs is that changing the parent of any object may have global consequences that are hard to analyze.

Comment 126

[Brendan Eich [:brendan]]

(In reply to comment #125)
> (In reply to comment #123)
> > The goal is to make things work when the outer functions are still active.
> 
> I do not see why this should be a goal. So far the only report we have is that
> with the debugger a particular injected eval does not work.

That would be the testcase for bug 489255 (which has morphed badly but started out as a real debugability problem). Indeed that injected eval needs only display reconstruction. Ok, I will simplify the patch:

(1) No Call reconstruction in WrapEscapingFunction.
(2) js_NewDebuggableFlatClosure will check for properties not found.

/be

Comment 127

[Brendan Eich [:brendan]]

(3) Revert js_FunctionClass to JSClass, dropping the wrapper pretense.

/be

Comment 128

[Igor Bukanov]

(In reply to comment #124)
> This big, up to "bigger", I do not buy. We don't know who uses indirect eval in
> escaping funargs to look at null-closure upvars.

I agree that it is a bad practice to use induction to judge about the whole web from the personal experience, but suggestions to use "e = eval" to compress the web pages and fragments like eval(variable_name) still happens. On the other hand I have not aware of code that would rely on closure identity. But you are right. At this point we just do not know. 

> But the real goal, again, is debugging in an active call stack. That seems
> worth supporting with Call object reification in WrapEscapingClosure. Is that
> code in the latest patch correct, though?

The code seems to be correct. It is just I do not have good feeling about changing the parent chain since I do not have clear picture what are the security implication of that. I guess mrbkap should look at it.

Comment 129

[Brendan Eich [:brendan]]

Attachment: proposed fix, v9

Comment 130

[Brendan Eich [:brendan]]

(In reply to comment #128)
> The code seems to be correct. It is just I do not have good feeling about
> changing the parent chain since I do not have clear picture what are the
> security implication of that. I guess mrbkap should look at it.

I'm with you in wanting to avoid such scary heavyweight scope-chain reconstruction code -- it is fraught with peril. The compiler did away with runtime scope objects, yet we are trying to recreate them from possibly too little information. At least, that could be the case with even more aggressive optimizations in the future.

Let's go with v9 if we can.

/be

Comment 131

[Igor Bukanov]

(In reply to comment #126)
> (2) js_NewDebuggableFlatClosure will check for properties not found.

Note that if we would get reports about this, then an option could be considered to patch GETDSLOT into GETDSLOT_DBG. The latter would perform a name lookup and would allow to deal with issues like the comment 121.

Another thing to simplify the patch is to throw exception in XDR code about any attempt to serialize escaped closures.

Comment 132

[Brendan Eich [:brendan]]

(In reply to comment #131)
> (In reply to comment #126)
> > (2) js_NewDebuggableFlatClosure will check for properties not found.
> 
> Note that if we would get reports about this, then an option could be
> considered to patch GETDSLOT into GETDSLOT_DBG. The latter would perform a name
> lookup and would allow to deal with issues like the comment 121.

Yeah, I played with that briefly. I'd rather not if we can avoid it. Even with your great suggestions for reducing it, the patch is too big.

> Another thing to simplify the patch is to throw exception in XDR code about any
> attempt to serialize escaped closures.

I will add that, and anything else once full reviews are "in".

/be

Comment 133

[Igor Bukanov]

Comment on attachment 381536 [details] [diff] [review]
proposed fix, v9

>@@ -1261,11 +1260,27 @@ JS_EvaluateUCInStackFrame(JSContext *cx,

>+    JSStackFrame *displayCopy[JS_DISPLAY_SIZE];
>+    if (cx->fp != fp) {
>+        memcpy(displayCopy, cx->display, sizeof displayCopy);
>+
>+        for (JSStackFrame *fp2 = cx->fp; fp2 != fp; fp2 = fp2->down) {

Nit: comment here that fp2 must present on the call stack so fp cannot be null here.

>diff --git a/js/src/jsfun.cpp b/js/src/jsfun.cpp
>@@ -245,7 +245,14 @@ js_GetArgsObject(JSContext *cx, JSStackF
>+static JS_REQUIRES_STACK JSBool
>+WrapEscapingClosure(JSContext *cx, JSStackFrame *fp,
>+                    JSObject *funobj, JSFunction *fun,
>+                    jsval *vp)
>+
>+    if (fun->hasLocalNames()) {
>+        void *mark = JS_ARENA_MARK(&cx->tempPool);
>+        jsuword *names = js_GetLocalNameArray(cx, fun, &cx->tempPool);

The check for null names is missed.

>+        for (uintN i = 0, n = fun->countLocalNames(); i != n; i++) {
...
>+            JSLocalKind localKind = (i < fun->nargs)
>+                                    ? JSLOCAL_ARG
>+                                    : (i < fun->nargs + fun->u.i.nvars)

Nit: use i < fun->countArgsAndVars() here.

>+    JSScript *wscript = js_NewScript(cx, script->length, nsrcnotes,
>+                                     script->atomMap.length,
>+                                     (script->objectsOffset != 0)
...
>+    if (!wscript)
>+        return false;
>+

Nit: comment that no GC operation should happen until wfun->u.i.script = wscript.

>+
>+    jsbytecode *pc = wscript->code;
>+    while (*pc != JSOP_STOP) {
>+        JSOp op = js_GetOpcode(cx, wscript, pc);

This does not deal with JSOP_TRAP.

> static JSBool
> args_getProperty(JSContext *cx, JSObject *obj, jsval id, jsval *vp)
> {

Nit: assert here that fun is not escaping upvar closure.

>@@ -592,12 +754,42 @@ JSClass js_ArgumentsClass = {
> 
> JSClass js_DeclEnvClass = {
>     js_Object_str,
>-    JSCLASS_HAS_CACHED_PROTO(JSProto_Object),
>+    JSCLASS_HAS_PRIVATE | JSCLASS_HAS_CACHED_PROTO(JSProto_Object),

Comment before the class that private stores the frame.

>     JS_PropertyStub,  JS_PropertyStub,  JS_PropertyStub,  JS_PropertyStub,
>     JS_EnumerateStub, JS_ResolveStub,   JS_ConvertStub,   JS_FinalizeStub,
>     JSCLASS_NO_OPTIONAL_MEMBERS
> };
> 
>+static JS_REQUIRES_STACK JSBool
>+CheckForEscapingClosure(JSContext *cx, JSObject *obj, jsval *vp)
>+{

Nit: assert here that obj is DeclEnvClass or CallClass.

>+    if (cx->fp && cx->fp->script && (cx->fp->script->flags & JSSF_ESCAPE_HAZARD)) {

AFAIK the debugger may ask for callee property of DeclEnvClass not only via injected eval. So this check should be dropped. 

>+        jsval v = *vp;
>+
>+        if (VALUE_IS_FUNCTION(cx, v)) {
>+            if (FUN_NULL_CLOSURE(fun) && fun->u.i.skipmin != 0) {
>+                JSStackFrame *fp = (JSStackFrame *) JS_GetPrivate(cx, obj);


>+                JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
>+                                     JSMSG_OPTIMIZED_CLOSURE_LEAK);
>+                return JS_FALSE;
...
>+    return JS_TRUE;

Nit: use return false/true here. 

>@@ -725,10 +919,12 @@ js_PutCallObject(JSContext *cx, JSStackF
...
>+    if ((fun->flags & JSFUN_LAMBDA) && fun->atom) 
>+        JS_SetPrivate(cx, STOBJ_GET_PARENT(callobj), NULL);

Nit: add an assert here that STOBJ_GET_PARENT(callobj) is DeclEnvClass.

>@@ -1288,7 +1489,8 @@ js_XDRFunctionObject(JSXDRState *xdr, JS
>                                  JS_GetFunctionName(fun));
>             return JS_FALSE;
>         }

I suggest to throw an exception here if fun->u.i.wrapper is true.

>+JSObject *
>+js_NewDebuggableFlatClosure(JSContext *cx, JSFunction *fun)
>+{
>+    JSObject *closure = js_AllocFlatClosure(cx, fun, cx->fp->scopeChain);
>+    if (!closure)
>+        return NULL;
>+    JSAutoTempValueRooter tvr(cx, closure);
>+
>+    void *mark = JS_ARENA_MARK(&cx->tempPool);
>+    jsuword *names = js_GetLocalNameArray(cx, fun, &cx->tempPool);

Check for null names is missing.

>+    bool ok = true;
>+
>+    for (uintN i = 0, n = fun->u.i.nupvars; i != n; i++) {
>+        uintN index = fun->countArgsAndVars() + i;
>+        JSAtom *atom = JS_LOCAL_NAME_TO_ATOM(names[index]);
>+        jsid id = ATOM_TO_JSID(atom);
>+
>+        JSObject *obj, *obj2;
>+        JSProperty *prop;
>+        ok = js_FindProperty(cx, id, &obj, &obj2, &prop);
>+        if (!ok)
>+            break;
>+
>+        if (!prop) {
>+            const char *printable = js_AtomToPrintableString(cx, atom);
>+            if (printable)
>+                js_ReportIsNotDefined(cx, printable);
>+            ok = false;
>+            break;
>+        }
>+
>+        JS_ASSERT(OBJ_IS_NATIVE(obj));

I do not see why obj must be native here. What about, for example, functions executed under some global with(fast_array_instance) and atom  == "length"?

Nit: use UNLOCK_OBJ here since obj is native.

>+    }
>+
>+    JS_ARENA_RELEASE(&cx->tempPool, mark);
>+    if (!ok)
>+        return NULL;
>+
>+    return closure;
>+}

>diff --git a/js/src/jsinterp.cpp b/js/src/jsinterp.cpp
>--- a/js/src/jsinterp.cpp
>+++ b/js/src/jsinterp.cpp
>@@ -2589,12 +2589,20 @@ AssertValidPropertyCacheHit(JSContext *c
> JS_STATIC_ASSERT(JSOP_NAME_LENGTH == JSOP_CALLNAME_LENGTH);
> JS_STATIC_ASSERT(JSOP_GETGVAR_LENGTH == JSOP_CALLGVAR_LENGTH);
> JS_STATIC_ASSERT(JSOP_GETUPVAR_LENGTH == JSOP_CALLUPVAR_LENGTH);
>+JS_STATIC_ASSERT(JSOP_GETUPVAR_DBG_LENGTH == JSOP_CALLUPVAR_DBG_LENGTH);
>+JS_STATIC_ASSERT(JSOP_GETUPVAR_DBG_LENGTH == JSOP_GETUPVAR_LENGTH);
> JS_STATIC_ASSERT(JSOP_GETDSLOT_LENGTH == JSOP_CALLDSLOT_LENGTH);
> JS_STATIC_ASSERT(JSOP_GETARG_LENGTH == JSOP_CALLARG_LENGTH);
> JS_STATIC_ASSERT(JSOP_GETLOCAL_LENGTH == JSOP_CALLLOCAL_LENGTH);
> JS_STATIC_ASSERT(JSOP_XMLNAME_LENGTH == JSOP_CALLXMLNAME_LENGTH);
> 
> /*
>+ * Same for debuggable flat closures defined at top level in another function
>+ * or program fragment.
>+ */
>+JS_STATIC_ASSERT(JSOP_DEFFUN_FC_LENGTH == JSOP_DEFFUN_DBGFC_LENGTH);
>+
>+/*
>  * Same for JSOP_SETNAME and JSOP_SETPROP, which differ only slightly but
>  * remain distinct for the decompiler.
>  */
>@@ -5733,6 +5741,40 @@ js_Interpret(JSContext *cx)
>           }
>           END_CASE(JSOP_GETUPVAR)
> 
>+          BEGIN_CASE(JSOP_GETUPVAR_DBG)
>+          BEGIN_CASE(JSOP_CALLUPVAR_DBG)
...
>+            {
>+                void *mark = JS_ARENA_MARK(&cx->tempPool);
>+                jsuword *names = js_GetLocalNameArray(cx, fun, &cx->tempPool);

Again, a check for null names is missing.

>+            JS_ASSERT(prop);
>+            JS_ASSERT(OBJ_IS_NATIVE(obj));

Again, I do not see why obj is native or that prop exists here.


>diff --git a/js/src/jsparse.cpp b/js/src/jsparse.cpp

I am not qualified to review the parser changes so this is for Blake to check.

Comment 134

[Igor Bukanov]

(In reply to comment #123)
> The goal is to make things work when the outer functions are still active. We
> can't reconstruct Call objects after their frames have popped, at least not at
> all easily!

What if there are some addon or a web page that depends on that? I think at least a plan how to deal with that should be considered.

Comment 135

[Igor Bukanov]

Note: I will have to leave for few hours, but I will be online from 23:00 CEST or 2:00 PM PST.

Comment 136

[Brendan Eich [:brendan]]

(In reply to comment #133)
> >+    jsbytecode *pc = wscript->code;
> >+    while (*pc != JSOP_STOP) {
> >+        JSOp op = js_GetOpcode(cx, wscript, pc);
> 
> This does not deal with JSOP_TRAP.

Nor does js_GetVariableBytecodeLength(pc) -- that seems like a bug on its own. I will fix here since it's necessary to fix this bug.

> >+    if (cx->fp && cx->fp->script && (cx->fp->script->flags & JSSF_ESCAPE_HAZARD)) {
> 
> AFAIK the debugger may ask for callee property of DeclEnvClass not only via
> injected eval. So this check should be dropped. 

How else could it ask? Calling a non-debugger-compiled function can't have accessed a null closure callee since that implies a hole in the compiler's analysis that ruled out escape.

Thanks for the great comments. Blake has already reviewed the analysis. I have a few tweaks in addition to the ones you suggested, but I'm also considering again the idea of wrapping flat closures so as to rewrite JSOP_{GET,CALL}DSLOTS, for the use-case of some debugger user changing an upvar that used to invariantly dominate the flat closure's uses of it.

/be

Comment 137

[Igor Bukanov]

(In reply to comment #136)
> > AFAIK the debugger may ask for callee property of DeclEnvClass not only via
> > injected eval. So this check should be dropped. 
> 
> How else could it ask?

The debugger can call JS_GetFrameFunctionObject exposed via jsdIStackFrame.scope property. Then it can get the parent of that getting DeclEnvClass. 

But I realized that this will also expose CallClass or DeclEnvClass or BlockClass to scripts via jsdIValue.getWrappedValue allowing to manipulate the instances in arbitrary ways. This is just a bad idea. But this hole exists for quite some time and is limited only to debuggers so it should go to another bug that should be just wanted 1.9.1.

Comment 138

[Igor Bukanov]

It is interesting that the wrapper automatically protects against exposing a closure using the functionInstance.caller property. In particular, the example below prints null, not the g closure: 

var e = eval;

function f(arg) {
    var x = 0;
    function g() {
        return function h() {
            e(arg);
            return x;
        }();
    }

    return g();
}

f("tmp = h.caller");
print(tmp);

The reason for that null comes from the following activation-searching loop in fun_getProperty:

     for (fp = js_GetTopStackFrame(cx);
           fp && (fp->fun != fun || (fp->flags & JSFRAME_SPECIAL));
           fp = fp->down) {
          continue;
      }

Here fp->fun != fun always holds as DeclEnvClass creates a new function wrapper. This wrapper can not match fp->fun for unwrapped closure. That lead to fp set to null after the loop and fun_getProperty returning null.

But this suggests to assert after the loop when fp is not null that fun is not escaping closure.

Comment 139

[Brendan Eich [:brendan]]

Attachment: proposed fix, v10

Comment 140

[Brendan Eich [:brendan]]

Attachment: proposed fix, v10a

Bugzilla interdiff should tell the tale. Easy fix to over-aggressive optimized closure escape testing in the last patch.

/be

Comment 141

[Igor Bukanov]

Comment on attachment 381642 [details] [diff] [review]
proposed fix, v10a

>diff --git a/js/src/jsfun.cpp b/js/src/jsfun.cpp
>@@ -378,6 +563,7 @@ args_getProperty(JSContext *cx, JSObject
...
>+    JS_ASSERT(!FUN_NULL_CLOSURE(fp->fun) || fp->fun->u.i.skipmin == 0);

...

>+static JS_REQUIRES_STACK JSBool
>+CheckForEscapingClosure(JSContext *cx, JSObject *obj, jsval *vp)
...
>+            if (FUN_OPTIMIZED_CLOSURE(fun) && fun->u.i.skipmin != 0) {
...
>@@ -1069,6 +1305,8 @@ fun_getProperty(JSContext *cx, JSObject 
>         continue;
>     }
> 
>+    JS_ASSERT_IF(fp, !FUN_OPTIMIZED_CLOSURE(fun) || fun->u.i.skipmin == 0);


As discussed, the above assert and two ifs should use one common macro as a condition.

>+JSObject *
>+js_NewDebuggableFlatClosure(JSContext *cx, JSFunction *fun)
>+{
>+    jsval v;    /* not rooted, just a brief pass-through */

Nit: please use JSAutoTempValueRooter here. It is not worth to optimize for the speed. The cost of mental efforts to check the GC safety is greatly dwarfs any benefits.

>diff --git a/js/src/jsinterp.cpp b/js/src/jsinterp.cpp
>+          BEGIN_CASE(JSOP_DEFFUN_DBGFC)
>             LOAD_FUNCTION(0);
> 
>-            obj = js_NewFlatClosure(cx, fun);
>+            obj = ((op == JSOP_DEFFUN_FC)
>+                   ? js_NewFlatClosure
>+                   : js_NewDebuggableFlatClosure)(cx, fun);

IMO it is easy to follow a less hackish code:

>+            obj = (op == JSOP_DEFFUN_FC)
>+                  ? js_NewFlatClosure(cx, fun)
>+                  : js_NewDebuggableFlatClosure(cx, fun);

r+ with this issues addressed.

Comment 142

[Brendan Eich [:brendan]]

Attachment: proposed fix, v11

Landing this shortly.

/be

Comment 143

[Brendan Eich [:brendan]]

Comment on attachment 381676 [details] [diff] [review]
proposed fix, v11

one last tweak coming (assertion macrology, not substantive)...

/be

Comment 144

[Brendan Eich [:brendan]]

Attachment: proposed fix, v11a

Comment 145

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/c074f0f0ad2d

/be

Comment 146

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/c074f0f0ad2d

Comment 147

[Robert Sayre]

I wasn't able to get this onto 1.9.1 because I wasn't sure how to handle the one conflict, in js_NewFlatClosure.

Comment 148

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/abd967e2173b

Comment 149

[John J. Barton]

The nightly build:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1pre) Gecko/20090609 Shiretoko/3.5pre
crashes 
http://crash-stats.mozilla.com/report/index/9f9abaaf-a515-4189-bcbe-5386d2090609
and the stack points to frame.eval()

The particular case was reproducible for me, but hard for anyone else I guess. I think there is a way to reproduce it....do you want a new bug?

Comment 150

[John J. Barton]

Simpler test case:
https://bugzilla.mozilla.org/show_bug.cgi?id=497119