From: George Karpenkov Date: Fri, 18 Jan 2019 03:13:53 +0000 (+0000) Subject: [analyzer] Introduce proper diagnostic for freeing unowned object X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=56934968fc4113de172a6d4e8d8b53d902084b63;p=clang [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 a39cd05128..cd9273c8a4 100644 --- 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( - 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 a04f9aba82..ddb4a9e36e 100644 --- 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 e9ce417730..b7a866d3ff 100644 --- 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 RefCountReportVisitor::VisitNode(const ExplodedNode *N, BugReporterContext &BRC, BugReport &BR) { + const auto &BT = static_cast(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()) @@ -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(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().getStmt(); diff --git a/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h b/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.h index fdfab9d77f..f6e4ded960 100644 --- 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 3aaaab3bef..9bc2e20454 100644 --- 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}} +} +