From b8076292abcfb10d87f3b107dd871b7327d18534 Mon Sep 17 00:00:00 2001 From: Devin Coughlin Date: Fri, 25 Mar 2016 21:18:22 +0000 Subject: [PATCH] [analyzer] Add CIFIlter modeling to DeallocChecker. The -dealloc method in CIFilter is highly unusual in that it will release instance variables belonging to its *subclasses* if the variable name starts with "input" or backs a property whose name starts with "input". Subclasses should not release these ivars in their own -dealloc method -- doing so could result in an over release. Before this commit, the DeallocChecker would warn about missing releases for such "input" properties -- which could cause users of the analyzer to add over releases to silence the warning. To avoid this, DeallocChecker now treats CIFilter "input-prefixed" ivars as MustNotReleaseDirectly and so will not require a release. Further, it will now warn when such an ivar is directly released in -dealloc. rdar://problem/25364901 llvm-svn: 264463 --- .../StaticAnalyzer/Checkers/CheckObjCDealloc.cpp | 66 +++++++++++++--- clang/test/Analysis/DeallocMissingRelease.m | 92 +++++++++++++++++++++- .../system-header-simulator-for-objc-dealloc.h | 3 + 3 files changed, 150 insertions(+), 11 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp b/clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp index 84856bd..bee9dead 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp @@ -98,7 +98,8 @@ class ObjCDeallocChecker check::PointerEscape, check::PreStmt> { - mutable IdentifierInfo *NSObjectII, *SenTestCaseII, *Block_releaseII; + mutable IdentifierInfo *NSObjectII, *SenTestCaseII, *Block_releaseII, + *CIFilterII; mutable Selector DeallocSel, ReleaseSel; std::unique_ptr MissingReleaseBugType; @@ -169,6 +170,8 @@ private: void initIdentifierInfoAndSelectors(ASTContext &Ctx) const; bool classHasSeparateTeardown(const ObjCInterfaceDecl *ID) const; + + bool isReleasedByCIFilterDealloc(const ObjCPropertyImplDecl *PropImpl) const; }; } // End anonymous namespace. @@ -688,19 +691,27 @@ bool ObjCDeallocChecker::diagnoseExtraRelease(SymbolRef ReleasedValue, assert(PropDecl->getSetterKind() == ObjCPropertyDecl::Weak || (PropDecl->getSetterKind() == ObjCPropertyDecl::Assign && - !PropDecl->isReadOnly())); + !PropDecl->isReadOnly()) || + isReleasedByCIFilterDealloc(PropImpl) + ); const ObjCImplDecl *Container = getContainingObjCImpl(C.getLocationContext()); OS << "The '" << *PropImpl->getPropertyIvarDecl() - << "' ivar in '" << *Container - << "' was synthesized for "; + << "' ivar in '" << *Container; - if (PropDecl->getSetterKind() == ObjCPropertyDecl::Weak) - OS << "a weak"; - else - OS << "an assign, readwrite"; - OS << " property but was released in 'dealloc'"; + if (isReleasedByCIFilterDealloc(PropImpl)) { + OS << "' will be released by '-[CIFilter dealloc]' but also released here"; + } else { + OS << "' was synthesized for "; + + if (PropDecl->getSetterKind() == ObjCPropertyDecl::Weak) + OS << "a weak"; + else + OS << "an assign, readwrite"; + + OS << " property but was released in 'dealloc'"; + } std::unique_ptr BR( new BugReport(*ExtraReleaseBugType, OS.str(), ErrNode)); @@ -751,7 +762,7 @@ bool ObjCDeallocChecker::diagnoseMistakenDealloc(SymbolRef DeallocedValue, ObjCDeallocChecker:: ObjCDeallocChecker() - : NSObjectII(nullptr), SenTestCaseII(nullptr) { + : NSObjectII(nullptr), SenTestCaseII(nullptr), CIFilterII(nullptr) { MissingReleaseBugType.reset( new BugType(this, "Missing ivar release (leak)", @@ -774,6 +785,7 @@ void ObjCDeallocChecker::initIdentifierInfoAndSelectors( NSObjectII = &Ctx.Idents.get("NSObject"); SenTestCaseII = &Ctx.Idents.get("SenTestCase"); Block_releaseII = &Ctx.Idents.get("_Block_release"); + CIFilterII = &Ctx.Idents.get("CIFilter"); IdentifierInfo *DeallocII = &Ctx.Idents.get("dealloc"); IdentifierInfo *ReleaseII = &Ctx.Idents.get("release"); @@ -894,6 +906,9 @@ ReleaseRequirement ObjCDeallocChecker::getDeallocReleaseRequirement( // the value in their instance variables must be released in -dealloc. case ObjCPropertyDecl::Retain: case ObjCPropertyDecl::Copy: + if (isReleasedByCIFilterDealloc(PropImpl)) + return ReleaseRequirement::MustNotReleaseDirectly; + return ReleaseRequirement::MustRelease; case ObjCPropertyDecl::Weak: @@ -1019,6 +1034,37 @@ bool ObjCDeallocChecker::classHasSeparateTeardown( return true; } +/// The -dealloc method in CIFilter highly unusual in that is will release +/// instance variables belonging to its *subclasses* if the variable name +/// starts with "input" or backs a property whose name starts with "input". +/// Subclasses should not release these ivars in their own -dealloc method -- +/// doing so could result in an over release. +/// +/// This method returns true if the property will be released by +/// -[CIFilter dealloc]. +bool ObjCDeallocChecker::isReleasedByCIFilterDealloc( + const ObjCPropertyImplDecl *PropImpl) const { + assert(PropImpl->getPropertyIvarDecl()); + StringRef PropName = PropImpl->getPropertyDecl()->getName(); + StringRef IvarName = PropImpl->getPropertyIvarDecl()->getName(); + + const char *ReleasePrefix = "input"; + if (!(PropName.startswith(ReleasePrefix) || + IvarName.startswith(ReleasePrefix))) { + return false; + } + + const ObjCInterfaceDecl *ID = + PropImpl->getPropertyIvarDecl()->getContainingInterface(); + for ( ; ID ; ID = ID->getSuperClass()) { + IdentifierInfo *II = ID->getIdentifier(); + if (II == CIFilterII) + return true; + } + + return false; +} + void ento::registerObjCDeallocChecker(CheckerManager &Mgr) { const LangOptions &LangOpts = Mgr.getLangOpts(); // These checker only makes sense under MRR. diff --git a/clang/test/Analysis/DeallocMissingRelease.m b/clang/test/Analysis/DeallocMissingRelease.m index 26f32db..01fcc6e 100644 --- a/clang/test/Analysis/DeallocMissingRelease.m +++ b/clang/test/Analysis/DeallocMissingRelease.m @@ -747,10 +747,100 @@ __attribute__((objc_root_class)) { #if NON_ARC // Only warn for synthesized ivars. - [okToDeallocDirectly dealloc]; // now-warning + [okToDeallocDirectly dealloc]; // no-warning [_ivar dealloc]; // expected-warning {{'_ivar' should be released rather than deallocated}} [super dealloc]; #endif } @end + +// CIFilter special cases. +// By design, -[CIFilter dealloc] releases (by calling -setValue: forKey: with +// 'nil') all ivars (even in its *subclasses*) with names starting with +// 'input' or that are backed by properties with names starting with 'input'. +// The Dealloc checker needs to take particular care to not warn about missing +// releases in this case -- if the user adds a release quiet the +// warning it may result in an over release. + +@interface ImmediateSubCIFilter : CIFilter { + NSObject *inputIvar; + NSObject *nonInputIvar; + NSObject *notPrefixedButBackingPrefixedProperty; + NSObject *inputPrefixedButBackingNonPrefixedProperty; +} + +@property(retain) NSObject *inputIvar; +@property(retain) NSObject *nonInputIvar; +@property(retain) NSObject *inputAutoSynthesizedIvar; +@property(retain) NSObject *inputExplicitlySynthesizedToNonPrefixedIvar; +@property(retain) NSObject *nonPrefixedPropertyBackedByExplicitlySynthesizedPrefixedIvar; + +@end + +@implementation ImmediateSubCIFilter +@synthesize inputIvar = inputIvar; +@synthesize nonInputIvar = nonInputIvar; +@synthesize inputExplicitlySynthesizedToNonPrefixedIvar = notPrefixedButBackingPrefixedProperty; +@synthesize nonPrefixedPropertyBackedByExplicitlySynthesizedPrefixedIvar = inputPrefixedButBackingNonPrefixedProperty; + +- (void)dealloc { +#if NON_ARC + // We don't want warnings here for: + // inputIvar + // inputAutoSynthesizedIvar + // inputExplicitlySynthesizedToNonPrefixedIvar + // inputPrefixedButBackingNonPrefixedProperty + [super dealloc]; + // expected-warning@-1 {{The 'nonInputIvar' ivar in 'ImmediateSubCIFilter' was retained by a synthesized property but not released before '[super dealloc]'}} +#endif +} +@end + +@interface SubSubCIFilter : CIFilter { + NSObject *inputIvarInSub; +} + +@property(retain) NSObject *inputIvarInSub; +@end + +@implementation SubSubCIFilter +@synthesize inputIvarInSub = inputIvarInSub; + +- (void)dealloc { +// Don't warn about inputIvarInSub. +#if NON_ARC + [super dealloc]; +#endif +} +@end +@interface OverreleasingCIFilter : CIFilter { + NSObject *inputIvar; +} + +@property(retain) NSObject *inputIvar; +@end + +@implementation OverreleasingCIFilter +@synthesize inputIvar = inputIvar; + +- (void)dealloc { +#if NON_ARC + // This is an over release because CIFilter's dealloc will also release it. + [inputIvar release]; // expected-warning {{The 'inputIvar' ivar in 'OverreleasingCIFilter' will be released by '-[CIFilter dealloc]' but also released here}} + [super dealloc]; // no-warning + #endif +} +@end + + +@interface NotMissingDeallocCIFilter : CIFilter { + NSObject *inputIvar; +} + +@property(retain) NSObject *inputIvar; +@end + +@implementation NotMissingDeallocCIFilter // no-warning +@synthesize inputIvar = inputIvar; +@end diff --git a/clang/test/Analysis/Inputs/system-header-simulator-for-objc-dealloc.h b/clang/test/Analysis/Inputs/system-header-simulator-for-objc-dealloc.h index 9850aec..231c0bf 100644 --- a/clang/test/Analysis/Inputs/system-header-simulator-for-objc-dealloc.h +++ b/clang/test/Analysis/Inputs/system-header-simulator-for-objc-dealloc.h @@ -30,3 +30,6 @@ typedef struct objc_selector *SEL; void _Block_release(const void *aBlock); #define Block_release(...) _Block_release((const void *)(__VA_ARGS__)) + +@interface CIFilter : NSObject +@end -- 2.7.4