[analyzer] Model base to derived casts more precisely.

Xazax-hun · Xazax-hun · commit 44583ce65a81 · 2016-08-08T09:22:59.000Z
Dynamic casts are handled relatively well by the static analyzer. BaseToDerived casts however are treated conservatively. This can cause some false positives with the NewDeleteLeaks checker. This patch alters the behavior of BaseToDerived casts. In case a dynamic cast would succeed use the same semantics. Otherwise fall back to the conservative approach. Differential Revision: https://reviews.llvm.org/D23014 llvm-svn: 277989
diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
@@ -123,15 +123,18 @@ class StoreManager {
   SVal evalDerivedToBase(SVal Derived, QualType DerivedPtrType,
                          bool IsVirtual);
 
-  /// \brief Evaluates C++ dynamic_cast cast.
+  /// \brief Attempts to do a down cast. Used to model BaseToDerived and C++
+  ///        dynamic_cast.
   /// The callback may result in the following 3 scenarios:
   ///  - Successful cast (ex: derived is subclass of base).
   ///  - Failed cast (ex: derived is definitely not a subclass of base).
+  ///    The distinction of this case from the next one is necessary to model
+  ///    dynamic_cast. 
   ///  - We don't know (base is a symbolic region and we don't have 
   ///    enough info to determine if the cast will succeed at run time).
   /// The function returns an SVal representing the derived class; it's
   /// valid only if Failed flag is set to false.
-  SVal evalDynamicCast(SVal Base, QualType DerivedPtrType, bool &Failed);
+  SVal attemptDownCast(SVal Base, QualType DerivedPtrType, bool &Failed);
 
   const ElementRegion *GetElementZeroRegion(const MemRegion *R, QualType T);
 
diff --git a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -552,7 +552,7 @@ void CXXInstanceCall::getInitialStackFrameContents(
 
       // FIXME: CallEvent maybe shouldn't be directly accessing StoreManager.
       bool Failed;
-      ThisVal = StateMgr.getStoreManager().evalDynamicCast(ThisVal, Ty, Failed);
+      ThisVal = StateMgr.getStoreManager().attemptDownCast(ThisVal, Ty, Failed);
       assert(!Failed && "Calling an incorrectly devirtualized method");
     }
 
diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -386,7 +386,7 @@ void ExprEngine::VisitCast(const CastExpr *CastE, const Expr *Ex,
           Failed = true;
         // Else, evaluate the cast.
         else
-          val = getStoreManager().evalDynamicCast(val, T, Failed);
+          val = getStoreManager().attemptDownCast(val, T, Failed);
 
         if (Failed) {
           if (T->isReferenceType()) {
@@ -412,6 +412,28 @@ void ExprEngine::VisitCast(const CastExpr *CastE, const Expr *Ex,
         Bldr.generateNode(CastE, Pred, state);
         continue;
       }
+      case CK_BaseToDerived: {
+        SVal val = state->getSVal(Ex, LCtx);
+        QualType resultType = CastE->getType();
+        if (CastE->isGLValue())
+          resultType = getContext().getPointerType(resultType);
+
+        bool Failed = false;
+
+        if (!val.isConstant()) {
+          val = getStoreManager().attemptDownCast(val, T, Failed);
+        }
+
+        // Failed to cast or the result is unknown, fall back to conservative.
+        if (Failed || val.isUnknown()) {
+          val =
+            svalBuilder.conjureSymbolVal(nullptr, CastE, LCtx, resultType,
+                                         currBldrCtx->blockCount());
+        }
+        state = state->BindExpr(CastE, LCtx, val);
+        Bldr.generateNode(CastE, Pred, state);
+        continue;
+      }
       case CK_NullToMemberPointer: {
         // FIXME: For now, member pointers are represented by void *.
         SVal V = svalBuilder.makeNull();
@@ -421,7 +443,6 @@ void ExprEngine::VisitCast(const CastExpr *CastE, const Expr *Ex,
       }
       // Various C++ casts that are not handled yet.
       case CK_ToUnion:
-      case CK_BaseToDerived:
       case CK_BaseToDerivedMemberPointer:
       case CK_DerivedToBaseMemberPointer:
       case CK_ReinterpretMemberPointer:
diff --git a/clang/lib/StaticAnalyzer/Core/Store.cpp b/clang/lib/StaticAnalyzer/Core/Store.cpp
@@ -292,7 +292,7 @@ static const CXXRecordDecl *getCXXRecordType(const MemRegion *MR) {
   return nullptr;
 }
 
-SVal StoreManager::evalDynamicCast(SVal Base, QualType TargetType,
+SVal StoreManager::attemptDownCast(SVal Base, QualType TargetType,
                                    bool &Failed) {
   Failed = false;
 
diff --git a/clang/test/Analysis/NewDelete-checker-test.cpp b/clang/test/Analysis/NewDelete-checker-test.cpp
@@ -377,3 +377,19 @@ void testDoubleDeleteEmptyClass() {
   delete foo;
   delete foo;  // expected-warning {{Attempt to delete released memory}}
 }
+
+struct Base {
+  virtual ~Base() {}
+};
+
+struct Derived : Base {
+};
+
+Base *allocate() {
+  return new Derived;
+}
+
+void shouldNotReportLeak() {
+  Derived *p = (Derived *)allocate();
+  delete p;
+}

Original file line number	Diff line number	Diff line change
`@@ -552,7 +552,7 @@ void CXXInstanceCall::getInitialStackFrameContents(`
`552`	`552`
`553`	`553`	`// FIXME: CallEvent maybe shouldn't be directly accessing StoreManager.`
`554`	`554`	`bool Failed;`
`555`		`- ThisVal = StateMgr.getStoreManager().evalDynamicCast(ThisVal, Ty, Failed);`
	`555`	`+ ThisVal = StateMgr.getStoreManager().attemptDownCast(ThisVal, Ty, Failed);`
`556`	`556`	`assert(!Failed && "Calling an incorrectly devirtualized method");`
`557`	`557`	`}`
`558`	`558`
Original file line number	Diff line number	Diff line change
`@@ -292,7 +292,7 @@ static const CXXRecordDecl getCXXRecordType(const MemRegion MR) {`
`292`	`292`	`return nullptr;`
`293`	`293`	`}`
`294`	`294`
`295`		`-SVal StoreManager::evalDynamicCast(SVal Base, QualType TargetType,`
	`295`	`+SVal StoreManager::attemptDownCast(SVal Base, QualType TargetType,`
`296`	`296`	`bool &Failed) {`
`297`	`297`	`Failed = false;`
`298`	`298`