Closed Bug 787813 Opened 8 years ago Closed 8 years ago

IonMonkey: Assertion failure: isFunctionFrame(), at vm/Stack.h:714 or Crash [@ js::ArgumentsObject::create]

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla18
Tracking Status
firefox15 --- unaffected
firefox16 --- unaffected
firefox17 --- unaffected
firefox18 --- fixed
firefox-esr10 --- unaffected
firefox-esr17 --- unaffected

People

(Reporter: decoder, Assigned: nbp)

References

Details

(Keywords: assertion, crash, testcase, Whiteboard: [jsbugmon:update,origRev=fdfaef738a00][ion:p1:fx18][fuzzblocker][adv-main18-])

Crash Data

Attachments

(1 file, 2 obsolete files)

The following testcase asserts on ionmonkey revision 68df0d67be37 (run with --ion -n -m --ion-eager):


function f(o) {
    var prop = "arguments";
    f[prop] = f[prop];
}
for(var i=0; i < 50; i++) {
    f();
}
Opt-crash:

==5985== Invalid read of size 8
==5985==    at 0x590180: js::ArgumentsObject::create(JSContext*, js::StackFrame*) (Barrier.h:172)
==5985==    by 0x471963: fun_getProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<long>, JS::MutableHandle<JS::Value>) (jsfun.cpp:122)
==5985==    by 0x4C794F: js::Shape::get(JSContext*, JS::Handle<JSObject*>, JSObject*, JSObject*, JS::MutableHandle<JS::Value>) (jscntxtinlines.h:439)
==5985==    by 0x4CE356: js::baseops::GetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<long>, JS::MutableHandle<JS::Value>) (jsobj.cpp:4461)
==5985==    by 0x46881F: JSObject::getProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, js::PropertyName*, JS::MutableHandle<JS::Value>) (jsobjinlines.h:173)
==5985==    by 0x70211C: js::ion::GetElementCache(JSContext*, unsigned long, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) (jsinterpinlines.h:729)
==5985==    by 0x4032527: ???
==5985==    by 0x633412: js::mjit::EnterMethodJIT(JSContext*, js::StackFrame*, void*, JS::Value*, bool) (MethodJIT.cpp:1045)
==5985==    by 0x634C00: js::mjit::JaegerShotAtSafePoint(JSContext*, void*, bool) (MethodJIT.cpp:1103)
==5985==    by 0x4B03C6: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:1484)
==5985==    by 0x4B35CD: js::RunScript(JSContext*, JSScript*, js::StackFrame*) (jsinterp.cpp:324)
==5985==    by 0x4B4370: js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) (jsinterp.cpp:509)
==5985==  Address 0x100000000 is not stack'd, malloc'd or (recently) free'd


Bisecting to find an owner more quickly now :)
Crash Signature: js::ArgumentsObject::create
Keywords: crash
Whiteboard: [jsbugmon:update] → [jsbugmon:update,bisect]
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   105607:6cd206b37176
parent:      104959:b63bb39ed1c0
parent:      105606:a0240c1043ee
user:        David Anderson
date:        Wed Aug 29 17:51:24 2012 -0700
summary:     Merge from mozilla-central.

Oops! We didn't test rev b63bb39ed1c0, a parent of the blamed revision! Let's do that now.
Rev b63bb39ed1c0: Updating... Compiling... Testing... Exit status: NORMAL (0.022 seconds)
good (not interesting) 
As expected, the parent's label is the opposite of the blamed rev's label.

Oops! We didn't test rev a0240c1043ee, a parent of the blamed revision! Let's do that now.
We did not test rev a0240c1043ee because it is not a descendant of either 4ceb3e9961e4 or 68df0d67be37.
Rev a0240c1043ee: Updating... Compiling... Testing... Exit status: ABNORMAL return code 1 (0.012 seconds)
good (not interesting) 
As expected, the parent's label is the opposite of the blamed rev's label.
Whiteboard: [jsbugmon:update] → [jsbugmon:update][ion:p1:fx18]
Quick look at this suggests we need to port ArgumentsObject::createUnexpected to use StackIter instead of js::StackFrame.
Whiteboard: [jsbugmon:update][ion:p1:fx18] → [jsbugmon:update,origRev=fdfaef738a00][ion:p1:fx18]
Version: Other Branch → Trunk
Assignee: general → nicolas.b.pierron
Status: NEW → ASSIGNED
Use StackIter instead of StackFrame for the creation of unexpected argument objects.

Fix latent bug related to the bailout of frame having a needsArgsObj after the bailout.
Attachment #661306 - Flags: review?(luke)
Attachment #661306 - Flags: review?(dvander)
Comment on attachment 661306 [details] [diff] [review]
Argument object, Use StackIter instead of StackFrame.

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

::: js/src/ion/Bailouts.cpp
@@ +584,5 @@
> +        do {
> +            JS_ASSERT(!iter.isIon());
> +            fp = iter.fp();
> +            if (iter.script()->needsArgsObj()) {
> +                ArgumentsObject *argsobj = ArgumentsObject::createExpected(cx, iter);

luke:

I had to add this piece of code to the bailout path to handle the conversion of frames which are now needing an argument object.  This is documented above the loop located at js/src/jsscript.cpp:2553 .  Can you explain why needsArgsObjs implies that all frames have an argument object?  We were hopping that the interpreter could build the argument object only when it needs one for the bailed frames instead of doing so inside the bailing code.  Can we remove the assertion and do so, or the assertion may prevent futher security/privacy issues?

In js/src/jsscript.cpp:2553, the loop use AllFramesIter, is it possible to run a script with a compartment which is not its own compartment, or we can just replace this loop iterator by ScriptFrameIter?
Attachment #661306 - Flags: feedback?(luke)
Comment on attachment 661306 [details] [diff] [review]
Argument object, Use StackIter instead of StackFrame.

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

::: js/src/ion/Bailouts.cpp
@@ +577,5 @@
> +    // As we cannot allocate during the bailout, we fallback here to allocate
> +    // argument objects which might now be needed if the function now need an
> +    // argument object.
> +    {
> +        br->entryfp()->clearRunningInIon();

I don't understand why this chunk is needed. The only way we could have an unexpected arguments object, is the case where we access f.arguments (since the .apply optimization is static, and if not, would be guarded anyway).

::: js/src/vm/ArgumentsObject.cpp
@@ +129,1 @@
>  ArgumentsObject::createUnexpected(JSContext *cx, StackFrame *fp)

Where does this still get called from, and could it be changed to use StackIter directly instead?

::: js/src/vm/Stack.h
@@ +1773,5 @@
>  
>      CallArgs nativeArgs() const { JS_ASSERT(isNativeCall()); return args_; }
>  
>      template <class Op>
> +    inline void forEachUnaliasedActual(Op op);

forEachUnaliasedActualArg?
(In reply to David Anderson [:dvander] from comment #6)
> Comment on attachment 661306 [details] [diff] [review]
> Argument object, Use StackIter instead of StackFrame.
> 
> Review of attachment 661306 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/ion/Bailouts.cpp
> @@ +577,5 @@
> > +    // As we cannot allocate during the bailout, we fallback here to allocate
> > +    // argument objects which might now be needed if the function now need an
> > +    // argument object.
> > +    {
> > +        br->entryfp()->clearRunningInIon();
> 
> I don't understand why this chunk is needed. The only way we could have an
> unexpected arguments object, is the case where we access f.arguments (since
> the .apply optimization is static, and if not, would be guarded anyway).

If we don't do so, then the frame might still be flagged as runningInIon before we enter the ThunkToInterpreter function and cause StackIter to skip this frame after a bailout, because this frame was telling us to look at the IonActivation.  Which makes me think that I should also unwind the IonActivation such as we don't have an off-by-one while iterating stackframes/ionframes.  Another option would be to teach StackIter to iterate properly on bailed entry frames.

> ::: js/src/vm/ArgumentsObject.cpp
> @@ +129,1 @@
> >  ArgumentsObject::createUnexpected(JSContext *cx, StackFrame *fp)
> 
> Where does this still get called from, and could it be changed to use
> StackIter directly instead?

I tried to remove it, but StackIter only iterates on the frames of the same context. AllFramesIter iterates on all StackFrame* and might reach a stack frame which is not reachable from StackIter if the function is used in another context.  Which is why I ask luke for feedback.

> ::: js/src/vm/Stack.h
> @@ +1773,5 @@
> >  
> >      CallArgs nativeArgs() const { JS_ASSERT(isNativeCall()); return args_; }
> >  
> >      template <class Op>
> > +    inline void forEachUnaliasedActual(Op op);
> 
> forEachUnaliasedActualArg?

I just followed the naming used for StackFrame, but I would agree, …
luke, any thoughts ?
Nicolas: assuming I understand the bug correctly, I would think the fix would be to add an "|| iter.isIon()" disjunct to the StackIter loop in fun_getProperty.  That way ion frames just don't appear and all the other issues don't arise.  Given that f.arguments forbids ion compilation of f, and we are in unspecified territory anyhow, this should be astronomically unlikely.

Even though I think most of this patch isn't necessary for this bug (I think the tiny fix above will work), I *do* think there is a latent issue when a script is active on the stack twice and we call JSScript::argumentsSpeculationFailed.  Perhaps this could be filed as a separate bug?

wrt the second question in comment 5: a script can only be run in its *compartment*, but that is not what ScriptFrameIter vs. AllFramesIter is about: the former only iterates over the frames in a single *context* (which can, of course, span multiple compartments).  Since a JSContext has no associativity with compartments/scripts/anything, AllFramesIter is necessary.
Attachment #661306 - Flags: review?(luke)
Attachment #661306 - Flags: review-
Attachment #661306 - Flags: feedback?(luke)
This is causing various crashes in opt-builds, marking as fuzzblocker.
Whiteboard: [jsbugmon:update,origRev=fdfaef738a00][ion:p1:fx18] → [jsbugmon:update,origRev=fdfaef738a00][ion:p1:fx18][fuzzblocker]
(In reply to Luke Wagner [:luke] from comment #8)
> Nicolas: assuming I understand the bug correctly, I would think the fix
> would be to add an "|| iter.isIon()" disjunct to the StackIter loop in
> fun_getProperty.  That way ion frames just don't appear and all the other
> issues don't arise.  Given that f.arguments forbids ion compilation of f,
> and we are in unspecified territory anyhow, this should be astronomically
> unlikely.

fun_getProperty should not skip Ion frames because even if this is unlikely we still need to honor f.arguments or f.caller.  One of the case is when a function using f.arguments is used for the first time after the compilation of f, such as the current test case with an eager compilation.  Skipping Ion frames will change the behavior and lookup for arbitrary *f* frame above, which is wrong.

(In reply to Luke Wagner [:luke] from comment #8)
> I *do* think there is a latent issue when a
> script is active on the stack twice and we call
> JSScript::argumentsSpeculationFailed.  Perhaps this could be filed as a
> separate bug?

I will.
(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #10)
> fun_getProperty should not skip Ion frames because even if this is unlikely
> we still need to honor f.arguments or f.caller.

Both of those are unspecified, so there is nothing to honor; just "don't break the web".  Given that any site should warm up and hence prevent ion compilation, I don't see skipping ion frames being a realistic problem.
Alternately, we can do what (I think) Crankshaft does and not skip optimized frames, but return a new Arguments object every time that is untracked and unaliased and in our case may have missing values.
(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #10)
> (In reply to Luke Wagner [:luke] from comment #8)
> > Nicolas: assuming I understand the bug correctly, I would think the fix
> > would be to add an "|| iter.isIon()" disjunct to the StackIter loop in
> > fun_getProperty.  That way ion frames just don't appear and all the other
> > issues don't arise.  Given that f.arguments forbids ion compilation of f,
> > and we are in unspecified territory anyhow, this should be astronomically
> > unlikely.
> 
> fun_getProperty should not skip Ion frames because even if this is unlikely
> we still need to honor f.arguments or f.caller.  One of the case is when a
> function using f.arguments is used for the first time after the compilation
> of f, such as the current test case with an eager compilation.  Skipping Ion
> frames will change the behavior and lookup for arbitrary *f* frame above,
> which is wrong.

Simply returning from fun_getProperty, when we hit the Ion frame, does not pass the test suite too.  The test suite fails with «unexpected null» type errors for "x.arguments".
This patch exclude modifications needed for Bug 792398 and provide a stackIter interface for ArgumentObject::create.
Attachment #661306 - Attachment is obsolete: true
Attachment #661306 - Flags: review?(dvander)
Attachment #665074 - Flags: review?(luke)
(In reply to David Anderson [:dvander] from comment #12)
> Alternately, we can do what (I think) Crankshaft does and not skip optimized
> frames, but return a new Arguments object every time that is untracked and
> unaliased and in our case may have missing values.

That's what we do also :)
Comment on attachment 665074 [details] [diff] [review]
Argument object, Use StackIter instead of StackFrame.

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

After more discussion, I think this patch is right idea.  I do have one non-trivial change/simplification request below, though:

::: js/src/jsfun.cpp
@@ -106,5 @@
>      }
>      if (iter.done())
>          return true;
>  
> -    StackFrame *fp = iter.fp();

Could we make fp() assert that this is not an ion frame?  If some code really does want to access the StackFrame of an ion frame, we could have an ionEntryFrame() which doesn't assert.

::: js/src/vm/ArgumentsObject.cpp
@@ +79,5 @@
>  
>      /*
> +     * If it exists and the arguments object aliases formals, the call object is
> +     * the canonical location for formals. If this is an IonFrame, the call
> +     * object is ignored.

How about a little more explanation:

If this is an IonFrame we break things a bit by ignoring the call object.  We can make this tradeoff since (1) this case only arises in the case of an Arguments::createUnexpected resulting from f.arguments (which is unspecified in non-strict code and disallowed in strict-mode code) and (2) web compatibility should not be affected since f.arguments disables ion compilation for f which makes this difficult to hit in practice.

@@ +114,5 @@
>  ArgumentsObject *
> +ArgumentsObject::createExpected(JSContext *cx, StackFrame *fp)
> +{
> +    StackIter wrapFp(cx, fp);
> +    return createExpected(cx, wrapFp);

Given that there are only calls to createExpected(cx, fp), not (cx, iter), can you just change the existing createExpected instead of adding a new one?  That would, e.g., avoid the "if (!iter.isIon())" branch above.

I'm also a bit uneasy adding this new StackIter constructor: it makes a degenerate iterator that doesn't support iteration.  Making it non-degenerate would I think make the createExpected case unnecessarily slow.  Can we instead make ArgumentsObject::create not take a StackIter but instead take the data it needs.  It seems there are:
 1. numActualArgs
 2. callee
 3. ability to do forEachCanonicalActualArg

Only the latter is hard, and I'd solve it by making create be:

template <class ForEachArgOp>
ArgumentsObject::create(JSContext *cx, unsigned numActualArgs, JSFunction &callee, ForEachArgOp forEachActualArg);

Lastly, for the UnexpectedForEachArgOp can you just insert the current StackFrame-copying logic that is create() instead of (re-)adding StackIter::forEachCanonicalActualArg?  I removed it in m-c since there is almost no reason we should be doing this (due to argument aliasing) so I didn't want to leave a handgun lying around.  I guess the decls got re-added in an IM merge...

Does this work for you?
Attachment #665074 - Flags: review?(luke)
(In reply to Luke Wagner [:luke] from comment #16)
> Comment on attachment 665074 [details] [diff] [review]
> Argument object, Use StackIter instead of StackFrame.
> 
> Review of attachment 665074 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> After more discussion, I think this patch is right idea.  I do have one
> non-trivial change/simplification request below, though:
> 
> ::: js/src/jsfun.cpp
> @@ -106,5 @@
> >      }
> >      if (iter.done())
> >          return true;
> >  
> > -    StackFrame *fp = iter.fp();
> 
> Could we make fp() assert that this is not an ion frame?  If some code
> really does want to access the StackFrame of an ion frame, we could have an
> ionEntryFrame() which doesn't assert.

They are still some issues related to that, I opened a follow-up bug for that (Bug 797977).

> ::: js/src/vm/ArgumentsObject.cpp
> @@ +79,5 @@
> >  
> >      /*
> > +     * If it exists and the arguments object aliases formals, the call object is
> > +     * the canonical location for formals. If this is an IonFrame, the call
> > +     * object is ignored.
> 
> How about a little more explanation:
> 
> If this is an IonFrame we break things a bit by ignoring the call object. 
> We can make this tradeoff since (1) this case only arises in the case of an
> Arguments::createUnexpected resulting from f.arguments (which is unspecified
> in non-strict code and disallowed in strict-mode code) and (2) web
> compatibility should not be affected since f.arguments disables ion
> compilation for f which makes this difficult to hit in practice.

This is not even that, when IonMonkey is on the stack, nothing else is considered alive except the arguments of the IonMonkey frames.  We are not maintaining the state of the call object of the entry frame.  So we can just not alias it.

> @@ +114,5 @@
> >  ArgumentsObject *
> > +ArgumentsObject::createExpected(JSContext *cx, StackFrame *fp)
> > +{
> > +    StackIter wrapFp(cx, fp);
> > +    return createExpected(cx, wrapFp);
> 
> Given that there are only calls to createExpected(cx, fp), not (cx, iter),
> can you just change the existing createExpected instead of adding a new one?
> That would, e.g., avoid the "if (!iter.isIon())" branch above.

Ok but I will need it for Bug 792398.

> […]
> 
> template <class ForEachArgOp>
> ArgumentsObject::create(JSContext *cx, unsigned numActualArgs, JSFunction
> &callee, ForEachArgOp forEachActualArg);

Ok.

> Lastly, for the UnexpectedForEachArgOp can you just insert the current
> StackFrame-copying logic that is create() instead of (re-)adding
> StackIter::forEachCanonicalActualArg?  I removed it in m-c since there is
> almost no reason we should be doing this (due to argument aliasing) so I
> didn't want to leave a handgun lying around.  I guess the decls got re-added
> in an IM merge...

I've removed it for others than IM. so it must be guarded by isIon().
Follow luke's nits, when possible for fx18, as documented in previous comment.
Attachment #665074 - Attachment is obsolete: true
Attachment #668109 - Flags: review?(luke)
Comment on attachment 668109 [details] [diff] [review]
Argument object, Use StackIter instead of StackFrame.

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

r+ assuming the code organization/nits below.

::: js/src/vm/ArgumentsObject.cpp
@@ +23,5 @@
>  using namespace js::gc;
>  
> +/* Erase formals which are not part of the actuals. */
> +static void
> +eraseNonActualFormalArgs(HeapValue *dstBase, unsigned numActuals, unsigned numFormals)

"erase" sounds odd, can we have SetMissingFormalArgsToUndefined (note capitalization of static function name).

@@ +42,5 @@
> +    { }
> +
> +    inline JSScript *script() const { return fp_->script(); }
> +    inline JSFunction *callee() const { return &fp_->callee(); }
> +    unsigned numActualArgs() const { return fp_->numActualArgs(); }

I think it would be better to pass (script, callee, numActualArgs) as explicit arguments to create().  There is hardly any code duplication to speak of and this would make CopyStack*Args more true to its name.  With this and the other changes below, these structs can be almost-trivial wrappers that call the static helper functions I recommend hoisting out below.

@@ +71,5 @@
> +    /*
> +     * If a call object exists and the arguments object aliases formals, the
> +     * call object is the canonical location for formals.
> +     */
> +    void fixFormalLocation(JSObject *obj, ArgumentsData *data) {

How about maybeForwardToCallObject?

@@ +87,5 @@
> +        if (!argsobj)
> +            return NULL;
> +
> +        fp_->initArgsObj(*argsobj);
> +        return argsobj;

Can you stick this back in ArgumentsObject::createExpected?  I don't see any benefit moving it into this class as a member.

@@ +107,5 @@
> +    void copyArgs(HeapValue *dst) const {
> +        if (!iter_.isIon()) {
> +            /* Prevent code duplication. */
> +            CopyStackFrameArgs sf(iter_.fp());
> +            sf.copyArgs(dst);

I think it would be more straightforward to make copyArgs a static function called from both classes.

@@ +125,5 @@
> +    void fixFormalLocation(JSObject *obj, ArgumentsData *data) {
> +        if (!iter_.isIon()) {
> +            /* Prevent code duplication. */
> +            CopyStackFrameArgs sf(iter_.fp());
> +            sf.fixFormalLocation(obj, data);

I think it would be more straightforward to make fixFormalLocation (maybeForwardToCallObject) a static function called by both classes.

::: js/src/vm/ArgumentsObject.h
@@ +109,4 @@
>      inline ArgumentsData *data() const;
>  
> +    struct CopyStackFrameArgs;
> +    struct CopyStackIterArgs;

IIUC, these don't need to be nested classes, they can be entirely local to ArgumentsObject.cpp.

::: js/src/vm/Stack-inl.h
@@ +592,2 @@
>        case NATIVE:
>          JS_NOT_REACHED("Unused ?");

This function may now only be used on ION frames; can you rename it ionForEachActualArg, take out the switch, and  JS_ASSERT(state_ == ION) at the beginning?
Attachment #668109 - Flags: review?(luke) → review+
(In reply to Luke Wagner [:luke] from comment #19)
> Comment on attachment 668109 [details] [diff] [review]
> Argument object, Use StackIter instead of StackFrame.
> 
> Review of attachment 668109 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/vm/ArgumentsObject.h
> @@ +109,4 @@
> >      inline ArgumentsData *data() const;
> >  
> > +    struct CopyStackFrameArgs;
> > +    struct CopyStackIterArgs;
> 
> IIUC, these don't need to be nested classes, they can be entirely local to
> ArgumentsObject.cpp.

I added this to get the visibility to MAYBE_CALL_SLOT in maybeForwardToCallObject.
https://hg.mozilla.org/mozilla-central/rev/47a17015ef4a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Looks great, thanks for being so amenable.
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Whiteboard: [jsbugmon:update,origRev=fdfaef738a00][ion:p1:fx18][fuzzblocker] → [jsbugmon:update,origRev=fdfaef738a00][ion:p1:fx18][fuzzblocker][adv-main18-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.