From: Jordy Rose Date: Mon, 16 Aug 2010 01:15:17 +0000 (+0000) Subject: - Allow making ElementRegions with complex offsets (expressions or symbols) for the... X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=e701117b21356d3c60133315b5bdd50232ec6cca;p=clang - Allow making ElementRegions with complex offsets (expressions or symbols) for the purpose of bounds-checking. - Rewrite GRState::AssumeInBound to actually do that checking, and to use the normal constraint path. - Remove ConstraintManager::AssumeInBound. - Teach RegionStore and FlatStore to ignore those regions for now. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@111116 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/Checker/PathSensitive/ConstraintManager.h b/include/clang/Checker/PathSensitive/ConstraintManager.h index ce7d1b3817..97535f55bf 100644 --- a/include/clang/Checker/PathSensitive/ConstraintManager.h +++ b/include/clang/Checker/PathSensitive/ConstraintManager.h @@ -34,9 +34,6 @@ public: virtual const GRState *Assume(const GRState *state, DefinedSVal Cond, bool Assumption) = 0; - virtual const GRState *AssumeInBound(const GRState *state, DefinedSVal Idx, - DefinedSVal UpperBound, bool Assumption) = 0; - std::pair AssumeDual(const GRState *state, DefinedSVal Cond) { return std::make_pair(Assume(state, Cond, true), diff --git a/include/clang/Checker/PathSensitive/GRState.h b/include/clang/Checker/PathSensitive/GRState.h index 36a9c8ce19..141ccece26 100644 --- a/include/clang/Checker/PathSensitive/GRState.h +++ b/include/clang/Checker/PathSensitive/GRState.h @@ -618,9 +618,42 @@ inline const GRState *GRState::AssumeInBound(DefinedOrUnknownSVal Idx, if (Idx.isUnknown() || UpperBound.isUnknown()) return this; - ConstraintManager &CM = *getStateManager().ConstraintMgr; - return CM.AssumeInBound(this, cast(Idx), - cast(UpperBound), Assumption); + // Build an expression for 0 <= Idx < UpperBound. + // This is the same as Idx + MIN < UpperBound + MIN, if overflow is allowed. + // FIXME: This should probably be part of SValuator. + GRStateManager &SM = getStateManager(); + ValueManager &VM = SM.getValueManager(); + SValuator &SV = VM.getSValuator(); + ASTContext &Ctx = VM.getContext(); + + // Get the offset: the minimum value of the array index type. + BasicValueFactory &BVF = VM.getBasicValueFactory(); + // FIXME: This should be using ValueManager::ArrayIndexTy...somehow. + QualType IndexTy = Ctx.IntTy; + nonloc::ConcreteInt Min = BVF.getMinValue(IndexTy); + + // Adjust the index. + SVal NewIdx = SV.EvalBinOpNN(this, BinaryOperator::Add, + cast(Idx), Min, IndexTy); + if (NewIdx.isUnknownOrUndef()) + return this; + + // Adjust the upper bound. + SVal NewBound = SV.EvalBinOpNN(this, BinaryOperator::Add, + cast(UpperBound), Min, IndexTy); + if (NewBound.isUnknownOrUndef()) + return this; + + // Build the actual comparison. + SVal InBound = SV.EvalBinOpNN(this, BinaryOperator::LT, + cast(NewIdx), cast(NewBound), + Ctx.IntTy); + if (InBound.isUnknownOrUndef()) + return this; + + // Finally, let the constraint manager take care of it. + ConstraintManager &CM = SM.getConstraintManager(); + return CM.Assume(this, cast(InBound), Assumption); } inline const GRState *GRState::bindLoc(SVal LV, SVal V) const { diff --git a/lib/Checker/FlatStore.cpp b/lib/Checker/FlatStore.cpp index 7c986a71df..21fa422166 100644 --- a/lib/Checker/FlatStore.cpp +++ b/lib/Checker/FlatStore.cpp @@ -90,8 +90,9 @@ StoreManager *clang::CreateFlatStoreManager(GRStateManager &StMgr) { SVal FlatStoreManager::Retrieve(Store store, Loc L, QualType T) { const MemRegion *R = cast(L).getRegion(); RegionInterval RI = RegionToInterval(R); - - assert(RI.R && "should handle regions with unknown interval"); + // FIXME: FlatStore should handle regions with unknown intervals. + if (!RI.R) + return UnknownVal(); RegionBindings B = getRegionBindings(store); const BindingVal *BV = B.lookup(RI.R); @@ -123,7 +124,9 @@ Store FlatStoreManager::Bind(Store store, Loc L, SVal val) { BV = *V; RegionInterval RI = RegionToInterval(R); - assert(RI.R && "should handle regions with unknown interval"); + // FIXME: FlatStore should handle regions with unknown intervals. + if (!RI.R) + return B.getRoot(); BV = BVFactory.Add(BV, RI.I, val); B = RBFactory.Add(B, RI.R, BV); return B.getRoot(); diff --git a/lib/Checker/RegionStore.cpp b/lib/Checker/RegionStore.cpp index b6ea696c4e..1c74c3f3a3 100644 --- a/lib/Checker/RegionStore.cpp +++ b/lib/Checker/RegionStore.cpp @@ -44,7 +44,7 @@ private: uint64_t Offset; explicit BindingKey(const MemRegion *r, uint64_t offset, Kind k) - : P(r, (unsigned) k), Offset(offset) { assert(r); } + : P(r, (unsigned) k), Offset(offset) {} public: bool isDefault() const { return P.getInt() == Default; } @@ -72,6 +72,10 @@ public: return P.getOpaqueValue() == X.P.getOpaqueValue() && Offset == X.Offset; } + + operator bool() const { + return getRegion() != NULL; + } }; } // end anonymous namespace @@ -1604,17 +1608,18 @@ BindingKey BindingKey::Make(const MemRegion *R, Kind k) { if (const ElementRegion *ER = dyn_cast(R)) { const RegionRawOffset &O = ER->getAsArrayOffset(); - if (O.getRegion()) - return BindingKey(O.getRegion(), O.getByteOffset(), k); - // FIXME: There are some ElementRegions for which we cannot compute - // raw offsets yet, including regions with symbolic offsets. + // raw offsets yet, including regions with symbolic offsets. These will be + // ignored by the store. + return BindingKey(O.getRegion(), O.getByteOffset(), k); } return BindingKey(R, 0, k); } RegionBindings RegionStoreManager::Add(RegionBindings B, BindingKey K, SVal V) { + if (!K) + return B; return RBFactory.Add(B, K, V); } @@ -1624,6 +1629,8 @@ RegionBindings RegionStoreManager::Add(RegionBindings B, const MemRegion *R, } const SVal *RegionStoreManager::Lookup(RegionBindings B, BindingKey K) { + if (!K) + return NULL; return B.lookup(K); } @@ -1634,6 +1641,8 @@ const SVal *RegionStoreManager::Lookup(RegionBindings B, } RegionBindings RegionStoreManager::Remove(RegionBindings B, BindingKey K) { + if (!K) + return B; return RBFactory.Remove(B, K); } diff --git a/lib/Checker/SimpleConstraintManager.cpp b/lib/Checker/SimpleConstraintManager.cpp index 321381b045..cc26a12ea4 100644 --- a/lib/Checker/SimpleConstraintManager.cpp +++ b/lib/Checker/SimpleConstraintManager.cpp @@ -296,28 +296,4 @@ const GRState *SimpleConstraintManager::AssumeSymRel(const GRState *state, } // end switch } -const GRState *SimpleConstraintManager::AssumeInBound(const GRState *state, - DefinedSVal Idx, - DefinedSVal UpperBound, - bool Assumption) { - - // Only support ConcreteInt for now. - if (!(isa(Idx) && isa(UpperBound))) - return state; - - const llvm::APSInt& Zero = state->getBasicVals().getZeroWithPtrWidth(false); - llvm::APSInt IdxV = cast(Idx).getValue(); - // IdxV might be too narrow. - if (IdxV.getBitWidth() < Zero.getBitWidth()) - IdxV.extend(Zero.getBitWidth()); - // UBV might be too narrow, too. - llvm::APSInt UBV = cast(UpperBound).getValue(); - if (UBV.getBitWidth() < Zero.getBitWidth()) - UBV.extend(Zero.getBitWidth()); - - bool InBound = (Zero <= IdxV) && (IdxV < UBV); - bool isFeasible = Assumption ? InBound : !InBound; - return isFeasible ? state : NULL; -} - } // end of namespace clang diff --git a/lib/Checker/SimpleConstraintManager.h b/lib/Checker/SimpleConstraintManager.h index 45057e64f3..96811b3e36 100644 --- a/lib/Checker/SimpleConstraintManager.h +++ b/lib/Checker/SimpleConstraintManager.h @@ -43,10 +43,6 @@ public: BinaryOperator::Opcode op, const llvm::APSInt& Int); - const GRState *AssumeInBound(const GRState *state, DefinedSVal Idx, - DefinedSVal UpperBound, - bool Assumption); - protected: //===------------------------------------------------------------------===// diff --git a/lib/Checker/Store.cpp b/lib/Checker/Store.cpp index e0e2c3ad7d..7c80eed0ea 100644 --- a/lib/Checker/Store.cpp +++ b/lib/Checker/Store.cpp @@ -284,10 +284,6 @@ SVal StoreManager::getLValueElement(QualType elementType, SVal Offset, if (Base.isUnknownOrUndef() || isa(Base)) return Base; - // Only handle integer offsets... for now. - if (!isa(Offset)) - return UnknownVal(); - const MemRegion* BaseRegion = cast(Base).getRegion(); // Pointer of any type can be cast and used as array base. @@ -316,6 +312,19 @@ SVal StoreManager::getLValueElement(QualType elementType, SVal Offset, return UnknownVal(); const llvm::APSInt& BaseIdxI = cast(BaseIdx).getValue(); + + // Only allow non-integer offsets if the base region has no offset itself. + // FIXME: This is a somewhat arbitrary restriction. We should be using + // SValuator here to add the two offsets without checking their types. + if (!isa(Offset)) { + if (isa(BaseRegion->StripCasts())) + return UnknownVal(); + + return loc::MemRegionVal(MRMgr.getElementRegion(elementType, Offset, + ElemR->getSuperRegion(), + Ctx)); + } + const llvm::APSInt& OffI = cast(Offset).getValue(); assert(BaseIdxI.isSigned()); diff --git a/test/Analysis/outofbound.c b/test/Analysis/outofbound.c index 529f0e77b2..ed51dc6ac0 100644 --- a/test/Analysis/outofbound.c +++ b/test/Analysis/outofbound.c @@ -79,3 +79,19 @@ void alloca_region(int a) { x[5] = 5; // expected-warning{{out-of-bound}} } } + +int symbolic_index(int a) { + int x[2] = {1, 2}; + if (a == 2) { + return x[a]; // expected-warning{{out-of-bound}} + } + return 0; +} + +int symbolic_index2(int a) { + int x[2] = {1, 2}; + if (a < 0) { + return x[a]; // expected-warning{{out-of-bound}} + } + return 0; +}