Last Comment Bug 602994 - Calling the eval function from native code, without any other JS active, should work
: Calling the eval function from native code, without any other JS active, shou...
Status: RESOLVED FIXED
fixed-in-tracemonkey
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Luke Wagner [:luke]
:
Mentors:
Depends on: 514568 604504 625559 626301 647337 650617 651298 653631
Blocks: es5 625199
  Show dependency treegraph
 
Reported: 2010-10-08 14:48 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2011-04-28 22:17 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (24.62 KB, patch)
2011-01-13 19:02 PST, Luke Wagner [:luke]
no flags Details | Diff | Review
WIP 2 (27.95 KB, patch)
2011-01-14 16:40 PST, Luke Wagner [:luke]
no flags Details | Diff | Review
WIP 3 (29.75 KB, patch)
2011-01-18 18:33 PST, Luke Wagner [:luke]
no flags Details | Diff | Review
WIP 4 (79.65 KB, patch)
2011-04-01 18:46 PDT, Luke Wagner [:luke]
no flags Details | Diff | Review
preparatory syntactic touchups (41.84 KB, patch)
2011-04-08 09:45 PDT, Luke Wagner [:luke]
jwalden+bmo: review+
Details | Diff | Review
clean up eval (44.70 KB, patch)
2011-04-08 10:08 PDT, Luke Wagner [:luke]
jwalden+bmo: review+
mrbkap: review+
Details | Diff | Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2010-10-08 14:48:47 PDT
ES5 says this is an indirect eval, but we currently throw.  Bug 592664 made a stab at this but ended up a little short, and this aspect got punted -- need to fix it right.

This will fit better either after, or amongst, cleanup patches to be posted to bug 514568, so marking this as depending on that.
Comment 1 Luke Wagner [:luke] 2011-01-13 19:02:32 PST
Created attachment 503717 [details] [diff] [review]
WIP

This patch does a few things and probably need to be split up.  Talking to Blake, it seems that with the new guarantees of wrappers (and bug 625559) a lot of the security checks in eval can be simplified.  With that, its not a big deal if there isn't a JS caller.
Comment 2 Luke Wagner [:luke] 2011-01-14 16:40:59 PST
Created attachment 504034 [details] [diff] [review]
WIP 2

Fix bug.  Weaken assert that an object's principals found via compartment and findObjectPrincipals are equal to just that they subsume each other.  Another go at try server.
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2011-01-18 10:24:15 PST
This is generally quite happymaking.

The "N.B. Infallible." comments would be unnecessary if you used house style of having a trailing cx parameter for those methods.  (This is more request than suggestion.)

How difficult would it be to remove the minor bit of bytecode inspection here into the interpreter and stubs::Eval (or DirectEval) and out of the workhorse, where it is not immediately and trivially obvious that bytecode inspection will never seize upon something it shouldn't (like, if eval were being called from a native callback like String.prototype.replace or somesuch)?  I tried to do that when I refactored this stuff and foundered somewhat upon the bits being fixed here.

But is that inspection even correct anyway?  I see what looks like bytecode inspection on behalf of Function.  That often means lies.  Or at least an assertion failure, looks like?  I leave details to you.

  var eval = Function;
  eval("return 'FAIL';"); // ???
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2011-01-18 10:27:05 PST
Or maybe not an assertion, on second look.  But it still concerns me, even if it doesn't assert.
Comment 5 Luke Wagner [:luke] 2011-01-18 18:33:00 PST
Created attachment 504947 [details] [diff] [review]
WIP 3

Waldo: thanks.  Like this?  (Try servering)
Comment 6 Luke Wagner [:luke] 2011-01-19 08:43:35 PST
Green on try (with TODO-remove exception for bug 626301.  Waiting for dependent bugs to resolve.
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2011-01-19 11:27:16 PST
(In reply to comment #5)
> Like this?

Not quite, it's still bytecode-inspecting from deep in a function invocation, rather than (properly) in the JSOP_EVAL opcode implementation itself.  For the direct eval case we should be able to read the line number directly from JSOP_LINENO in the interpreter without having to be careful that we might or might not be parked on JSOP_EVAL, and the filename directly from the script (with filtering out for protected filenames and all, sigh).  For the indirect case it's always going to be 0 and NULL, so we can just pass those to EvalKernel directly.  For Function...sigh.  That's always treated as indirect here, so it'd have the if-caller-then-0/NULL-else-filtered-filename-and-framepctoline dance.

In any case, the way all these filename/linenumber bits are funneled through one place obscures more than it helps, at least for me.  I like it for the principals bit, though.


A couple other things I noted on this read-through:

>diff --git a/js/src/jsfun.cpp b/js/src/jsfun.cpp

> static JSBool
> Function(JSContext *cx, uintN argc, Value *vp)
> {

>-    /* N.B. overwriting callee with return value */
>-    JSObject *parent = vp[0].toObject().getParent();
>-    vp[0].setObject(*obj);
>-
...
>     if (caller) {
>-        JSObject *callee = &JS_CALLEE(cx, vp).toObject();

Lies!  Will definitely be good to fix this, even if it doesn't appear to me to cause problems.


>-    /* Belt-and-braces: check that the caller has access to parent. */
>-    if (!js_CheckPrincipalsAccess(cx, parent, principals,
>-                                  CLASS_ATOM(cx, Function))) {
>-        return JS_FALSE;
>-    }

I was going to suggest asserting truthiness of this, but I guess we already did this in one of the methods called a few lines earlier.


>diff --git a/js/src/jsinterp.h b/js/src/jsinterp.h

>+    inline JSPrincipals *principals(JSContext *cx) const;

Needs a /* NB: Infallible. */ comment.  Also, on the rather nitpicky side, note that, as far as I can remember, we always use "NB:".


>diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp

>-    /* FIXME Bug 602994: This really should be perfectly cromulent. */
>-    if (!caller) {
>-        /* Eval code needs to inherit principals from the caller. */
>-        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
>-                             JSMSG_BAD_INDIRECT_CALL, js_eval_str);

JSMSG_BAD_INDIRECT_CALL is unused, isn't it?


>+     * In the case of an indirect call without a caller frame, avoid a
>+     * potential warning-flood.

Interesting wrinkle.  I hadn't thought of that, but this does seem like the best approach.


>+    JSObject &callee = JS_CALLEE(cx, vp).toObject();
>+    JSPrincipals *calleePrincipals = callee.compartment()->principals;

Since callee is just for line-wrapping purposes, it seems slightly better to me to not bother with it and just wrap and indent the RHS on the next line.  Fewer names to keep in mind is better when the abbreviated expression's not too big.  Here the introduced name just disrupts my train of thought, without a future-use benefit to offset.


>+    SwitchToCompartment(cx, caller->compartment());

Something's wrong here!  Do you see it?

Assuming this sticks around somewhere, as it seems it should to me, please fix this problem globally for this class.  rs=me if you probably want to make it a separate patch.


> static JSBool
> obj_watch_handler(JSContext *cx, JSObject *obj, jsid id, jsval old,
>                   jsval *nvp, void *closure)

>-    if (callbacks && callbacks->findObjectPrincipals) {
>-        /* Skip over any obj_watch_* frames between us and the real subject. */
>-        caller = js_GetScriptedCaller(cx, NULL);
>-        if (caller) {
>-            /*
>-             * Only call the watch handler if the watcher is allowed to watch
>-             * the currently executing script.
>-             */
>-            watcher = callbacks->findObjectPrincipals(cx, callable);
>-            subject = js_StackFramePrincipals(cx, caller);
>-
>-            if (watcher && subject && !watcher->subsume(watcher, subject)) {
>-                /* Silently don't call the watch handler. */
>-                return JS_TRUE;
>+    JSObject *callable = (JSObject *) closure;
>+    if (JSPrincipals *watcher = callable->principals(cx)) {
>+        if (JSStackFrame *caller = js_GetScriptedCaller(cx, NULL)) {
>+            if (JSPrincipals *subject = caller->principals(cx)) {
>+                if (!watcher->subsume(watcher, subject)) {
>+                    /* Silently don't call the watch handler. */
>+                    return JS_TRUE;
>+                }
>             }
>         }
>     }

So we might call the watch if we have script running to trigger this watchpoint, or we might not.  But if we don't, we always call it.  This seems broken: it's easy to trigger a watchpoint without script running.  Are we assuming privileged scripts never do this?  Or is it some latent requirement that might come back to bite us?  Or am I missing something entirely?
Comment 8 Luke Wagner [:luke] 2011-01-19 11:59:54 PST
(In reply to comment #7)
> >+    SwitchToCompartment(cx, caller->compartment());
> 
> Something's wrong here!  Do you see it?

Nice; maybe those GUARD thingies are ok after all ;-)

> So we might call the watch if we have script running to trigger this
> watchpoint, or we might not.  But if we don't, we always call it.  This seems
> broken: it's easy to trigger a watchpoint without script running.  Are we
> assuming privileged scripts never do this?  Or is it some latent requirement
> that might come back to bite us?  Or am I missing something entirely?

Hmm, I think here I was just factoring to use the new interface, preserving behavior.  In attachment 504948 [details] [diff] [review], if you search for obj_watch_handler, you can see I just removed the whole blip for the reason in the comment.  (Wrappers prevent the evil situations from even getting here, as Blake explained to me.)
Comment 9 Luke Wagner [:luke] 2011-01-19 12:00:54 PST
(So, since bug 625199 won't be landing soon, we can just do that here.)
Comment 10 Jeff Walden [:Waldo] (remove +bmo to email) 2011-01-19 12:27:45 PST
(In reply to comment #8)
> Hmm, I think here I was just factoring to use the new interface, preserving
> behavior.

Yes, true -- meant to say this was an orthogonal consideration.
Comment 11 Luke Wagner [:luke] 2011-04-01 18:46:36 PDT
Created attachment 523742 [details] [diff] [review]
WIP 4

This patch makes a lot more simplifications to the eval logic.  Several paths and variables removed; stuff moved to an RAII guard.

I also turned a bunch of vp/argv manipulation into using js::CallArgs.  Hopefully we can gradually do this more so that if/when we change the JSNative signature to use js::CallArgs (bug 580337) there isn't that much to convert.

Still some TODOs left.
Comment 12 Luke Wagner [:luke] 2011-04-08 09:45:03 PDT
Created attachment 524641 [details] [diff] [review]
preparatory syntactic touchups

While working on the next patch, I wanted to make a few syntactic touchups, so I split that off into this one.
Comment 13 Luke Wagner [:luke] 2011-04-08 10:08:20 PDT
Created attachment 524647 [details] [diff] [review]
clean up eval

Again, as a recap: this patch removes a lot of the dependencies on the caller's frame (except for optional warning/filename logic).  This came up while removing dummy frames.  It also removes several, from what Blake tells me, unnecessary security checks.  Lastly, since EvalKernel was lookin' so purty, I made a stack guard to simplify the logic further.

Green on try.  SS/V8 in shell unaffected.  A micro-benchmark I made that always hits in the eval cache got about 3% faster.  Still need to build browsers to see if it matters there.

Asking mrbkap review for the security-check stuff and Waldo for the rest since he did the last eval refactoring.
Comment 14 Jeff Walden [:Waldo] (remove +bmo to email) 2011-04-11 18:24:26 PDT
Comment on attachment 524641 [details] [diff] [review]
preparatory syntactic touchups

I see one case of |&obj1 == &obj2|; is there a reason not to have bool operator==(const JSObject &other) const { return this == &other; }|?
Comment 15 Jeff Walden [:Waldo] (remove +bmo to email) 2011-04-12 01:13:52 PDT
Comment on attachment 524647 [details] [diff] [review]
clean up eval

>diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp
>+    void setNewScript(JSScript *script) {
>+        /* NewScriptFromCG has already called js_CallNewScriptHook. */
>+        JS_ASSERT(!script_ && script);

n assertions for an n-ary && condition, because otherwise if you hit this one you don't immediately know which condition failed.


>diff --git a/js/src/jsscript.cpp b/js/src/jsscript.cpp
>+const char *
>+js::CurrentScriptFileAndLineSlow(JSContext *cx, uintN *lineop)

I think you meant |linenop| -- or does "lineop" actually mean something?


>diff --git a/js/src/jsscript.h b/js/src/jsscript.h
>+
>+/*
>+ * This function returns the file and line number of the script currently
>+ * executing on cx. If there is no current script executing on cx (e.g., a
>+ * native called directly through JSAPI (e.g., by setTimeout)), NULL and 0 are
>+ * returned as the file and line. Additionally, this function avoids the full
>+ * linear scan for direct eval, i.e., eval called via JSOP_EVAL.
>+ */
>+inline const char *
>+CurrentScriptFileAndLine(JSContext *cx, uintN *lineop, bool isDirectEval = false);

bool is easier to pass, but it's not obvious what it means to the reader -- emphatically so for this signature.  I have a vague recollection of a previous iteration of this having a method for direct eval to use and a method (methods?) for everything else to use.  I would prefer something like that, although I'm not sure what it'd look like, and how you'd enforce that the user couldn't use the wrong method.


>diff --git a/js/src/jsscriptinlines.h b/js/src/jsscriptinlines.h
>+extern const char *
>+CurrentScriptFileAndLineSlow(JSContext *cx, uintN *lineop);
>+
>+inline const char *
>+CurrentScriptFileAndLine(JSContext *cx, uintN *lineop, bool isDirectEval)

Again lineop.
Comment 16 Luke Wagner [:luke] 2011-04-12 10:38:08 PDT
(In reply to comment #14)
> I see one case of |&obj1 == &obj2|; is there a reason not to have bool
> operator==(const JSObject &other) const { return this == &other; }|?

I've thought that before, and done that in different projects where used a lot of pass-by-reference.  I held off because I was worried it would be a little to cute/creepy for people's tastes.  OTOH, comparing JSObject's any other way doesn't make sense.  I can try it and see if anyone yelps :)
Comment 17 Luke Wagner [:luke] 2011-04-12 11:31:00 PDT
(In reply to comment #15)
> n assertions for an n-ary && condition, because otherwise if you hit this one
> you don't immediately know which condition failed.

In general I agree and, having personally had the frustration of wondering which conjunct failed, split assertions.  However, this case is particularly vacuous and the assert is really just for documentation/readability.  Multiple lines is just unneeded vertical verbosity.

> >+const char *
> >+js::CurrentScriptFileAndLineSlow(JSContext *cx, uintN *lineop)
> 
> I think you meant |linenop| -- or does "lineop" actually mean something?

Nope, bad fingers.

> >+CurrentScriptFileAndLine(JSContext *cx, uintN *lineop, bool isDirectEval = false);
> 
> bool is easier to pass, but it's not obvious what it means to the reader --
> emphatically so for this signature.

Right on.  I usually try to use enums over bools; I was being lazy here b/c the EvalType enum wasn't quite right (the Function constructor isn't an INDIRECT_EVAL).  But you're right, I'll add a new bool-ish enum.

Note You need to log in before you can comment on or make changes to this bug.