[PATCH] D48521: [analyzer] Highlight STL object destruction in MallocChecker

[Reka Kovacs via Phabricator via cfe-commits]

rnkovacs created this revision.
rnkovacs added reviewers: NoQ, xazax.hun, george.karpenkov, dcoughlin.
Herald added subscribers: mikhail.ramalho, a.sidorin, dkrupp, szepet, 
baloghadamsoftware, whisperity.

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


Repository:
  rC Clang

https://reviews.llvm.org/D48521

Files:
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  test/Analysis/dangling-internal-buffer.cpp


Index: test/Analysis/dangling-internal-buffer.cpp
===================================================================
--- test/Analysis/dangling-internal-buffer.cpp
+++ test/Analysis/dangling-internal-buffer.cpp
@@ -26,7 +26,7 @@
   {
     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@-1 {{Use of memory after it is freed}}
 }
@@ -36,7 +36,7 @@
   {
     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@-1 {{Use of memory after it is freed}}
 }
@@ -46,7 +46,7 @@
   {
     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@-1 {{Use of memory after it is freed}}
 }
@@ -56,7 +56,7 @@
   {
     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@-1 {{Use of memory after it is freed}}
 }
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -478,11 +478,10 @@
                            SPrev->isAllocatedOfSizeZero())));
     }
 
-    inline bool isReleased(const RefState *S, const RefState *SPrev,
-                           const Stmt *Stmt) {
+    inline bool isReleased(const RefState *S, const RefState *SPrev) {
       // 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.
+      return (S && S->isReleased()) && (!SPrev || !SPrev->isReleased());
     }
 
     inline bool isRelinquished(const RefState *S, const RefState *SPrev,
@@ -2851,8 +2850,17 @@
 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 STL 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();
@@ -2877,14 +2885,6 @@
     }
   }
 
-  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,8 +2896,23 @@
       Msg = "Memory is allocated";
       StackHint = new StackHintGeneratorForSymbol(Sym,
                                                   "Returned allocated memory");
-    } else if (isReleased(RS, RSPrev, S)) {
-      Msg = "Memory is released";
+    } else if (isReleased(RS, RSPrev)) {
+      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");
 
@@ -2968,8 +2983,18 @@
   assert(StackHint);
 
   // Generate the extra diagnostic.
-  PathDiagnosticLocation Pos(S, BRC.getSourceManager(),
-                             N->getLocationContext());
+  PathDiagnosticLocation Pos;
+  if (!S) {
+    auto PostImplCall = N->getLocation().getAs<PostImplicitCall>();
+    if (!PostImplCall.hasValue())
+      return nullptr;
+    Pos = PathDiagnosticLocation(PostImplCall.getValue().getLocation(),
+                                 BRC.getSourceManager());
+  } else {
+    Pos = PathDiagnosticLocation(S, BRC.getSourceManager(),
+                                 N->getLocationContext());
+  }
+
   return std::make_shared<PathDiagnosticEventPiece>(Pos, Msg, true, StackHint);
 }
 

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits