[PATCH] [Clang SA] Fix double delete warning reported at wrong location.

[Karthik Bhat]

  Hi Jordan,
  Implement review comments. Removed TriggerStmt from CallEvent;using DestructorCall to check if a symbol is already deleted or not.
  Thanks

Hi jordan_rose,

http://llvm-reviews.chandlerc.com/D2441

CHANGE SINCE LAST DIFF
  http://llvm-reviews.chandlerc.com/D2441?vs=6188&id=6261#toc

Files:
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  lib/StaticAnalyzer/Core/PathDiagnostic.cpp
  test/Analysis/NewDelete-checker-test.cpp
  include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h

Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -157,6 +157,7 @@
                                      eval::Assume>
 {
   mutable OwningPtr<BugType> BT_DoubleFree;
+  mutable OwningPtr<BugType> BT_DoubleDelete;
   mutable OwningPtr<BugType> BT_Leak;
   mutable OwningPtr<BugType> BT_UseFree;
   mutable OwningPtr<BugType> BT_BadFree;
@@ -279,6 +280,8 @@
 
   bool checkUseAfterFree(SymbolRef Sym, CheckerContext &C, const Stmt *S) const;
 
+  bool checkDoubleDelete(SymbolRef Sym, CheckerContext &C) const;
+
   /// Check if the function is known free memory, or if it is
   /// "interesting" and should be modeled explicitly.
   ///
@@ -322,6 +325,8 @@
   void ReportDoubleFree(CheckerContext &C, SourceRange Range, bool Released,
                         SymbolRef Sym, SymbolRef PrevSym) const;
 
+  void ReportDoubleDelete(CheckerContext &C, SymbolRef Sym) const;
+
   /// Find the location of the allocation for Sym on the path leading to the
   /// exploded node N.
   LeakInfo getAllocationSite(const ExplodedNode *N, SymbolRef Sym,
@@ -1415,6 +1420,28 @@
   }
 }
 
+
+void MallocChecker::ReportDoubleDelete(CheckerContext &C, SymbolRef Sym) const {
+
+  if (!Filter.CNewDeleteChecker)
+    return;
+
+  if (!isTrackedByCurrentChecker(C, Sym))
+    return;
+
+  if (ExplodedNode *N = C.generateSink()) {
+    if (!BT_DoubleDelete)
+      BT_DoubleDelete.reset(new BugType("double delete", "Memory Error"));
+
+    BugReport *R = new BugReport(*BT_DoubleDelete,
+                                 "Attempt to delete released memory", N);
+
+    R->markInteresting(Sym);
+    R->addVisitor(new MallocBugVisitor(Sym));
+    C.emitReport(R);
+  }
+}
+
 ProgramStateRef MallocChecker::ReallocMem(CheckerContext &C,
                                           const CallExpr *CE,
                                           bool FreesOnFail) const {
@@ -1695,6 +1722,13 @@
 void MallocChecker::checkPreCall(const CallEvent &Call,
                                  CheckerContext &C) const {
 
+
+  if (const CXXDestructorCall *DC = dyn_cast<CXXDestructorCall>(&Call)) {
+    SymbolRef Sym = DC->getCXXThisVal().getAsSymbol();
+    if (!Sym || checkDoubleDelete(Sym, C))
+      return;
+  }
+
   // We will check for double free in the post visit.
   if (const AnyFunctionCall *FC = dyn_cast<AnyFunctionCall>(&Call)) {
     const FunctionDecl *FD = FC->getDecl();
@@ -1709,7 +1743,6 @@
         isStandardNewDelete(FD, C.getASTContext()))
       return;
   }
-
   // Check if the callee of a method is deleted.
   if (const CXXInstanceCall *CC = dyn_cast<CXXInstanceCall>(&Call)) {
     SymbolRef Sym = CC->getCXXThisVal().getAsSymbol();
@@ -1803,8 +1836,7 @@
 bool MallocChecker::checkUseAfterFree(SymbolRef Sym, CheckerContext &C,
                                       const Stmt *S) const {
 
-  // FIXME: Handle destructor called from delete more precisely.
-  if (isReleased(Sym, C) && S) {
+  if (isReleased(Sym, C)) {
     ReportUseAfterFree(C, S->getSourceRange(), Sym);
     return true;
   }
@@ -1812,6 +1844,15 @@
   return false;
 }
 
+bool MallocChecker::checkDoubleDelete(SymbolRef Sym, CheckerContext &C) const {
+
+  if (isReleased(Sym, C)) {
+    ReportDoubleDelete(C, Sym);
+    return true;
+  }
+  return false;
+}
+
 // Check if the location is a freed symbolic region.
 void MallocChecker::checkLocation(SVal l, bool isLoad, const Stmt *S,
                                   CheckerContext &C) const {
Index: lib/StaticAnalyzer/Core/PathDiagnostic.cpp
===================================================================
--- lib/StaticAnalyzer/Core/PathDiagnostic.cpp
+++ lib/StaticAnalyzer/Core/PathDiagnostic.cpp
@@ -706,6 +706,33 @@
   return 0;
 }
 
+const Stmt *PathDiagnosticLocation::getPrevStmt(const ExplodedNode *N) {
+  for (N = N->getFirstPred(); N; N = N->getFirstPred()) {
+    if (const Stmt *S = getStmt(N)) {
+      // Check if the statement is '?' or '&&'/'||'.  These are "merges",
+      // not actual statement points.
+      switch (S->getStmtClass()) {
+        case Stmt::ChooseExprClass:
+        case Stmt::BinaryConditionalOperatorClass:
+        case Stmt::ConditionalOperatorClass:
+          continue;
+        case Stmt::BinaryOperatorClass: {
+          BinaryOperatorKind Op = cast<BinaryOperator>(S)->getOpcode();
+          if (Op == BO_LAnd || Op == BO_LOr)
+            continue;
+          break;
+        }
+        default:
+          break;
+      }
+      // We found the statement, so return it.
+      return S;
+    }
+  }
+
+  return 0;
+}
+
 const Stmt *PathDiagnosticLocation::getNextStmt(const ExplodedNode *N) {
   for (N = N->getFirstSucc(); N; N = N->getFirstSucc()) {
     if (const Stmt *S = getStmt(N)) {
@@ -739,6 +766,13 @@
   assert(N && "Cannot create a location with a null node.");
   const Stmt *S = getStmt(N);
 
+  // If this is an implicit call find the stmt that triggered it.
+  if (N && !S) {
+    ProgramPoint P = N->getLocation();
+    if (P.getAs<ImplicitCallPoint>())
+      S = getPrevStmt(N);
+  }
+
   if (!S)
     S = getNextStmt(N);
 
Index: test/Analysis/NewDelete-checker-test.cpp
===================================================================
--- test/Analysis/NewDelete-checker-test.cpp
+++ test/Analysis/NewDelete-checker-test.cpp
@@ -338,13 +338,13 @@
 public:
   int *x;
   DerefClass() {}
-  ~DerefClass() {*x = 1;} //expected-warning {{Use of memory after it is freed}}
+  ~DerefClass() {*x = 1;} 
 };
 
 void testDoubleDeleteClassInstance() {
   DerefClass *foo = new DerefClass();
   delete foo;
-  delete foo; // FIXME: We should ideally report warning here instead of inside the destructor.
+  delete foo; //expected-warning {{Attempt to delete released memory}}
 }
 
 class EmptyClass{
@@ -356,5 +356,5 @@
 void testDoubleDeleteEmptyClass() {
   EmptyClass *foo = new EmptyClass();
   delete foo;
-  delete foo;  //expected-warning {{Attempt to free released memory}}
+  delete foo;  //expected-warning {{Attempt to delete released memory}}
 }
Index: include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
===================================================================
--- include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
+++ include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
@@ -296,6 +296,8 @@
 
   /// \brief Retrieve the statement corresponding to the successor node.
   static const Stmt *getNextStmt(const ExplodedNode *N);
+
+  static const Stmt *getPrevStmt(const ExplodedNode *N);
 };
 
 class PathDiagnosticLocationPair {
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D2441.3.patch
Type: text/x-patch
Size: 6664 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131224/50aea07c/attachment.bin>