Closed
Bug 382509
Opened 18 years ago
Closed 18 years ago
Disallow indirect eval
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
People
(Reporter: Waldo, Assigned: mrbkap)
References
Details
(Keywords: verified1.8.1.13, Whiteboard: [needs branch patch])
Attachments
(2 files, 2 obsolete files)
(deleted),
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
brendan
:
review+
dveditz
:
approval1.8.1.13+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•18 years ago
|
||
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(...).
Assignee | ||
Comment 2•18 years ago
|
||
This already has brendan's review.
Updated•18 years ago
|
Summary: Indirect eval is bad → Disallow indirect eval
Assignee | ||
Comment 3•18 years ago
|
||
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).
Attachment #266670 -
Attachment is obsolete: true
Attachment #266681 -
Flags: review?(brendan)
Comment 4•18 years ago
|
||
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•18 years ago
|
||
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
Attachment #266681 -
Flags: review?(brendan) → review+
Assignee | ||
Comment 6•18 years ago
|
||
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•18 years ago
|
||
(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
Assignee | ||
Comment 8•18 years ago
|
||
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 9•18 years ago
|
||
I had to back this out -- XPCSafeJSObjectWrapper depends on this.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 10•18 years ago
|
||
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.
Attachment #266681 -
Attachment is obsolete: true
Attachment #267210 -
Flags: review?(brendan)
Comment 11•18 years ago
|
||
Comment on attachment 267210 [details] [diff] [review]
Fixed patch
Yay for thread-unsafe js shell builds.
/be
Attachment #267210 -
Flags: review?(brendan) → review+
Assignee | ||
Comment 12•18 years ago
|
||
Let's try that "fixed on trunk" thing again!
Status: REOPENED → RESOLVED
Closed: 18 years ago → 18 years ago
Resolution: --- → FIXED
Comment 13•18 years ago
|
||
this seems to be caused layout regression like Bug 383381
Comment 14•18 years ago
|
||
<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)
Flags: in-testsuite+
Comment 15•18 years ago
|
||
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•18 years ago
|
||
What was the net change here, after bug 383381 was fixed?
Comment 17•18 years ago
|
||
(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•18 years ago
|
||
(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•18 years ago
|
||
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•18 years ago
|
||
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•18 years ago
|
||
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•18 years ago
|
||
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
Assignee | ||
Updated•17 years ago
|
Flags: blocking1.8.1.12?
Comment 24•17 years ago
|
||
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.
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.13?
Flags: blocking1.8.1.12?
Updated•17 years ago
|
Whiteboard: [needs branch patch]
Assignee | ||
Comment 25•17 years ago
|
||
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.
Attachment #307136 -
Flags: review?(brendan)
Attachment #307136 -
Flags: approval1.8.1.13?
Comment 26•17 years ago
|
||
Comment on attachment 307136 [details] [diff] [review]
1.8 branch patch
An API change for sure, but it's necessary.
/be
Attachment #307136 -
Flags: review?(brendan) → review+
Updated•17 years ago
|
Flags: blocking1.8.1.13? → blocking1.8.1.13+
Comment 27•17 years ago
|
||
Comment on attachment 307136 [details] [diff] [review]
1.8 branch patch
approved for 1.8.1.13, a=dveditz for release-drivers
Attachment #307136 -
Flags: approval1.8.1.13? → approval1.8.1.13+
Updated•17 years ago
|
Flags: blocking1.8.0.15+
Comment 31•17 years ago
|
||
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.
(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•17 years ago
|
||
(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
You need to log in
before you can comment on or make changes to this bug.
Description
•