New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

September 2018 Security Update #5688

Merged
merged 10 commits into from Sep 11, 2018

fix issue where hoisted bound checks incorrectly calculated range of …

…indexes
  • Loading branch information...
MikeHolman committed Jul 18, 2018
commit f12d847c3beff4c2722a650fa9ac2418de41fb14
Copy path View file
@@ -709,7 +709,7 @@ class GlobOpt
void DetermineLoopCount(Loop *const loop);
void GenerateLoopCount(Loop *const loop, LoopCount *const loopCount);
void GenerateLoopCountPlusOne(Loop *const loop, LoopCount *const loopCount);
void GenerateSecondaryInductionVariableBound(Loop *const loop, StackSym *const inductionVariableSym, const LoopCount *const loopCount, const int maxMagnitudeChange, StackSym *const boundSym);
void GenerateSecondaryInductionVariableBound(Loop *const loop, StackSym *const inductionVariableSym, LoopCount *const loopCount, const int maxMagnitudeChange, const bool needsMagnitudeAdjustment, StackSym *const boundSym);

private:
void DetermineArrayBoundCheckHoistability(bool needLowerBoundCheck, bool needUpperBoundCheck, ArrayLowerBoundCheckHoistInfo &lowerHoistInfo, ArrayUpperBoundCheckHoistInfo &upperHoistInfo, const bool isJsArray, StackSym *const indexSym, Value *const indexValue, const IntConstantBounds &indexConstantBounds, StackSym *const headSegmentLengthSym, Value *const headSegmentLengthValue, const IntConstantBounds &headSegmentLengthConstantBounds, Loop *const headSegmentLengthInvariantLoop, bool &failedToUpdateCompatibleLowerBoundCheck, bool &failedToUpdateCompatibleUpperBoundCheck);
Copy path View file
@@ -902,30 +902,37 @@ void GlobOpt::ArraySrcOpt::DoLowerBoundCheck()
Assert(hoistInfo.Loop()->bailOutInfo);
globOpt->EnsureBailTarget(hoistInfo.Loop());

bool needsMagnitudeAdjustment = false;
if (hoistInfo.LoopCount())
{
// Generate the loop count and loop count based bound that will be used for the bound check
if (!hoistInfo.LoopCount()->HasBeenGenerated())
{
globOpt->GenerateLoopCount(hoistInfo.Loop(), hoistInfo.LoopCount());
}
needsMagnitudeAdjustment = (hoistInfo.MaxMagnitudeChange() > 0)
? (hoistInfo.IndexOffset() < hoistInfo.MaxMagnitudeChange())
: (hoistInfo.IndexOffset() > hoistInfo.MaxMagnitudeChange());

globOpt->GenerateSecondaryInductionVariableBound(
hoistInfo.Loop(),
indexVarSym->GetInt32EquivSym(nullptr),
hoistInfo.LoopCount(),
hoistInfo.MaxMagnitudeChange(),
needsMagnitudeAdjustment,
hoistInfo.IndexSym());
}

IR::Opnd* lowerBound = IR::IntConstOpnd::New(0, TyInt32, instr->m_func, true);
IR::Opnd* upperBound = IR::RegOpnd::New(indexIntSym, TyInt32, instr->m_func);
int offset = needsMagnitudeAdjustment ? (hoistInfo.IndexOffset() - hoistInfo.Offset()) : hoistInfo.Offset();
upperBound->SetIsJITOptimizedReg(true);

// 0 <= indexSym + offset (src1 <= src2 + dst)
IR::Instr *const boundCheck = globOpt->CreateBoundsCheckInstr(
lowerBound,
upperBound,
hoistInfo.Offset(),
offset,
hoistInfo.IsLoopCountBasedBound()
? IR::BailOutOnFailedHoistedLoopCountBasedBoundCheck
: IR::BailOutOnFailedHoistedBoundCheck,
@@ -1148,18 +1155,23 @@ void GlobOpt::ArraySrcOpt::DoUpperBoundCheck()
Assert(hoistInfo.Loop()->bailOutInfo);
globOpt->EnsureBailTarget(hoistInfo.Loop());

bool needsMagnitudeAdjustment = false;
if (hoistInfo.LoopCount())
{
// Generate the loop count and loop count based bound that will be used for the bound check
if (!hoistInfo.LoopCount()->HasBeenGenerated())
{
globOpt->GenerateLoopCount(hoistInfo.Loop(), hoistInfo.LoopCount());
}
needsMagnitudeAdjustment = (hoistInfo.MaxMagnitudeChange() > 0)
? (hoistInfo.IndexOffset() < hoistInfo.MaxMagnitudeChange())
: (hoistInfo.IndexOffset() > hoistInfo.MaxMagnitudeChange());
globOpt->GenerateSecondaryInductionVariableBound(
hoistInfo.Loop(),
indexVarSym->GetInt32EquivSym(nullptr),
hoistInfo.LoopCount(),
hoistInfo.MaxMagnitudeChange(),
needsMagnitudeAdjustment,
hoistInfo.IndexSym());
}

@@ -1174,11 +1186,13 @@ void GlobOpt::ArraySrcOpt::DoUpperBoundCheck()
IR::Opnd* upperBound = IR::RegOpnd::New(headSegmentLengthSym, headSegmentLengthSym->GetType(), instr->m_func);
upperBound->SetIsJITOptimizedReg(true);

int offset = needsMagnitudeAdjustment ? (hoistInfo.IndexOffset() + hoistInfo.Offset()) : hoistInfo.Offset();

// indexSym <= headSegmentLength + offset (src1 <= src2 + dst)
IR::Instr *const boundCheck = globOpt->CreateBoundsCheckInstr(
lowerBound,
upperBound,
hoistInfo.Offset(),
offset,
hoistInfo.IsLoopCountBasedBound()
? IR::BailOutOnFailedHoistedLoopCountBasedBoundCheck
: IR::BailOutOnFailedHoistedBoundCheck,
@@ -1197,7 +1211,7 @@ void GlobOpt::ArraySrcOpt::DoUpperBoundCheck()
landingPad->GetBlockNum(),
hoistInfo.IndexSym()->m_id,
headSegmentLengthSym->m_id,
hoistInfo.Offset());
offset);
}
else
{
@@ -1209,7 +1223,7 @@ void GlobOpt::ArraySrcOpt::DoUpperBoundCheck()
landingPad->GetBlockNum(),
hoistInfo.IndexConstantBounds().LowerBound(),
headSegmentLengthSym->m_id,
hoistInfo.Offset());
offset);
}

TESTTRACE_PHASE_INSTR(
@@ -78,6 +78,7 @@ void GlobOpt::ArrayLowerBoundCheckHoistInfo::SetLoop(
void GlobOpt::ArrayLowerBoundCheckHoistInfo::SetLoop(
::Loop *const loop,
StackSym *const indexSym,
const int indexOffset,
const int offset,
Value *const indexValue,
const IntConstantBounds &indexConstantBounds,
@@ -91,6 +92,7 @@ void GlobOpt::ArrayLowerBoundCheckHoistInfo::SetLoop(

this->loop = loop;
this->indexSym = indexSym;
this->indexOffset = indexOffset;
this->offset = offset;
this->indexValueNumber = indexValue->GetValueNumber();
this->indexValue = indexValue;
@@ -142,6 +144,7 @@ void GlobOpt::ArrayUpperBoundCheckHoistInfo::SetLoop(
void GlobOpt::ArrayUpperBoundCheckHoistInfo::SetLoop(
::Loop *const loop,
StackSym *const indexSym,
const int indexOffset,
const int offset,
Value *const indexValue,
const IntConstantBounds &indexConstantBounds,
@@ -151,7 +154,7 @@ void GlobOpt::ArrayUpperBoundCheckHoistInfo::SetLoop(
{
Assert(headSegmentLengthValue);

SetLoop(loop, indexSym, offset, indexValue, indexConstantBounds, isLoopCountBasedBound);
SetLoop(loop, indexSym, indexOffset, offset, indexValue, indexConstantBounds, isLoopCountBasedBound);
this->headSegmentLengthValue = headSegmentLengthValue;
this->headSegmentLengthConstantBounds = headSegmentLengthConstantBounds;
}
@@ -1831,8 +1834,9 @@ void GlobOpt::GenerateLoopCountPlusOne(Loop *const loop, LoopCount *const loopCo
void GlobOpt::GenerateSecondaryInductionVariableBound(
Loop *const loop,
StackSym *const inductionVariableSym,
const LoopCount *const loopCount,
LoopCount *const loopCount,
const int maxMagnitudeChange,
const bool needsMagnitudeAdjustment,
StackSym *const boundSym)
{
Assert(loop);
@@ -1857,18 +1861,33 @@ void GlobOpt::GenerateSecondaryInductionVariableBound(
Assert(insertBeforeInstr);
Func *const func = bailOutInfo->bailOutFunc;

StackSym* loopCountSym = nullptr;

// If indexOffset < maxMagnitudeChange, we need to account for the difference between them in the bound check
// i.e. BoundCheck: inductionVariable + loopCountMinusOne * maxMagnitudeChange + maxMagnitudeChange - indexOffset <= length - offset
// Since the BoundCheck instruction already deals with offset, we can simplify this to
// BoundCheck: inductionVariable + loopCount * maxMagnitudeChange <= length + indexOffset - offset
if (needsMagnitudeAdjustment)
{
GenerateLoopCountPlusOne(loop, loopCount);
loopCountSym = loopCount->LoopCountSym();
}
else
{
loopCountSym = loopCount->LoopCountMinusOneSym();
}
// intermediateValue = loopCount * maxMagnitudeChange
StackSym *intermediateValueSym;
if(maxMagnitudeChange == 1 || maxMagnitudeChange == -1)
{
intermediateValueSym = loopCount->LoopCountMinusOneSym();
intermediateValueSym = loopCountSym;
}
else
{
IR::BailOutInstr *const instr = IR::BailOutInstr::New(Js::OpCode::Mul_I4, bailOutKind, bailOutInfo, func);

instr->SetSrc1(
IR::RegOpnd::New(loopCount->LoopCountMinusOneSym(), loopCount->LoopCountMinusOneSym()->GetType(), func));
IR::RegOpnd::New(loopCountSym, loopCountSym->GetType(), func));
instr->GetSrc1()->SetIsJITOptimizedReg(true);

instr->SetSrc2(IR::IntConstOpnd::New(maxMagnitudeChange, TyInt32, func, true));
@@ -2418,6 +2437,7 @@ void GlobOpt::DetermineArrayBoundCheckHoistability(
loop,
indexSym,
lowerOffset,
lowerOffset,
landingPadIndexValue,
landingPadIndexConstantBounds);
}
@@ -2469,11 +2489,13 @@ void GlobOpt::DetermineArrayBoundCheckHoistability(
// Normalize the offset such that:
// boundBase <= headSegmentLength + offset
// Where (offset = -1 - boundOffset), and -1 is to simulate < instead of <=.
int indexOffset = upperOffset;
upperOffset = -1 - upperOffset;

upperHoistInfo.SetLoop(
loop,
indexSym,
indexOffset,
upperOffset,
landingPadIndexValue,
landingPadIndexConstantBounds,
@@ -2619,6 +2641,7 @@ void GlobOpt::DetermineArrayBoundCheckHoistability(
loop,
indexBoundBaseSym,
offset,
offset,
landingPadIndexBoundBaseValue,
landingPadIndexBoundBaseConstantBounds);
break;
@@ -2643,11 +2666,13 @@ void GlobOpt::DetermineArrayBoundCheckHoistability(
// Normalize the offset such that:
// boundBase <= headSegmentLength + offset
// Where (offset = -1 - boundOffset), and -1 is to simulate < instead of <=.
int indexOffset = offset;
offset = -1 - offset;

upperHoistInfo.SetLoop(
loop,
indexBoundBaseSym,
indexOffset,
offset,
landingPadIndexBoundBaseValue,
landingPadIndexBoundBaseConstantBounds,
@@ -3139,6 +3164,7 @@ void GlobOpt::DetermineArrayBoundCheckHoistability(
lowerHoistInfo.SetLoop(
currentLoop,
indexLoopCountBasedBoundBaseSym,
indexOffset,
offset,
indexLoopCountBasedBoundBaseValue,
indexLoopCountBasedBoundBaseConstantBounds,
@@ -3153,6 +3179,7 @@ void GlobOpt::DetermineArrayBoundCheckHoistability(
upperHoistInfo.SetLoop(
currentLoop,
indexLoopCountBasedBoundBaseSym,
indexOffset,
offset,
indexLoopCountBasedBoundBaseValue,
indexLoopCountBasedBoundBaseConstantBounds,
Copy path View file
@@ -240,6 +240,7 @@ class GlobOpt::ArrayLowerBoundCheckHoistInfo

// Info populated for a compatible bound check and for hoisting out of loop
StackSym *indexSym;
int indexOffset;
int offset;
ValueNumber indexValueNumber;

@@ -287,6 +288,12 @@ class GlobOpt::ArrayLowerBoundCheckHoistInfo
return offset;
}

int32 IndexOffset() const
{
Assert(HasAnyInfo());
return indexOffset;
}

void UpdateOffset(int newOffset)
{
Assert(HasAnyInfo());
@@ -334,7 +341,7 @@ class GlobOpt::ArrayLowerBoundCheckHoistInfo
public:
void SetCompatibleBoundCheck(BasicBlock *const compatibleBoundCheckBlock, StackSym *const indexSym, const int offset, const ValueNumber indexValueNumber);
void SetLoop(::Loop *const loop, const int indexConstantValue, const bool isLoopCountBasedBound = false);
void SetLoop(::Loop *const loop, StackSym *const indexSym, const int offset, Value *const indexValue, const IntConstantBounds &indexConstantBounds, const bool isLoopCountBasedBound = false);
void SetLoop(::Loop *const loop, StackSym *const indexSym, const int indexOffset, const int offset, Value *const indexValue, const IntConstantBounds &indexConstantBounds, const bool isLoopCountBasedBound = false);
void SetLoopCount(::LoopCount *const loopCount, const int maxMagnitudeChange);
};

@@ -353,6 +360,7 @@ class GlobOpt::ArrayUpperBoundCheckHoistInfo : protected ArrayLowerBoundCheckHoi
using Base::CompatibleBoundCheckBlock;
using Base::Loop;
using Base::IndexSym;
using Base::IndexOffset;
using Base::Offset;
using Base::UpdateOffset;
using Base::IndexValueNumber;
@@ -385,5 +393,5 @@ class GlobOpt::ArrayUpperBoundCheckHoistInfo : protected ArrayLowerBoundCheckHoi
public:
void SetCompatibleBoundCheck(BasicBlock *const compatibleBoundCheckBlock, const int indexConstantValue);
void SetLoop(::Loop *const loop, const int indexConstantValue, Value *const headSegmentLengthValue, const IntConstantBounds &headSegmentLengthConstantBounds, const bool isLoopCountBasedBound = false);
void SetLoop(::Loop *const loop, StackSym *const indexSym, const int offset, Value *const indexValue, const IntConstantBounds &indexConstantBounds, Value *const headSegmentLengthValue, const IntConstantBounds &headSegmentLengthConstantBounds, const bool isLoopCountBasedBound = false);
void SetLoop(::Loop *const loop, StackSym *const indexSym, const int indexOffset, const int offset, Value *const indexValue, const IntConstantBounds &indexConstantBounds, Value *const headSegmentLengthValue, const IntConstantBounds &headSegmentLengthConstantBounds, const bool isLoopCountBasedBound = false);
};
ProTip! Use n and p to navigate between commits in a pull request.