From e6fea68c465d175a31445d31b6ff21f7ec9b7818 Mon Sep 17 00:00:00 2001 From: Ted Kremenek Date: Wed, 15 Jul 2009 02:31:43 +0000 Subject: [PATCH] More test cases revealed that the logic in StoreManager::InvalidateRegion() needs more finesse when handling the invalidation of pointers. Pointers that were invalidated as integers could later cause problems for clients using them as pointers. It is easier for us to model a symbolic value as a pointer rather than modeling a non-symbolic value as a pointer. This patch causes: - StoreManager::InvalidateRegion() to not used the casted type of a region if it would cause a pointer type to be invalidated as a non-pointer type. - Pushes RegionStore::RetrieveElement() further by handling retrievals from symbolic arrays that have been invalidated. This uses the new SymbolDerived construct that was recently introduced. The result is that the failing test in misc-ps-region-store-x86_64.m now passes. Both misc-ps-region-store-x86_64.m and misc-ps-region-store-i386.m contain a test case that motivated this change. llvm-svn: 75730 --- .../clang/Analysis/PathSensitive/MemRegion.h | 3 +- clang/lib/Analysis/RegionStore.cpp | 28 +++++++++++-- clang/lib/Analysis/Store.cpp | 16 +++++--- .../test/Analysis/misc-ps-region-store-i386.m | 35 +++++------------ .../Analysis/misc-ps-region-store-x86_64.m | 39 ++++++------------- clang/test/Analysis/misc-ps-region-store.m | 25 ++++++++++-- 6 files changed, 79 insertions(+), 67 deletions(-) diff --git a/clang/include/clang/Analysis/PathSensitive/MemRegion.h b/clang/include/clang/Analysis/PathSensitive/MemRegion.h index cc7c44c06a80..7f8c5c29037f 100644 --- a/clang/include/clang/Analysis/PathSensitive/MemRegion.h +++ b/clang/include/clang/Analysis/PathSensitive/MemRegion.h @@ -59,9 +59,10 @@ private: protected: MemRegion(Kind k) : kind(k) {} virtual ~MemRegion(); - ASTContext &getContext() const; public: + ASTContext &getContext() const; + virtual void Profile(llvm::FoldingSetNodeID& ID) const = 0; virtual MemRegionManager* getMemRegionManager() const = 0; diff --git a/clang/lib/Analysis/RegionStore.cpp b/clang/lib/Analysis/RegionStore.cpp index 4e83720f9fa1..b4eb4b8e194a 100644 --- a/clang/lib/Analysis/RegionStore.cpp +++ b/clang/lib/Analysis/RegionStore.cpp @@ -781,6 +781,16 @@ SVal RegionStoreManager::EvalBinOp(const GRState *state, // Loading values from regions. //===----------------------------------------------------------------------===// +static bool IsReinterpreted(QualType RTy, QualType UsedTy, ASTContext &Ctx) { + RTy = Ctx.getCanonicalType(RTy); + UsedTy = Ctx.getCanonicalType(UsedTy); + + if (RTy == UsedTy) + return false; + + return !(Loc::IsLocType(RTy) && Loc::IsLocType(UsedTy)); +} + SVal RegionStoreManager::Retrieve(const GRState *state, Loc L, QualType T) { assert(!isa(L) && "location unknown"); @@ -805,13 +815,14 @@ SVal RegionStoreManager::Retrieve(const GRState *state, Loc L, QualType T) { if (isa(MR)) { ASTContext &Ctx = getContext(); SVal idx = ValMgr.makeIntVal(0, Ctx.IntTy); + assert(!T.isNull()); MR = MRMgr.getElementRegion(T, idx, MR, Ctx); } // FIXME: Perhaps this method should just take a 'const MemRegion*' argument // instead of 'Loc', and have the other Loc cases handled at a higher level. const TypedRegion *R = cast(MR); - assert(R && "bad region"); + QualType RTy = R->getValueType(getContext()); // FIXME: We should eventually handle funny addressing. e.g.: // @@ -822,7 +833,13 @@ SVal RegionStoreManager::Retrieve(const GRState *state, Loc L, QualType T) { // // Such funny addressing will occur due to layering of regions. - QualType RTy = R->getValueType(getContext()); + ASTContext &Ctx = getContext(); + if (!T.isNull() && IsReinterpreted(RTy, T, Ctx)) { + SVal idx = ValMgr.makeIntVal(0, Ctx.IntTy); + R = MRMgr.getElementRegion(T, idx, R, Ctx); + RTy = T; + assert(RTy == R->getValueType(Ctx)); + } if (RTy->isStructureType()) return RetrieveStruct(state, R); @@ -929,8 +946,11 @@ SVal RegionStoreManager::RetrieveElement(const GRState* state, } // Check if the super region has a binding. - if (B.lookup(superR)) { - // We do not extract the bit value from super region for now. + if (const SVal *V = B.lookup(superR)) { + if (SymbolRef parentSym = V->getAsSymbol()) + return ValMgr.getDerivedRegionValueSymbolVal(parentSym, R); + + // Other cases: give up. return UnknownVal(); } diff --git a/clang/lib/Analysis/Store.cpp b/clang/lib/Analysis/Store.cpp index 2910f49c80c9..bbda565cec5b 100644 --- a/clang/lib/Analysis/Store.cpp +++ b/clang/lib/Analysis/Store.cpp @@ -238,16 +238,20 @@ const GRState *StoreManager::InvalidateRegion(const GRState *state, } const TypedRegion *TR = cast(R); + QualType T = TR->getValueType(Ctx); - QualType T; - - // If the region is cast to another type, use that type. + // If the region is cast to another type, use that type. if (const QualType *CastTy = getCastType(state, R)) { assert(!(*CastTy)->isObjCObjectPointerType()); - T = (*CastTy)->getAsPointerType()->getPointeeType(); - } else - T = TR->getValueType(Ctx); + QualType NewT = (*CastTy)->getAsPointerType()->getPointeeType(); + // The only exception is if the original region had a location type as its + // value type we always want to treat the region as binding to a location. + // This issue can arise when pointers are casted to integers and back. + if (!Loc::IsLocType(T) || Loc::IsLocType(NewT)) + T = NewT; + } + if (Loc::IsLocType(T) || (T->isIntegerType() && T->isScalarType())) { SVal V = ValMgr.getConjuredSymbolVal(E, T, Count); return Bind(state, ValMgr.makeLoc(TR), V); diff --git a/clang/test/Analysis/misc-ps-region-store-i386.m b/clang/test/Analysis/misc-ps-region-store-i386.m index f501dbe7ad3b..c2c4d5b9413c 100644 --- a/clang/test/Analysis/misc-ps-region-store-i386.m +++ b/clang/test/Analysis/misc-ps-region-store-i386.m @@ -1,29 +1,14 @@ // RUN: clang-cc -triple i386-apple-darwin9 -analyze -checker-cfref --analyzer-store=region --verify -fblocks %s -typedef struct _BStruct { void *grue; } BStruct; -void testB_aux(void *ptr); -void testB(BStruct *b) { - { - int *__gruep__ = ((int *)&((b)->grue)); - int __gruev__ = *__gruep__; - int __gruev2__ = *__gruep__; - if (__gruev__ != __gruev2__) { - int *p = 0; - *p = 0xDEADBEEF; - } - - testB_aux(__gruep__); - } - { - int *__gruep__ = ((int *)&((b)->grue)); - int __gruev__ = *__gruep__; - int __gruev2__ = *__gruep__; - if (__gruev__ != __gruev2__) { - int *p = 0; - *p = 0xDEADBEEF; - } - - if (~0 != __gruev__) {} - } +// Here is a case where a pointer is treated as integer, invalidated as an +// integer, and then used again as a pointer. This test just makes sure +// we don't crash. +typedef unsigned uintptr_t; +void test_pointer_invalidated_as_int_aux(uintptr_t* ptr); +void test_pointer_invalidated_as_int() { + void *x; + test_pointer_invalidated_as_int_aux((uintptr_t*) &x); + // Here we have a pointer to integer cast. + uintptr_t y = (uintptr_t) x; } diff --git a/clang/test/Analysis/misc-ps-region-store-x86_64.m b/clang/test/Analysis/misc-ps-region-store-x86_64.m index 2f74904d9c32..154ffaf3a003 100644 --- a/clang/test/Analysis/misc-ps-region-store-x86_64.m +++ b/clang/test/Analysis/misc-ps-region-store-x86_64.m @@ -1,31 +1,14 @@ // RUN: clang-cc -triple x86_64-apple-darwin9 -analyze -checker-cfref --analyzer-store=region --verify -fblocks %s -// This test case appears in misc-ps-region-store-i386.m, but fails under x86_64. -// The reason is that 'int' is smaller than a pointer on a 64-bit architecture, -// and we aren't reasoning yet about just the first 32-bits of the pointer. -typedef struct _BStruct { void *grue; } BStruct; -void testB_aux(void *ptr); -void testB(BStruct *b) { - { - int *__gruep__ = ((int *)&((b)->grue)); - int __gruev__ = *__gruep__; - int __gruev2__ = *__gruep__; - if (__gruev__ != __gruev2__) { - int *p = 0; - *p = 0xDEADBEEF; // no-warning - } - - testB_aux(__gruep__); - } - { - int *__gruep__ = ((int *)&((b)->grue)); - int __gruev__ = *__gruep__; - int __gruev2__ = *__gruep__; - if (__gruev__ != __gruev2__) { - int *p = 0; - *p = 0xDEADBEEF; // expected-warning{{null}} - } - - if (~0 != __gruev__) {} - } +// Here is a case where a pointer is treated as integer, invalidated as an +// integer, and then used again as a pointer. This test just makes sure +// we don't crash. +typedef unsigned long uintptr_t; +void test_pointer_invalidated_as_int_aux(uintptr_t* ptr); +void test_pointer_invalidated_as_int() { + void *x; + test_pointer_invalidated_as_int_aux((uintptr_t*) &x); + // Here we have a pointer to integer cast. + uintptr_t y = (uintptr_t) x; } + diff --git a/clang/test/Analysis/misc-ps-region-store.m b/clang/test/Analysis/misc-ps-region-store.m index 245273b22085..c5341a013888 100644 --- a/clang/test/Analysis/misc-ps-region-store.m +++ b/clang/test/Analysis/misc-ps-region-store.m @@ -89,9 +89,28 @@ typedef struct _BStruct { void *grue; } BStruct; void testB_aux(void *ptr); void testB(BStruct *b) { - // This case has moved to 'misc-ps-region-store-i386.m' and - // 'misc-ps-region-store-x86_64.m'. It succeeds under x86_64. When it - // passes it both, pull it in here. + { + int *__gruep__ = ((int *)&((b)->grue)); + int __gruev__ = *__gruep__; + int __gruev2__ = *__gruep__; + if (__gruev__ != __gruev2__) { + int *p = 0; + *p = 0xDEADBEEF; // no-warning + } + + testB_aux(__gruep__); + } + { + int *__gruep__ = ((int *)&((b)->grue)); + int __gruev__ = *__gruep__; + int __gruev2__ = *__gruep__; + if (__gruev__ != __gruev2__) { + int *p = 0; + *p = 0xDEADBEEF; // no-warning + } + + if (~0 != __gruev__) {} + } } void testB_2(BStruct *b) {