406477 - eval of function x() in a function with an argument "x" and "let x" overwrites the wrong x

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P4)

Description

[Igor Bukanov]

Consider the following test case:

function test_param(x, src)
{
    {
        let x = 2;
        eval(src);
    }
    return x;
}

function test_var(src)
{
    var x = 1;
    {
        let x = 2;
        eval(src);
    }
    return x;
}

var test_param_result = test_param(1, "function x() { }");
if (test_param_result !== 1)
    throw "Unexpected test_param_result value: "+uneval(test_param_result);

var test_var_result = test_var("function x() { }");
if (test_var_result !== 1)
    throw "Unexpected test_var_result value: "+uneval(test_var_result);


Currently the test case throws an exception when executed in js shell:

uncaught exception: Unexpected test_var_result value: function x() {}

That is, in test_var the eval overwrites the top level var x, not the "let x", despite the eval done in the let block. Note that test_param works as expected with eval modifing the let variable. 

This is a regression introduced in bug 395907.

Comment 1

[Brendan Eich [:brendan]]

function definitions never bind in the block object, only in the variable object. So the test_param success is not showing eval binding function x() {} in the let block -- it's simply showing that the getarg opcode flowing into return fetches the unmodified argv[0] value. Adding print('in block: ' + x) calls after each eval, but still in the block scope of the let, proves this.

ES4 is forbidding eval from overwriting DontDelete bindings with delete-able bindings:

http://bugs.ecmascript.org/ticket/235

That ticket does not propose how to prevent eval from doing this, but throwing an exception is the only consistent way to go. Not sure whether TypeError is the right kind of exception to throw. Could we bend EvalError to fit here?

/be

Comment 2

[Igor Bukanov]

When filing the bug I missed the fact that the expected bahavior in SM for function expression statements or function statements not at the top level is to add a property at the top level no matter how deeply the statement is nested inside the let/with blocks. 

Thus in the above test in both cases the top-level x should be overwritten in both cases and the bug is that in the first case it does not happen. Here is a test case to capture this expected behavior:

function test(x, src)
{
    var y = 1;
    {
        let x = 2;
        let y = 2;
        eval(src);
    }
    return [x, y];
}

var [test_param_result, test_var_result] =
    test(1, "function x() { }\nfunction y() { }\n");

var e = "";

if (typeof test_param_result != "function")
    e += "Unexpected test_param_result value: "+uneval(test_param_result)+"\n";

if (typeof test_var_result != "function")
    e += "Unexpected test_var_result value: "+uneval(test_var_result)+"\n";

if (e)
    throw e;

Currently the test case generates:
uncaught exception: Unexpected test_param_result value: 1

Comment 3

[Brendan Eich [:brendan]]

The bugs.ecmascript.org ticket #235 points to bug 395868 -- no browser does what ECMA-262 says, according to that bug. No other browser supports let, but that is not relevant. We should try to do what ES4 proposes, pinning down the exception to throw, rather than try to follow the spec when no one else does, and when doing so violates integrity and overcomplicates our code.

/be

Comment 4

[Igor Bukanov]

(In reply to comment #1)
> function definitions never bind in the block object, only in the variable
> ES4 is forbidding eval from overwriting DontDelete bindings with delete-able
> bindings:
> 

As bug 395868 points out, nobody implements that DontDelete overwrite from ES3. In case of SM the logic can be described as if eval-introduced declarations do not alter any attributes but rather performs:

varobject.x = value 

where x is the name of var or function from the eval. If x does not exist, then after it is added it would be deletable and enumeratable property. If x exists, then SM either updates x or ignores the assignment for ReadOnly x. Moreover, in the strict mode, SM will throw an exception. I suspect this is what other browsers do as well.

On the other hand what the ticket #235 proposes is that eval should just throw an exception on any name collision with DontDelete properties. This is unreasonable IMHO since expanding the eval from the test in comment 2 gives:

function test(x, src)
{
    var y = 1;
    {
        let x = 2;
        let y = 2;
        eval(src);
    }
    return [x, y];
}

=>

function test(x, src)
{
    var y = 1;
    {
        let x = 2;
        let y = 2;
        function x() { }
        function y() { }
    }
    return [x, y];
}

This is reasonable code IMO with semantics of varobj.x = function assignment.  

The bottom line is that the ticket #235 goes too far IMO.
 

Comment 5

[Brendan Eich [:brendan]]

The ES4 trac isn't good at linking related bugs, as bugzilla is. See also

http://bugs.ecmascript.org/ticket/297

where it is proposed that eval in ES4 (with version=4 on the application/ecmascript type, or version=2 on application/javascript -- and text/ is allowed too, but frowned upon, per RFC 4329) should always use a fresh variable object. That takes the sting out, but it's not going to fit into JS1.8.

How would you like to fix this for JS1.8 / mozilla1.9?

/be

Comment 6

[Igor Bukanov]

Attachment: possible fix

(In reply to comment #5)
> How would you like to fix this for JS1.8 / mozilla1.9?

The patch for bug 395907 tried to replace DEFFUN with DEFLOCALFUN during the compilation. But that is equivalent to replacing OBJ_DEFINE_PROPERTY with OBJ_SET_PROPERTY. So the patch makes that explicit which made the checks in the compiler unnecessary.

This patched code passes both the test case from bug 395907 and the test case from comment 2. But I have not done any other testing.

Comment 7

[Igor Bukanov]

(In reply to comment #6)
> Created an attachment (id=291149) [details]
> possible fix

An alternative compiler-based solution is to add something like DEFPARAMFUN. But this would more code than the runtime fix. 

Comment 8

[Brendan Eich [:brendan]]

Note also that eval users are few, abusers many. If you eval a string in an environment where bindings may collide, you probably would rather have an exception than an integrity hole, which could be a security hole. It is common to use an anonymous function applied immediately to get a fresh scope and variable object.

/be

Comment 9

[Igor Bukanov]

(In reply to comment #3)
> No other browser supports let, but that is not relevant. 

This is very relevant since with let SM/ES4 can at least try to fix the mess with the scope of function expression statements. The current behavior of SM does not match ES3 emulation using temporary function like in:

function foo() {

    (function() {
        function bar() { }
        ...
    })();
}

That, of cause, does not add bar to the scope of foo while the let version would. Moreover, the current behavior leads to unintuitive:

~/m/trunk/mozilla> js
js> function test()
{
    var x = 1;
    {
        let x = 2;
        function x() {
            print(uneval(x));
        }
    }
    x();
}

js> test();
2

I.e. not only function x overwrites the top level x, not x in the let block, but also referring to x inside the function x gives the value of the x introduced by the let block, not the function itself. 

One may argue that one can get the same result with classic with statement like in:

~/m/trunk/mozilla> js
js> function test2()
{
    var x = 1;
    with ({x: 2}) {
        function x() {
            print(uneval(x));
        }
    }
    x();
}

js> 
js> test2();
2

But why should the let block follows the historic behavior of the with statement?

Comment 10

[Igor Bukanov]

Attachment: v1

The new patch extends special runtime support for JSOP_CLOSURE executed from eval. It passes the test suite without the regressions.

Comment 11

[Brendan Eich [:brendan]]

The "function statements" (function definitions not at top-level within another function or a program) are an extension, and not implemented the same in all the major browsers. IE (and Opera following IE) always bind their names. I'm not sure what Safari (JavaScriptCore) does. We've done control-flow-based binding forever.

Since let does not change the "variable object" in which function f(){...} binds 'f', I don't agree that we should consider the interaction of this JSOP_CLOSURE extension and the more recent, prefiguring-ES4, let declarations a bug. ES4 has let function f() ... as well, for block-scoped function definitions.

If we could get away with it, I would remove function statements, but I suspect SpiderMonkey users depend on them so much that we are stuck supporting them -- including keeping the variable object as their binding object.

/be

Comment 12

[Igor Bukanov]

(In reply to comment #11)
> Since let does not change the "variable object" in which function f(){...}
> If we could get away with it, I would remove function statements, but I suspect
> SpiderMonkey users depend on them so much that we are stuck supporting them --
> including keeping the variable object as their binding object.

what about throwing a syntax error when a compilation unit contains both let and function expression statements? It would also solve the issue from comment 9.

Comment 13

[Igor Bukanov]

(In reply to comment #12)
> what about throwing a syntax error when a compilation unit contains both let
> and function expression statements? It would also solve the issue from comment
> 9.

This also suggests to treat any let in the whole compilation unit as marker for a new JS which allows only explicit eval (or even turns eval into a keyword) with ES4 semantics including introduction of a new variable object. It would allow to write more secure code in FF: one would need just to replace all var into let. But this is for another bug.  

Comment 14

[Igor Bukanov]

(In reply to comment #13)
> This also suggests to treat any let in the whole compilation unit as marker for
> a new JS which allows only explicit eval (or even turns eval into a keyword)
> with ES4 semantics including introduction of a new variable object. It would
> allow to write more secure code in FF: one would need just to replace all var
> into let. But this is for another bug.  

Or another suggestions: what about disallowing let and var in one compilation unit in ES4 and warning about the mix in FF3?

Comment 15

[Brendan Eich [:brendan]]

We require opt-in version selection to make let a keyword in context already. We could change eval rules per the ES4 proposals, but the open tickets at http://bugs.ecmascript.org/ need to be hashed out and closed. Until then, could we deprioritize this bug and work on other (perf ;-) ones? Not saying this bug took a lot of time, just trying to keep it from doing so yet.

/be

Comment 16

[Brendan Eich [:brendan]]

I think mixing let and var should be allowed, both because of general "migration should not be taxed" principles and specific "destructuring parameters bind vars (not lets)". The latter is because of arguments aliasing:

function f([a,b]) { arguments[0] = 42; return a }
f([1,2]) => 42

/be

Comment 17

[Igor Bukanov]

(In reply to comment #15)
> Not saying this bug
> took a lot of time, just trying to keep it from doing so yet.

This bug come up from trying to create a patch for bug 406356. To avoid creating object wrappers for JSFunction* there it is necessary to make sure that the wrappers are not used for storing the anything. 

Currently this is still not the case for JSOP_DEFFUN which uses the wrapper to store the block chain via the parent slot. AFAICS it is straightforward to fix this if JSOP_DEFFUN would only be used for compiled functions and not for eval scripts. This is how I discovered this bug and fixing it via the patch from comment 10 makes things easier for solving the bug 406356.

Comment 18

[Damon Sicore (:damons)]

+'ing with P4.

Comment 19

[Brendan Eich [:brendan]]

Comment on attachment 291446 [details] [diff] [review]
v1

Looks good, but I was wondering whether JSOP_DEFFUN could be leaner still (always define, not set) if you always selected JSOP_CLOSURE for top-level function compiled by eval? I think you mentioned that idea somewhere recently.

/be

Comment 20

[Brendan Eich [:brendan]]

Comment on attachment 291446 [details] [diff] [review]
v1


>+                    ok = OBJ_DEFINE_PROPERTY(cx, parent, id, rval,
>+                                             (flags & JSPROP_GETTER)
>+                                             ? JS_EXTENSION (JSPropertyOp) obj
>+                                             : NULL,
>+                                             (flags & JSPROP_SETTER)
>+                                             ? JS_EXTENSION (JSPropertyOp) obj
>+                                             : NULL,
>+                                             attrs,
>+                                             &prop);
>+                    if (ok)
>+                        OBJ_DROP_PROPERTY(cx, parent, prop);

Forgot to say: get rid of prop, pass NULL instead of &prop. It was left over from the JSOP_GETGVAR etc. work and perhaps could be used to speed up global function accesses, but it's unused (except to drop it) now.

/be

Comment 21

[Igor Bukanov]

Attachment: v2

The new version avoids JSOP_DEFFUN for eval saving an if check when interpreting JSOP_DEFUN.

Comment 22

[Igor Bukanov]

(In reply to comment #20)
> Forgot to say: get rid of prop, pass NULL instead of &prop. It was left over
> from the JSOP_GETGVAR etc. work and perhaps could be used to speed up global
> function accesses, but it's unused (except to drop it) now.

I think a better idea would be to get rid of JSOP_CLOSURE at all. Surely it adds a couple of an extra if checks compared with the patch from the comment 21, but that can be more than offset with a fix for bug 407327.

Comment 23

[Brendan Eich [:brendan]]

Comment on attachment 292046 [details] [diff] [review]
v2

Looks good. Agreed on avoiding relookups via prop result from js_CheckRedeclaration. Followup bug for that on file?

/be

Comment 24

[Igor Bukanov]

(In reply to comment #23)
> (From update of attachment 292046 [details] [diff] [review])
> Agreed on avoiding relookups via prop result from
> js_CheckRedeclaration. Followup bug for that on file?

Bug 407327

Comment 25

[Igor Bukanov]

I checked in the patch from comment 21 to the trunk of CVS:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1197369488&maxdate=1197369825&who=igor%mir2.org

Comment 26

[Bob Clary [:bc] (inactive)]

Checking in js1_7/regress/regress-406477.js;
/cvsroot/mozilla/js/tests/js1_7/regress/regress-406477.js,v  <--  regress-406477.js
initial revision: 1.1
done

Comment 27

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0