442333 - Remove eval's optional second argument

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brendan Eich [:brendan]]

It is non-standard and it makes "private variables" via closures advisory only. See the URL.

IIRC Venkman and other Mozilla apps use eval(s, o). These XUL apps should use evalInSandbox, if they aren't already.

/be

Comment 1

[Brendan Eich [:brendan]]

Brian, can you take this onto your list to investigate? mrbkap is your Obi-Wan. Thanks,

/be

Comment 2

[Brian Crowder]

Sure!

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

bclary has an extension that depends on the 2nd argument. At the moment, I don't think it's possible to use evalInSandbox as a drop-in because you can't control the variables object with it. It seems that this is a pretty common use-case, though, so we should add that feature, somehow.

Comment 4

[Bob Clary [:bc] (inactive)]

actually, atm the extension depends on window.eval working even with the warning about being called directly. eval(...) nor eval(..., window) work. I do need a way to compile a script at least into the current window.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Oh, never mind me. I got my eval forms messed up. Carry on with the removal!

Comment 6

[Brian Crowder]

Attachment: as clumsy as a blaster

Help me, Obi Wan.  You're my only reviewer.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 327463 [details] [diff] [review]
as clumsy as a blaster

A thermal detonator would have been nicer (who needs eval anyway?) but this'll do nicely for now.

Comment 8

[Brian Crowder]

Should I be reporting a strict warning here, perhaps?  Would that make it easier for extensions depending on this, errr, extension to track it down?

Comment 9

[Brian Crowder]

Attachment: with a strict warning instead

Trivial, but I thought you might like to review the error message itself.

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 327674 [details] [diff] [review]
with a strict warning instead

I think the message is ok. Saying too much would probably just beg questions ;-).

>-     * Script.prototype.compile/exec and Object.prototype.eval all take an
>-     * optional trailing argument that overrides the scope object.
>+     * Script.prototype.compile/exec and Object.prototype.eval no longer take
>+     * an optional trailing argument.

Comment is still out of date (was before, but may as well fix). Strike the Script.prototype.* noise and just say "eval", since eval is no longer in Object.prototype.

>      */
>     if (argc >= 2) {
>-        if (!js_ValueToObject(cx, argv[1], &scopeobj))
>-            return JS_FALSE;
>-        argv[1] = OBJECT_TO_JSVAL(scopeobj);
>+        if (!JS_ReportErrorFlagsAndNumber(cx,
>+                                          JSREPORT_WARNING | JSREPORT_STRICT,
>+                                          js_GetErrorMessage, NULL,
>+                                          JSMSG_EVAL_ARITY)) {
>+            return JS_FALSE;
>+        }
>     }

Could use if (&&) instead of if-if, but the diff is nicer this way. No big, your call.

r=me with comment fix and optional nesting change.

/be

Comment 11

[Brian Crowder]

Comment on attachment 327674 [details] [diff] [review]
with a strict warning instead

Cleaned up the comment and merged the conditional (preferring code over patch), landed as:

changeset:   15612:e4fea8e9d036

Comment 12

[Brian Crowder]

Can we do this before 1.9.1?

Comment 13

[Andrea Giammarchi]

Could we move them into bogus section and perform a repository ticket reverse?

Comment 14

[Brian Crowder]

I'm not sure I understand your question in relation to this bug, Andrea

Comment 15

[Andrea Giammarchi]

My question was:
could we please ignore this bug and leave this feature that for some reason "we discovered only today" and instead of use them for amazing stuff we are scared about "some undefined closure virus"?

What I mean, seriously, is that this is not a bug, but a Gecko feature, and this is not a security problem, no more than eval itself, because if this is a security problem, our site is not secure at all even before somebody could use this feature.

So, why did you choose to "fix" them killing hundreds of possibilities for code introspection, sandbox possibilities, and who knows what else?

I do like your way to work, but this time, in my opinion, it was too quick :)

Best Regards

Comment 16

[Brian Crowder]

Andrea:  Do you have a specific compelling use-case for this "feature"?  Are you sure you can't implement it via other techniques?

Comment 17

[Andrea Giammarchi]

I wrote these two example rught now without testing (and thinking) too much, just as example:

{{{
// sandbox: redefined environemnt
var myEnvironMent = {
	eval:function(script){
		return	eval(script);
	},
	alert:function(String){
		(document.body || document.documentElement)
			.appendChild(document.createTextNode(String));
	}
};

// evaluate a script from a string or a textarea
eval("alert('Hello World')", myEnvironMent);

/* you cannot do this with a with statement
with(myEnvironMent)
	eval("alert('Hello World')");
*/



// privileged protected methods in prototype
Protected = function(){
	function _showMessage(message){
		alert(this.name.concat(" say ", message));
	}
	return function(name){
		this.name = name;
	};
}();
Protected.prototype._ = function(method){
	return	eval("_" + method, this).apply(this, Array.prototype.slice.call(arguments, 1));
};

var p = new Protected("Andrea");
p._("showMessage", "Hello World");

}}}

Comment 18

[Andrea Giammarchi]

You probably know that, but please note that last example could be executed inside the closure as well, but could not be executed outside, and that is only a method.
So, it means that you cannot redefine a prototype of your own script runtime, outside one closure, using that feature.

Regards

Comment 19

[Brendan Eich [:brendan]]

It's true we are taking away functionality not available otherwise.

One question of whether the loss is worth the gain in standard-following and integrity is hard to answer in ways that convince different parties. Those valuing integrity hate this extension. Those using it for reflection and extensibility love it. Dessert topping and floor wax.

Another question: is there a way of fixing this bug without removing the optional second argument?

Could we avoid exposing SpiderMonkey's intrinsic scope chain link, the parent slot (__parent__, called [[Scope]] in ES3), which is an implementation detail?

That is, the hole for private variables lies in the fact that eval(s, o) uses o, o.__parent__, etc. up the scope chain. If o is a function scoped by an activation containing private variables, o.__parent__ is the activation and the privacy has no integrity.

We do not want to switch to an extrinsic link, but could we censor the intrinsic one in just this case? If we wrap o in a With object, then it behaves as would an object in an implementation with extrinsic scope chain links. Crowder, the with wrapping is already there for the o.eval(s) code, in the

#if JS_HAS_EVAL_THIS_SCOPE

code. This should be reused.

Sorry I didn't raise this possibility sooner.

/be

Comment 20

[Andrea Giammarchi]

Brendan, first of all thank you to consider my point of view.
Secondly, your suggestion removes the feature as well, because doing them, I will not be able to create something like that:

----------------
const safeEnvironment = function(A, B, D, F, N, O, R, S){
    const   Array   = A,
            Boolean = B,
            Date    = D,
            Function= F,
            Number  = N,
            Object  = O,
            RegExp  = R,
            String  = S,
            eval    = function(String){
                return  self.eval("(" + String + ")");
            };
    return  new function(){};
}(
    Array,
    Boolean,
    Date,
    Function,
    Number,
    Object,
    RegExp,
    String
);
----------------

as a secure environment for code execution and/or evaluation, i.e.

----------------
eval('Array=function(){}; alert(Array);', safeEnvironment);
// native Array, no redefinition possibilities thanks to const keyword
----------------


Please reconsider this bogus and ask to Webkit and Opera guys to implement this feature, instead of removing them from every browser :D

As a clever developer said in John blog, it is like somebody asks to remove Reflection from .NET or other languages because it is able to read private stuff.

Does it make any sense? So, why should we block this language possibility?

Kind Regards

Comment 21

[Brian Crowder]

Attachment: applied to 1.9 branch

Comment 22

[Brian Crowder]

Would it be alright to handle restoring the reflection/utility of eval() in another bug?  I'd like to just get this fix landed, it is simple and straightforward.

Comment 23

[Andrea Giammarchi]

Brian, I would like to move this ticket into bogus, or mark them as WONT FIX.

There is no reason at all to consider this one a bug, while this other one, for example, could be more interesting, isn't it?

----------------

eval.call(this, "var PARTIALLY_HIDDEN = 123;");
alert(PARTIALLY_HIDDEN)           // undefined !!! unexpected
alert(this.PARTIALLY_HIDDEN)      // 123, as expected
alert("PARTIALLY_HIDDEN" in this) // true, as expected

// the same with
(function(){
    eval.call(this, "var PARTIALLY_HIDDEN = 123;");
})();

// the interesting stuff come with const, that works as expected
eval.call(this, "const GLOBAL = 123;");
alert(GLOBAL) // 123
----------------

Please, do not fix / close this ticket related Gecko feature. Thank you.

Comment 24

[Brian Crowder]

As noted in comment #11, I have already landed a patch on the main-trunk.  I have not landed that same patch on the 1.9 branch, but I think it is sufficient, doesn't break web-content, and is an important step in addressing legitimate concerns about this feature.  I still feel the remaining development (ie., restoring the utility of the second argument in a secure way) should continue in another bug.

I think the examples you cite in comment #23 are an issue with duplicates another bug already in bugzilla; though I cannot find it right now.

Comment 25

[Samuel Sidler (old account; do not CC)]

(In reply to comment #24)
> As noted in comment #11, I have already landed a patch on the main-trunk.

Is there any reason to not re-resolve this bug as FIXED then?

Comment 26

[Brian Crowder]

I marked it fixed, but Brendan re-opened.  I asked for forgiveness in comment #22 and he hasn't replied yet.  I'd like to do my penance in another bug.

Comment 27

[Brendan Eich [:brendan]]

Regressing and then restoring "later" risks never. If this is a bad enough change for some people, we should avoid regressing.

/be

Comment 28

[Brian Crowder]

We're not regressing, we're making a change in order to achieve compliance with the spec.  The restoration of whatever reflection utility is desired is an enhancement.  It's not entirely clear to me what's required for that, anyway, since Andrea seems dissatisfied with the "with" wrapping fix you suggested.  I would like to land this fix for 1.9.0.2 in a timely fashion, and address Andrea's concerns in another bug.  Yes, that bug would be an enhancement bug, and yes, there would be a risk of "never" fixing it, but in the worst possible case, that would mean that we behave exactly as IE, Safari, and Opera do today.

Comment 29

[Brendan Eich [:brendan]]

In the intro to chapter 15, paragraphs 5 and 6, ES3 says (IIRC ES1 said):

"Unless otherwise specified in the description of a particular function, if a function or constructor described in this section is given more arguments than the function is specified to allow, the behaviour of the function or constructor is undefined. In particular, an implementation is permitted (but not required) to throw a TypeError exception in this case.

NOTE Implementations that add additional capabilities to the set of built-in functions are encouraged to do so by adding new functions rather than adding new parameters to existing functions."

So there's not a spec compliance issue, we just didn't do as encouraged 10 ago (see rev 3.3 of jsobj.c). Changing our ~10 year old behavior in a breaking way is a regression, no point in denying it.

Undoing the regression while avoiding the problem Peter Michaux pointed out, all in a separate bug, is ok -- just don't LATER it :-/. Please cite it here and mark this one FIXED. Thanks,

/be

Comment 30

[Biju]

(disclosure: I came to know about this eval feature only 
             after reading Peter Michaux)

One of the possibilities I see with this eval feature is,
you can write smaller bookmarklet or Greasemonkey script 
to fix some website problem.
And the fact is most intranet sites are still not supporting Firefox.

I would like to give power to the end user than a web developer.
so want I vote for making this WONTFIX

And for "caja" they can solve it by a rewrite rule for eval.

Comment 31

[Brendan Eich [:brendan]]

It's not just Caja -- some people view private variables as advisory while others want mandatory cross-browser enforcement.

I think the followup bug, or this one if the patch backs out (no need, just saying that would make the REOPEN => ASSIGNED transition sensible), should be patched to hide the intrinsic scope chain link implementation detail. [[Scope]] in a function is JSSLOT_PARENT in SpiderMonkey, and universal for all objects.

But that can be hidden with some special casing in obj_eval for function object as 2nd arg. One way is to skip all Call objects, using the global reached at the end of the scope chain.

/be

Comment 32

[Brian Crowder]

Isn't one of the main utilities of this function that of being able to reflect into functions?  Can someone clarify for me the use-cases we're trying to preserve?  The use-cases we're trying to defend against?  They seem to be rather at-odds with one another in my current understanding; but maybe I'm missing something.  I'm increasingly tempted to just back-out my changeset for this on the trunk, and WONTFIX this bug (with another bug perhaps to be opened to refine this feature as needed?).

Comment 33

[Brian Crowder]

After a discussion with mrbkap, I've decided to follow my original plan to proceed with the current removal (already done on trunk) patches, and follow-up to restore whatever functionality Andrea, Brendan, and Biju are looking for in bug 446026.

Comment 34

[Brendan Eich [:brendan]]

(In reply to comment #32)
> Isn't one of the main utilities of this function that of being able to reflect
> into functions?

No. But that's not what Peter Michaux showed: he showed peeking into closures from functions.

> Can someone clarify for me the use-cases we're trying to
> preserve?

See comment 17 and comment 20, again.

/be

Comment 35

[Bill Wood]

I'm disappointed to see this capability removed before the replacement capability is defined, discussed, and implemented (even if in the same release).  It seems to have great utility for allowing eval in a controlled, sandboxed environment.

Comment 36

[Brendan Eich [:brendan]]

Bill, there's nothing controlled or sandboxed about eval(s,o) -- too many ways to attack o from within s, get out of the sandbox.

The evalInSandbox method of Components.utils is the better way to go. The most to be done for eval(s, o) is avoid leaking the scope chain abstraction's intrinsic link implementation.

/be

Comment 37

[Brendan Eich [:brendan]]

I trust crowder to restore eval(s, o) to some semblance of its former self, or to add evalInSandbox and show that it covers the cited use-cases. If this does not happen in the 3.1 development cycle (a short one!), then I will be the first to re-open this bug and ask for a back-out.

/be

Comment 38

[Samuel Sidler (old account; do not CC)]

Should we get a test for this before landing on the stable branch?

Comment 39

[Dão Gottwald [:dao]]

It seems like this shouldn't actually land on branch unless bug 446026 is a branch bug as well.

Comment 40

[Brian Crowder]

I don't think that bug 446026 should block this bug landing anywhere, and that is one of the reasons I advocated moving that work there.  Here's a test:

function env() { var foo = "bar"; return function () { foo } }; eval("foo", env())

That should throw a reference error, if this bug is fixed.

Comment 41

[Samuel Sidler (old account; do not CC)]

As I said to Brian on IRC earlier, let's get an automated test for this before approval.

Comment 42

[Cyrus Omar]

There is a clear need for certain types of applications to be able to access closure variables of functions. In particular, serialization comes to mind. If I want to serialize an object where one of its properties is a function which accesses enclosed variables, how should I go about that without access to __parent__ or using this method. Serialization is not something I would call a "bug", especially since it has been available as long as I can remember in SpiderMonkey. Removing it is a regression and will break code I have written. In fact, if anything needs to be fixed, its that we need to use eval to do it rather than simply giving back access to __parent__, which was another hastily decided "bug fix".

If we want to explore ways to strictly enforce private variables, I would consider that a feature request, not a bug fix, since SpiderMonkey hasn't had strictly enforced private variables before.

Comment 43

[Brian Crowder]

There are other ways to implement serializable structures in the language and libraries that were specified in ECMA-262.  This feature was not specified in the language or libraries documented there and it presents other issues that must be resolved (documented elsewhere).  I am interested in restoring the utility of a feature along these lines that better suits the recommendations of the language spec, but this is neither to place to do that work, nor to advocate for it.

Bug 446026 is the place for the work, but bugzilla is not a good place for advocacy in general.  A number of different blog entries have been written on the matter, and newsgroups are also available for discussions like this.  Please direct concerns like this to those places, instead.

Comment 44

[kanasimi]

How about this:

--------------------------------------------
var
classT=

(function(){

var pv=(Math.random()+'').replace(/\./,''),
_=function(){};
eval('pv=(function(){var _'+pv+'=[12,34];return function(){return _'+pv+';}})();');
pv.toString=function(){return '';};

_.get=function(){
 return pv();
};

return _;
})();

alert(typeof pv);
alert(classT.get());
eval('alert(""+pv);',classT);

--------------------------------------------

If we can't get the variables pv() returns, we may not need to fix the bug.

Comment 45

[kanasimi]

(In reply to comment #44)
> How about this:

Sorry, there's still some way to get the variables.
My test is not enough.

Comment 46

[Samuel Sidler (old account; do not CC)]

Any update here on getting a test? Bob?

Comment 47

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_8/regress/regress-442333-01.js,v  <--  regress-442333-01.js
initial revision: 1.1

mozilla-central: changeset:   16480:df0c529a1069

Comment 48

[Samuel Sidler (old account; do not CC)]

Comment on attachment 329522 [details] [diff] [review]
applied to 1.9 branch

Approved for 1.9.0.2. Please land in CVS. a=ss

Comment 49

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0/trunk linux/mac/win

Comment 50

[Dave Townsend [:mossop]]

What was the reasoning behind taking this behaviour change on the stable branch where it has broken a number (albeit small) of extensions and presumably websites. Was there any announcement of the change? I do not see it in the 3.0.2 release notes.

Comment 51

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

I wouldn't presume that it broke web sites, but it's possible that it did -- have you any examples?  (Running script in another context isn't nearly as widely done on the web, this technique was not very well known, and it's not available in any other browser.)

Comment 52

[Brendan Eich [:brendan]]

(In reply to comment #39)
> It seems like this shouldn't actually land on branch unless bug 446026 is a
> branch bug as well.

Dao was right, and I did not pay attention to bugmail -- sorry. This patch should not have gone into cvs.

/be

Comment 53

[Dave Townsend [:mossop]]

(In reply to comment #52)
> (In reply to comment #39)
> > It seems like this shouldn't actually land on branch unless bug 446026 is a
> > branch bug as well.
> 
> Dao was right, and I did not pay attention to bugmail -- sorry. This patch
> should not have gone into cvs.

Should we back it out of CVS for a coming update? I presume the 3.0.3 boat has sailed already.

Comment 54

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

Not clear to me that more churn helps, especially if we're not sure what the 3.1 behaviour will finally look like.  There are available workarounds in the form of eIS for extension script, and I don't think it'll matter by the time 3.0.4 has shipped, unless people stay on 3.0.1.  That would be very unfortunate, since there are some important security fixes in 3.0.2.

(3.0.3 is indeed not an available vehicle.)

Comment 55

[Brian Crowder]

Alright, this is totally my fault.  I think this should be backed out (in CVS and m-c) until I have a better understanding of what extension authors need here.  The "bug" being fixed isn't a huge one in any case.

Comment 56

[Dão Gottwald [:dao]]

What's the current status here?

Comment 57

[Brian Crowder]

The "restore utility of eval()" patch has landed on tracemonkey, so this test is no longer valid.

Comment 58

[Caio Tiago Oliveira (asrail)]

The "verified" on this bug makes no sense on 1.9.1 right now (won't make sense when tracemonkey changes land there).
But the test will still valid for 1.9.0.x, doesn't it?

Comment 59

[Bob Clary [:bc] (inactive)]

caio, the test will be excluded on 1.9.1. i suppose we could retarget this bug to just 1.9.0. i'll leave that call to crowder.