451657 - TM: use cx->executingTrace to prevent all js_Interpret re-entry from JITed builtins

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brendan Eich [:brendan]]

Summary says it all. Only way to be sure of coverage is to centralize the check and propagate a unique exception back to the builtin so it can fail to its guard.

/be

Comment 1

[Brendan Eich [:brendan]]

I landed an assertion at the top of js_Interpret:

    /* We had better not be entering the interpreter from JIT-compiled code. */
    JS_ASSERT(!cx->runningJittedCode);

after renaming gcDontBlock to runningJittedCode (agonized over spelling, this is easiest on the eyes ;-).

If anyone sees this assertion botch, please attach a testcase.

/be

Comment 2

[Boris Zbarsky [:bzbarsky]]

Isn't that assert hit by the steps to reproduce in bug 451666?

Note that it's not just built-ins.  In bug 451666 we're calling a JS resolve hook from inside the tracer code directly, which leads to arbitrary script execution.  So just fixing this bug may not fix the other if the fix is limited to builtins.

Comment 3

[Brendan Eich [:brendan]]

Yes, the true bug is not limited to built-ins. There are also call-outs from JSOP_ADD, etc., via OBJ_DEFAULT_VALUE, as well as the resolve hook, etc.

Some of these should be made traceable by trampolining back to the interpreter. Others should do a deep abort. That's one reason I kept both bugs around.

/be

Comment 4

[Brendan Eich [:brendan]]

Look for the "!cx->runningJittedCode" assertbotch skidmark when marking bugs as dependents.. Dependents should have distinct stacks at least in the line number of the js_Interpret frame nearest top of stack.

/be

Comment 5

[Brendan Eich [:brendan]]

Blake and I fixed this:

http://wiki.ecmascript.org/doku.php?id=proposals:hashcodes

I'm going to make bug 451666 be about the trampoline idea for all ops that can call indirectly back into js_Interpret.

/be

Comment 6

[Brendan Eich [:brendan]]

Oops, shouldn't mark fixed until this is in mozilla-central. Blake, any hope of a weekend push? My cable internet is flaking...

/be

Comment 7

[Mike Shaver (:shaver emeritus)]

(In reply to comment #5)
> Blake and I fixed this:
> 
> http://wiki.ecmascript.org/doku.php?id=proposals:hashcodes

I think you mean http://hg.mozilla.org/tracemonkey/rev/66ce4cd00877 ?

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

I could push this tomorrow. Others are welcome to jump in if that's not soon enough.

Comment 9

[Brendan Eich [:brendan]]

Shaver: oops, thanks -- copy didn't "take", d'oh.

Probably worth a push ahead of the impending tracemonkey landing on m-c, for the nightly build kids' sake.

/be

Comment 10

[Brendan Eich [:brendan]]

Fixed in m-c (tm already):

http://hg.mozilla.org/mozilla-central/rev/66ce4cd00877

/be