382509 - Disallow indirect eval

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Indirect eval is bad, among other reasons because indirect eval precludes many name-capture optimizations; at least for this reason ECMA-262 ed. 3 allows prohibiting it.  We should prohibit using eval except by directly calling it (or by w.eval, where w is a Window), per bug 380236 comment 14.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Neil, I see:
http://lxr.mozilla.org/mozilla/source/extensions/venkman/resources/content/venkman-static.js#463
http://lxr.mozilla.org/mozilla/source/extensions/venkman/resources/content/venkman-debugger.js#1490

I can't easily figure out what getCurrentFrame or currentFrame is. If it's always going to be a global object (window or component) then things should be fine. Otherwise, we might need to somehow relax our restrictions or change the Venkman code to use with (...) eval(...).

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch to fix

This already has brendan's review.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Better fix

This patch disallows [array].map(eval) entirely. The easy workaround is to simply wrap the eval in a function that passes the parameter through. This removes the crazy with stuff, and the choice is to pass a 2nd parameter as the scope chain, or to simply use dynamic scope (which appears to be what IE does, so we get compatibility with them with this patch).

Comment 4

[James Ross]

Blake, the Venkman code points are calling this function, not SpiderMonkey's eval:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/jsd/jsd_xpc.cpp&rev=1.80#1895

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 266681 [details] [diff] [review]
Better fix

>+        /*
>+         * Compile using caller's current scope object.
>+         *
>+         * Note: This means that native callers (who reach this point through
>+         * the C API) must use the two parameter form.

This is traditionally more "NB:" than "Note:" but in either case, what's the consequence of a comment in obj_eval on native callers (if any)? Perhaps instead we should restrict JS API calls to obj_eval without an intervening scripted frame.

/be

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

That's effectively what this does. I was hoping that if a SpiderMonkey embedder ran into this problem, they'd look at the source, see the comment, and know why their code wasn't working properly. Perhaps this comment belongs in another document 

By the way, should I file a new bug on IE-like not-quite-dynamic eval scope?

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #6)
> By the way, should I file a new bug on IE-like not-quite-dynamic eval scope?

Not yet. Reverse engineering a bit more, jaw dropping...

/be

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

I had to back this out -- XPCSafeJSObjectWrapper depends on this.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fixed patch

It turned out that XPCSafeJSObjectWrapper indirectly depended on being able to find eval on Object.prototype. This patch has a hunk to make it look on the global object instead (which does find it). This is safe, since we attach the safe constructor object from nsXPConnect::InitClasses, which runs before any content code could possibly muck with the global object.

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 267210 [details] [diff] [review]
Fixed patch

Yay for thread-unsafe js shell builds.

/be

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Let's try that "fixed on trunk" thing again!

Comment 13

[Carsten Book [:Tomcat]]

this seems to be caused layout regression like Bug 383381

Comment 14

[Bob Clary [:bc] (inactive)]

<http://lxr.mozilla.org/mozilla/source/js/src/jsobj.c#1285> comment seems incorrect now that GLOBAL.foo = eval; GLOBAL.foo("...") works.

/cvsroot/mozilla/js/tests/js1_5/Regress/regress-382509.js,v  <--  regress-382509.js
initial revision: 1.1

passes

/cvsroot/mozilla/js/tests/js1_6/Regress/regress-382509.js,v  <--  regress-382509.js
initial revision: 1.1

fails to throw error with [...].map(indirect-eval)

Comment 15

[Bob Clary [:bc] (inactive)]

mrbkap explained the scope issue. Test now passes.

/cvsroot/mozilla/js/tests/js1_6/Regress/regress-382509.js,v  <--  regress-382509.js
new revision: 1.2; previous revision: 1.1

Comment 16

[Jesse Ruderman]

What was the net change here, after bug 383381 was fixed?

Comment 17

[Brendan Eich [:brendan]]

(In reply to comment #16)
> What was the net change here, after bug 383381 was fixed?

(mrbkap: the comment near the top of obj_eval now lies -- please fix, r=me, so it does not say "Ban all indirect uses of eval...".)

Per ES4's proposal [resurrected eval], you can call eval(s), o.eval(s) where o is a window object, and evil = eval; evil(s) or o.evil(s) where o must again be a window object. The latter evil (renaming) cases get a strict warning. All other kinds of evals -- any that bind eval's |this| to a non-global object -- get an error.

/be

Comment 18

[John Resig]

(In reply to comment #17)
> Per ES4's proposal [resurrected eval], you can call eval(s), o.eval(s) where o
> is a window object, and evil = eval; evil(s) or o.evil(s) where o must again be
> a window object. The latter evil (renaming) cases get a strict warning. All
> other kinds of evals -- any that bind eval's |this| to a non-global object --
> get an error.

And I just want to clarify: with(obj) eval(s) will, or will not, be effected by this? It isn't clear to me, even after reading bug 383682, if this is something that is currently removed, and will be added back in, or if it does currently work, and needs to be removed.

(I'm slightly concerned because with(obj) eval(s) is an integral part to one of my current "JavaScript Sandbox" solutions.)

Comment 19

[Bob Clary [:bc] (inactive)]

added warning tests. Probably should move this to /extensions/...

/cvsroot/mozilla/js/tests/js1_5/Regress/regress-382509.js,v  <--  regress-382509.js
new revision: 1.2; previous revision: 1.1

Comment 20

[Brendan Eich [:brendan]]

with (o) eval(s) works, eval is found in a global object and its |this| binds to that base-of-reference-to-'eval'-the-identifier-being-called object ;-).

Bob, yeah: extensions.

/be

Comment 21

[Brendan Eich [:brendan]]

John: could you say more about your sandbox'ing with-eval usage? Want to make sure it is safe. Dynamic scope means eval in a with body will see the with object on the scope chain. I'm more concerned that the sandbox is leaky, if you know what I mean...

Bob: one more test question: how do we decide what is js1_5 vs. ecma3? This bug is beyond ecma3, but IIRC we have other tests under js1_5 that could be pure ECMA-262 Edition 3 topics.

/be

Comment 22

[Bob Clary [:bc] (inactive)]

I don't have a good procedure for deciding, I am ashamed to admit. I should be more proactive in identifying the section of ecma262-3 to which a test applies. I've filed Bug 384708 to track the outstanding cases.

Checking in extensions/regress-382509.js;
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-382509.js,v  <--  regress-382509.js
initial revision: 1.1
done
Removing Regress/regress-382509.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-382509.js,v  <--  regress-382509.js
new revision: delete; previous revision: 1.2

Comment 23

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0 linux/mac*/windows.

Comment 24

[Daniel Veditz [:dveditz]]

Definitely wanted on the branch, but nervous about "blocking" due to the potential for breaking sites (as seen in the regressions).

At the very least we need a branch-merged patch that includes the regressions fixes to approve. If you're around enough to do that we can reconsider blocking for this (1.8.1.12) release, otherwise will wait for later.

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: 1.8 branch patch

This is a roll-up of what actually landed and stuck on the trunk. In particular, the trunk patch lets most calls to indirect eval through (except for the eval.call(non-window) case) and keeps the varobj-setting code.

Comment 26

[Brendan Eich [:brendan]]

Comment on attachment 307136 [details] [diff] [review]
1.8 branch patch

An API change for sure, but it's necessary.

/be

Comment 27

[Daniel Veditz [:dveditz]]

Comment on attachment 307136 [details] [diff] [review]
1.8 branch patch

approved for 1.8.1.13, a=dveditz for release-drivers

Comment 28

[Blake Kaplan (:mrbkap) (inactive)]

Fixed on the 1.8 branch.

Comment 29

[Bob Clary [:bc] (inactive)]

verified fixed 1.8.1.13

Comment 31

[John Resig]

I realize that we've prohibited the use of moving eval to another variable, like so:
  var e = window.eval;
  e("1 + 1")

However, what is supposed to be the expected result when it's moved to another variable and then restored again?
  var e = window.eval;
  window.eval = function() { };
  window.eval = e;
  eval("1 + 1");

It isn't clear to me what the expected result should be - but it is something that has broken in Firefox 3 vs. Firefox 2 - and it affects the Microsoft Ajax Framework.

Comment 32

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

(That's an indirect eval, though possibly one that is worth special-casing.  Expectation is that it will behave identically to:

var e = window.eval;
window.eval = function(s) { e(s); }
eval("1+1");

I'm pretty sure.)

Comment 33

[Brendan Eich [:brendan]]

(In reply to comment #31)
> I realize that we've prohibited the use of moving eval to another variable,
> like so:
>   var e = window.eval;
>   e("1 + 1")

No, this is allowed and some web apps depened (or did depend until recently) on it. Note how at global scope, it's just another way to call global.eval. And in a function, because e(s) is not analyzed as eval(s), you get global.eval again:

js> function evil(s){var e=eval; return e(s)}
js> evil("1+1")
2
js> function evil(s){var e=eval; print(eval(s)); return e(s)}
js> evil("1+2")                                               
3
3

Testing |this| binding:

js> x = 42
42
js> function f(s){var e=eval; return e(s)}
js> f("x+1")
43
js> function f(s){return eval(s)}          
js> f("x+1")                      
43
js> f("this.x+1")                 
43
js> o={m:f}
[object Object]
js> o.m("this.x+1")
NaN
js> function f(s){var e=eval; return e(s)}
js> f("this.x+1")                          
43
js> o={m:f}                                
[object Object]
js> o.m("this.x+1")                        
NaN
js> o={m:f, x:22}       
[object Object]
js> o.m("this.x+1") 
23

This may be a bug, because IIRC ES4 mandates indirect eval is global.eval with no dynamic |this| propagation. Blake, what do you think?

> However, what is supposed to be the expected result when it's moved to another
> variable and then restored again?
>   var e = window.eval;
>   window.eval = function() { };
>   window.eval = e;
>   eval("1 + 1");
> 
> It isn't clear to me what the expected result should be - but it is something
> that has broken in Firefox 3 vs. Firefox 2 - and it affects the Microsoft Ajax
> Framework.

What broke? Can you attach a minimal testcase? Is there a js shell testcase? File a new bug in any event!

/be