CVE-2019-1335

chakra-core · Oct 7, 2019 · a4e5654 · a4e5654
1 parent 64376de
commit a4e5654
Showing 1 changed file with 52 additions and 13 deletions.
diff --git a/lib/Backend/GlobOpt.cpp b/lib/Backend/GlobOpt.cpp
@@ -2161,27 +2161,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)
             {
@@ -2246,7 +2265,6 @@ GlobOpt::CollectMemOpInfo(IR::Instr *instrBegin, IR::Instr *instr, Value *src1Va
                     {
                         inductionVariableChangeInfo.unroll++;
                     }
-
                     inductionVariableChangeInfo.isIncremental = isIncr;
                     loop->memOpInfo->inductionVariableChangeInfoMap->Item(inductionSymID, inductionVariableChangeInfo);
                 }
@@ -2284,6 +2302,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;