The default bug view has changed. See this FAQ.

Remove eval's optional second argument

VERIFIED FIXED in mozilla1.9.1a1

Status

()

Core
JavaScript Engine
VERIFIED FIXED
9 years ago
6 years ago

People

(Reporter: brendan, Assigned: Brian Crowder)

Tracking

({verified1.9.0.2})

Trunk
mozilla1.9.1a1
verified1.9.0.2
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.1 ?
wanted1.9.0.x +
wanted1.8.0.x ?
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

9 years ago
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
Flags: wanted1.9.1?
(Reporter)

Comment 1

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

/be
Assignee: general → crowder
(Assignee)

Comment 2

9 years ago
Sure!
Status: NEW → ASSIGNED
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

9 years ago
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.
Oh, never mind me. I got my eval forms messed up. Carry on with the removal!
(Assignee)

Comment 6

9 years ago
Created attachment 327463 [details] [diff] [review]
as clumsy as a blaster

Help me, Obi Wan.  You're my only reviewer.
Attachment #327463 - Flags: review?(mrbkap)
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.
Attachment #327463 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 8

9 years ago
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?
(Assignee)

Comment 9

9 years ago
Created attachment 327674 [details] [diff] [review]
with a strict warning instead

Trivial, but I thought you might like to review the error message itself.
Attachment #327463 - Attachment is obsolete: true
Attachment #327674 - Flags: review?(brendan)
(Reporter)

Comment 10

9 years ago
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
Attachment #327674 - Flags: review?(brendan) → review+
(Assignee)

Comment 11

9 years ago
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
Attachment #327674 - Flags: approval1.9.0.2?
(Assignee)

Updated

9 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

9 years ago
Target Milestone: mozilla2.0 → mozilla1.9.1a1
(Assignee)

Comment 12

9 years ago
Can we do this before 1.9.1?
Flags: wanted1.9.0.x?

Comment 13

9 years ago
Could we move them into bogus section and perform a repository ticket reverse?
(Assignee)

Comment 14

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

Comment 15

9 years ago
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
(Assignee)

Comment 16

9 years ago
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

9 years ago
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

9 years ago
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
(Reporter)

Comment 19

9 years ago
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 20

9 years ago
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
(Assignee)

Comment 21

9 years ago
Created attachment 329522 [details] [diff] [review]
applied to 1.9 branch
Attachment #329522 - Flags: approval1.9.0.2?
(Assignee)

Updated

9 years ago
Attachment #327674 - Flags: approval1.9.0.2?
(Assignee)

Comment 22

9 years ago
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

9 years ago
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.
(Assignee)

Comment 24

9 years ago
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.
(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?
(Assignee)

Comment 26

9 years ago
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.
(Reporter)

Comment 27

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

/be
(Assignee)

Comment 28

9 years ago
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.
(Reporter)

Comment 29

9 years ago
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

9 years ago
(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.
(Reporter)

Comment 31

9 years ago
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
(Assignee)

Comment 32

9 years ago
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?).
(Assignee)

Comment 33

9 years ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Flags: wanted1.8.0.x?
Resolution: --- → FIXED
(Reporter)

Comment 34

9 years ago
(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

9 years ago
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.
(Reporter)

Comment 36

9 years ago
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
(Reporter)

Comment 37

9 years ago
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
Should we get a test for this before landing on the stable branch?
Flags: in-testsuite?
It seems like this shouldn't actually land on branch unless bug 446026 is a branch bug as well.
(Assignee)

Comment 40

9 years ago
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.
As I said to Brian on IRC earlier, let's get an automated test for this before approval.

Comment 42

9 years ago
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.
(Assignee)

Comment 43

9 years ago
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.
Whiteboard: [needs test]

Comment 44

9 years ago
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

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

Sorry, there's still some way to get the variables.
My test is not enough.
Flags: wanted1.9.0.x? → wanted1.9.0.x+
Any update here on getting a test? Bob?

Comment 47

9 years ago
/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
Flags: in-testsuite?
Flags: in-testsuite+
Flags: in-litmus-
Whiteboard: [needs test]
Comment on attachment 329522 [details] [diff] [review]
applied to 1.9 branch

Approved for 1.9.0.2. Please land in CVS. a=ss
Attachment #329522 - Flags: approval1.9.0.2? → approval1.9.0.2+
(Assignee)

Updated

9 years ago
Keywords: fixed1.9.0.2

Comment 49

9 years ago
verified fixed 1.9.0/trunk linux/mac/win
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.0.2 → verified1.9.0.2
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.
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.)
(Reporter)

Comment 52

9 years ago
(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
(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.
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.)
Depends on: 457068
No longer depends on: 457068
Depends on: 457068
(Assignee)

Comment 55

9 years ago
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.
What's the current status here?
(Assignee)

Comment 57

8 years ago
The "restore utility of eval()" patch has landed on tracemonkey, so this test is no longer valid.
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

8 years ago
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.

Updated

8 years ago
Depends on: 446026
You need to log in before you can comment on or make changes to this bug.