From f2edbec1d9817df109304f9c19ae2b34fec1feea Mon Sep 17 00:00:00 2001 From: Jordan Rose Date: Mon, 22 Apr 2013 21:36:49 +0000 Subject: [PATCH] [analyzer] Treat reinterpret_cast like a base cast in certain cases. The analyzer represents all pointer-to-pointer bitcasts the same way, but this can be problematic if an implicit base cast gets layered on top of a manual base cast (performed with reinterpret_cast instead of static_cast). Fix this (and avoid a valid assertion) by looking through cast regions. Using reinterpret_cast this way is only valid if the base class is at the same offset as the derived class; this is checked by -Wreinterpret-base-class. In the interest of performance, the analyzer doesn't repeat this check anywhere; it will just silently do the wrong thing (use the wrong offsets for fields of the base class) if the user code is wrong. PR15394 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@180052 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/StaticAnalyzer/Core/Store.cpp | 88 +++++++++++++++++++------------ test/Analysis/derived-to-base.cpp | 87 ++++++++++++++++++++++++++++++ 2 files changed, 141 insertions(+), 34 deletions(-) diff --git a/lib/StaticAnalyzer/Core/Store.cpp b/lib/StaticAnalyzer/Core/Store.cpp index a0c24fedcf..690ed08ffc 100644 --- a/lib/StaticAnalyzer/Core/Store.cpp +++ b/lib/StaticAnalyzer/Core/Store.cpp @@ -289,62 +289,82 @@ SVal StoreManager::evalDerivedToBase(SVal Derived, QualType BaseType, return loc::MemRegionVal(BaseReg); } -SVal StoreManager::evalDynamicCast(SVal Base, QualType DerivedType, +/// Returns the static type of the given region, if it represents a C++ class +/// object. +/// +/// This handles both fully-typed regions, where the dynamic type is known, and +/// symbolic regions, where the dynamic type is merely bounded (and even then, +/// only ostensibly!), but does not take advantage of any dynamic type info. +static const CXXRecordDecl *getCXXRecordType(const MemRegion *MR) { + if (const TypedValueRegion *TVR = dyn_cast(MR)) + return TVR->getValueType()->getAsCXXRecordDecl(); + if (const SymbolicRegion *SR = dyn_cast(MR)) + return SR->getSymbol()->getType()->getPointeeCXXRecordDecl(); + return 0; +} + +SVal StoreManager::evalDynamicCast(SVal Base, QualType TargetType, bool &Failed) { Failed = false; - Optional BaseRegVal = Base.getAs(); - if (!BaseRegVal) + const MemRegion *MR = Base.getAsRegion(); + if (!MR) return UnknownVal(); - const MemRegion *BaseRegion = BaseRegVal->stripCasts(/*StripBases=*/false); // Assume the derived class is a pointer or a reference to a CXX record. - DerivedType = DerivedType->getPointeeType(); - assert(!DerivedType.isNull()); - const CXXRecordDecl *DerivedDecl = DerivedType->getAsCXXRecordDecl(); - if (!DerivedDecl && !DerivedType->isVoidType()) + TargetType = TargetType->getPointeeType(); + assert(!TargetType.isNull()); + const CXXRecordDecl *TargetClass = TargetType->getAsCXXRecordDecl(); + if (!TargetClass && !TargetType->isVoidType()) return UnknownVal(); // Drill down the CXXBaseObject chains, which represent upcasts (casts from // derived to base). - const MemRegion *SR = BaseRegion; - while (const TypedRegion *TSR = dyn_cast_or_null(SR)) { - QualType BaseType = TSR->getLocationType()->getPointeeType(); - assert(!BaseType.isNull()); - const CXXRecordDecl *SRDecl = BaseType->getAsCXXRecordDecl(); - if (!SRDecl) - return UnknownVal(); - + while (const CXXRecordDecl *MRClass = getCXXRecordType(MR)) { // If found the derived class, the cast succeeds. - if (SRDecl == DerivedDecl) - return loc::MemRegionVal(TSR); + if (MRClass == TargetClass) + return loc::MemRegionVal(MR); - if (!DerivedType->isVoidType()) { + if (!TargetType->isVoidType()) { // Static upcasts are marked as DerivedToBase casts by Sema, so this will // only happen when multiple or virtual inheritance is involved. CXXBasePaths Paths(/*FindAmbiguities=*/false, /*RecordPaths=*/true, /*DetectVirtual=*/false); - if (SRDecl->isDerivedFrom(DerivedDecl, Paths)) - return evalDerivedToBase(loc::MemRegionVal(TSR), Paths.front()); + if (MRClass->isDerivedFrom(TargetClass, Paths)) + return evalDerivedToBase(loc::MemRegionVal(MR), Paths.front()); } - if (const CXXBaseObjectRegion *R = dyn_cast(TSR)) + if (const CXXBaseObjectRegion *BaseR = dyn_cast(MR)) { // Drill down the chain to get the derived classes. - SR = R->getSuperRegion(); - else { - // We reached the bottom of the hierarchy. - - // If this is a cast to void*, return the region. - if (DerivedType->isVoidType()) - return loc::MemRegionVal(TSR); + MR = BaseR->getSuperRegion(); + continue; + } - // We did not find the derived class. We we must be casting the base to - // derived, so the cast should fail. - Failed = true; - return UnknownVal(); + // If this is a cast to void*, return the region. + if (TargetType->isVoidType()) + return loc::MemRegionVal(MR); + + // Strange use of reinterpret_cast can give us paths we don't reason + // about well, by putting in ElementRegions where we'd expect + // CXXBaseObjectRegions. If it's a valid reinterpret_cast (i.e. if the + // derived class has a zero offset from the base class), then it's safe + // to strip the cast; if it's invalid, -Wreinterpret-base-class should + // catch it. In the interest of performance, the analyzer will silently + // do the wrong thing in the invalid case (because offsets for subregions + // will be wrong). + const MemRegion *Uncasted = MR->StripCasts(/*IncludeBaseCasts=*/false); + if (Uncasted == MR) { + // We reached the bottom of the hierarchy and did not find the derived + // class. We we must be casting the base to derived, so the cast should + // fail. + break; } + + MR = Uncasted; } - + + // We failed if the region we ended up with has perfect type info. + Failed = isa(MR); return UnknownVal(); } diff --git a/test/Analysis/derived-to-base.cpp b/test/Analysis/derived-to-base.cpp index b846d2c28b..0664189a95 100644 --- a/test/Analysis/derived-to-base.cpp +++ b/test/Analysis/derived-to-base.cpp @@ -2,6 +2,7 @@ // RUN: %clang_cc1 -analyze -analyzer-checker=core,debug.ExprInspection -DCONSTRUCTORS=1 -analyzer-config c++-inlining=constructors -verify %s void clang_analyzer_eval(bool); +void clang_analyzer_checkInlined(bool); class A { protected: @@ -363,3 +364,89 @@ namespace Redeclaration { } }; +namespace PR15394 { + namespace Original { + class Base { + public: + virtual int f() = 0; + int i; + }; + + class Derived1 : public Base { + public: + int j; + }; + + class Derived2 : public Derived1 { + public: + virtual int f() { + clang_analyzer_checkInlined(true); // expected-warning{{TRUE}} + return i + j; + } + }; + + void testXXX() { + Derived1 *d1p = reinterpret_cast(new Derived2); + d1p->i = 1; + d1p->j = 2; + clang_analyzer_eval(d1p->f() == 3); // expected-warning{{TRUE}} + } + } + + namespace VirtualInDerived { + class Base { + public: + int i; + }; + + class Derived1 : public Base { + public: + virtual int f() = 0; + int j; + }; + + class Derived2 : public Derived1 { + public: + virtual int f() { + clang_analyzer_checkInlined(true); // expected-warning{{TRUE}} + return i + j; + } + }; + + void test() { + Derived1 *d1p = reinterpret_cast(new Derived2); + d1p->i = 1; + d1p->j = 2; + clang_analyzer_eval(d1p->f() == 3); // expected-warning{{TRUE}} + } + } + + namespace NoCast { + class Base { + public: + int i; + }; + + class Derived1 : public Base { + public: + virtual int f() = 0; + int j; + }; + + class Derived2 : public Derived1 { + public: + virtual int f() { + clang_analyzer_checkInlined(true); // expected-warning{{TRUE}} + return i + j; + } + }; + + void test() { + Derived1 *d1p = new Derived2; + d1p->i = 1; + d1p->j = 2; + clang_analyzer_eval(d1p->f() == 3); // expected-warning{{TRUE}} + } + } +}; + -- 2.40.0