r336489 - [analyzer] Highlight container object destruction in MallocChecker.

[Reka Kovacs via cfe-commits]

Author: rkovacs
Date: Sat Jul  7 10:22:45 2018
New Revision: 336489

URL: http://llvm.org/viewvc/llvm-project?rev=336489&view=rev
Log:
[analyzer] Highlight container object destruction in MallocChecker.

Extend MallocBugVisitor to place a note at the point where objects with
AF_InternalBuffer allocation family are destroyed.

Differential Revision: https://reviews.llvm.org/D48521

Modified:
    cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
    cfe/trunk/test/Analysis/dangling-internal-buffer.cpp

Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp?rev=336489&r1=336488&r2=336489&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp Sat Jul  7 10:22:45 2018
@@ -480,8 +480,13 @@ private:
     inline bool isReleased(const RefState *S, const RefState *SPrev,
                            const Stmt *Stmt) {
       // Did not track -> released. Other state (allocated) -> released.
-      return (Stmt && (isa<CallExpr>(Stmt) || isa<CXXDeleteExpr>(Stmt)) &&
-              (S && S->isReleased()) && (!SPrev || !SPrev->isReleased()));
+      // The statement associated with the release might be missing.
+      bool IsReleased = (S && S->isReleased()) &&
+                        (!SPrev || !SPrev->isReleased());
+      assert(!IsReleased ||
+             (Stmt && (isa<CallExpr>(Stmt) || isa<CXXDeleteExpr>(Stmt))) ||
+             (!Stmt && S->getAllocationFamily() == AF_InternalBuffer));
+      return IsReleased;
     }
 
     inline bool isRelinquished(const RefState *S, const RefState *SPrev,
@@ -2850,8 +2855,17 @@ static bool isReferenceCountingPointerDe
 std::shared_ptr<PathDiagnosticPiece> MallocChecker::MallocBugVisitor::VisitNode(
     const ExplodedNode *N, const ExplodedNode *PrevN, BugReporterContext &BRC,
     BugReport &BR) {
+
+  ProgramStateRef state = N->getState();
+  ProgramStateRef statePrev = PrevN->getState();
+
+  const RefState *RS = state->get<RegionState>(Sym);
+  const RefState *RSPrev = statePrev->get<RegionState>(Sym);
+
   const Stmt *S = PathDiagnosticLocation::getStmt(N);
-  if (!S)
+  // When dealing with containers, we sometimes want to give a note
+  // even if the statement is missing.
+  if (!S && (!RS || RS->getAllocationFamily() != AF_InternalBuffer))
     return nullptr;
 
   const LocationContext *CurrentLC = N->getLocationContext();
@@ -2876,14 +2890,6 @@ std::shared_ptr<PathDiagnosticPiece> Mal
     }
   }
 
-  ProgramStateRef state = N->getState();
-  ProgramStateRef statePrev = PrevN->getState();
-
-  const RefState *RS = state->get<RegionState>(Sym);
-  const RefState *RSPrev = statePrev->get<RegionState>(Sym);
-  if (!RS)
-    return nullptr;
-
   // FIXME: We will eventually need to handle non-statement-based events
   // (__attribute__((cleanup))).
 
@@ -2896,7 +2902,22 @@ std::shared_ptr<PathDiagnosticPiece> Mal
       StackHint = new StackHintGeneratorForSymbol(Sym,
                                                   "Returned allocated memory");
     } else if (isReleased(RS, RSPrev, S)) {
-      Msg = "Memory is released";
+      const auto Family = RS->getAllocationFamily();
+      switch(Family) {
+        case AF_Alloca:
+        case AF_Malloc:
+        case AF_CXXNew:
+        case AF_CXXNewArray:
+        case AF_IfNameIndex:
+          Msg = "Memory is released";
+          break;
+        case AF_InternalBuffer:
+          Msg = "Internal buffer is released because the object was destroyed";
+          break;
+        case AF_None:
+        default:
+          llvm_unreachable("Unhandled allocation family!");
+      }
       StackHint = new StackHintGeneratorForSymbol(Sym,
                                              "Returning; memory was released");
 
@@ -2967,8 +2988,19 @@ std::shared_ptr<PathDiagnosticPiece> Mal
   assert(StackHint);
 
   // Generate the extra diagnostic.
-  PathDiagnosticLocation Pos(S, BRC.getSourceManager(),
-                             N->getLocationContext());
+  PathDiagnosticLocation Pos;
+  if (!S) {
+    assert(RS->getAllocationFamily() == AF_InternalBuffer);
+    auto PostImplCall = N->getLocation().getAs<PostImplicitCall>();
+    if (!PostImplCall)
+      return nullptr;
+    Pos = PathDiagnosticLocation(PostImplCall->getLocation(),
+                                 BRC.getSourceManager());
+  } else {
+    Pos = PathDiagnosticLocation(S, BRC.getSourceManager(),
+                                 N->getLocationContext());
+  }
+
   return std::make_shared<PathDiagnosticEventPiece>(Pos, Msg, true, StackHint);
 }
 

Modified: cfe/trunk/test/Analysis/dangling-internal-buffer.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/dangling-internal-buffer.cpp?rev=336489&r1=336488&r2=336489&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/dangling-internal-buffer.cpp (original)
+++ cfe/trunk/test/Analysis/dangling-internal-buffer.cpp Sat Jul  7 10:22:45 2018
@@ -26,7 +26,7 @@ void deref_after_scope_char() {
   {
     std::string s;
     c = s.c_str();
-  }
+  } // expected-note {{Internal buffer is released because the object was destroyed}}
   consume(c); // expected-warning {{Use of memory after it is freed}}
   // expected-note at -1 {{Use of memory after it is freed}}
 }
@@ -36,7 +36,7 @@ void deref_after_scope_wchar_t() {
   {
     std::wstring ws;
     w = ws.c_str();
-  }
+  } // expected-note {{Internal buffer is released because the object was destroyed}}
   consume(w); // expected-warning {{Use of memory after it is freed}}
   // expected-note at -1 {{Use of memory after it is freed}}
 }
@@ -46,7 +46,7 @@ void deref_after_scope_char16_t() {
   {
     std::u16string s16;
     c16 = s16.c_str();
-  }
+  } // expected-note {{Internal buffer is released because the object was destroyed}}
   consume(c16); // expected-warning {{Use of memory after it is freed}}
   // expected-note at -1 {{Use of memory after it is freed}}
 }
@@ -56,7 +56,7 @@ void deref_after_scope_char32_t() {
   {
     std::u32string s32;
     c32 = s32.c_str();
-  }
+  } // expected-note {{Internal buffer is released because the object was destroyed}}
   consume(c32); // expected-warning {{Use of memory after it is freed}}
   // expected-note at -1 {{Use of memory after it is freed}}
 }