Index: clang/include/clang/StaticAnalyzer/Core/RetainSummaryManager.h =================================================================== --- clang/include/clang/StaticAnalyzer/Core/RetainSummaryManager.h +++ clang/include/clang/StaticAnalyzer/Core/RetainSummaryManager.h @@ -154,7 +154,10 @@ /// Indicates that the tracked object could be a CF or Objective-C object. AnyObj, /// Indicates that the tracked object is a generalized object. - Generalized + Generalized, + + /// A descendant of OSObject. + OS }; private: @@ -318,14 +321,21 @@ /// this is the effect applied to the state of the receiver. ArgEffect Receiver; + /// Effect on "this" pointer - applicable only to C++ method calls. + ArgEffect This; + /// Ret - The effect on the return value. Used to indicate if the /// function/method call returns a new tracked symbol. RetEffect Ret; public: - RetainSummary(ArgEffects A, RetEffect R, ArgEffect defaultEff, - ArgEffect ReceiverEff) - : Args(A), DefaultArgEffect(defaultEff), Receiver(ReceiverEff), Ret(R) {} + RetainSummary(ArgEffects A, + RetEffect R, + ArgEffect defaultEff, + ArgEffect ReceiverEff, + ArgEffect ThisEff) + : Args(A), DefaultArgEffect(defaultEff), Receiver(ReceiverEff), + This(ThisEff), Ret(R) {} /// getArg - Return the argument effect on the argument specified by /// idx (starting from 0). @@ -359,12 +369,20 @@ /// This is only meaningful if the summary applies to an ObjCMessageExpr*. ArgEffect getReceiverEffect() const { return Receiver; } + ArgEffect getThisEffect() const { return This; } + + bool isNoop() const { + return Ret == RetEffect::MakeNoRet() && Receiver == DoNothing + && DefaultArgEffect == MayEscape && This == DoNothing + && Args.isEmpty(); + } + /// Test if two retain summaries are identical. Note that merely equivalent /// summaries are not necessarily identical (for example, if an explicit /// argument effect matches the default effect). bool operator==(const RetainSummary &Other) const { return Args == Other.Args && DefaultArgEffect == Other.DefaultArgEffect && - Receiver == Other.Receiver && Ret == Other.Ret; + Receiver == Other.Receiver && This == Other.This && Ret == Other.Ret; } /// Profile this summary for inclusion in a FoldingSet. @@ -372,6 +390,7 @@ ID.Add(Args); ID.Add(DefaultArgEffect); ID.Add(Receiver); + ID.Add(This); ID.Add(Ret); } @@ -460,6 +479,9 @@ /// Records whether or not the analyzed code runs in ARC mode. const bool ARCEnabled; + /// Track sublcasses of OSObject + const bool TrackOSObjects; + /// FuncSummaries - A map from FunctionDecls to summaries. FuncSummariesTy FuncSummaries; @@ -496,6 +518,19 @@ /// data in ScratchArgs. ArgEffects getArgEffects(); + /// Create an OS object at +1. + const RetainSummary *getOSSummaryCreateRule(const FunctionDecl *FD); + + /// Get an OS object at +0. + const RetainSummary *getOSSummaryGetRule(const FunctionDecl *FD); + + /// Increment the reference count on OS object. + const RetainSummary *getOSSummaryRetainRule(const FunctionDecl *FD); + + /// Decrement the reference count on OS object. + const RetainSummary *getOSSummaryReleaseRule(const FunctionDecl *FD); + + enum UnaryFuncKind { cfretain, cfrelease, cfautorelease, cfmakecollectable }; const RetainSummary *getUnarySummary(const FunctionType* FT, @@ -509,8 +544,10 @@ const RetainSummary *getPersistentSummary(RetEffect RetEff, ArgEffect ReceiverEff = DoNothing, - ArgEffect DefaultEff = MayEscape) { - RetainSummary Summ(getArgEffects(), RetEff, DefaultEff, ReceiverEff); + ArgEffect DefaultEff = MayEscape, + ArgEffect ThisEff = DoNothing) { + RetainSummary Summ(getArgEffects(), RetEff, DefaultEff, ReceiverEff, + ThisEff); return getPersistentSummary(Summ); } @@ -584,9 +621,12 @@ bool &AllowAnnotations); public: - RetainSummaryManager(ASTContext &ctx, bool usesARC) + RetainSummaryManager(ASTContext &ctx, + bool usesARC, + bool trackOSObject) : Ctx(ctx), ARCEnabled(usesARC), + TrackOSObjects(trackOSObject), AF(BPAlloc), ScratchArgs(AF.getEmptyMap()), ObjCAllocRetE(usesARC ? RetEffect::MakeNotOwned(RetEffect::ObjC) : RetEffect::MakeOwned(RetEffect::ObjC)), Index: clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h =================================================================== --- clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h +++ clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.h @@ -98,7 +98,7 @@ /// The kind of object being tracked (CF or ObjC), if known. /// /// See the RetEffect::ObjKind enum for possible values. - unsigned RawObjectKind : 2; + unsigned RawObjectKind : 3; /// True if the current state and/or retain count may turn out to not be the /// best possible approximation of the reference counting state. @@ -268,6 +268,8 @@ mutable std::unique_ptr Summaries; mutable SummaryLogTy SummaryLog; + + AnalyzerOptions &Options; mutable bool ShouldResetSummaryLog; /// Optional setting to indicate if leak reports should include @@ -275,12 +277,17 @@ mutable bool IncludeAllocationLine; public: - RetainCountChecker(AnalyzerOptions &AO) - : ShouldResetSummaryLog(false), - IncludeAllocationLine(shouldIncludeAllocationSiteInLeakDiagnostics(AO)) {} + RetainCountChecker(AnalyzerOptions &Options) + : Options(Options), ShouldResetSummaryLog(false), + IncludeAllocationLine( + shouldIncludeAllocationSiteInLeakDiagnostics(Options)) {} ~RetainCountChecker() override { DeleteContainerSeconds(DeadSymbolTags); } + bool shouldCheckOSObjectRetainCount() const { + return Options.getBooleanOption("CheckOSObject", false, this); + } + void checkEndAnalysis(ExplodedGraph &G, BugReporter &BR, ExprEngine &Eng) const { // FIXME: This is a hack to make sure the summary log gets cleared between @@ -333,10 +340,12 @@ // FIXME: We don't support ARC being turned on and off during one analysis. // (nor, for that matter, do we support changing ASTContexts) bool ARCEnabled = (bool)Ctx.getLangOpts().ObjCAutoRefCount; - if (!Summaries) - Summaries.reset(new RetainSummaryManager(Ctx, ARCEnabled)); - else + if (!Summaries) { + Summaries.reset(new RetainSummaryManager( + Ctx, ARCEnabled, shouldCheckOSObjectRetainCount())); + } else { assert(Summaries->isARCEnabled() == ARCEnabled); + } return *Summaries; } Index: clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp @@ -418,13 +418,18 @@ } // Consult the summary for the return value. + SymbolRef Sym = CallOrMsg.getReturnValue().getAsSymbol(); RetEffect RE = Summ.getRetEffect(); - if (RE.getKind() == RetEffect::NoRetHard) { - SymbolRef Sym = CallOrMsg.getReturnValue().getAsSymbol(); - if (Sym) - state = removeRefBinding(state, Sym); + if (const auto *MCall = dyn_cast(&CallOrMsg)) { + if (Optional updatedRefVal = + refValFromRetEffect(RE, MCall->getResultType())) { + state = setRefBinding(state, Sym, *updatedRefVal); + } } + if (RE.getKind() == RetEffect::NoRetHard && Sym) + state = removeRefBinding(state, Sym); + C.addTransition(state); } @@ -490,11 +495,10 @@ } } - // Evaluate the effect on the message receiver. + // Evaluate the effect on the message receiver / `this` argument. bool ReceiverIsTracked = false; if (!hasErr) { - const ObjCMethodCall *MsgInvocation = dyn_cast(&CallOrMsg); - if (MsgInvocation) { + if (const auto *MsgInvocation = dyn_cast(&CallOrMsg)) { if (SymbolRef Sym = MsgInvocation->getReceiverSVal().getAsLocSymbol()) { if (const RefVal *T = getRefBinding(state, Sym)) { ReceiverIsTracked = true; @@ -506,6 +510,17 @@ } } } + } else if (const auto *MCall = dyn_cast(&CallOrMsg)) { + if (SymbolRef Sym = MCall->getCXXThisVal().getAsLocSymbol()) { + if (const RefVal *T = getRefBinding(state, Sym)) { + state = updateSymbol(state, Sym, *T, Summ.getThisEffect(), + hasErr, C); + if (hasErr) { + ErrorRange = MCall->getOriginExpr()->getSourceRange(); + ErrorSym = Sym; + } + } + } } } Index: clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp +++ clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp @@ -120,6 +120,9 @@ if (CurrV.getObjKind() == RetEffect::CF) { os << " returns a Core Foundation object of type " << Sym->getType().getAsString() << " with a "; + } else if (CurrV.getObjKind() == RetEffect::OS) { + os << " returns an OSObject of type " + << Sym->getType().getAsString() << " with a "; } else if (CurrV.getObjKind() == RetEffect::Generalized) { os << " returns an object of type " << Sym->getType().getAsString() << " with a "; Index: clang/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp =================================================================== --- clang/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp +++ clang/lib/StaticAnalyzer/Core/RetainSummaryManager.cpp @@ -53,6 +53,31 @@ return Summ; } +static bool isOSObjectSubclass(QualType T); + +static bool isOSObjectSubclass(const CXXRecordDecl *RD) { + if (RD->getDeclName().getAsString() == "OSObject") + return true; + + const CXXRecordDecl *RDD = RD->getDefinition(); + if (!RDD) + return false; + + for (const CXXBaseSpecifier Spec : RDD->bases()) { + if (isOSObjectSubclass(Spec.getType())) + return true; + } + return false; +} + +/// \return Whether type represents an OSObject successor. +static bool isOSObjectSubclass(QualType T) { + if (const auto *RD = T->getAsCXXRecordDecl()) { + return isOSObjectSubclass(RD); + } + return false; +} + static bool hasRCAnnotation(const Decl *D, StringRef rcAnnotation) { for (const auto *Ann : D->specific_attrs()) { if (Ann->getAnnotation() == rcAnnotation) @@ -196,6 +221,17 @@ } if (RetTy->isPointerType()) { + if (TrackOSObjects && isOSObjectSubclass(RetTy->getPointeeType())) { + if (const IdentifierInfo *II = FD->getIdentifier()) { + StringRef FuncName = II->getName(); + if (FuncName.contains_lower("with") + || FuncName.contains_lower("create") + || FuncName.contains_lower("copy")) + return getOSSummaryCreateRule(FD); + } + return getOSSummaryGetRule(FD); + } + // For CoreFoundation ('CF') types. if (cocoa::isRefType(RetTy, "CF", FName)) { if (isRetain(FD, FName)) { @@ -241,6 +277,17 @@ } } + if (const auto *MD = dyn_cast(FD)) { + const CXXRecordDecl *Parent = MD->getParent(); + if (TrackOSObjects && isOSObjectSubclass(Parent)) { + if (isRelease(FD, FName)) + return getOSSummaryReleaseRule(FD); + + if (isRetain(FD, FName)) + return getOSSummaryRetainRule(FD); + } + } + // Check for release functions, the only kind of functions that we care // about that don't return a pointer type. if (FName.size() >= 2 && FName[0] == 'C' && @@ -279,6 +326,14 @@ } } + if (const auto *MD = dyn_cast(FD)) { + + // Stop tracking arguments passed to C++ methods, as those might be + // wrapping smart pointers. + return getPersistentSummary(RetEffect::MakeNoRet(), DoNothing, StopTracking, + DoNothing); + } + return getDefaultSummary(); } @@ -411,6 +466,8 @@ Summ = getFunctionSummary(cast(Call).getDecl()); break; case CE_CXXMember: + Summ = getFunctionSummary(cast(Call).getDecl()); + break; case CE_CXXMemberOperator: case CE_Block: case CE_CXXConstructor: @@ -513,6 +570,32 @@ return getPersistentSummary(RetEffect::MakeNoRet(), DoNothing, DoNothing); } +const RetainSummary * +RetainSummaryManager::getOSSummaryRetainRule(const FunctionDecl *FD) { + return getPersistentSummary(RetEffect::MakeNoRet(), + /*ReceiverEff=*/DoNothing, + /*DefaultEff=*/DoNothing, + /*ThisEff=*/IncRef); +} + +const RetainSummary * +RetainSummaryManager::getOSSummaryReleaseRule(const FunctionDecl *FD) { + return getPersistentSummary(RetEffect::MakeNoRet(), + /*ReceiverEff=*/DoNothing, + /*DefaultEff=*/DoNothing, + /*ThisEff=*/DecRef); +} + +const RetainSummary * +RetainSummaryManager::getOSSummaryCreateRule(const FunctionDecl *FD) { + return getPersistentSummary(RetEffect::MakeOwned(RetEffect::OS)); +} + +const RetainSummary * +RetainSummaryManager::getOSSummaryGetRule(const FunctionDecl *FD) { + return getPersistentSummary(RetEffect::MakeNotOwned(RetEffect::OS)); +} + const RetainSummary * RetainSummaryManager::getCFSummaryCreateRule(const FunctionDecl *FD) { assert (ScratchArgs.isEmpty()); @@ -877,7 +960,7 @@ CallEffects CallEffects::getEffect(const ObjCMethodDecl *MD) { ASTContext &Ctx = MD->getASTContext(); LangOptions L = Ctx.getLangOpts(); - RetainSummaryManager M(Ctx, L.ObjCAutoRefCount); + RetainSummaryManager M(Ctx, L.ObjCAutoRefCount, /*TrackOSObjects=*/false); const RetainSummary *S = M.getMethodSummary(MD); CallEffects CE(S->getRetEffect()); CE.Receiver = S->getReceiverEffect(); @@ -891,7 +974,7 @@ CallEffects CallEffects::getEffect(const FunctionDecl *FD) { ASTContext &Ctx = FD->getASTContext(); LangOptions L = Ctx.getLangOpts(); - RetainSummaryManager M(Ctx, L.ObjCAutoRefCount); + RetainSummaryManager M(Ctx, L.ObjCAutoRefCount, /*TrackOSObjects=*/false); const RetainSummary *S = M.getFunctionSummary(FD); CallEffects CE(S->getRetEffect()); unsigned N = FD->param_size(); Index: clang/test/Analysis/osobject-retain-release.cpp =================================================================== --- /dev/null +++ clang/test/Analysis/osobject-retain-release.cpp @@ -0,0 +1,105 @@ +// RUN: %clang_analyze_cc1 -analyze -analyzer-checker=core,osx.cocoa.RetainCount -analyzer-config osx.cocoa.RetainCount:CheckOSObject=true -analyzer-output=text -verify %s + +struct OSObject { + virtual void retain(); + virtual void release(); + + virtual ~OSObject(){} +}; + +struct OSArray : public OSObject { + unsigned int getCount(); + + static OSArray *withCapacity(unsigned int capacity); +}; + +void use_after_release() { + OSArray *arr = OSArray::withCapacity(10); // expected-note{{Call to function 'withCapacity' returns an OSObject of type struct OSArray * with a +1 retain count}} + arr->release(); // expected-note{{Object released}} + arr->getCount(); // expected-warning{{Reference-counted object is used after it is released}} + // expected-note@-1{{Reference-counted object is used after it is released}} +} + +void potential_leak() { + OSArray *arr = OSArray::withCapacity(10); // expected-note{{Call to function 'withCapacity' returns an OSObject of type struct 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}} + arr->getCount(); +} // expected-warning{{Potential leak of an object stored into 'arr'}} + // expected-note@-1{{Object leaked: object allocated and stored into 'arr' is not referenced later in this execution path and has a retain count of +1}} + +void proper_cleanup() { + OSArray *arr = OSArray::withCapacity(10); // +1 + arr->retain(); // +2 + arr->release(); // +1 + arr->getCount(); + arr->release(); // 0 +} + +struct ArrayOwner { + OSArray *arr; + + OSArray *getArray() { + return arr; + } + + OSArray *createArray() { + return OSArray::withCapacity(10); + } + + OSArray *createArraySourceUnknown(); + + OSArray *getArraySourceUnknown(); +}; + +//unsigned int leak_on_create_no_release(ArrayOwner *owner) { + //OSArray *myArray = + +//} + +unsigned int no_warning_on_getter(ArrayOwner *owner) { + OSArray *arr = owner->getArray(); + return arr->getCount(); +} + +unsigned int warn_on_overrelease(ArrayOwner *owner) { + OSArray *arr = owner->getArray(); // expected-note{{function call returns an OSObject of type struct OSArray * with a +0 retain count}} + arr->release(); // expected-warning{{Incorrect decrement of the reference count of an object that is not owned at this point by the caller}} + // expected-note@-1{{Incorrect decrement of the reference count of an object that is not owned at this point by the caller}} + return arr->getCount(); +} + +unsigned int nowarn_on_release_of_created(ArrayOwner *owner) { + OSArray *arr = owner->createArray(); + unsigned int out = arr->getCount(); + arr->release(); + return out; +} + +unsigned int nowarn_on_release_of_created_source_unknown(ArrayOwner *owner) { + OSArray *arr = owner->createArraySourceUnknown(); + unsigned int out = arr->getCount(); + arr->release(); + return out; +} + +unsigned int no_warn_ok_release(ArrayOwner *owner) { + OSArray *arr = owner->getArray(); // +0 + arr->retain(); // +1 + arr->release(); // +0 + return arr->getCount(); // no-warning +} + +unsigned int warn_on_overrelease_with_unknown_source(ArrayOwner *owner) { + OSArray *arr = owner->getArraySourceUnknown(); // expected-note{{function call returns an OSObject of type struct OSArray * with a +0 retain count}} + arr->release(); // expected-warning{{Incorrect decrement of the reference count of an object that is not owned at this point by the caller}} + // expected-note@-1{{Incorrect decrement of the reference count of an object that is not owned at this point by the caller}} + return arr->getCount(); +} + +unsigned int ok_release_with_unknown_source(ArrayOwner *owner) { + OSArray *arr = owner->getArraySourceUnknown(); // +0 + arr->retain(); // +1 + arr->release(); // +0 + return arr->getCount(); +}