Closed Bug 822938 Opened 7 years ago Closed 7 years ago

IonMonkey: Crash [@ js::ion::GetPropertyCache] or near null or "Assertion failure: live->empty(),"

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla20
Tracking Status
firefox17 --- unaffected
firefox18 --- unaffected
firefox19 --- unaffected
firefox20 + fixed
firefox-esr10 --- unaffected
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: gkw, Assigned: nbp)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [fuzzblocker][jsbugmon:update][adv-main20-])

Crash Data

Attachments

(3 files)

Attached file stack
(function f() {
    for (var x in []) {
        (function() {})();
    };
    f();
})();

asserts js debug shell on m-c changeset 287a7d7cf7f0 with --no-jm at Assertion failure: live->empty(), and crashes js opt shell near null.

s-s because it seems to be crashing at an unknown address, but it seems to be null, but locking just in case.

autoBisecting now.
Marking [fuzzblocker] because this opt crash does not have a signature that we can ignore.
Whiteboard: [jsbugmon:update] → [fuzzblocker][jsbugmon:update]
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   116398:aec01763cb6b
user:        Nicolas B. Pierron
date:        Tue Dec 18 05:57:29 2012 -0800
summary:     Bug 819865 - Make the callee canonical in IonBuilder. r=jandem
The problem comes from the move group which is only defining the callee on the left-hand side of the basic block and as it does not show up in the OSR block and is not captured by any Phi this assertion is raised.

Brian, do you know why we are spilling values on the argument slot, and is there a way to mark this slot as a constant slot ?
Flags: needinfo?(nicolas.b.pierron) → needinfo?(bhackett1024)
Flag as sec-critical as this bug is using any register returned by the register allocator instead of using the LArgument value where it is used.  This might not be a null-deref.

As jandem remark, JSOP_CALLEE is not supported in JM, and as named lambdas are not used in PdfJS this should not change the performances.  So at last we can just remove the compilation support for JSOP_CALLEE while keeping it for the scope chain.
Keywords: sec-critical
jandem, dvander,  Do you mind If I replace Bug 819865 by its previous implementation namely attachment 692975 [details] [diff] [review] ?
Flags: needinfo?(dvander)
Flags: needinfo?(jdemooij)
What if we just change how it's lowered? It should work to define() LCallee as a register, and then in CG, loadPtr(<address>, output). That avoids every problem and with GVN it won't make any difference in practice.
Flags: needinfo?(dvander)
(In reply to David Anderson [:dvander] from comment #6)
> What if we just change how it's lowered? It should work to define() LCallee
> as a register, and then in CG, loadPtr(<address>, output). That avoids every
> problem and with GVN it won't make any difference in practice.

This won't change the problem encountered here, because the problem seen here is that a register can be used in the normal entry point, but not for the OSR entry point.  When we hit the OSR entry point we have to provide the same allocation.  With MParameters we have no problem because the OSR block is defining then, and as formal arguments (aka MParameters) are captured in the basic blocks / resume points, we introduce MPhi at the top of each block and rely on the SSA form to do the allocation for us.

The other case similar to this MCallee is the Magic lazy-argument constant, which is only defined in the entry point only, and this does not cause any issue since the constant does not appear as part of the allocations.

As MCallee is not captured in the slots of the basic blocks, as opposed to MParameters, and as it needs an allocation which cannot be inlined, as opposed to the magic value of arguments, the register allocator complains because the OSR entry point has no definition and this cannot be solved easily except at considering the MCallee as a special slot, which would be costly for functions which are not using any callee.

Weaking the RegAlloc assertion to avoid complaining about clobber of arguments should be enough, and GVN would get rid of extra definitions of MCallee.  Without any GVN, this will lead to less optimized code (which is fine for me) which will have to load the callee everytime from the frame to a register.
Can also crash [@ js::ion::GetPropertyCache]
Crash Signature: [@ js::ion::GetPropertyCache]
Summary: IonMonkey: Crash near null or "Assertion failure: live->empty()," → IonMonkey: Crash [@ js::ion::GetPropertyCache] or near null or "Assertion failure: live->empty(),"
Assignee: general → nicolas.b.pierron
As discuss with dvander, this relies on GVN to remove extra MCallee, and avoid weakening the assertion by making the load in the CodeGen of the LCallee instead of using the move groups of the register allocator.
Attachment #694827 - Flags: review?(jdemooij)
Flags: needinfo?(jdemooij)
Comment on attachment 694827 [details] [diff] [review]
LCallee use a register instead of a fixed LArgument.

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

::: js/src/ion/IonBuilder.cpp
@@ +1035,5 @@
> +      {
> +        MCallee *callee = MCallee::New();
> +        current->add(callee);
> +        current->push(callee);
> +        return callee;

"return true;"

::: js/src/ion/IonBuilder.h
@@ +512,5 @@
>      // True if script->failedShapeGuard is set for the current script or
>      // an outer script.
>      bool failedShapeGuard_;
>  
> +    // If this script can use a lazy arguments object, it wil be pre-created

Nit: don't revert the typo fix (s/wil/will)
Attachment #694827 - Flags: review?(jdemooij) → review+
(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #11)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/bc52b06ea2ea

landed with test case as it only affects 20 and the feature can easily be disabled, by removing JSOP_CALLEE support, if there is any security issue.

Remove need-info from Brian Hackett as we no longer have any LArgument used for spilling.
Flags: needinfo?(bhackett1024) → in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/bc52b06ea2ea
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Whiteboard: [fuzzblocker][jsbugmon:update] → [fuzzblocker][jsbugmon:update][adv-main20-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.