r198779 - [analyzer] Warn about double-delete in C++ at the second delete...

[Jordan Rose]

Author: jrose
Date: Wed Jan  8 12:46:55 2014
New Revision: 198779

URL: http://llvm.org/viewvc/llvm-project?rev=198779&view=rev
Log:
[analyzer] Warn about double-delete in C++ at the second delete...

...rather somewhere in the destructor when we try to access something and
realize the object has already been deleted. This is necessary because
the destructor is processed before the 'delete' itself.

Patch by Karthik Bhat!

Modified:
    cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
    cfe/trunk/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
    cfe/trunk/test/Analysis/NewDelete-checker-test.cpp

Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp?rev=198779&r1=198778&r2=198779&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp Wed Jan  8 12:46:55 2014
@@ -155,6 +155,7 @@ class MallocChecker : public Checker<che
                                      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 @@ private:
 
   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 @@ private:
   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,27 @@ void MallocChecker::ReportDoubleFree(Che
   }
 }
 
+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 +1719,12 @@ void MallocChecker::checkDeadSymbols(Sym
 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();
@@ -1801,8 +1833,7 @@ bool MallocChecker::isReleased(SymbolRef
 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 +1841,15 @@ bool MallocChecker::checkUseAfterFree(Sy
   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 {

Modified: cfe/trunk/lib/StaticAnalyzer/Core/PathDiagnostic.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/PathDiagnostic.cpp?rev=198779&r1=198778&r2=198779&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Core/PathDiagnostic.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/PathDiagnostic.cpp Wed Jan  8 12:46:55 2014
@@ -739,8 +739,12 @@ PathDiagnosticLocation
   assert(N && "Cannot create a location with a null node.");
   const Stmt *S = getStmt(N);
 
-  if (!S)
+  if (!S) {
+    // If this is an implicit call, return the implicit call point location.
+    if (Optional<PreImplicitCall> PIE = N->getLocationAs<PreImplicitCall>())
+      return PathDiagnosticLocation(PIE->getLocation(), SM);
     S = getNextStmt(N);
+  }
 
   if (S) {
     ProgramPoint P = N->getLocation();

Modified: cfe/trunk/test/Analysis/NewDelete-checker-test.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/NewDelete-checker-test.cpp?rev=198779&r1=198778&r2=198779&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/NewDelete-checker-test.cpp (original)
+++ cfe/trunk/test/Analysis/NewDelete-checker-test.cpp Wed Jan  8 12:46:55 2014
@@ -338,13 +338,13 @@ class DerefClass{
 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 @@ public:
 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}}
 }