[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

484–490

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.

2921

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

2994

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.

2995

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

484–490

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.

2867–2869

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.

2921

Or exclamantion mark! :)

2997

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.

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>();
 

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
 }