From d7a31ba6db617e38bb064df0ab09dbd41cdfed18 Mon Sep 17 00:00:00 2001 From: Argyrios Kyrtzidis Date: Tue, 11 Jan 2011 19:45:25 +0000 Subject: [PATCH] [analyzer] Introduce ObjCSelfInitChecker, which checks initialization methods to verify that they assign 'self' to the result of an initialization call (e.g. [super init], or [self initWith..]) before using any instance variable or returning 'self'. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@123264 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/StaticAnalyzer/Checkers/CMakeLists.txt | 1 + lib/StaticAnalyzer/Checkers/ExprEngine.cpp | 1 + .../Checkers/ExprEngineInternalChecks.h | 1 + .../Checkers/ObjCSelfInitChecker.cpp | 306 ++++++++++++++++++ test/Analysis/retain-release.m | 4 +- test/Analysis/self-init.m | 139 ++++++++ 6 files changed, 450 insertions(+), 2 deletions(-) create mode 100644 lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp create mode 100644 test/Analysis/self-init.m diff --git a/lib/StaticAnalyzer/Checkers/CMakeLists.txt b/lib/StaticAnalyzer/Checkers/CMakeLists.txt index abb10fba9f..a999cfc3e2 100644 --- a/lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ b/lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -35,6 +35,7 @@ add_clang_library(clangStaticAnalyzerCheckers NoReturnFunctionChecker.cpp OSAtomicChecker.cpp ObjCAtSyncChecker.cpp + ObjCSelfInitChecker.cpp ObjCUnusedIVarsChecker.cpp PointerArithChecker.cpp PointerSubChecker.cpp diff --git a/lib/StaticAnalyzer/Checkers/ExprEngine.cpp b/lib/StaticAnalyzer/Checkers/ExprEngine.cpp index f19f806423..e912273e1a 100644 --- a/lib/StaticAnalyzer/Checkers/ExprEngine.cpp +++ b/lib/StaticAnalyzer/Checkers/ExprEngine.cpp @@ -270,6 +270,7 @@ static void RegisterInternalChecks(ExprEngine &Eng) { RegisterUndefResultChecker(Eng); RegisterStackAddrLeakChecker(Eng); RegisterObjCAtSyncChecker(Eng); + registerObjCSelfInitChecker(Eng); // This is not a checker yet. RegisterNoReturnFunctionChecker(Eng); diff --git a/lib/StaticAnalyzer/Checkers/ExprEngineInternalChecks.h b/lib/StaticAnalyzer/Checkers/ExprEngineInternalChecks.h index f67371da0f..2903f2d4cf 100644 --- a/lib/StaticAnalyzer/Checkers/ExprEngineInternalChecks.h +++ b/lib/StaticAnalyzer/Checkers/ExprEngineInternalChecks.h @@ -46,6 +46,7 @@ void RegisterUndefResultChecker(ExprEngine &Eng); void RegisterUndefinedArraySubscriptChecker(ExprEngine &Eng); void RegisterUndefinedAssignmentChecker(ExprEngine &Eng); void RegisterVLASizeChecker(ExprEngine &Eng); +void registerObjCSelfInitChecker(ExprEngine &Eng); // API checks. void RegisterMacOSXAPIChecker(ExprEngine &Eng); diff --git a/lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp b/lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp new file mode 100644 index 0000000000..9eb6b19ec7 --- /dev/null +++ b/lib/StaticAnalyzer/Checkers/ObjCSelfInitChecker.cpp @@ -0,0 +1,306 @@ +//== ObjCSelfInitChecker.cpp - Checker for 'self' initialization -*- C++ -*--=// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// +// +// This defines ObjCSelfInitChecker, a builtin check that checks for uses of +// 'self' before proper initialization. +// +//===----------------------------------------------------------------------===// + +// This checks initialization methods to verify that they assign 'self' to the +// result of an initialization call (e.g. [super init], or [self initWith..]) +// before using 'self' or any instance variable. +// +// To perform the required checking, values are tagged wih flags that indicate +// 1) if the object is the one pointed to by 'self', and 2) if the object +// is the result of an initializer (e.g. [super init]). +// +// Uses of an object that is true for 1) but not 2) trigger a diagnostic. +// The uses that are currently checked are: +// - Using instance variables. +// - Returning the object. +// +// Note that we don't check for an invalid 'self' that is the receiver of an +// obj-c message expression to cut down false positives where logging functions +// get information from self (like its class) or doing "invalidation" on self +// when the initialization fails. +// +// Because the object that 'self' points to gets invalidated when a call +// receives a reference to 'self', the checker keeps track and passes the flags +// for 1) and 2) to the new object that 'self' points to after the call. +// +// FIXME (rdar://7937506): In the case of: +// [super init]; +// return self; +// Have an extra PathDiagnosticPiece in the path that says "called [super init], +// but didn't assign the result to self." + +//===----------------------------------------------------------------------===// + +// FIXME: Somehow stick the link to Apple's documentation about initializing +// objects in the diagnostics. +// http://developer.apple.com/library/mac/#documentation/Cocoa/Conceptual/ObjectiveC/Articles/ocAllocInit.html + +#include "ExprEngineInternalChecks.h" +#include "clang/StaticAnalyzer/PathSensitive/CheckerVisitor.h" +#include "clang/StaticAnalyzer/PathSensitive/GRStateTrait.h" +#include "clang/StaticAnalyzer/BugReporter/BugType.h" +#include "clang/Analysis/DomainSpecific/CocoaConventions.h" +#include "clang/AST/ParentMap.h" + +using namespace clang; +using namespace ento; + +static bool shouldRunOnFunctionOrMethod(const NamedDecl *ND); +static bool isInitializationMethod(const ObjCMethodDecl *MD); +static bool isInitMessage(const ObjCMessageExpr *E); +static bool isSelfVar(SVal location, CheckerContext &C); + +namespace { +enum SelfFlagEnum { + /// \brief No flag set. + SelfFlag_None = 0x0, + /// \brief Value came from 'self'. + SelfFlag_Self = 0x1, + /// \brief Value came from the result of an initializer (e.g. [super init]). + SelfFlag_InitRes = 0x2 +}; +} + +namespace { +class ObjCSelfInitChecker : public CheckerVisitor { + /// \brief A call receiving a reference to 'self' invalidates the object that + /// 'self' contains. This field keeps the "self flags" assigned to the 'self' + /// object before the call and assign them to the new object that 'self' + /// points to after the call. + SelfFlagEnum preCallSelfFlags; + +public: + static void *getTag() { static int tag = 0; return &tag; } + void PostVisitObjCMessageExpr(CheckerContext &C, const ObjCMessageExpr *E); + void PostVisitObjCIvarRefExpr(CheckerContext &C, const ObjCIvarRefExpr *E); + void PreVisitReturnStmt(CheckerContext &C, const ReturnStmt *S); + void PreVisitGenericCall(CheckerContext &C, const CallExpr *CE); + void PostVisitGenericCall(CheckerContext &C, const CallExpr *CE); + virtual void visitLocation(CheckerContext &C, const Stmt *S, SVal location, + bool isLoad); +}; +} // end anonymous namespace + +void ento::registerObjCSelfInitChecker(ExprEngine &Eng) { + if (Eng.getContext().getLangOptions().ObjC1) + Eng.registerCheck(new ObjCSelfInitChecker()); +} + +namespace { + +class InitSelfBug : public BugType { + const std::string desc; +public: + InitSelfBug() : BugType("missing \"self = [{initializer}]\"", + "missing \"self = [{initializer}]\"") {} +}; + +} // end anonymous namespace + +typedef llvm::ImmutableMap SelfFlag; + +namespace clang { +namespace ento { + template<> + struct GRStateTrait : public GRStatePartialTrait { + static void* GDMIndex() { + static int index = 0; + return &index; + } + }; +} +} + +static SelfFlagEnum getSelfFlags(SVal val, const GRState *state) { + if (SymbolRef sym = val.getAsSymbol()) + if (const unsigned *attachedFlags = state->get(sym)) + return (SelfFlagEnum)*attachedFlags; + return SelfFlag_None; +} + +static SelfFlagEnum getSelfFlags(SVal val, CheckerContext &C) { + return getSelfFlags(val, C.getState()); +} + +static void addSelfFlag(SVal val, SelfFlagEnum flag, CheckerContext &C) { + const GRState *state = C.getState(); + // FIXME: We tag the symbol that the SVal wraps but this is conceptually + // wrong, we should tag the SVal; the fact that there is a symbol behind the + // SVal is irrelevant. + if (SymbolRef sym = val.getAsSymbol()) + C.addTransition(state->set(sym, getSelfFlags(val, C) | flag)); +} + +static bool hasSelfFlag(SVal val, SelfFlagEnum flag, CheckerContext &C) { + return getSelfFlags(val, C) & flag; +} + +/// \brief Returns true of the value of the expression is the object that 'self' +/// points to and is an object that did not come from the result of calling +/// an initializer. +static bool isInvalidSelf(const Expr *E, CheckerContext &C) { + SVal exprVal = C.getState()->getSVal(E); + if (!hasSelfFlag(exprVal, SelfFlag_Self, C)) + return false; // value did not come from 'self'. + if (hasSelfFlag(exprVal, SelfFlag_InitRes, C)) + return false; // 'self' is properly initialized. + + return true; +} + +static void checkForInvalidSelf(const Expr *E, CheckerContext &C, + const char *errorStr) { + if (!E) + return; + if (!isInvalidSelf(E, C)) + return; + + // Generate an error node. + ExplodedNode *N = C.generateSink(); + if (!N) + return; + + EnhancedBugReport *report = + new EnhancedBugReport(*new InitSelfBug(), errorStr, N); + C.EmitReport(report); +} + +void ObjCSelfInitChecker::PostVisitObjCMessageExpr(CheckerContext &C, + const ObjCMessageExpr *E) { + // When encountering a message that does initialization (init rule), + // tag the return value so that we know later on that if self has this value + // then it is properly initialized. + + // FIXME: A callback should disable checkers at the start of functions. + if (!shouldRunOnFunctionOrMethod(dyn_cast( + C.getCurrentAnalysisContext()->getDecl()))) + return; + + if (isInitMessage(E)) { + // Tag the return value as the result of an initializer. + const GRState *state = C.getState(); + SVal V = state->getSVal(E); + addSelfFlag(V, SelfFlag_InitRes, C); + return; + } + + // We don't check for an invalid 'self' in an obj-c message expression to cut + // down false positives where logging functions get information from self + // (like its class) or doing "invalidation" on self when the initialization + // fails. +} + +void ObjCSelfInitChecker::PostVisitObjCIvarRefExpr(CheckerContext &C, + const ObjCIvarRefExpr *E) { + // FIXME: A callback should disable checkers at the start of functions. + if (!shouldRunOnFunctionOrMethod(dyn_cast( + C.getCurrentAnalysisContext()->getDecl()))) + return; + + checkForInvalidSelf(E->getBase(), C, + "Using an ivar before setting 'self' to the result of an initializer"); +} + +void ObjCSelfInitChecker::PreVisitReturnStmt(CheckerContext &C, + const ReturnStmt *S) { + // FIXME: A callback should disable checkers at the start of functions. + if (!shouldRunOnFunctionOrMethod(dyn_cast( + C.getCurrentAnalysisContext()->getDecl()))) + return; + + checkForInvalidSelf(S->getRetValue(), C, + "Returning 'self' before setting it to the result of an initializer"); +} + +// When a call receives a reference to 'self', [Pre/Post]VisitGenericCall pass +// the SelfFlags from the object 'self' point to before the call, to the new +// object after the call. + +void ObjCSelfInitChecker::PreVisitGenericCall(CheckerContext &C, + const CallExpr *CE) { + const GRState *state = C.getState(); + for (CallExpr::const_arg_iterator + I = CE->arg_begin(), E = CE->arg_end(); I != E; ++I) { + SVal argV = state->getSVal(*I); + if (isSelfVar(argV, C)) { + preCallSelfFlags = getSelfFlags(state->getSVal(cast(argV)), C); + return; + } + } +} + +void ObjCSelfInitChecker::PostVisitGenericCall(CheckerContext &C, + const CallExpr *CE) { + const GRState *state = C.getState(); + for (CallExpr::const_arg_iterator + I = CE->arg_begin(), E = CE->arg_end(); I != E; ++I) { + SVal argV = state->getSVal(*I); + if (isSelfVar(argV, C)) { + addSelfFlag(state->getSVal(cast(argV)), preCallSelfFlags, C); + return; + } + } +} + +void ObjCSelfInitChecker::visitLocation(CheckerContext &C, const Stmt *S, + SVal location, bool isLoad) { + // Tag the result of a load from 'self' so that we can easily know that the + // value is the object that 'self' points to. + const GRState *state = C.getState(); + if (isSelfVar(location, C)) + addSelfFlag(state->getSVal(cast(location)), SelfFlag_Self, C); +} + +// FIXME: A callback should disable checkers at the start of functions. +static bool shouldRunOnFunctionOrMethod(const NamedDecl *ND) { + if (!ND) + return false; + + const ObjCMethodDecl *MD = dyn_cast(ND); + if (!MD) + return false; + if (!MD->getClassInterface()->getSuperClass()) + return false; + if (!isInitializationMethod(MD)) + return false; + + return true; +} + +/// \brief Returns true if the location is 'self'. +static bool isSelfVar(SVal location, CheckerContext &C) { + AnalysisContext *analCtx = C.getCurrentAnalysisContext(); + if (!analCtx->getSelfDecl()) + return false; + if (!isa(location)) + return false; + + loc::MemRegionVal MRV = cast(location); + if (const DeclRegion *DR = dyn_cast(MRV.getRegion())) + return (DR->getDecl() == analCtx->getSelfDecl()); + + return false; +} + +static bool isInitializationMethod(const ObjCMethodDecl *MD) { + // Init methods with prefix like '-(id)_init' are private and the requirements + // are less strict so we don't check those. + return MD->isInstanceMethod() && + cocoa::deriveNamingConvention(MD->getSelector(), + /*ignorePrefix=*/false) == cocoa::InitRule; +} + +static bool isInitMessage(const ObjCMessageExpr *E) { + return cocoa::deriveNamingConvention(E->getSelector()) == cocoa::InitRule; +} diff --git a/test/Analysis/retain-release.m b/test/Analysis/retain-release.m index f6e43c99e8..3bf350f0a5 100644 --- a/test/Analysis/retain-release.m +++ b/test/Analysis/retain-release.m @@ -703,7 +703,7 @@ typedef CFTypeRef OtherRef; NSString *_foo; } - (id)initReturningNewClass; -- (id)initReturningNewClassBad; +- (id)_initReturningNewClassBad; - (id)initReturningNewClassBad2; @end @@ -716,7 +716,7 @@ typedef CFTypeRef OtherRef; self = [[RDar6320065Subclass alloc] init]; // no-warning return self; } -- (id)initReturningNewClassBad { +- (id)_initReturningNewClassBad { [self release]; [[RDar6320065Subclass alloc] init]; // expected-warning {{leak}} return self; diff --git a/test/Analysis/self-init.m b/test/Analysis/self-init.m new file mode 100644 index 0000000000..8b2b82faba --- /dev/null +++ b/test/Analysis/self-init.m @@ -0,0 +1,139 @@ +// RUN: %clang_cc1 -analyze -analyzer-check-objc-mem %s -verify + +@class NSZone, NSCoder; +@protocol NSObject +@end +@protocol NSCopying - (id)copyWithZone:(NSZone *)zone; +@end +@protocol NSMutableCopying - (id)mutableCopyWithZone:(NSZone *)zone; +@end +@protocol NSCoding - (void)encodeWithCoder:(NSCoder *)aCoder; +@end +@interface NSObject {} ++ (id)allocWithZone:(NSZone *)zone; ++ (id)alloc; +- (void)dealloc; +-(id)class; +-(id)init; +-(id)release; +@end +//#import "Foundation/NSObject.h" +typedef unsigned NSUInteger; +typedef int NSInteger; + +@class NSInvocation, NSMethodSignature, NSCoder, NSString, NSEnumerator; +@interface NSString : NSObject +- (NSUInteger)length; ++ (id)stringWithUTF8String:(const char *)nullTerminatedCString; +@end extern NSString * const NSBundleDidLoadNotification; +@interface NSAssertionHandler : NSObject {} ++ (NSAssertionHandler *)currentHandler; +- (void)handleFailureInMethod:(SEL)selector object:(id)object file:(NSString *)fileName lineNumber:(NSInteger)line description:(NSString *)format,...; +@end +extern NSString * const NSConnectionReplyMode; + +@interface NSBundle : NSObject ++(id)loadNibNamed:(NSString*)s owner:(id)o; +@end + +void log(void *obj); +extern void *somePtr; + +@interface MyObj : NSObject { + id myivar; + int myint; +} +-(id)_init; +-(id)initWithSomething:(int)x; +-(void)doSomething; +@end + +@implementation MyObj + +-(id)init { + do { if (!((somePtr != 0))) { [[NSAssertionHandler currentHandler] handleFailureInMethod:_cmd object:self file:[NSString stringWithUTF8String:"init.m"] lineNumber:21 description:(@"Invalid parameter not satisfying: %s"), ("x != 0"), (0), (0), (0), (0)]; } } while(0); + return [self initWithSomething:0]; +} + +-(id)init2 { + self = [self initWithSomething:0]; + return self; +} + +-(id)init3 { + log([self class]); + return [self initWithSomething:0]; +} + +-(id)init4 { + self = [super init]; + if (self) { + log(&self); + } + return self; +} + +- (id)initWithSomething:(int)x { + if ((self = [super init])) + myint = x; + return self; +} + +-(id)_init { + myivar = 0; + return self; +} + +-(id)init5 { + [NSBundle loadNibNamed:@"Window" owner:self]; + return [self initWithSomething:0]; +} + +-(id)init6 { + [NSBundle loadNibNamed:@"Window" owner:myivar]; // expected-warning {{Using an ivar}} + return [self initWithSomething:0]; +} + +-(id)init7 { + if (0 != (self = [self _init])) + myivar = 0; + return self; +} + +-(id)init8 { + if ((self = [super init])) { + log(&self); + myivar = 0; + } + return self; +} + +-(id)init9 { + [self doSomething]; + return self; // expected-warning {{Returning 'self'}} +} + +-(id)init10 { + myivar = 0; // expected-warning {{Using an ivar}} + return self; +} + +-(id)init11 { + return self; // expected-warning {{Returning 'self'}} +} + +-(id)init12 { + [super init]; + return self; // expected-warning {{Returning 'self'}} +} + +-(id)init13 { + if ((self == [super init])) { + myivar = 0; // expected-warning {{Using an ivar}} + } + return self; // expected-warning {{Returning 'self'}} +} + +-(void)doSomething {} + +@end -- 2.40.0