Closed Bug 446026 Opened 12 years ago Closed 11 years ago

restore utility of eval(s, o)

Categories

(Core :: JavaScript Engine, defect, P2, minor)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.1

People

(Reporter: crowderbt, Assigned: crowderbt)

References

Details

(Keywords: testcase, verified1.9.1)

Attachments

(1 file, 4 obsolete files)

Bug 442333 remove eval(o, s) entirely, but parts of it are useful.  With appropriate restrictions we should find a way to restore this feature (perhaps under another function name, other than eval).

I've proposed 1.9.1 as the target milestone for this work.
It sounds like what's hoped-for here is something like evalInSandbox() for content.  I'm adding Jesse since he may have some thoughts on this.
Sorry, I'm still not sure from those examples precisely what behavior is needed here.
The behavior that used to be there, on any Firefox 3.0.x release.

For 3.1 the idea in the latter part of bug 442333 comment 19 is still worth trying. That was what I thought this bug was for, with nothing touching a release until this bug was fixed. That's why I wrote

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

in bug 442333 comment 27.

/be
Does that wrapping change not still incompatibly break extensions with wonky code like the code in bug 457068?
This is probably wrong, but I want to see if it's the right -idea-.
Attachment #340588 - Flags: review?(brendan)
Attachment #340588 - Flags: review?(mrbkap)
Attachment #340588 - Flags: review?(brendan)
Attachment #340588 - Flags: review+
Comment on attachment 340588 [details] [diff] [review]
What I think Brendan is proposing

This looks right, mrbkap should review too. Testing is the thing.

/be
Comment on attachment 340588 [details] [diff] [review]
What I think Brendan is proposing

I think this looks good.
Attachment #340588 - Flags: review?(mrbkap) → review+
In the eval(s, o) case, the proposed code creates a With object using o as
parent, not proto.  But it regresses the private variables problem.

Also, with the patch, eval(s, o) clobbers caller's scope chain with o.

eval("", {alert:1});
alert(1);

With the patch, this throws a type error "alert is not a function".
Comment on attachment 340588 [details] [diff] [review]
What I think Brendan is proposing

New patch coming?

/be
Attachment #340588 - Flags: review+ → review-
Comment on attachment 340588 [details] [diff] [review]
What I think Brendan is proposing

Yeah, back to the drawing board.
Attachment #340588 - Flags: review+ → review-
Attached patch v2 (obsolete) — Splinter Review
Ok, with this patch the original problem is pretty straightforward.  I still end up making the With object's parent be the Function object in the "private variables" case.  The proto is the global object.  This basically doesn't solve the problem.

My next attempt was to make the parent object be null in the argc >= 2 case, but this totally breaks the o(s, environmentObject) because again, the proto is the global object, and there is no parent.

If I make the With object's proto be the original scopeobj, we're back in the same boat on the private variables case; you'll still be able to invade functions.  The scope-chain clobbering was a bug that I have since fixed.  New patch coming that does this (still leaves Peter Michaux's private variables case broken).
Attachment #340588 - Attachment is obsolete: true
(In reply to comment #12)
> Ok, with this patch the original problem is pretty straightforward.

Rather, with the original patch....
>+        if (indirectCall || argc >= 2) {
>+            callerScopeChain = scopeobj ? scopeobj : js_GetScopeChain(cx,
caller);

1347 out:
1348 #if JS_HAS_EVAL_THIS_SCOPE
1349     /* Restore OBJ_GET_PARENT(scopeobj) not callerScopeChain in case of
Call. */
1350     if (setCallerScopeChain) {
1351         caller->scopeChain = callerScopeChain;

The scope chain is still clobbered after calling eval(s, o).

>+                if (scopeobj)
>+                    scopeobj = js_NewWithObject(cx, scopeobj, NULL, -1);

js_NewWithObject(cx, scopeobj, NULL, -1) creates a With object whose parent is
the scopeobj's parent, thus the private variables problem is not solved.  What
happens if we create a With object using o's global object as parent?
I have a patch in my queue that fixes both of the items from comment #14.  Will post shortly.
Attached patch v3 (obsolete) — Splinter Review
The following testcase:

====================
var b = 45;

// Getting "private" variables
var obj = (function() {
  var a = 21;
  return {
    // public function must reference 'a'
    fn: function() {a;}
  };
})();

var foo;

try {
  eval('bar = b; foo=a', obj.fn);
} catch (e) {
  print("caught: " + e);
}
print(foo + " | " + bar); // 21

eval("", {print:1})
print(1);
====================

Prints:
caught: ReferenceError: a is not defined
undefined | 45
1

in the shell.  I believe this is correct for the cases tested.
Attachment #340937 - Attachment is obsolete: true
Summary: restore utility of eval(o, s) → restore utility of eval(s, o)
With v3 patch, the scope chain is clobbered with o's global object.

testcase:
var x = "global";
(function() {
  var x = "local";
  (function() {
    print(x);
    eval("", {});
    print(x);
  })();
})();

Prints:
local
global
Do we really need to touch caller->scopeChain?  In the old code, we did not
touch caller->scopeChain at all in the eval(s, o) case, right?  So, I'm
wondering if what we need to do would be only wrapping o in a With object.

Something like this?:

    if (!scopeobj) {
        ...
    }
    else {
        ok = js_CheckPrincipalsAccess(cx, scopeobj,
                                      caller->script->principals,
                                      cx->runtime->atomState.evalAtom);
        if (!ok)
            goto out;

        scopeobj = js_NewWithObject(cx, scopeobj,
                                    JS_GetGlobalForObject(cx, scopeobj), -1);
        if (!scopeobj) {
            ok = JS_FALSE;
            goto out;
        }
    }
Does this bug need to block Firefox 3.1?
This is an API break. Old API to boot. We should try to fix, which is what I was insisting on in the other bug.

/be
Flags: blocking1.9.1+
Attached patch v4 (obsolete) — Splinter Review
Attachment #342465 - Attachment is obsolete: true
Comment on attachment 349247 [details] [diff] [review]
v4

Basically the organization you suggested.  Please check it out, thanks.
Attachment #349247 - Flags: review?(moz_bug_r_a4)
So the patch in v4 seems to address the original Peter Michaux issue (privacy violation through closures).  I'm not sure, though, whether or not it will save Venkman or other extensions; if they're using the Michaux-style privacy violation intentionally, it surely won't.
v4 patch looks good to me.  (I cannot edit the review flag.)

By wrapping o in a With object, we skip __parent__ regardless of whether or not
it is a Call object.  So there is a difference from the old behavior:

  //document.body.__parent__ is document
  alert(eval("body", document.body));

  Fx3.0.1:
  [object HTMLBodyElement]

  Trunk with v4 patch:
  ReferenceError: body is not defined

Is this a problem?
(In reply to comment #24)
> v4 patch looks good to me.  (I cannot edit the review flag.)

I've granted your account the necessary privileges, you should be able to edit it now.
Attachment #349247 - Flags: review?(moz_bug_r_a4) → review+
Comment on attachment 349247 [details] [diff] [review]
v4

mrbkap:  Any thoughts on comment #24?  I think that's the -intent- of using the With object here, but I also think it constitutes an API-breakage.  Perhaps less severe?
Attachment #349247 - Flags: review?(mrbkap)
Blocks: 446030
Does Venkman rely on obj.__parent__?
Comment on attachment 349247 [details] [diff] [review]
v4

>--- mozilla.5b81a5dc7485/js/src/jsobj.cpp	2008-11-20 11:21:51.000000000 -0800
>+        ok = js_CheckPrincipalsAccess(cx, scopeobj,
>+                                      caller->script->principals,
>+                                      cx->runtime->atomState.evalAtom);

I think this should move after the computation of |principals| and use that value, since this will incorrectly report errors for cloned function objects whose original functions were compiled as chrome (or without principals, as we sometimes do).
Attachment #349247 - Flags: review?(mrbkap)
ping? is this ready?
Another swing; this should agree with our IRC chat.
Attachment #349247 - Attachment is obsolete: true
Attachment #352640 - Flags: review?(mrbkap)
Comment on attachment 352640 [details] [diff] [review]
per discussion from IRC

Thanks.
Attachment #352640 - Flags: review?(mrbkap) → review+
Keywords: checkin-needed
Whiteboard: fixed-in-tracemonkey
So, this is fixed-in-tracemonkey but checkin-needed... Should it still be landed on the trunk by itself, or should it wait until another tracemonkey landing is done?
Depends on: 469696
No longer depends on: 469696
No one ever answered the question from comment #27....

reed:  I don't know when the next merge from tracemonkey will be.  If it's in the next day or so, then I think waiting for that is fine.
Priority: -- → P2
merged in mc
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Blocks: 457068
(In reply to comment #34)
> merged in mc

That was
http://hg.mozilla.org/mozilla-central/rev/e905fe64c1b2

(In reply to comment #33)
> No one ever answered the question from comment #27....
> 
> reed:  I don't know when the next merge from tracemonkey will be.  If it's in
> the next day or so, then I think waiting for that is fine.

Same question(s) for 1.9.1 checkin: wait for global or do individual ?
Whiteboard: fixed-in-tracemonkey → [c-n: baking for 1.9.1, wait for answer to comment 34]
Version: unspecified → Trunk
Whiteboard: [c-n: baking for 1.9.1, wait for answer to comment 34] → [c-n: baking for 1.9.1, wait for answer to comment 35]
Keywords: testcase
Checking in js1_5/Scope/regress-446026-01.js
Checking in js1_5/Scope/regress-446026-02.js
http://hg.mozilla.org/mozilla-central/rev/55ae5a45a1d1
Flags: in-testsuite+
Flags: in-litmus-
v 1.9.1, 1.9.2
Status: RESOLVED → VERIFIED
Whiteboard: [c-n: baking for 1.9.1, wait for answer to comment 35]
Since this was broken in a 1.9.0.x point release should we also land the fix on 1.9.0.x?
Flags: wanted1.9.0.x?
Flags: blocking1.9.0.10?
We won't block on this for now.

Brendan: Is this something we should land on the 1.9.0 branch (keeping in mind bug 446030 and ensuring it stays fixed)?
Flags: blocking1.9.0.10?
How did we survive not fixing this on the 1.9.0 branch? Are the add-ons authors who complained using only the bleeding edge, and never looking back? Do they have no users on the last stable release?

/be
No idea, no one ever responded to my queries in bug 457068
You need to log in before you can comment on or make changes to this bug.