[analyzer] Introduce proper diagnostic for freeing unowned object

[george.karpenkov]

Summary

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

[NoQ]

Yay, visitors.

lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h

: public Checker< check::Bind,
                     check::PostCall,
                     check::RegionChanges,
                     eval::Assume,
                     eval::Call > {
 
   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};
   RefCountBug leakAtReturn{this, RefCountBug::LeakAtReturn};
 
   mutable std::unique_ptr<RetainSummaryManager> Summaries;
 public:
   static constexpr const char *DeallocTagDescription = "DeallocSent";
public:
   void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
   void checkBeginFunction(CheckerContext &C) const;
   void checkEndFunction(const ReturnStmt *RS, CheckerContext &C) const;
 
   ProgramStateRef updateSymbol(ProgramStateRef state, SymbolRef sym,
                                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,
                            CheckerContext &C) const;
 
   void processObjCLiterals(CheckerContext &C, const Expr *Ex) const;
 
   ProgramStateRef handleSymbolDeath(ProgramStateRef state,

lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp

case DecRefAndStopTrackingHard:
           break;
       }
       break;
   }
   return setRefBinding(state, sym, V);
 }
 
 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.");
   }
 }
 
 void RetainCountChecker::processNonLeakError(ProgramStateRef St,
                                              SourceRange ErrorRange,
if (const RefVal *RV = getRefBinding(St, Sym))
     if (RV->getIvarAccessHistory() != RefVal::IvarAccessHistory::None)
       return;
 
   ExplodedNode *N = C.generateErrorNode(St);
   if (!N)
     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));
 }
 
 //===----------------------------------------------------------------------===//
 // Handle the return values of retain-count-related functions.
 //===----------------------------------------------------------------------===//
 

lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h

 namespace retaincountchecker {
 
 class RefCountBug : public BugType {
 public:
   enum RefCountBugType {
     UseAfterRelease,
     ReleaseNotOwned,
     DeallocNotOwned,
+    FreeNotOwned,
     OverAutorelease,
     ReturnNotOwnedForOwned,
     LeakWithinFunction,
     LeakAtReturn,
   };
   RefCountBug(const CheckerBase *checker, RefCountBugType BT);
   StringRef getDescription() const;
   RefCountBugType getBugType() const {

lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp

 StringRef RefCountBug::bugTypeToName(RefCountBug::RefCountBugType BT) {
   switch (BT) {
   case UseAfterRelease:
     return "Use-after-release";
   case ReleaseNotOwned:
     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:
     return "Method should return an owned object";
   case LeakWithinFunction:
     return "Leak";
   case LeakAtReturn:
     return "Leak of returned object";
   }
 }
 
 StringRef RefCountBug::getDescription() const {
   switch (BT) {
   case UseAfterRelease:
     return "Reference-counted object is used after it is released";
   case ReleaseNotOwned:
     return "Incorrect decrement of the reference count of an object that is "
            "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:
     return "Object with a +0 retain count returned to caller where a +1 "
            "(owning) retain count is expected";
   case LeakWithinFunction:
   case LeakAtReturn:
     return "";
if (!PT.isNull() && !QT->getAs<TypedefType>())
     if (const auto *RD = PT->getAsCXXRecordDecl())
       return RD->getName();
   return QT.getAsString();
 }
 
 /// 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;
 
   // Specially handle -dealloc.
   if (DeallocSent) {
     // Determine if the object's reference count was pushed to zero.
     assert(!PrevV.hasSameState(CurrV) && "The state should have changed.");
annotateConsumedSummaryMismatch(const ExplodedNode *N,
   PathDiagnosticLocation L = PathDiagnosticLocation::create(CallExitLoc, SM);
   return std::make_shared<PathDiagnosticEventPiece>(L, os.str());
 }
 
 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>())
     if (auto PD = annotateConsumedSummaryMismatch(N, *CE, SM, CEMgr))
       return PD;
 
   // FIXME: We will eventually need to handle non-statement-based events
   // (__attribute__((cleanup))).
   if (!N->getLocation().getAs<StmtPoint>())
     return nullptr;
 
   // Check if the type state has changed.
   const ExplodedNode *PrevNode = N->getFirstPred();
   ProgramStateRef PrevSt = PrevNode->getState();
   ProgramStateRef CurrSt = N->getState();
   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);
 
   // Create a string buffer to constain all the useful things we want
   // to tell the user.
   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) {
     const Stmt *S = N->getLocation().castAs<StmtPoint>().getStmt();
 
     if (isa<ObjCIvarRefExpr>(S) &&
         isSynthesizedAccessor(LCtx->getStackFrame())) {
       S = LCtx->getStackFrame()->getCallSite();
if (!PrevT) {
     PathDiagnosticLocation Pos(S, SM, N->getLocationContext());
     return std::make_shared<PathDiagnosticEventPiece>(Pos, os.str());
   }
 
   // Gather up the effects that were performed on the object at this
   // program point
   bool DeallocSent = false;
 
-  if (N->getLocation().getTag() &&
-      N->getLocation().getTag()->getTagDescription().contains(
+  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();
 
     if (const CallExpr *CE = dyn_cast<CallExpr>(S)) {
       // Iterate through the parameter expressions and see if the symbol
       // was ever passed as an argument.
       unsigned i = 0;

test/Analysis/osobject-retain-release.cpp

 }
 
 void test_ostypealloc_correct_diagnostic_name() {
   OSArray *arr = OSTypeAlloc(OSArray); // expected-note{{Call to method 'OSMetaClass::alloc' returns an OSObject of type 'OSArray' with a +1 retain count}}
   arr->retain(); // expected-note{{Reference count incremented. The object now has a +2 retain count}}
   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}}
+}
+