Closed Bug 592664 Opened 14 years ago Closed 14 years ago

Indirect eval should be allowed under ES5

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: emk, Assigned: jorendorff)

References

Details

(Whiteboard: [fixed-in-tracemonkey])

Attachments

(3 files, 4 obsolete files)

Although ES3 allowed prohibiting indirect eval, ES5 doesn't anymore (even in strict mode).
[eval][0]("this") should not throw EvalError.
Busted! I was talking about this in other fora recently.

This should block. Thanks for reporting,

/be
blocking2.0: --- → ?
blocking2.0: ? → final+
Likewise (from bug 600193 comment 3), this should be allowed:

   var x = 1;
   var obj = {x: 2};
   obj.eval = eval;
   assertEq(obj.eval("x"), 1);

Currently the call to eval throws an EvalError.
Attached patch v1 (obsolete) — Splinter Review
What the heck, I'm in this code anyway.

There's a lot of not-particularly-relevant tidying up in the patch. I think it's still possible to see what's going on, but if not, I can split that out.
Assignee: general → jorendorff
Attachment #480439 - Flags: review?(brendan)
v1 was produced by hg qdiff. It has the tests in it, and it shows exactly what changed in the eval cache code (nothing significant I hope), so it's worth looking at.

Here is what GNU diff has to say about the exact same set of changes to jsobj.cpp. It shows what changed about the actual eval part of obj_eval, which is more interesting.
Comment on attachment 480439 [details] [diff] [review]
v1

Thanks for the better alterna-diff.
>+    /*
>+     * CSP check: Is eval() allowed at all?
>+     * Report errors via CSP is done in the script security mgr.
>+     */
>+    if (!js_CheckContentSecurityPolicy(cx)) {
>+        JS_ReportError(cx, "call to eval() blocked by CSP");
>+        return false;
>+    }

The comment (you touched it ;-) seems wrong -- the JS_ReportError call (no js.msg l10n support) is here, not in some callback impl'ed in the script secmgr. Fix or followup (or a bit of both -- fix the comment now and cite the followup on js.msg usage).

Great tests.

/be
Attachment #480439 - Flags: review?(brendan) → review+
Attached patch v2 (obsolete) — Splinter Review
v1 wouldn't even compile; I must have built and run tests in the wrong tree.

The fix is pretty straightforward though.

I noticed that we used to cache even eval scripts that could never be used, such as indirect eval scripts. With this patch we don't do that anymore.

I also fixed the CSP error message and comment.
Attachment #480439 - Attachment is obsolete: true
Attachment #480440 - Attachment is obsolete: true
Attachment #481241 - Flags: review?(brendan)
Attached patch v2 (obsolete) — Splinter Review
How about we try that one with Unix newlines.
Attachment #481241 - Attachment is obsolete: true
Attachment #481243 - Flags: review?(brendan)
Attachment #481241 - Flags: review?(brendan)
Sorry, undercaffeinated this morning. The CSP error message patch is in bug 602212 after all.
Comment on attachment 481243 [details] [diff] [review]
v2

Does JS_ALWAYS_INLINE matter for EvalCacheLookup? r=me in any event, just thought this might be better for perf and codesize (both).

/be
Attachment #481243 - Flags: review?(brendan) → review+
Sadly, it *does* matter, in MSVC anyway -- even though this is a static function only called from one place. I wonder why MSVC doesn't inline that unless we insist.

Anyway, I made it JS_ALWAYS_INLINE for checkin.

http://hg.mozilla.org/tracemonkey/rev/89006937466d
Whiteboard: [fixed-in-tracemonkey]
Backed out. Crashing tests.
Whiteboard: [fixed-in-tracemonkey]
I deleted a little too eagerly. This version of the patch restores the code that bails out early if there's no calling script.
Attachment #481243 - Attachment is obsolete: true
Attachment #481363 - Flags: review?(brendan)
Attachment #481364 - Flags: review?(brendan)
The diff between v2 and (the two v3 patches combined).
Now, v2 was crashing because I had deleted the "if (!caller)" check quoted below, and then soon after that we do caller->script(). My first inclination was to go through and fix up all the places where obj_eval uses caller.

The comment I'm adding here is meant to explain why I didn't do that:

>     if (!caller) {
>+        /* Eval code needs to inherit principals from the caller. */
>         JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
>                              JSMSG_BAD_INDIRECT_CALL, js_eval_str);
>         return JS_FALSE;
>     }

To be a little more explicit, one of the places obj_eval uses caller is here:

>    JSPrincipals *principals = js_EvalFramePrincipals(cx, callee, caller);
js_EvalFramePrincipals does have some code to cope with NULL caller, but I'm not sure it does exactly what we want. So to be on the safe side I'm going to leave that fix for another day.
Attachment #481363 - Flags: review?(brendan) → review+
Attachment #481364 - Flags: review?(brendan) → review+
Blocks: 514568
Landed part 1:     http://hg.mozilla.org/tracemonkey/rev/8402d56a7777
and part 2:        http://hg.mozilla.org/tracemonkey/rev/7598b7ab2e76
and backed it out: http://hg.mozilla.org/tracemonkey/rev/a4383c16a9b5
and relanded it:   http://hg.mozilla.org/tracemonkey/rev/2e20ecdd1194

To top it off I put the wrong bug number in all four commits. Awesome. :-(
Whiteboard: [fixed-in-tracemonkey]
http://hg.mozilla.org/mozilla-central/rev/ab87d9da6e2f
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.