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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: Detailed Crash Information

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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?

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: another stack

Comment 8

[André Bargull [:anba]]

(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;

Comment 9

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(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. :)

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Patch

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.

Comment 11

[Jan de Mooij [:jandem]]

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 12

[Nicolas B. Pierron [:nbp]]

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.

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Wontfix for 53/54 as we are close to 54 release and it seems uncertain if 54 is even affected.

Comment 14

[Jan de Mooij [:jandem]]

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.

Comment 15

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 16

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c6ee0bd191b7ff7195870b0674d4574b2851b9f

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9c6ee0bd191b

Comment 18

[Ryan VanderMeulen [:RyanVM]]

This grafts cleanly to ESR52. Please nominate it for approval when you get a chance.

Comment 19

[Jan de Mooij [:jandem]]

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.

Comment 20

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.

Comment 21

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8873382 [details] [diff] [review]
Patch

Giving ESR52 approval since it has been sitting for five days.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/5a51a9ef8149