Closed Bug 792398 Opened 8 years ago Closed 8 years ago

IonMonkey: JSScript::argumentsOptimizationFailed does not consider Ion frames. (Assertion failure: flags_ & HAS_ARGS_OBJ, at vm/Stack-inl.h:321)

Categories

(Core :: JavaScript Engine, defect)

18 Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18
Tracking Status
firefox16 --- unaffected
firefox17 --- unaffected
firefox18 + fixed
firefox-esr10 --- unaffected

People

(Reporter: nbp, Assigned: nbp)

References

Details

(6 keywords, Whiteboard: [ion:p1:fx18][adv-main18-])

Crash Data

Attachments

(1 file, 1 obsolete file)

The issue appear when a function first request an argument object and current compiled & active versions of the function have no argument objects.  Ion frames are not iterated on by JSScript::argumentsSpeculationFailed and arguments are not recovered.

The original thoughts was to expect that the IonMonkey frame will bail and return to the interpreter which would then build the argument object when needed.

One of the major question for this bug is, do we have to create all arguments objects as soon as one argument object is constructed since they no longer alias each others?  If we can prevent this, then we might just bail without creating an argument object and let the interpreter allocate one for us.

One test case which illustrate the current failure is a variant of jit-test/tests/basic/testFunApplyMadness2.js which assert when ran with --ion-eager:


function g() { assertEq(false, true) }
var max = 3; ct = 0;

function f(b) {
    if (b) {
        f(b - 1);
    } else {
        g = {
            apply:function(x,y) {
                assertEq(x, null);
                assertEq(y[0], ct);
                ++ct;
            }
        };
    }
    g.apply(null, arguments);
}
f(max - 1);
assertEq(ct, max);
Requiring lazy arguments object creation will require auditing all sites that mention needsArgsObj/hasArgsObj/argsObj.  Probably the fix is to turn StackFrame::argsObj into a fallible method StackFrame::getArgsObj(cx) which asserts script->needsArgsObj is true but creates one if !hasArgsObj.  Note: the jm/im jit JSOP_(GET|SET)ARG jit paths also need to be updated to branch on HAS_ARGS_OBJ.  The question is whether all this is much simpler than just having IM create an argsobj when it creates the StackFrame.
What security rating would you give this?
https://wiki.mozilla.org/Security_Severity_Ratings
(In reply to Andrew McCreight [:mccr8] from comment #2)
> What security rating would you give this?
> https://wiki.mozilla.org/Security_Severity_Ratings

This issue is sec-critical because we can read an argument out of an entry frame of an Ion Activation.  Such arguments are no longer supposed to be kept alive for GC since the Ion stack of arguments is supposed to be the only live sample.  So we might use a pointer which is no longer alive if a GC happens before we restore an Argument object.
Keywords: sec-critical
Duplicate of this bug: 793097
From Bug 793097, report bp-c6a91010-2c6e-43a5-adc8-407aa2120919
Crash Signature: [@ JSScript::argumentsOptimizationFailed(JSContext*, JSScript*)]
Keywords: crash, crashreportid
Summary: IonMonkey: JSScript::argumentsSpeculationFailed does not consider Ion frames. (Assertion failure: flags_ & HAS_ARGS_OBJ, at vm/Stack-inl.h:321) → IonMonkey: JSScript::argumentsOptimizationFailed does not consider Ion frames. (Assertion failure: flags_ & HAS_ARGS_OBJ, at vm/Stack-inl.h:321)
Assignee: general → nicolas.b.pierron
Status: NEW → ASSIGNED
Blocks: 795945
Attached patch Recover arguments from bailouts. (obsolete) — Splinter Review
This patch comes on top of Bug 787813 patch.
It recovers the expected argument object just after a bailout and just before re-entering the interpreter.
It also enable the assertion checking that StackIter is not used with Ion activation frames and filter out Ion activation frames from argumentsOptimizationFailed because Ion frames cannot hold the argument object.
Attachment #667662 - Flags: review?(luke)
Whiteboard: [ion:t] → [ion:p1:fx18]
Comment on attachment 667662 [details] [diff] [review]
Recover arguments from bailouts.

Review of attachment 667662 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like the right general approach; a few requests/questions below:

::: js/src/ion/Bailouts.cpp
@@ +595,5 @@
>      if (JSOp(*pc) == JSOP_LOOPENTRY)
>          cx->regs().pc = GetNextPc(pc);
>  
> +    // As we cannot allocate during the bailout, we fallback here to allocate
> +    // argument objects which might now be needed if the function.

A little more explanation here would be good.  Perhaps:

When JSScript::argumentsOptimizationFailed, we cannot access Ion frames in order to create an arguments object for them.  However, there is an invariant that script->needsArgsObj() implies fp->hasArgsObj() (after the prologue), so we must create one now.

@@ +598,5 @@
> +    // As we cannot allocate during the bailout, we fallback here to allocate
> +    // argument objects which might now be needed if the function.
> +    {
> +        br->entryfp()->clearRunningInIon();
> +        ScriptFrameIter iter(cx);

I've seen how slow ScriptFrameIter is; this is run unconditionally so perhaps we could just iterate over fp->prev()?

@@ +603,5 @@
> +        StackFrame *fp = NULL;
> +        do {
> +            JS_ASSERT(!iter.isIon());
> +            fp = iter.fp();
> +            if (iter.script()->needsArgsObj()) {

Should this also be "&& !fp->hasArgsObj" ?

@@ +610,5 @@
> +                    return false;
> +                Rooted<JSScript*> script(cx, iter.script());
> +                InternalBindingsHandle bindings(script, &script->bindings);
> +                const unsigned var = Bindings::argumentsVarIndex(cx, bindings);
> +                JS_ASSERT(fp->unaliasedLocal(var).isMagic(JS_OPTIMIZED_ARGUMENTS));

I think the test is needed here for the same reason as JSScript::argumentsOptimizationFailed ('arguments' can be overwritten).

@@ +612,5 @@
> +                InternalBindingsHandle bindings(script, &script->bindings);
> +                const unsigned var = Bindings::argumentsVarIndex(cx, bindings);
> +                JS_ASSERT(fp->unaliasedLocal(var).isMagic(JS_OPTIMIZED_ARGUMENTS));
> +                fp->unaliasedLocal(var) = ObjectValue(*argsobj);
> +                fp->initArgsObj(*argsobj);

I think ArgumentsObject::createExpected already calls initArgsObj for you.

::: js/src/vm/Stack-inl.h
@@ -551,5 @@
>          mjit::JITChunk *chunk = fp->jit()->chunk(regs.pc);
>          JS_ASSERT(inlined->inlineIndex < chunk->nInlineFrames);
>          mjit::InlineFrame *frame = &chunk->inlineFrames()[inlined->inlineIndex];
>          JSScript *script = frame->fun->script();
> -        if (!allowCrossCompartment && script->compartment() != cx_->compartment)

I think this may have been unintentionally removed...
Attachment #667662 - Flags: review?(luke)
(In reply to Luke Wagner [:luke] from comment #7)
> Review of attachment 667662 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +598,5 @@
> > +    // As we cannot allocate during the bailout, we fallback here to allocate
> > +    // argument objects which might now be needed if the function.
> > +    {
> > +        br->entryfp()->clearRunningInIon();
> > +        ScriptFrameIter iter(cx);
> 
> I've seen how slow ScriptFrameIter is; this is run unconditionally so
> perhaps we could just iterate over fp->prev()?

I would say that recovering from a Bailout, which implies decompressing snapshots and manipulating the stack is probably still slower than ScriptFrameIter.  In addition, this is a slow path and we are going back to the interpreter after that.  So I would not worry about performances for the moment.

> @@ +603,5 @@
> > +        StackFrame *fp = NULL;
> > +        do {
> > +            JS_ASSERT(!iter.isIon());
> > +            fp = iter.fp();
> > +            if (iter.script()->needsArgsObj()) {
> 
> Should this also be "&& !fp->hasArgsObj" ?

No, because we are not always bailing out because of the lack of Arguments object.  So checking if this is needed would tell us if we have to restore them.  And we might even have to restore a frame which has an argument variable but which does not need it, in which case the JS_OPTIMIZED_ARGUMENTS magic value is still useful.

> @@ +610,5 @@
> > +                    return false;
> > +                Rooted<JSScript*> script(cx, iter.script());
> > +                InternalBindingsHandle bindings(script, &script->bindings);
> > +                const unsigned var = Bindings::argumentsVarIndex(cx, bindings);
> > +                JS_ASSERT(fp->unaliasedLocal(var).isMagic(JS_OPTIMIZED_ARGUMENTS));
> 
> I think the test is needed here for the same reason as
> JSScript::argumentsOptimizationFailed ('arguments' can be overwritten).

No because we would not have compiled it if needsArgsObj was set in the first place.  So the argument variable is garanteed to be a JS_OPTIMIZED_ARGUMENTS magic.

> @@ +612,5 @@
> > +                InternalBindingsHandle bindings(script, &script->bindings);
> > +                const unsigned var = Bindings::argumentsVarIndex(cx, bindings);
> > +                JS_ASSERT(fp->unaliasedLocal(var).isMagic(JS_OPTIMIZED_ARGUMENTS));
> > +                fp->unaliasedLocal(var) = ObjectValue(*argsobj);
> > +                fp->initArgsObj(*argsobj);
> 
> I think ArgumentsObject::createExpected already calls initArgsObj for you.

Indeed, I removed it from my patch.
Follow nits: Improve comment and remove unwanted lines.
Attachment #667662 - Attachment is obsolete: true
Attachment #668222 - Flags: review?(luke)
Comment on attachment 668222 [details] [diff] [review]
Recover arguments from bailouts.

Review of attachment 668222 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ion/Bailouts.cpp
@@ +604,5 @@
> +        StackFrame *fp = NULL;
> +        do {
> +            JS_ASSERT(!iter.isIon());
> +            fp = iter.fp();
> +            if (iter.script()->needsArgsObj()) {

From irl: this would need "&& hasArgsObj" if IM ever supported script->needsArgsObj() == true.

@@ +608,5 @@
> +            if (iter.script()->needsArgsObj()) {
> +                ArgumentsObject *argsobj = ArgumentsObject::createExpected(cx, fp);
> +                if (!argsobj)
> +                    return Interpret_Error;
> +                Rooted<JSScript*> script(cx, iter.script());

Can you hoist/use in the "iter.script()" above?

@@ +611,5 @@
> +                    return Interpret_Error;
> +                Rooted<JSScript*> script(cx, iter.script());
> +                InternalBindingsHandle bindings(script, &script->bindings);
> +                const unsigned var = Bindings::argumentsVarIndex(cx, bindings);
> +                JS_ASSERT(fp->unaliasedLocal(var).isMagic(JS_OPTIMIZED_ARGUMENTS));

From irl: this can arise in edge cases since assignment to 'arguments' doesn't cause script->needsArgsObj() to be true and thus 'arguments' may have already been clobbered in this frame.
Attachment #668222 - Flags: review?(luke) → review+
https://hg.mozilla.org/mozilla-central/rev/3b458f4e0f42
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Duplicate of this bug: 795945
Whiteboard: [ion:p1:fx18] → [ion:p1:fx18][adv-main18-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.