[analyzer] Introduce proper diagnostic for freeing unowned object

Insert a note when the object becomes not (exclusively) owned.

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

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@351514 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp b/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
index a39cd051286acae84d0e15ca7a7dda6cfbcaead4..cd9273c8a4736564cf798bec7bde2969967f8f1e 100644 (file)
--- a/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
+++ b/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
@@ -803,13 +803,16 @@ ProgramStateRef RetainCountChecker::updateSymbol(ProgramStateRef state,
 }
 
 const RefCountBug &
-RetainCountChecker::errorKindToBugKind(RefVal::Kind ErrorKind) const {
+RetainCountChecker::errorKindToBugKind(RefVal::Kind ErrorKind,
+                                       SymbolRef Sym) const {
   switch (ErrorKind) {
     case RefVal::ErrorUseAfterRelease:
       return useAfterRelease;
     case RefVal::ErrorReleaseNotOwned:
       return releaseNotOwned;
     case RefVal::ErrorDeallocNotOwned:
+      if (Sym->getType()->getPointeeCXXRecordDecl())
+        return freeNotOwned;
       return deallocNotOwned;
     default:
       llvm_unreachable("Unhandled error.");
@@ -836,7 +839,8 @@ void RetainCountChecker::processNonLeakError(ProgramStateRef St,
     return;
 
   auto report = llvm::make_unique<RefCountReport>(
-      errorKindToBugKind(ErrorKind), C.getASTContext().getLangOpts(), N, Sym);
+      errorKindToBugKind(ErrorKind, Sym),
+      C.getASTContext().getLangOpts(), N, Sym);
   report->addRange(ErrorRange);
   C.emitReport(std::move(report));
 }

diff --git a/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h b/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h
index a04f9aba8240edabec99908c0d834a3b92d0517e..ddb4a9e36e2d15a90bf44e256bf182f551586dc4 100644 (file)
--- a/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h
+++ b/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h
@@ -255,6 +255,7 @@ class RetainCountChecker
   RefCountBug useAfterRelease{this, RefCountBug::UseAfterRelease};
   RefCountBug releaseNotOwned{this, RefCountBug::ReleaseNotOwned};
   RefCountBug deallocNotOwned{this, RefCountBug::DeallocNotOwned};
+  RefCountBug freeNotOwned{this, RefCountBug::FreeNotOwned};
   RefCountBug overAutorelease{this, RefCountBug::OverAutorelease};
   RefCountBug returnNotOwnedForOwned{this, RefCountBug::ReturnNotOwnedForOwned};
   RefCountBug leakWithinFunction{this, RefCountBug::LeakWithinFunction};
@@ -336,8 +337,8 @@ public:
                                RefVal V, ArgEffect E, RefVal::Kind &hasErr,
                                CheckerContext &C) const;
 
-
-  const RefCountBug &errorKindToBugKind(RefVal::Kind ErrorKind) const;
+  const RefCountBug &errorKindToBugKind(RefVal::Kind ErrorKind,
+                                        SymbolRef Sym) const;
 
   void processNonLeakError(ProgramStateRef St, SourceRange ErrorRange,
                            RefVal::Kind ErrorKind, SymbolRef Sym,

diff --git a/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp b/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
index e9ce417730b87ff26328098aea4fcb63005beee3..b7a866d3ff857a1f265e7b53929a2383232d70e1 100644 (file)
--- a/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
+++ b/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp
@@ -27,6 +27,8 @@ StringRef RefCountBug::bugTypeToName(RefCountBug::RefCountBugType BT) {
     return "Bad release";
   case DeallocNotOwned:
     return "-dealloc sent to non-exclusively owned object";
+  case FreeNotOwned:
+    return "freeing non-exclusively owned object";
   case OverAutorelease:
     return "Object autoreleased too many times";
   case ReturnNotOwnedForOwned:
@@ -47,6 +49,8 @@ StringRef RefCountBug::getDescription() const {
            "not owned at this point by the caller";
   case DeallocNotOwned:
     return "-dealloc sent to object that may be referenced elsewhere";
+  case FreeNotOwned:
+    return  "'free' called on an object that may be referenced elsewhere";
   case OverAutorelease:
     return "Object autoreleased too many times";
   case ReturnNotOwnedForOwned:
@@ -86,7 +90,8 @@ static std::string getPrettyTypeName(QualType QT) {
 /// Write information about the type state change to {@code os},
 /// return whether the note should be generated.
 static bool shouldGenerateNote(llvm::raw_string_ostream &os,
-                               const RefVal *PrevT, const RefVal &CurrV,
+                               const RefVal *PrevT,
+                               const RefVal &CurrV,
                                bool DeallocSent) {
   // Get the previous type state.
   RefVal PrevV = *PrevT;
@@ -416,6 +421,11 @@ std::shared_ptr<PathDiagnosticPiece>
 RefCountReportVisitor::VisitNode(const ExplodedNode *N,
                               BugReporterContext &BRC, BugReport &BR) {
 
+  const auto &BT = static_cast<const RefCountBug&>(BR.getBugType());
+
+  bool IsFreeUnowned = BT.getBugType() == RefCountBug::FreeNotOwned ||
+                       BT.getBugType() == RefCountBug::DeallocNotOwned;
+
   const SourceManager &SM = BRC.getSourceManager();
   CallEventManager &CEMgr = BRC.getStateManager().getCallEventManager();
   if (auto CE = N->getLocationAs<CallExitBegin>())
@@ -434,7 +444,8 @@ RefCountReportVisitor::VisitNode(const ExplodedNode *N,
   const LocationContext *LCtx = N->getLocationContext();
 
   const RefVal* CurrT = getRefBinding(CurrSt, Sym);
-  if (!CurrT) return nullptr;
+  if (!CurrT)
+    return nullptr;
 
   const RefVal &CurrV = *CurrT;
   const RefVal *PrevT = getRefBinding(PrevSt, Sym);
@@ -444,6 +455,12 @@ RefCountReportVisitor::VisitNode(const ExplodedNode *N,
   std::string sbuf;
   llvm::raw_string_ostream os(sbuf);
 
+  if (PrevT && IsFreeUnowned && CurrV.isNotOwned() && PrevT->isOwned()) {
+    os << "Object is now not exclusively owned";
+    auto Pos = PathDiagnosticLocation::create(N->getLocation(), SM);
+    return std::make_shared<PathDiagnosticEventPiece>(Pos, os.str());
+  }
+
   // This is the allocation site since the previous node had no bindings
   // for this symbol.
   if (!PrevT) {
@@ -490,9 +507,9 @@ RefCountReportVisitor::VisitNode(const ExplodedNode *N,
   // program point
   bool DeallocSent = false;
 
-  if (N->getLocation().getTag() &&
-      N->getLocation().getTag()->getTagDescription().contains(
-          RetainCountChecker::DeallocTagDescription)) {
+  const ProgramPointTag *Tag = N->getLocation().getTag();
+  if (Tag && Tag->getTagDescription().contains(
+                 RetainCountChecker::DeallocTagDescription)) {
     // We only have summaries attached to nodes after evaluating CallExpr and
     // ObjCMessageExprs.
     const Stmt *S = N->getLocation().castAs<StmtPoint>().getStmt();

diff --git a/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h b/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h
index fdfab9d77f9fec9c4d562ae218cadde21a0ce2de..f6e4ded960c0a43a4a050c44e828a41153d57422 100644 (file)
--- a/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h
+++ b/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h
@@ -30,6 +30,7 @@ public:
     UseAfterRelease,
     ReleaseNotOwned,
     DeallocNotOwned,
+    FreeNotOwned,
     OverAutorelease,
     ReturnNotOwnedForOwned,
     LeakWithinFunction,

diff --git a/test/Analysis/osobject-retain-release.cpp b/test/Analysis/osobject-retain-release.cpp
index 3aaaab3bef020f55aa912ef7e1bb977a7a9486a3..9bc2e20454505823809375e01d7d03d7f6235f99 100644 (file)
--- a/test/Analysis/osobject-retain-release.cpp
+++ b/test/Analysis/osobject-retain-release.cpp
@@ -634,3 +634,13 @@ void test_ostypealloc_correct_diagnostic_name() {
   arr->release(); // expected-note{{Reference count decremented. The object now has a +1 retain count}}
 } // expected-note{{Object leaked: object allocated and stored into 'arr' is not referenced later in this execution path and has a retain count of +1}}
   // expected-warning@-1{{Potential leak of an object stored into 'arr'}}
+
+void escape_elsewhere(OSObject *obj);
+
+void test_free_on_escaped_object_diagnostics() {
+  OSObject *obj = new OSObject; // expected-note{{Operator 'new' returns an OSObject of type 'OSObject' with a +1 retain count}}
+  escape_elsewhere(obj); // expected-note{{Object is now not exclusively owned}}
+  obj->free(); // expected-note{{'free' called on an object that may be referenced elsewhere}}
+  // expected-warning@-1{{'free' called on an object that may be referenced elsewhere}}
+}
+