[analyzer] Don't treat calls to system headers as escaping in CheckObjCDealloc.
This prevents false negatives when a -dealloc method, for example, removes itself as
as an observer with [[NSNotificationCenter defaultCenter] removeObserver:self]. It is
unlikely that passing 'self' to a system header method will release 'self''s instance
variables, so this is unlikely to produce false positives.
A challenge here is that while CheckObjCDealloc no longer treats these calls as
escaping, the rest of the analyzer still does. In particular, this means that loads
from the same instance variable before and after a call to a system header will
result in different symbols being loaded by the region store. To account for this,
the checker now treats different ivar symbols with the same instance and ivar decl as
the same for the purpose of release checking and more eagerly removes a release
requirement when an instance variable is assumed to be nil. This was not needed before
because when an ivar escaped its release requirement was always removed -- now the
requirement is not removed for calls to system headers.
llvm-svn: 262261
diff --git a/clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp b/clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
index 42999e2..f72a1ae 100644
--- a/clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/CheckObjCDealloc.cpp
@@ -93,6 +93,7 @@
: public Checker<check::ASTDecl<ObjCImplementationDecl>,
check::PreObjCMessage, check::PostObjCMessage,
check::BeginFunction, check::EndFunction,
+ eval::Assume,
check::PointerEscape,
check::PreStmt<ReturnStmt>> {
@@ -111,6 +112,9 @@
void checkPreObjCMessage(const ObjCMethodCall &M, CheckerContext &C) const;
void checkPostObjCMessage(const ObjCMethodCall &M, CheckerContext &C) const;
+ ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond,
+ bool Assumption) const;
+
ProgramStateRef checkPointerEscape(ProgramStateRef State,
const InvalidatedSymbols &Escaped,
const CallEvent *Call,
@@ -129,6 +133,7 @@
SymbolRef getValueReleasedByNillingOut(const ObjCMethodCall &M,
CheckerContext &C) const;
+ const ObjCIvarRegion *getIvarRegionForIvarSymbol(SymbolRef IvarSym) const;
SymbolRef getInstanceSymbolFromIvarSymbol(SymbolRef IvarSym) const;
ReleaseRequirement
@@ -284,20 +289,31 @@
}
}
+/// Given a symbol for an ivar, return the ivar region it was loaded from.
+/// Returns nullptr if the instance symbol cannot be found.
+const ObjCIvarRegion *
+ObjCDeallocChecker::getIvarRegionForIvarSymbol(SymbolRef IvarSym) const {
+ const MemRegion *RegionLoadedFrom = nullptr;
+ if (auto *DerivedSym = dyn_cast<SymbolDerived>(IvarSym))
+ RegionLoadedFrom = DerivedSym->getRegion();
+ else if (auto *RegionSym = dyn_cast<SymbolRegionValue>(IvarSym))
+ RegionLoadedFrom = RegionSym->getRegion();
+ else
+ return nullptr;
+
+ return dyn_cast<ObjCIvarRegion>(RegionLoadedFrom);
+}
+
/// Given a symbol for an ivar, return a symbol for the instance containing
/// the ivar. Returns nullptr if the instance symbol cannot be found.
SymbolRef
ObjCDeallocChecker::getInstanceSymbolFromIvarSymbol(SymbolRef IvarSym) const {
- if (auto *SRV = dyn_cast<SymbolRegionValue>(IvarSym)) {
- const TypedValueRegion *TVR = SRV->getRegion();
- const ObjCIvarRegion *IvarRegion = dyn_cast_or_null<ObjCIvarRegion>(TVR);
- if (!IvarRegion)
- return nullptr;
- return IvarRegion->getSymbolicBase()->getSymbol();
- }
+ const ObjCIvarRegion *IvarRegion = getIvarRegionForIvarSymbol(IvarSym);
+ if (!IvarRegion)
+ return nullptr;
- return nullptr;
+ return IvarRegion->getSymbolicBase()->getSymbol();
}
/// If we are in -dealloc or -dealloc is on the stack, handle the call if it is
@@ -362,11 +378,58 @@
diagnoseMissingReleases(C);
}
+/// When a symbol is assumed to be nil, remove it from the set of symbols
+/// require to be nil.
+ProgramStateRef ObjCDeallocChecker::evalAssume(ProgramStateRef State, SVal Cond,
+ bool Assumption) const {
+ if (State->get<UnreleasedIvarMap>().isEmpty())
+ return State;
+
+ auto *CondBSE = dyn_cast_or_null<BinarySymExpr>(Cond.getAsSymExpr());
+ if (!CondBSE)
+ return State;
+
+ BinaryOperator::Opcode OpCode = CondBSE->getOpcode();
+ if (Assumption) {
+ if (OpCode != BO_EQ)
+ return State;
+ } else {
+ if (OpCode != BO_NE)
+ return State;
+ }
+
+ SymbolRef NullSymbol = nullptr;
+ if (auto *SIE = dyn_cast<SymIntExpr>(CondBSE)) {
+ const llvm::APInt &RHS = SIE->getRHS();
+ if (RHS != 0)
+ return State;
+ NullSymbol = SIE->getLHS();
+ } else if (auto *SIE = dyn_cast<IntSymExpr>(CondBSE)) {
+ const llvm::APInt &LHS = SIE->getLHS();
+ if (LHS != 0)
+ return State;
+ NullSymbol = SIE->getRHS();
+ } else {
+ return State;
+ }
+
+ SymbolRef InstanceSymbol = getInstanceSymbolFromIvarSymbol(NullSymbol);
+ if (!InstanceSymbol)
+ return State;
+
+ State = removeValueRequiringRelease(State, InstanceSymbol, NullSymbol);
+
+ return State;
+}
+
/// If a symbol escapes conservatively assume unseen code released it.
ProgramStateRef ObjCDeallocChecker::checkPointerEscape(
ProgramStateRef State, const InvalidatedSymbols &Escaped,
const CallEvent *Call, PointerEscapeKind Kind) const {
+ if (State->get<UnreleasedIvarMap>().isEmpty())
+ return State;
+
// Don't treat calls to '[super dealloc]' as escaping for the purposes
// of this checker. Because the checker diagnoses missing releases in the
// post-message handler for '[super dealloc], escaping here would cause
@@ -376,7 +439,17 @@
return State;
for (const auto &Sym : Escaped) {
- State = State->remove<UnreleasedIvarMap>(Sym);
+ if (!Call || (Call && !Call->isInSystemHeader())) {
+ // If Sym is a symbol for an object with instance variables that
+ // must be released, remove these obligations when the object escapes
+ // unless via a call to a system function. System functions are
+ // very unlikely to release instance variables on objects passed to them,
+ // and are frequently called on 'self' in -dealloc (e.g., to remove
+ // observers) -- we want to avoid false negatives from escaping on
+ // them.
+ State = State->remove<UnreleasedIvarMap>(Sym);
+ }
+
SymbolRef InstanceSymbol = getInstanceSymbolFromIvarSymbol(Sym);
if (!InstanceSymbol)
@@ -424,9 +497,10 @@
if (SelfRegion != IvarRegion->getSuperRegion())
continue;
+ const ObjCIvarDecl *IvarDecl = IvarRegion->getDecl();
// Prevent an inlined call to -dealloc in a super class from warning
// about the values the subclass's -dealloc should release.
- if (IvarRegion->getDecl()->getContainingInterface() !=
+ if (IvarDecl->getContainingInterface() !=
cast<ObjCMethodDecl>(LCtx->getDecl())->getClassInterface())
continue;
@@ -452,7 +526,6 @@
std::string Buf;
llvm::raw_string_ostream OS(Buf);
- const ObjCIvarDecl *IvarDecl = IvarRegion->getDecl();
const ObjCInterfaceDecl *Interface = IvarDecl->getContainingInterface();
// If the class is known to have a lifecycle with teardown that is
// separate from -dealloc, do not warn about missing releases. We
@@ -521,15 +594,7 @@
// values that must not be released in the state. This is because even if
// these values escape, it is still an error under the rules of MRR to
// release them in -dealloc.
- const MemRegion *RegionLoadedFrom = nullptr;
- if (auto *DerivedSym = dyn_cast<SymbolDerived>(ReleasedValue))
- RegionLoadedFrom = DerivedSym->getRegion();
- else if (auto *RegionSym = dyn_cast<SymbolRegionValue>(ReleasedValue))
- RegionLoadedFrom = RegionSym->getRegion();
- else
- return false;
-
- auto *ReleasedIvar = dyn_cast<ObjCIvarRegion>(RegionLoadedFrom);
+ auto *ReleasedIvar = getIvarRegionForIvarSymbol(ReleasedValue);
if (!ReleasedIvar)
return false;
@@ -680,6 +745,9 @@
ProgramStateRef State, SymbolRef Instance, SymbolRef Value) const {
assert(Instance);
assert(Value);
+ const ObjCIvarRegion *RemovedRegion = getIvarRegionForIvarSymbol(Value);
+ if (!RemovedRegion)
+ return State;
const SymbolSet *Unreleased = State->get<UnreleasedIvarMap>(Instance);
if (!Unreleased)
@@ -687,7 +755,14 @@
// Mark the value as no longer requiring a release.
SymbolSet::Factory &F = State->getStateManager().get_context<SymbolSet>();
- SymbolSet NewUnreleased = F.remove(*Unreleased, Value);
+ SymbolSet NewUnreleased = *Unreleased;
+ for (auto &Sym : *Unreleased) {
+ const ObjCIvarRegion *UnreleasedRegion = getIvarRegionForIvarSymbol(Sym);
+ assert(UnreleasedRegion);
+ if (RemovedRegion->getDecl() == UnreleasedRegion->getDecl()) {
+ NewUnreleased = F.remove(NewUnreleased, Sym);
+ }
+ }
if (NewUnreleased.isEmpty()) {
return State->remove<UnreleasedIvarMap>(Instance);
diff --git a/clang/test/Analysis/DeallocMissingRelease.m b/clang/test/Analysis/DeallocMissingRelease.m
index c7769db..bb67e436 100644
--- a/clang/test/Analysis/DeallocMissingRelease.m
+++ b/clang/test/Analysis/DeallocMissingRelease.m
@@ -1,7 +1,7 @@
// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.osx.cocoa.Dealloc -fblocks -verify %s
// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.osx.cocoa.Dealloc -fblocks -triple x86_64-apple-darwin10 -fobjc-arc -fobjc-runtime-has-weak -verify %s
-#define nil ((id)0)
+#include "Inputs/system-header-simulator-for-objc-dealloc.h"
#define NON_ARC !__has_feature(objc_arc)
@@ -16,22 +16,6 @@
// expected-no-diagnostics
#endif
-typedef signed char BOOL;
-@protocol NSObject
-- (BOOL)isEqual:(id)object;
-- (Class)class;
-@end
-
-@interface NSObject <NSObject> {}
-+ (instancetype)alloc;
-- (void)dealloc;
-- (id)init;
-- (id)retain;
-- (oneway void)release;
-@end
-
-typedef struct objc_selector *SEL;
-
// Do not warn about missing release in -dealloc for ivars.
@interface MyIvarClass1 : NSObject {
@@ -447,6 +431,50 @@
}
@end
+
+// Don't treat calls to system headers as escapes
+
+@interface ClassWhereSelfEscapesViaCallToSystem : NSObject
+@property (retain) NSObject *ivar1;
+@property (retain) NSObject *ivar2;
+@property (retain) NSObject *ivar3;
+@property (retain) NSObject *ivar4;
+@property (retain) NSObject *ivar5;
+@property (retain) NSObject *ivar6;
+@end
+
+@implementation ClassWhereSelfEscapesViaCallToSystem
+- (void)dealloc; {
+#if NON_ARC
+ [_ivar2 release];
+ if (_ivar3) {
+ [_ivar3 release];
+ }
+#endif
+
+ [[NSRunLoop currentRunLoop] cancelPerformSelectorsWithTarget:self];
+ [[NSNotificationCenter defaultCenter] removeObserver:self];
+
+#if NON_ARC
+ [_ivar4 release];
+
+ if (_ivar5) {
+ [_ivar5 release];
+ }
+#endif
+
+ [[NSNotificationCenter defaultCenter] removeObserver:self];
+
+#if NON_ARC
+ if (_ivar6) {
+ [_ivar6 release];
+ }
+
+ [super dealloc]; // expected-warning {{The '_ivar1' ivar in 'ClassWhereSelfEscapesViaCallToSystem' was retained by a synthesized property but not released before '[super dealloc]'}}
+#endif
+}
+@end
+
// Don't warn when value escapes.
@interface ClassWhereIvarValueEscapes : NSObject
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
new file mode 100644
index 0000000..0b9888a
--- /dev/null
+++ b/clang/test/Analysis/Inputs/system-header-simulator-for-objc-dealloc.h
@@ -0,0 +1,29 @@
+#pragma clang system_header
+
+#define nil ((id)0)
+
+typedef signed char BOOL;
+@protocol NSObject
+- (BOOL)isEqual:(id)object;
+- (Class)class;
+@end
+
+@interface NSObject <NSObject> {}
++ (instancetype)alloc;
+- (void)dealloc;
+- (id)init;
+- (id)retain;
+- (oneway void)release;
+@end
+
+@interface NSRunLoop : NSObject
++ (NSRunLoop *)currentRunLoop;
+- (void)cancelPerformSelectorsWithTarget:(id)target;
+@end
+
+@interface NSNotificationCenter : NSObject
++ (NSNotificationCenter *)defaultCenter;
+- (void)removeObserver:(id)observer;
+@end
+
+typedef struct objc_selector *SEL;