[1.11>master] [MERGE #6302 @pleath] ChakraCore servicing update for 1…

…9-10 Merge pull request #6302 from pleath:servicing/1910 Addresses the following issues: CVE-2019-1307 CVE-2019-1308 CVE-2019-1335 CVE-2019-1366
chakra-core · Oct 9, 2019 · f24008e · f24008e
2 parents 6b98ef8 + cc87151
commit f24008e
Show file tree

Hide file tree

Showing 7 changed files with 97 additions and 24 deletions.
diff --git a/lib/Backend/BackwardPass.cpp b/lib/Backend/BackwardPass.cpp
@@ -2261,7 +2261,7 @@ BackwardPass::DeadStoreTypeCheckBailOut(IR::Instr * instr)
 
     // By default, do not do this for stores, as it makes the presence of type checks unpredictable in the forward pass.
     // For instance, we can't predict which stores may cause reallocation of aux slots.
-    if (!PHASE_ON(Js::DeadStoreTypeChecksOnStoresPhase, this->func) && instr->GetDst() && instr->GetDst()->IsSymOpnd())
+    if (instr->GetDst() && instr->GetDst()->IsSymOpnd())
     {
         return;
     }

diff --git a/lib/Backend/GlobOpt.cpp b/lib/Backend/GlobOpt.cpp
@@ -2190,27 +2190,46 @@ GlobOpt::CollectMemOpInfo(IR::Instr *instrBegin, IR::Instr *instr, Value *src1Va
             return false;
         }
         break;
-    case Js::OpCode::Decr_A:
-        isIncr = false;
-    case Js::OpCode::Incr_A:
-        isChangedByOne = true;
-        goto MemOpCheckInductionVariable;
     case Js::OpCode::Sub_I4:
-    case Js::OpCode::Sub_A:
         isIncr = false;
-    case Js::OpCode::Add_A:
     case Js::OpCode::Add_I4:
     {
-MemOpCheckInductionVariable:
-        StackSym *sym = instr->GetSrc1()->GetStackSym();
-        if (!sym)
+        // The only case in which these OpCodes can contribute to an inductionVariableChangeInfo
+        // is when the induction variable is being modified and overwritten aswell (ex: j = j + 1)
+        // and not when the induction variable is modified but not overwritten (ex: k = j + 1).
+        // This can either be detected in IR as
+        // s1     = Add_I4 s1     1  // Case #1, can be seen with "j++".
+        // or as
+        // s4(s2) = Add_I4 s3(s1) 1  // Case #2, can be see with "j = j + 1".
+        // s1     = Ld_A   s2
+        bool isInductionVar = false;
+        IR::Instr* nextInstr = instr->m_next;
+        if (
+            // Checks for Case #1 and Case #2
+            instr->GetDst()->GetStackSym() != nullptr &&
+            instr->GetDst()->IsRegOpnd() &&
+            (
+                // Checks for Case #1
+                (instr->GetDst()->GetStackSym() == instr->GetSrc1()->GetStackSym()) ||
+
+                // Checks for Case #2
+                (nextInstr&& nextInstr->m_opcode == Js::OpCode::Ld_A &&
+                 nextInstr->GetSrc1()->IsRegOpnd() &&
+                 nextInstr->GetDst()->IsRegOpnd() &&
+                 GetVarSymID(instr->GetDst()->GetStackSym()) == nextInstr->GetSrc1()->GetStackSym()->m_id &&
+                 GetVarSymID(instr->GetSrc1()->GetStackSym()) == nextInstr->GetDst()->GetStackSym()->m_id)
+            )
+        )
         {
-            sym = instr->GetSrc2()->GetStackSym();
+            isInductionVar = true;
         }
+
+        // Even if dstIsInductionVar then dst == src1 so it's safe to use src1 as the induction sym always.
+        StackSym* sym = instr->GetSrc1()->GetStackSym();
 
         SymID inductionSymID = GetVarSymID(sym);
 
-        if (IsSymIDInductionVariable(inductionSymID, this->currentBlock->loop))
+        if (isInductionVar && IsSymIDInductionVariable(inductionSymID, this->currentBlock->loop))
         {
             if (!isChangedByOne)
             {
@@ -2290,7 +2309,6 @@ GlobOpt::CollectMemOpInfo(IR::Instr *instrBegin, IR::Instr *instr, Value *src1Va
                     {
                         inductionVariableChangeInfo.unroll++;
                     }
-
                     inductionVariableChangeInfo.isIncremental = isIncr;
                     loop->memOpInfo->inductionVariableChangeInfoMap->Item(inductionSymID, inductionVariableChangeInfo);
                     if (sym->m_id != inductionSymID)
@@ -2333,6 +2351,27 @@ GlobOpt::CollectMemOpInfo(IR::Instr *instrBegin, IR::Instr *instr, Value *src1Va
             }
         }
         NEXT_INSTR_IN_RANGE;
+        IR::Instr* prevInstr = instr->m_prev;
+
+        // If an instr where the dst is an induction variable (and thus is being written to) is not caught by a case in the above
+        // switch statement (which implies that this instr does not contributes to a inductionVariableChangeInfo) and in the default
+        // case does not set doMemOp to false (which implies that this instr does not invalidate this MemOp), then FailFast as we
+        // should not be performing a MemOp under these conditions. 
+        AssertOrFailFast(!instr->GetDst() || instr->m_opcode == Js::OpCode::IncrLoopBodyCount || !loop->memOpInfo ||
+
+            // Refer to "Case #2" described above in this function. For the following IR:
+            // Line #1: s4(s2) = Add_I4 s3(s1) 1
+            // Line #2: s3(s1) = Ld_A   s4(s2)
+            // do not consider line #2 as a violating instr
+            (instr->m_opcode == Js::OpCode::Ld_I4 &&
+                prevInstr && (prevInstr->m_opcode == Js::OpCode::Add_I4 || prevInstr->m_opcode == Js::OpCode::Sub_I4) &&
+                instr->GetSrc1()->IsRegOpnd() &&
+                instr->GetDst()->IsRegOpnd() &&
+                prevInstr->GetDst()->IsRegOpnd() &&
+                instr->GetDst()->GetStackSym() == prevInstr->GetSrc1()->GetStackSym() &&
+                instr->GetSrc1()->GetStackSym() == prevInstr->GetDst()->GetStackSym()) ||
+
+            !loop->memOpInfo->inductionVariableChangeInfoMap->ContainsKey(GetVarSymID(instr->GetDst()->GetStackSym())));
     }
 
     return true;
@@ -3619,7 +3658,7 @@ GlobOpt::OptSrc(IR::Opnd *opnd, IR::Instr * *pInstr, Value **indirIndexValRef, I
 
         opnd->SetValueType(valueType);
 
-        if(!IsLoopPrePass() && opnd->IsSymOpnd() && valueType.IsDefinite())
+        if(!IsLoopPrePass() && opnd->IsSymOpnd() && (valueType.IsDefinite() || valueType.IsNotTaggedValue()))
         {
             if (opnd->AsSymOpnd()->m_sym->IsPropertySym())
             {

diff --git a/lib/Backend/GlobOpt.h b/lib/Backend/GlobOpt.h
@@ -366,6 +366,7 @@ class JsArrayKills
             (valueType.IsArrayOrObjectWithArray() &&
              (
               (killsArraysWithNoMissingValues && valueType.HasNoMissingValues()) ||
+              (killsObjectArraysWithNoMissingValues && !valueType.IsArray() && valueType.HasNoMissingValues()) ||
               (killsNativeArrays && !valueType.HasVarElements())
              )
             );

diff --git a/lib/Backend/GlobOptFields.cpp b/lib/Backend/GlobOptFields.cpp
@@ -905,7 +905,7 @@ GlobOpt::FinishOptPropOp(IR::Instr *instr, IR::PropertySymOpnd *opnd, BasicBlock
 
         SymID opndId = opnd->HasObjectTypeSym() ? opnd->GetObjectTypeSym()->m_id : -1;
 
-        if (!isObjTypeChecked)
+        if (!isObjTypeSpecialized || opnd->IsBeingAdded())
         {
             if (block->globOptData.maybeWrittenTypeSyms == nullptr)
             {
@@ -1154,6 +1154,19 @@ GlobOpt::ProcessPropOpInTypeCheckSeq(IR::Instr* instr, IR::PropertySymOpnd *opnd
     Assert(opnd->IsTypeCheckSeqCandidate());
     Assert(opnd->HasObjectTypeSym());
 
+    if (opnd->HasTypeMismatch())
+    {
+        if (emitsTypeCheckOut != nullptr)
+        {
+            *emitsTypeCheckOut = false;
+        }
+        if (changesTypeValueOut != nullptr)
+        {
+            *changesTypeValueOut = false;
+        }
+        return false;
+    }
+
     bool isStore = opnd == instr->GetDst();
     bool isTypeDead = opnd->IsTypeDead();
     bool consumeType = makeChanges && !IsLoopPrePass();
@@ -1261,7 +1274,7 @@ GlobOpt::ProcessPropOpInTypeCheckSeq(IR::Instr* instr, IR::PropertySymOpnd *opnd
                 // a new type value here.
                 isSpecialized = false;
 
-                if (consumeType)
+                if (makeChanges)
                 {
                     opnd->SetTypeMismatch(true);
                 }
@@ -1305,7 +1318,7 @@ GlobOpt::ProcessPropOpInTypeCheckSeq(IR::Instr* instr, IR::PropertySymOpnd *opnd
                 // a new type value here.
                 isSpecialized = false;
 
-                if (consumeType)
+                if (makeChanges)
                 {
                     opnd->SetTypeMismatch(true);
                 }
@@ -1356,7 +1369,7 @@ GlobOpt::ProcessPropOpInTypeCheckSeq(IR::Instr* instr, IR::PropertySymOpnd *opnd
             {
                 // Indicates failure/mismatch
                 isSpecialized = false;
-                if (consumeType)
+                if (makeChanges)
                 {
                     opnd->SetTypeMismatch(true);
                 }
@@ -1456,7 +1469,7 @@ GlobOpt::ProcessPropOpInTypeCheckSeq(IR::Instr* instr, IR::PropertySymOpnd *opnd
             // a new type value here.
             isSpecialized = false;
 
-            if (consumeType)
+            if (makeChanges)
             {
                 opnd->SetTypeMismatch(true);
             }

diff --git a/lib/Backend/Lower.cpp b/lib/Backend/Lower.cpp
@@ -7427,9 +7427,6 @@ Lowerer::GenerateStFldWithCachedType(IR::Instr *instrStFld, bool* continueAsHelp
 
     if (hasTypeCheckBailout)
     {
-        AssertMsg(PHASE_ON1(Js::ObjTypeSpecIsolatedFldOpsWithBailOutPhase) || !PHASE_ON(Js::DeadStoreTypeChecksOnStoresPhase, this->m_func) || !propertySymOpnd->IsTypeDead() || propertySymOpnd->TypeCheckRequired(),
-            "Why does a field store have a type check bailout, if its type is dead?");
-
         if (instrStFld->GetBailOutInfo()->bailOutInstr != instrStFld)
         {
             // Set the cache index in the bailout info so that the generated code will write it into the
@@ -7489,7 +7486,7 @@ Lowerer::GenerateCachedTypeCheck(IR::Instr *instrChk, IR::PropertySymOpnd *prope
     // cache and no type check bailout. In the latter case, we can wind up doing expensive failed equivalence checks
     // repeatedly and never rejit.
     bool doEquivTypeCheck =
-        instrChk->HasEquivalentTypeCheckBailOut() ||
+        (instrChk->HasEquivalentTypeCheckBailOut() && (propertySymOpnd->TypeCheckRequired() || propertySymOpnd == instrChk->GetDst())) ||
         (propertySymOpnd->HasEquivalentTypeSet() &&
          !(propertySymOpnd->HasFinalType() && propertySymOpnd->HasInitialType()) &&
          !propertySymOpnd->MustDoMonoCheck() &&

diff --git a/test/fieldopts/OS23440664.js b/test/fieldopts/OS23440664.js
@@ -0,0 +1,17 @@
+//Reduced Switches: -printsystemexception -maxinterpretcount:1 -maxsimplejitruncount:1 -werexceptionsupport -oopjit- -bvt -off:bailonnoprofile -force:fixdataprops -forcejitloopbody 
+var shouldBailout = false;
+var IntArr0 = [];
+function test0() {
+  var loopInvariant = shouldBailout;
+  function makeArrayLength() {
+    return Math.floor();
+  }
+  makeArrayLength();
+  makeArrayLength();
+  prop0 = 1;
+  Object;
+  for (; shouldBailout ? (Object()) : (IntArr0[Object & 1] = '') ? Object : 0;) {
+  }
+}
+test0();
+WScript.Echo('pass');
diff --git a/test/fieldopts/rlexe.xml b/test/fieldopts/rlexe.xml
@@ -857,4 +857,10 @@
       <files>argobjlengthhoist.js</files>
     </default>
   </test>
+  <test>
+    <default>
+      <files>OS23440664.js</files>
+      <compile-flags>-off:bailonnoprofile -force:fixdataprops -forcejitloopbody</compile-flags>
+    </default>
+  </test>
 </regress-exe>