Closed Bug 1368576 Opened 3 years ago Closed 3 years ago

Assertion failure: !ins->hasDefUses(), at js/src/jit/TypePolicy.cpp:302

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla55
Tracking Status
firefox-esr52 55+ fixed
firefox53 --- wontfix
firefox54 + wontfix
firefox55 + fixed
firefox56 --- verified

People

(Reporter: gkw, Assigned: jandem)

References

(Blocks 2 open bugs)

Details

(4 keywords, Whiteboard: [jsbugmon:update][adv-main55+][adv-esr52.3+])

Attachments

(3 files)

The following testcase crashes on mozilla-central revision 34ac1a5d6576 (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --ion-eager):

(function () {
    "use strict";
    for (let x of []) {
        let y = {} = this;
    }
})();


Backtrace:

#0  0x0000000000833a40 in js::jit::TypeBarrierPolicy::adjustInputs (this=<optimized out>, alloc=..., def=0x7f16e739eaa0) at js/src/jit/TypePolicy.cpp:302
#1  0x00000000006e37c5 in (anonymous namespace)::TypeAnalyzer::adjustInputs (def=0x7f16e739eaa0, this=0x7ffe134dd810) at js/src/jit/IonAnalysis.cpp:1686
#2  (anonymous namespace)::TypeAnalyzer::insertConversions (this=0x7ffe134dd810) at js/src/jit/IonAnalysis.cpp:1753
#3  (anonymous namespace)::TypeAnalyzer::analyze (this=0x7ffe134dd810) at js/src/jit/IonAnalysis.cpp:2000
#4  js::jit::ApplyTypeInformation (mir=mir@entry=0x7f16e738c2b0, graph=...) at js/src/jit/IonAnalysis.cpp:2012
#5  0x00000000006ff477 in js::jit::OptimizeMIR (mir=mir@entry=0x7f16e738c2b0) at js/src/jit/Ion.cpp:1538
/snip

For detailed crash information, see attachment.

MIR is on the stack, setting s-s as a start.
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/714b7caf7e00
user:        André Bargull
date:        Thu Apr 27 09:24:08 2017 -0700
summary:     Bug 1360220 - Replace emitRequireObjectCoercible with JSOP_CHECKOBJCOERCIBLE. r=shu

Andre, is bug 1360220 a likely regressor?
Blocks: 1360220
Flags: needinfo?(andrebargull)
Duplicate of this bug: 1368573
Duplicate of this bug: 1368574
Duplicate of this bug: 1368575
Duplicate of this bug: 1368584
Attached file another stack
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #2)
> Andre, is bug 1360220 a likely regressor?

Sounds reasonable given that the test case involves object destructuring.


Later: I know a simple patch to paper over this issue, but I don't know the actual cause and how to fix MCheckObjCoercible to do what it's actually supposed to do. Someone who knows Ion needs to take a look at this bug.


diff --git a/js/src/frontend/BytecodeEmitter.cpp b/js/src/frontend/BytecodeEmitter.cpp
--- a/js/src/frontend/BytecodeEmitter.cpp
+++ b/js/src/frontend/BytecodeEmitter.cpp
@@ -5903,17 +5903,23 @@ BytecodeEmitter::emitComputedPropertyNam
 bool
 BytecodeEmitter::emitDestructuringOpsObject(ParseNode* pattern, DestructuringFlavor flav)
 {
     MOZ_ASSERT(pattern->isKind(PNK_OBJECT));
     MOZ_ASSERT(pattern->isArity(PN_LIST));
 
     MOZ_ASSERT(this->stackDepth > 0);                             // ... RHS
 
-    if (!emit1(JSOP_CHECKOBJCOERCIBLE))                           // ... RHS
+    if (!emit1(JSOP_DUP))                                         // ... RHS RHS
+        return false;
+
+    if (!emit1(JSOP_CHECKOBJCOERCIBLE))                           // ... RHS RHS
+        return false;
+
+    if (!emit1(JSOP_POP))                                         // ... RHS
         return false;
 
     bool needsRestPropertyExcludedSet = pattern->pn_count > 1 &&
                                         pattern->last()->isKind(PNK_SPREAD);
     if (needsRestPropertyExcludedSet) {
         if (!emitDestructuringObjRestExclusionSet(pattern))       // ... RHS SET
             return false;
Flags: needinfo?(andrebargull)
(In reply to André Bargull from comment #8)
> Someone who knows Ion needs to take a look at this bug.

Throwing the ball to Jan. :)
Flags: needinfo?(jdemooij)
Attached patch PatchSplinter Review
There's an assumption in TypeBarrierPolicy::adjustInputs that MTypeBarrier with type MIRType::Undefined has no def uses (because IonBuilder inserts an MConstant(undefined) after it).

However, here that doesn't hold because we call IonBuilder::addOsrValueTypeBarrier with type = MIRType::Value and a typeSet that only contains |undefined|. So the MTypeBarrier has type MIRType::Undefined but addOsrValueTypeBarrier doesn't add the constant.

The fix is simple, if type == MIRType::Value, we can refine it based on the TypeSet so everything is consistent.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8873382 - Flags: review?(nicolas.b.pierron)
It's hard to say if it was possible to trigger this *before* bug 1360220 landed. Considering the patch is simple, I think we should just backport it if possible.
Comment on attachment 8873382 [details] [diff] [review]
Patch

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

Sounds good to me, based on the explanations from comment 10.

I will note that the Phi type which is going to be in the join block between the OSR block and the rest of the graph has a Value type, thus the switch case which follow in this OSR block will probably box the result of the barrier, which is totally fine and safe.
Attachment #8873382 - Flags: review?(nicolas.b.pierron) → review+
Keywords: sec-high
Wontfix for 53/54 as we are close to 54 release and it seems uncertain if 54 is even affected.
Comment on attachment 8873382 [details] [diff] [review]
Patch

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Probably hard.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.

> Which older supported branches are affected by this flaw?
It's unclear if it was possible to trigger this before bug 1360220 landed. The fix is simple so it makes sense to backport everywhere I think.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Trivial to backport.

> How likely is this patch to cause regressions; how much testing does it need?
Unlikely.
Attachment #8873382 - Flags: sec-approval?
Comment on attachment 8873382 [details] [diff] [review]
Patch

We can't backport this right now but since the primary issue was only clearly triggerable on 55, I'm going to give sec-approval+ for trunk. This is too late to make 54 but we should port it to ESR52 in late June during the 55 beta cycle.
Attachment #8873382 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/9c6ee0bd191b
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Group: javascript-core-security → core-security-release
This grafts cleanly to ESR52. Please nominate it for approval when you get a chance.
Flags: needinfo?(jdemooij)
Comment on attachment 8873382 [details] [diff] [review]
Patch

[Approval Request Comment]
User impact if declined: Potential security bug or crashes.
Fix Landed on Version: 55.
Risk to taking this patch (and alternatives if risky): Low risk, small patch, has been on m-c for a few weeks.
String or UUID changes made by this patch: None.
Flags: needinfo?(jdemooij)
Attachment #8873382 - Flags: approval-mozilla-esr52?
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Comment on attachment 8873382 [details] [diff] [review]
Patch

Giving ESR52 approval since it has been sitting for five days.
Attachment #8873382 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main55+][adv-esr52.3+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.