[analyzer] Highlight container object destruction in MallocChecker

[rnkovacs]

Summary

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

[xazax.hun]

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

483 ↗

(On Diff #152600)

To be honest, I am not sure what was the purpose of the AST based checks in the original version. IMHO, looking at the states only should be sufficient without examining the AST.

2914 ↗

(On Diff #152600)

I think these messages should be full sentences starting with a capital letter and ending with a period.

2988 ↗

(On Diff #152600)

This new code should only be used for AF_InternalBuffer allocation family? If that is the case, maybe adding an assert here and running it on some large codebases to se if this is triggered would be great.

2989 ↗

(On Diff #152600)

LLVM's optional can implicitly convert to bool. I think it is perfectly fine to rely on the implicit conversion here.

[NoQ]

Only minor nits, as usual :)

lib/StaticAnalyzer/Checkers/MallocChecker.cpp

483 ↗

(On Diff #152600)

Yeah, same. I guess we could replace the check with an assertion (i.e. if there's statement, it's a call or delete-expression and it's AF_InternalBuffer), and see if anything crashes.

2861 ↗

(On Diff #152600)

I don't think our check will be limited to STL containers only. We might cover other containers as well, such as LLVM or WebKit custom strings and string views.

2914 ↗

(On Diff #152600)

Or exclamantion mark! :)

2991 ↗

(On Diff #152600)

PostImplCall->getLocation().

[rnkovacs]

Thanks for the comments!
I'll run this on some projects and see if any assertions fail.

[xazax.hun]

LGTM, given that the assert does not fire for the projects you tested on.

[rnkovacs]

No crashes on Harfbuzz, ICU, Bitcoin, and LLVM. I'll commit.

cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

inline bool isAllocated(const RefState *S, const RefState *SPrev,
               (S && (S->isAllocated() || S->isAllocatedOfSizeZero())) &&
               (!SPrev || !(SPrev->isAllocated() ||
                            SPrev->isAllocatedOfSizeZero())));
     }
 
     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,
                                const Stmt *Stmt) {
       // Did not track -> relinquished. Other state (allocated) -> relinquished.
       return (Stmt && (isa<CallExpr>(Stmt) || isa<ObjCMessageExpr>(Stmt) ||
                                               isa<ObjCPropertyRefExpr>(Stmt)) &&
               (S && S->isRelinquished()) &&
if (const IdentifierInfo *II = DD->getParent()->getIdentifier()) {
     }
   }
   return false;
 }
 
 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();
 
   // If we find an atomic fetch_add or fetch_sub within the destructor in which
   // the pointer was released (before the release), this is likely a destructor
   // of a shared pointer.
   // Because we don't model atomics, and also because we don't know that the
   // original reference count is positive, we should not report use-after-frees
   // on objects deleted in such destructors. This can probably be improved
   // through better shared pointer modeling.
   if (ReleaseDestructorLC) {
     if (const auto *AE = dyn_cast<AtomicExpr>(S)) {
       AtomicExpr::AtomicOp Op = AE->getOp();
       if (Op == AtomicExpr::AO__c11_atomic_fetch_add ||
           Op == AtomicExpr::AO__c11_atomic_fetch_sub) {
         if (ReleaseDestructorLC == CurrentLC ||
             ReleaseDestructorLC->isParentOf(CurrentLC)) {
           BR.markInvalid(getTag(), S);
         }
       }
     }
   }
 
-  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))).
 
   // Find out if this is an interesting point and what is the kind.
   const char *Msg = nullptr;
   StackHintGeneratorForSymbol *StackHint = nullptr;
   if (Mode == Normal) {
     if (isAllocated(RS, RSPrev, S)) {
       Msg = "Memory is allocated";
       StackHint = new StackHintGeneratorForSymbol(Sym,
                                                   "Returned allocated memory");
     } else if (isReleased(RS, RSPrev, S)) {
+      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");
 
       // See if we're releasing memory while inlining a destructor
       // (or one of its callees). This turns on various common
       // false positive suppressions.
       bool FoundAnyDestructor = false;
       for (const LocationContext *LC = CurrentLC; LC; LC = LC->getParent()) {
// We are in a special mode if a reallocation failed later in the path.
     }
   }
 
   if (!Msg)
     return nullptr;
   assert(StackHint);
 
   // Generate the extra diagnostic.
-  PathDiagnosticLocation Pos(S, BRC.getSourceManager(),
+  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);
 }
 
 void MallocChecker::printState(raw_ostream &Out, ProgramStateRef State,
                                const char *NL, const char *Sep) const {
 
   RegionStateTy RS = State->get<RegionState>();
 

cfe/trunk/test/Analysis/dangling-internal-buffer.cpp

 void consume(const char16_t *) {}
 void consume(const char32_t *) {}
 
 void deref_after_scope_char() {
   const char *c;
   {
     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}}
 }
 
 void deref_after_scope_wchar_t() {
   const wchar_t *w;
   {
     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}}
 }
 
 void deref_after_scope_char16_t() {
   const char16_t *c16;
   {
     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}}
 }
 
 void deref_after_scope_char32_t() {
   const char32_t *c32;
   {
     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}}
 }
 
 void deref_after_scope_ok() {
   const char *c;
   std::string s;
   {
     c = s.c_str();
   }
   consume(c); // no-warning
 }