From 656fdd55dd0dffb9177b8008fe030c24300507cb Mon Sep 17 00:00:00 2001 From: Jordan Rose Date: Wed, 8 Jan 2014 18:46:55 +0000 Subject: [PATCH] [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! llvm-svn: 198779 --- .../lib/StaticAnalyzer/Checkers/MallocChecker.cpp | 44 +++++++++++++++++++++- clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp | 6 ++- clang/test/Analysis/NewDelete-checker-test.cpp | 6 +-- 3 files changed, 50 insertions(+), 6 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp index 87d7e89..92297ca 100644 --- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -155,6 +155,7 @@ class MallocChecker : public Checker { mutable OwningPtr BT_DoubleFree; + mutable OwningPtr BT_DoubleDelete; mutable OwningPtr BT_Leak; mutable OwningPtr BT_UseFree; mutable OwningPtr 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(CheckerContext &C, SourceRange Range, } } +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(SymbolReaper &SymReaper, void MallocChecker::checkPreCall(const CallEvent &Call, CheckerContext &C) const { + if (const CXXDestructorCall *DC = dyn_cast(&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(&Call)) { const FunctionDecl *FD = FC->getDecl(); @@ -1801,8 +1833,7 @@ bool MallocChecker::isReleased(SymbolRef Sym, CheckerContext &C) const { 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(SymbolRef Sym, CheckerContext &C, 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 { diff --git a/clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp b/clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp index 054c9307..2d3caf9 100644 --- a/clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp +++ b/clang/lib/StaticAnalyzer/Core/PathDiagnostic.cpp @@ -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 PIE = N->getLocationAs()) + return PathDiagnosticLocation(PIE->getLocation(), SM); S = getNextStmt(N); + } if (S) { ProgramPoint P = N->getLocation(); diff --git a/clang/test/Analysis/NewDelete-checker-test.cpp b/clang/test/Analysis/NewDelete-checker-test.cpp index cc972512..855b05d 100644 --- a/clang/test/Analysis/NewDelete-checker-test.cpp +++ b/clang/test/Analysis/NewDelete-checker-test.cpp @@ -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}} } -- 2.7.4