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

[Karthik Bhat]

  Implement review comments from Jordan.
  Use ImplicitCallPoint location info to conclude the PathDiagnosticLocation instead of traversing Previous Stmnts.
  This is on similar lines of how PathDiagnosticLocation::create is implemented for PostImplicitCall.
  Please let me know if this change is fine.
  Thanks
  Karthik

Hi jordan_rose,

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

CHANGE SINCE LAST DIFF
  http://llvm-reviews.chandlerc.com/D2441?vs=6261&id=6360#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
@@ -155,6 +155,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;
@@ -277,6 +278,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.
   ///
@@ -320,6 +323,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,
@@ -1413,6 +1418,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 {
@@ -1693,6 +1720,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();
@@ -1707,7 +1741,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();
@@ -1801,8 +1834,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;
   }
@@ -1810,6 +1842,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
@@ -739,6 +739,13 @@
   assert(N && "Cannot create a location with a null node.");
   const Stmt *S = getStmt(N);
 
+  // If this is an implicit call return the implicit call ponit location
+  if (N && !S) {
+    ProgramPoint P = N->getLocation();
+    if (Optional<PreImplicitCall> PIE = P.getAs<PreImplicitCall>())
+      return PathDiagnosticLocation(PIE->getLocation(), SM);
+  }
+
   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,7 @@
 
   /// \brief Retrieve the statement corresponding to the successor node.
   static const Stmt *getNextStmt(const ExplodedNode *N);
+
 };
 
 class PathDiagnosticLocationPair {
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D2441.4.patch
Type: text/x-patch
Size: 5633 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140105/e8cec78a/attachment.bin>