Index: clang/include/clang/Analysis/RetainSummaryManager.h =================================================================== --- clang/include/clang/Analysis/RetainSummaryManager.h +++ clang/include/clang/Analysis/RetainSummaryManager.h @@ -326,14 +326,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). @@ -367,12 +374,16 @@ /// This is only meaningful if the summary applies to an ObjCMessageExpr*. ArgEffect getReceiverEffect() const { return Receiver; } + ArgEffect getThisEffect() const { return This; } + /// 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. @@ -380,6 +391,7 @@ ID.Add(Args); ID.Add(DefaultArgEffect); ID.Add(Receiver); + ID.Add(This); ID.Add(Ret); } @@ -504,21 +516,36 @@ /// 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); + + const RetainSummary *getCFSummaryGetRule(const FunctionDecl *FD); + enum UnaryFuncKind { cfretain, cfrelease, cfautorelease, cfmakecollectable }; const RetainSummary *getUnarySummary(const FunctionType* FT, UnaryFuncKind func); const RetainSummary *getCFSummaryCreateRule(const FunctionDecl *FD); - const RetainSummary *getCFSummaryGetRule(const FunctionDecl *FD); const RetainSummary *getCFCreateGetRuleSummary(const FunctionDecl *FD); const RetainSummary *getPersistentSummary(const RetainSummary &OldSumm); 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); } Index: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h =================================================================== --- clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h +++ clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h @@ -719,6 +719,10 @@ return 0; } + SVal getThisSVal() const { + return getSVal(getCXXThisExpr()); + } + const Expr *getArgExpr(unsigned Index) const override { return getOriginExpr()->getArg(Index); } Index: clang/lib/Analysis/RetainSummaryManager.cpp =================================================================== --- clang/lib/Analysis/RetainSummaryManager.cpp +++ clang/lib/Analysis/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) @@ -107,6 +132,7 @@ // filters. assert(ScratchArgs.isEmpty()); + // TODO: do those cases do anything at all since we have IPA already? if (FName == "pthread_create" || FName == "pthread_setspecific") { // Part of: and . // This will be addressed better with IPA. @@ -196,6 +222,14 @@ } if (RetTy->isPointerType()) { + if (isOSObjectSubclass(RetTy->getPointeeType())) { + const IdentifierInfo *II = FD->getIdentifier(); + if (II && (II->getName().startswith_lower("with") + || II->getName().contains_lower("create"))) + return getOSSummaryCreateRule(FD); + return getOSSummaryGetRule(FD); + } + // For CoreFoundation ('CF') types. if (cocoa::isRefType(RetTy, "CF", FName)) { if (isRetain(FD, FName)) { @@ -241,6 +275,17 @@ } } + if (const auto *MD = dyn_cast(FD)) { + const CXXRecordDecl *Parent = MD->getParent(); + if (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' && @@ -411,6 +456,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 +560,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()); 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. Index: clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp @@ -398,21 +398,43 @@ } // Evaluate the effect on the message receiver. - const ObjCMethodCall *MsgInvocation = dyn_cast(&CallOrMsg); - if (MsgInvocation) { + RefVal::Kind hasErr = (RefVal::Kind) 0; // TODO? + if (const auto *MsgInvocation = dyn_cast(&CallOrMsg)) { if (SymbolRef Sym = MsgInvocation->getReceiverSVal().getAsLocSymbol()) { if (Summ.getReceiverEffect() == StopTrackingHard) { state = removeRefBinding(state, Sym); } } + } else if (const auto *MCall = dyn_cast(&CallOrMsg)) { + // TODO: code duplication with branch above? + // TODO: is it valid to have this code here? + llvm::errs() << "Processing summary of inlined" << "\n"; + if (SymbolRef Sym = + MCall->getThisSVal().getAsLocSymbol()) { + if (const RefVal *T = getRefBinding(state, Sym)) { + state = updateSymbol(state, Sym, *T, Summ.getThisEffect(), + hasErr, C); + + } + } } // Consult the summary for the return value. RetEffect RE = Summ.getRetEffect(); if (RE.getKind() == RetEffect::NoRetHard) { SymbolRef Sym = CallOrMsg.getReturnValue().getAsSymbol(); - if (Sym) + if (Sym) { state = removeRefBinding(state, Sym); + + if (const auto *MCall = dyn_cast(&CallOrMsg)) { + if (const RefVal *T = getRefBinding(state, Sym)) { + Summ.getRetEffect().isOwned(); // TODO + //state = setRefBinding(state, Sym, + //Summ.getRetEffect()) + //state = updateSymbol(state, Sym, *T, Summ.getRetEffect(), hasErr, C); + } + } + } } C.addTransition(state); @@ -480,11 +502,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; @@ -496,6 +517,15 @@ } } } + } else if (const auto *MCall = dyn_cast(&CallOrMsg)) { + // TODO: code duplication with branch above? + if (SymbolRef Sym = MCall->getThisSVal().getAsLocSymbol()) { + if (const RefVal *T = getRefBinding(state, Sym)) { + state = updateSymbol(state, Sym, *T, Summ.getThisEffect(), + hasErr, C); + + } + } } } @@ -1004,7 +1034,7 @@ // does not understand. ProgramStateRef state = C.getState(); - if (Optional regionLoc = loc.getAs()) { + if (auto regionLoc = loc.getAs()) { escapes = !regionLoc->getRegion()->hasStackStorage(); if (!escapes) { @@ -1028,7 +1058,7 @@ // If we are storing the value into an auto function scope variable annotated // with (__attribute__((cleanup))), stop tracking the value to avoid leak // false positives. - if (const VarRegion *LVR = dyn_cast_or_null(loc.getAsRegion())) { + if (const auto *LVR = dyn_cast_or_null(loc.getAsRegion())) { const VarDecl *VD = LVR->getDecl(); if (VD->hasAttr()) { escapes = true; @@ -1048,8 +1078,8 @@ } ProgramStateRef RetainCountChecker::evalAssume(ProgramStateRef state, - SVal Cond, - bool Assumption) const { + SVal Cond, + bool Assumption) const { // FIXME: We may add to the interface of evalAssume the list of symbols // whose assumptions have changed. For now we just iterate through the // bindings and check if any of the tracked symbols are NULL. This isn't 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 "; @@ -457,8 +460,7 @@ " This violates the naming convention rules" " given in the Memory Management Guide for Cocoa"; } - } - else { + } else { const FunctionDecl *FD = cast(D); os << "whose name ('" << *FD << "') does not contain 'Copy' or 'Create'. This violates the naming" Index: clang/test/Analysis/osobject-retain-release.cpp =================================================================== --- /dev/null +++ clang/test/Analysis/osobject-retain-release.cpp @@ -0,0 +1,90 @@ +// RUN: %clang_analyze_cc1 -analyze -analyzer-checker=core,osx -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); + arr->release(); // 0 + 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(); // +0 + arr->release(); // -1 + return arr->getCount(); // expected-warning{{}} +} + +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(); // +0 + 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(); +}