diff --git a/clang/include/clang/Analysis/PathSensitive/SValuator.h b/clang/include/clang/Analysis/PathSensitive/SValuator.h index 490c04e5978e..6bc63ac7c95d 100644 --- a/clang/include/clang/Analysis/PathSensitive/SValuator.h +++ b/clang/include/clang/Analysis/PathSensitive/SValuator.h @@ -31,9 +31,17 @@ public: SValuator(ValueManager &valMgr) : ValMgr(valMgr) {} virtual ~SValuator() {} - virtual SVal EvalCast(NonLoc val, QualType castTy) = 0; + virtual SVal EvalCastNL(NonLoc val, QualType castTy) = 0; - virtual SVal EvalCast(Loc val, QualType castTy) = 0; + virtual SVal EvalCastL(Loc val, QualType castTy) = 0; + + SVal EvalCast(SVal val, QualType castTy) { + if (val.isUnknownOrUndef()) + return val; + + return isa(val) ? EvalCastL(cast(val), castTy) + : EvalCastNL(cast(val), castTy); + } virtual SVal EvalMinus(NonLoc val) = 0; diff --git a/clang/lib/Analysis/MemRegion.cpp b/clang/lib/Analysis/MemRegion.cpp index 6a531b991795..9b3c36d02218 100644 --- a/clang/lib/Analysis/MemRegion.cpp +++ b/clang/lib/Analysis/MemRegion.cpp @@ -259,7 +259,8 @@ MemRegionManager::getCompoundLiteralRegion(const CompoundLiteralExpr* CL) { ElementRegion* MemRegionManager::getElementRegion(QualType elementType, SVal Idx, - const MemRegion* superRegion, ASTContext& Ctx){ + const MemRegion* superRegion, + ASTContext& Ctx){ QualType T = Ctx.getCanonicalType(elementType); diff --git a/clang/lib/Analysis/RegionStore.cpp b/clang/lib/Analysis/RegionStore.cpp index af731c78d24d..0d2467f55e0c 100644 --- a/clang/lib/Analysis/RegionStore.cpp +++ b/clang/lib/Analysis/RegionStore.cpp @@ -500,6 +500,9 @@ SVal RegionStoreManager::getLValueElement(const GRState *St, // Pointer of any type can be cast and used as array base. const ElementRegion *ElemR = dyn_cast(BaseRegion); + // Convert the offset to the appropriate size and signedness. + Offset = ValMgr.convertToArrayIndex(Offset); + if (!ElemR) { // // If the base region is not an ElementRegion, create one. @@ -510,16 +513,6 @@ SVal RegionStoreManager::getLValueElement(const GRState *St, // // Observe that 'p' binds to an AllocaRegion. // - - // Offset might be unsigned. We have to convert it to signed ConcreteInt. - if (nonloc::ConcreteInt* CI = dyn_cast(&Offset)) { - const llvm::APSInt& OffI = CI->getValue(); - if (OffI.isUnsigned()) { - llvm::APSInt Tmp = OffI; - Tmp.setIsSigned(true); - Offset = ValMgr.makeIntVal(Tmp); - } - } return loc::MemRegionVal(MRMgr.getElementRegion(elementType, Offset, BaseRegion, getContext())); } @@ -533,29 +526,11 @@ SVal RegionStoreManager::getLValueElement(const GRState *St, const llvm::APSInt& OffI = cast(Offset).getValue(); assert(BaseIdxI.isSigned()); - // FIXME: This appears to be the assumption of this code. We should review - // whether or not BaseIdxI.getBitWidth() < OffI.getBitWidth(). If it - // can't we need to put a comment here. If it can, we should handle it. - assert(BaseIdxI.getBitWidth() >= OffI.getBitWidth()); - - const MemRegion *ArrayR = ElemR->getSuperRegion(); - SVal NewIdx; + // Compute the new index. + SVal NewIdx = nonloc::ConcreteInt(getBasicVals().getValue(BaseIdxI + OffI)); - if (OffI.isUnsigned() || OffI.getBitWidth() < BaseIdxI.getBitWidth()) { - // 'Offset' might be unsigned. We have to convert it to signed and - // possibly extend it. - llvm::APSInt Tmp = OffI; - - if (OffI.getBitWidth() < BaseIdxI.getBitWidth()) - Tmp.extend(BaseIdxI.getBitWidth()); - - Tmp.setIsSigned(true); - Tmp += BaseIdxI; // Compute the new offset. - NewIdx = ValMgr.makeIntVal(Tmp); - } - else - NewIdx = nonloc::ConcreteInt(getBasicVals().getValue(BaseIdxI + OffI)); - + // Construct the new ElementRegion. + const MemRegion *ArrayR = ElemR->getSuperRegion(); return loc::MemRegionVal(MRMgr.getElementRegion(elementType, NewIdx, ArrayR, getContext())); } @@ -760,20 +735,13 @@ SVal RegionStoreManager::EvalBinOp(const GRState *state, // Only support concrete integer indexes for now. if (Base && Offset) { - // FIXME: For now, convert the signedness and bitwidth of offset in case - // they don't match. This can result from pointer arithmetic. In reality, - // we should figure out what are the proper semantics and implement them. - // - // This addresses the test case test/Analysis/ptr-arith.c - // - nonloc::ConcreteInt OffConverted(getBasicVals().Convert(Base->getValue(), - Offset->getValue())); - SVal NewIdx = Base->evalBinOp(ValMgr, Op, OffConverted); + // FIXME: Should use SValuator here. + SVal NewIdx = Base->evalBinOp(ValMgr, Op, + cast(ValMgr.convertToArrayIndex(*Offset))); const MemRegion* NewER = - MRMgr.getElementRegion(ER->getElementType(), NewIdx,ER->getSuperRegion(), - getContext()); + MRMgr.getElementRegion(ER->getElementType(), NewIdx, ER->getSuperRegion(), + getContext()); return ValMgr.makeLoc(NewER); - } return UnknownVal(); @@ -837,8 +805,8 @@ SVal RegionStoreManager::Retrieve(const GRState *state, Loc L, QualType T) { ASTContext &Ctx = getContext(); if (!T.isNull() && IsReinterpreted(RTy, T, Ctx)) { - SVal idx = ValMgr.makeIntVal(0, Ctx.IntTy); - R = MRMgr.getElementRegion(T, idx, R, Ctx); + SVal ZeroIdx = ValMgr.makeZeroArrayIndex(); + R = MRMgr.getElementRegion(T, ZeroIdx, R, Ctx); RTy = T; assert(Ctx.getCanonicalType(RTy) == Ctx.getCanonicalType(R->getValueType(Ctx))); @@ -1083,11 +1051,9 @@ SVal RegionStoreManager::RetrieveArray(const GRState *state, ConstantArrayType* CAT = cast(T.getTypePtr()); llvm::ImmutableList ArrayVal = getBasicVals().getEmptySValList(); - llvm::APSInt Size(CAT->getSize(), false); - llvm::APSInt i = getBasicVals().getZeroWithPtrWidth(false); - - for (; i < Size; ++i) { - SVal Idx = ValMgr.makeIntVal(i); + uint64_t size = CAT->getSize().getZExtValue(); + for (uint64_t i = 0; i < size; ++i) { + SVal Idx = ValMgr.makeArrayIndex(i); ElementRegion* ER = MRMgr.getElementRegion(CAT->getElementType(), Idx, R, getContext()); QualType ETy = ER->getElementType(); @@ -1160,15 +1126,14 @@ RegionStoreManager::BindCompoundLiteral(const GRState *state, } const GRState *RegionStoreManager::BindArray(const GRState *state, - const TypedRegion* R, + const TypedRegion* R, SVal Init) { QualType T = R->getValueType(getContext()); ConstantArrayType* CAT = cast(T.getTypePtr()); QualType ElementTy = CAT->getElementType(); - llvm::APSInt Size(CAT->getSize(), false); - llvm::APSInt i(llvm::APInt::getNullValue(Size.getBitWidth()), false); + uint64_t size = CAT->getSize().getZExtValue(); // Check if the init expr is a StringLiteral. if (isa(Init)) { @@ -1181,12 +1146,13 @@ const GRState *RegionStoreManager::BindArray(const GRState *state, // Copy bytes from the string literal into the target array. Trailing bytes // in the array that are not covered by the string literal are initialized // to zero. - for (; i < Size; ++i, ++j) { + for (uint64_t i = 0; i < size; ++i, ++j) { if (j >= len) break; - SVal Idx = ValMgr.makeIntVal(i); - ElementRegion* ER = MRMgr.getElementRegion(ElementTy, Idx,R,getContext()); + SVal Idx = ValMgr.makeArrayIndex(i); + ElementRegion* ER = MRMgr.getElementRegion(ElementTy, Idx, R, + getContext()); SVal V = ValMgr.makeIntVal(str[j], sizeof(char)*8, true); state = Bind(state, loc::MemRegionVal(ER), V); @@ -1197,13 +1163,14 @@ const GRState *RegionStoreManager::BindArray(const GRState *state, nonloc::CompoundVal& CV = cast(Init); nonloc::CompoundVal::iterator VI = CV.begin(), VE = CV.end(); - - for (; i < Size; ++i, ++VI) { + uint64_t i = 0; + + for (; i < size; ++i, ++VI) { // The init list might be shorter than the array length. if (VI == VE) break; - SVal Idx = ValMgr.makeIntVal(i); + SVal Idx = ValMgr.makeArrayIndex(i); ElementRegion* ER = MRMgr.getElementRegion(ElementTy, Idx, R, getContext()); if (CAT->getElementType()->isStructureType()) @@ -1214,7 +1181,7 @@ const GRState *RegionStoreManager::BindArray(const GRState *state, // If the init list is shorter than the array length, set the array default // value. - if (i < Size) { + if (i < size) { if (ElementTy->isIntegerType()) { SVal V = ValMgr.makeZeroVal(ElementTy); state = setDefaultValue(state, R, V); diff --git a/clang/lib/Analysis/SimpleSValuator.cpp b/clang/lib/Analysis/SimpleSValuator.cpp index eea50d13cf10..58b169658d01 100644 --- a/clang/lib/Analysis/SimpleSValuator.cpp +++ b/clang/lib/Analysis/SimpleSValuator.cpp @@ -23,8 +23,8 @@ public: SimpleSValuator(ValueManager &valMgr) : SValuator(valMgr) {} virtual ~SimpleSValuator() {} - virtual SVal EvalCast(NonLoc val, QualType castTy); - virtual SVal EvalCast(Loc val, QualType castTy); + virtual SVal EvalCastNL(NonLoc val, QualType castTy); + virtual SVal EvalCastL(Loc val, QualType castTy); virtual SVal EvalMinus(NonLoc val); virtual SVal EvalComplement(NonLoc val); virtual SVal EvalBinOpNN(BinaryOperator::Opcode op, NonLoc lhs, NonLoc rhs, @@ -44,7 +44,7 @@ SValuator *clang::CreateSimpleSValuator(ValueManager &valMgr) { // Transfer function for Casts. //===----------------------------------------------------------------------===// -SVal SimpleSValuator::EvalCast(NonLoc val, QualType castTy) { +SVal SimpleSValuator::EvalCastNL(NonLoc val, QualType castTy) { if (!isa(val)) return UnknownVal(); @@ -64,7 +64,7 @@ SVal SimpleSValuator::EvalCast(NonLoc val, QualType castTy) { return ValMgr.makeIntVal(i); } -SVal SimpleSValuator::EvalCast(Loc val, QualType castTy) { +SVal SimpleSValuator::EvalCastL(Loc val, QualType castTy) { // Casts from pointers -> pointers, just return the lval. // diff --git a/clang/test/Analysis/misc-ps.m b/clang/test/Analysis/misc-ps.m index 958576f4558d..503352b6f71d 100644 --- a/clang/test/Analysis/misc-ps.m +++ b/clang/test/Analysis/misc-ps.m @@ -415,3 +415,18 @@ void rdar_7062158_2() { *p = 0xDEADBEEF; // no-warning } +// This test reproduces a case for a crash when analyzing ClamAV using +// RegionStoreManager (the crash doesn't exhibit in BasicStoreManager because +// it isn't doing anything smart about arrays). The problem is that on the +// second line, 'p = &p[i]', p is assigned an ElementRegion whose index +// is a 16-bit integer. On the third line, a new ElementRegion is created +// based on the previous region, but there the region uses a 32-bit integer, +// resulting in a clash of values (an assertion failure at best). We resolve +// this problem by implicitly converting index values to 'int' when the +// ElementRegion is created. +unsigned char test_array_index_bitwidth(const unsigned char *p) { + unsigned short i = 0; + for (i = 0; i < 2; i++) p = &p[i]; + return p[i+1]; +} +