Skip to content

Commit cc87151

Browse files
committed
[MERGE #6302 @pleath] ChakraCore servicing update for 19-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
2 parents 7e9a2ee + 5989c6e commit cc87151

File tree

9 files changed

+105
-25
lines changed

9 files changed

+105
-25
lines changed

Build/NuGet/.pack-version

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
1.11.13
1+
1.11.14

lib/Backend/BackwardPass.cpp

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2195,6 +2195,13 @@ BackwardPass::DeadStoreTypeCheckBailOut(IR::Instr * instr)
21952195
return;
21962196
}
21972197

2198+
// By default, do not do this for stores, as it makes the presence of type checks unpredictable in the forward pass.
2199+
// For instance, we can't predict which stores may cause reallocation of aux slots.
2200+
if (instr->GetDst() && instr->GetDst()->IsSymOpnd())
2201+
{
2202+
return;
2203+
}
2204+
21982205
IR::BailOutKind oldBailOutKind = instr->GetBailOutKind();
21992206
if (!IR::IsTypeCheckBailOutKind(oldBailOutKind))
22002207
{

lib/Backend/GlobOpt.cpp

Lines changed: 53 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -2161,27 +2161,46 @@ GlobOpt::CollectMemOpInfo(IR::Instr *instrBegin, IR::Instr *instr, Value *src1Va
21612161
return false;
21622162
}
21632163
break;
2164-
case Js::OpCode::Decr_A:
2165-
isIncr = false;
2166-
case Js::OpCode::Incr_A:
2167-
isChangedByOne = true;
2168-
goto MemOpCheckInductionVariable;
21692164
case Js::OpCode::Sub_I4:
2170-
case Js::OpCode::Sub_A:
21712165
isIncr = false;
2172-
case Js::OpCode::Add_A:
21732166
case Js::OpCode::Add_I4:
21742167
{
2175-
MemOpCheckInductionVariable:
2176-
StackSym *sym = instr->GetSrc1()->GetStackSym();
2177-
if (!sym)
2168+
// The only case in which these OpCodes can contribute to an inductionVariableChangeInfo
2169+
// is when the induction variable is being modified and overwritten aswell (ex: j = j + 1)
2170+
// and not when the induction variable is modified but not overwritten (ex: k = j + 1).
2171+
// This can either be detected in IR as
2172+
// s1 = Add_I4 s1 1 // Case #1, can be seen with "j++".
2173+
// or as
2174+
// s4(s2) = Add_I4 s3(s1) 1 // Case #2, can be see with "j = j + 1".
2175+
// s1 = Ld_A s2
2176+
bool isInductionVar = false;
2177+
IR::Instr* nextInstr = instr->m_next;
2178+
if (
2179+
// Checks for Case #1 and Case #2
2180+
instr->GetDst()->GetStackSym() != nullptr &&
2181+
instr->GetDst()->IsRegOpnd() &&
2182+
(
2183+
// Checks for Case #1
2184+
(instr->GetDst()->GetStackSym() == instr->GetSrc1()->GetStackSym()) ||
2185+
2186+
// Checks for Case #2
2187+
(nextInstr&& nextInstr->m_opcode == Js::OpCode::Ld_A &&
2188+
nextInstr->GetSrc1()->IsRegOpnd() &&
2189+
nextInstr->GetDst()->IsRegOpnd() &&
2190+
GetVarSymID(instr->GetDst()->GetStackSym()) == nextInstr->GetSrc1()->GetStackSym()->m_id &&
2191+
GetVarSymID(instr->GetSrc1()->GetStackSym()) == nextInstr->GetDst()->GetStackSym()->m_id)
2192+
)
2193+
)
21782194
{
2179-
sym = instr->GetSrc2()->GetStackSym();
2195+
isInductionVar = true;
21802196
}
2197+
2198+
// Even if dstIsInductionVar then dst == src1 so it's safe to use src1 as the induction sym always.
2199+
StackSym* sym = instr->GetSrc1()->GetStackSym();
21812200

21822201
SymID inductionSymID = GetVarSymID(sym);
21832202

2184-
if (IsSymIDInductionVariable(inductionSymID, this->currentBlock->loop))
2203+
if (isInductionVar && IsSymIDInductionVariable(inductionSymID, this->currentBlock->loop))
21852204
{
21862205
if (!isChangedByOne)
21872206
{
@@ -2246,7 +2265,6 @@ GlobOpt::CollectMemOpInfo(IR::Instr *instrBegin, IR::Instr *instr, Value *src1Va
22462265
{
22472266
inductionVariableChangeInfo.unroll++;
22482267
}
2249-
22502268
inductionVariableChangeInfo.isIncremental = isIncr;
22512269
loop->memOpInfo->inductionVariableChangeInfoMap->Item(inductionSymID, inductionVariableChangeInfo);
22522270
}
@@ -2284,6 +2302,27 @@ GlobOpt::CollectMemOpInfo(IR::Instr *instrBegin, IR::Instr *instr, Value *src1Va
22842302
}
22852303
}
22862304
NEXT_INSTR_IN_RANGE;
2305+
IR::Instr* prevInstr = instr->m_prev;
2306+
2307+
// 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
2308+
// switch statement (which implies that this instr does not contributes to a inductionVariableChangeInfo) and in the default
2309+
// case does not set doMemOp to false (which implies that this instr does not invalidate this MemOp), then FailFast as we
2310+
// should not be performing a MemOp under these conditions.
2311+
AssertOrFailFast(!instr->GetDst() || instr->m_opcode == Js::OpCode::IncrLoopBodyCount || !loop->memOpInfo ||
2312+
2313+
// Refer to "Case #2" described above in this function. For the following IR:
2314+
// Line #1: s4(s2) = Add_I4 s3(s1) 1
2315+
// Line #2: s3(s1) = Ld_A s4(s2)
2316+
// do not consider line #2 as a violating instr
2317+
(instr->m_opcode == Js::OpCode::Ld_I4 &&
2318+
prevInstr && (prevInstr->m_opcode == Js::OpCode::Add_I4 || prevInstr->m_opcode == Js::OpCode::Sub_I4) &&
2319+
instr->GetSrc1()->IsRegOpnd() &&
2320+
instr->GetDst()->IsRegOpnd() &&
2321+
prevInstr->GetDst()->IsRegOpnd() &&
2322+
instr->GetDst()->GetStackSym() == prevInstr->GetSrc1()->GetStackSym() &&
2323+
instr->GetSrc1()->GetStackSym() == prevInstr->GetDst()->GetStackSym()) ||
2324+
2325+
!loop->memOpInfo->inductionVariableChangeInfoMap->ContainsKey(GetVarSymID(instr->GetDst()->GetStackSym())));
22872326
}
22882327

22892328
return true;
@@ -3564,7 +3603,7 @@ GlobOpt::OptSrc(IR::Opnd *opnd, IR::Instr * *pInstr, Value **indirIndexValRef, I
35643603

35653604
opnd->SetValueType(valueType);
35663605

3567-
if(!IsLoopPrePass() && opnd->IsSymOpnd() && valueType.IsDefinite())
3606+
if(!IsLoopPrePass() && opnd->IsSymOpnd() && (valueType.IsDefinite() || valueType.IsNotTaggedValue()))
35683607
{
35693608
if (opnd->AsSymOpnd()->m_sym->IsPropertySym())
35703609
{

lib/Backend/GlobOpt.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -370,6 +370,7 @@ class JsArrayKills
370370
(valueType.IsArrayOrObjectWithArray() &&
371371
(
372372
(killsArraysWithNoMissingValues && valueType.HasNoMissingValues()) ||
373+
(killsObjectArraysWithNoMissingValues && !valueType.IsArray() && valueType.HasNoMissingValues()) ||
373374
(killsNativeArrays && !valueType.HasVarElements())
374375
)
375376
);

lib/Backend/GlobOptFields.cpp

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -905,7 +905,7 @@ GlobOpt::FinishOptPropOp(IR::Instr *instr, IR::PropertySymOpnd *opnd, BasicBlock
905905

906906
SymID opndId = opnd->HasObjectTypeSym() ? opnd->GetObjectTypeSym()->m_id : -1;
907907

908-
if (!isObjTypeChecked)
908+
if (!isObjTypeSpecialized || opnd->IsBeingAdded())
909909
{
910910
if (block->globOptData.maybeWrittenTypeSyms == nullptr)
911911
{
@@ -1122,6 +1122,19 @@ GlobOpt::ProcessPropOpInTypeCheckSeq(IR::Instr* instr, IR::PropertySymOpnd *opnd
11221122
Assert(opnd->IsTypeCheckSeqCandidate());
11231123
Assert(opnd->HasObjectTypeSym());
11241124

1125+
if (opnd->HasTypeMismatch())
1126+
{
1127+
if (emitsTypeCheckOut != nullptr)
1128+
{
1129+
*emitsTypeCheckOut = false;
1130+
}
1131+
if (changesTypeValueOut != nullptr)
1132+
{
1133+
*changesTypeValueOut = false;
1134+
}
1135+
return false;
1136+
}
1137+
11251138
bool isStore = opnd == instr->GetDst();
11261139
bool isTypeDead = opnd->IsTypeDead();
11271140
bool consumeType = makeChanges && !IsLoopPrePass();
@@ -1229,7 +1242,7 @@ GlobOpt::ProcessPropOpInTypeCheckSeq(IR::Instr* instr, IR::PropertySymOpnd *opnd
12291242
// a new type value here.
12301243
isSpecialized = false;
12311244

1232-
if (consumeType)
1245+
if (makeChanges)
12331246
{
12341247
opnd->SetTypeMismatch(true);
12351248
}
@@ -1273,7 +1286,7 @@ GlobOpt::ProcessPropOpInTypeCheckSeq(IR::Instr* instr, IR::PropertySymOpnd *opnd
12731286
// a new type value here.
12741287
isSpecialized = false;
12751288

1276-
if (consumeType)
1289+
if (makeChanges)
12771290
{
12781291
opnd->SetTypeMismatch(true);
12791292
}
@@ -1324,7 +1337,7 @@ GlobOpt::ProcessPropOpInTypeCheckSeq(IR::Instr* instr, IR::PropertySymOpnd *opnd
13241337
{
13251338
// Indicates failure/mismatch
13261339
isSpecialized = false;
1327-
if (consumeType)
1340+
if (makeChanges)
13281341
{
13291342
opnd->SetTypeMismatch(true);
13301343
}
@@ -1423,7 +1436,7 @@ GlobOpt::ProcessPropOpInTypeCheckSeq(IR::Instr* instr, IR::PropertySymOpnd *opnd
14231436
// a new type value here.
14241437
isSpecialized = false;
14251438

1426-
if (consumeType)
1439+
if (makeChanges)
14271440
{
14281441
opnd->SetTypeMismatch(true);
14291442
}

lib/Backend/Lower.cpp

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7420,9 +7420,6 @@ Lowerer::GenerateStFldWithCachedType(IR::Instr *instrStFld, bool* continueAsHelp
74207420

74217421
if (hasTypeCheckBailout)
74227422
{
7423-
AssertMsg(PHASE_ON1(Js::ObjTypeSpecIsolatedFldOpsWithBailOutPhase) || !propertySymOpnd->IsTypeDead() || propertySymOpnd->TypeCheckRequired(),
7424-
"Why does a field store have a type check bailout, if its type is dead?");
7425-
74267423
if (instrStFld->GetBailOutInfo()->bailOutInstr != instrStFld)
74277424
{
74287425
// Set the cache index in the bailout info so that the generated code will write it into the
@@ -7482,7 +7479,7 @@ Lowerer::GenerateCachedTypeCheck(IR::Instr *instrChk, IR::PropertySymOpnd *prope
74827479
// cache and no type check bailout. In the latter case, we can wind up doing expensive failed equivalence checks
74837480
// repeatedly and never rejit.
74847481
bool doEquivTypeCheck =
7485-
(instrChk->HasEquivalentTypeCheckBailOut() && propertySymOpnd->TypeCheckRequired()) ||
7482+
(instrChk->HasEquivalentTypeCheckBailOut() && (propertySymOpnd->TypeCheckRequired() || propertySymOpnd == instrChk->GetDst())) ||
74867483
(propertySymOpnd->HasEquivalentTypeSet() &&
74877484
!(propertySymOpnd->HasFinalType() && propertySymOpnd->HasInitialType()) &&
74887485
!propertySymOpnd->MustDoMonoCheck() &&

lib/Common/ChakraCoreVersion.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
// ChakraCore version number definitions (used in ChakraCore binary metadata)
1818
#define CHAKRA_CORE_MAJOR_VERSION 1
1919
#define CHAKRA_CORE_MINOR_VERSION 11
20-
#define CHAKRA_CORE_PATCH_VERSION 13
20+
#define CHAKRA_CORE_PATCH_VERSION 14
2121
#define CHAKRA_CORE_VERSION_RELEASE_QFE 0 // Redundant with PATCH_VERSION. Keep this value set to 0.
2222

2323
// -------------

test/fieldopts/OS23440664.js

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
//Reduced Switches: -printsystemexception -maxinterpretcount:1 -maxsimplejitruncount:1 -werexceptionsupport -oopjit- -bvt -off:bailonnoprofile -force:fixdataprops -forcejitloopbody
2+
var shouldBailout = false;
3+
var IntArr0 = [];
4+
function test0() {
5+
var loopInvariant = shouldBailout;
6+
function makeArrayLength() {
7+
return Math.floor();
8+
}
9+
makeArrayLength();
10+
makeArrayLength();
11+
prop0 = 1;
12+
Object;
13+
for (; shouldBailout ? (Object()) : (IntArr0[Object & 1] = '') ? Object : 0;) {
14+
}
15+
}
16+
test0();
17+
WScript.Echo('pass');

test/fieldopts/rlexe.xml

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -852,4 +852,10 @@
852852
<files>argobjlengthhoist.js</files>
853853
</default>
854854
</test>
855+
<test>
856+
<default>
857+
<files>OS23440664.js</files>
858+
<compile-flags>-off:bailonnoprofile -force:fixdataprops -forcejitloopbody</compile-flags>
859+
</default>
860+
</test>
855861
</regress-exe>

0 commit comments

Comments
 (0)