[analyzer] Make analyzer less aggressive when dealing with [self init].

With inlining, retain count checker starts tracking 'self' through the
init methods. The analyser results were too noisy if the developer
did not follow 'self = [super init]' pattern (which is common
especially in older code bases) - we reported self init anti-pattern AND
possible use-after-free. This patch teaches the retain count
checker to assume that [super init] does not fail when it's not consumed
by another expression. This silences the retain count warning that warns
about possibility of use-after-free when init fails, while preserving
all the other checking on 'self'.

llvm-svn: 162508

5 files changed, 171 insertions, 15 deletions

diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
index c78db524f8d..367d3926dc6 100644
--- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
+++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
@@ -153,16 +153,6 @@ protected:
     : State(Original.State), LCtx(Original.LCtx), Origin(Original.Origin),
       Data(Original.Data), Location(Original.Location), RefCount(0) {}
 
-
-  ProgramStateRef getState() const {
-    return State;
-  }
-
-  const LocationContext *getLocationContext() const {
-    return LCtx;
-  }
-
-
   /// Copies this CallEvent, with vtable intact, into a new block of memory.
   virtual void cloneTo(void *Dest) const = 0;
 
@@ -192,6 +182,16 @@ public:
     return Origin.dyn_cast<const Decl *>();
   }
 
+  /// \brief The state in which the call is being evaluated.
+  ProgramStateRef getState() const {
+    return State;
+  }
+
+  /// \brief The context in which the call is being evaluated.
+  const LocationContext *getLocationContext() const {
+    return LCtx;
+  }
+
   /// \brief Returns the definition of the function or method that will be
   /// called.
   virtual RuntimeDefinition getRuntimeDefinition() const = 0;
@@ -810,6 +810,9 @@ public:
   /// \brief Returns the value of the receiver at the time of this call.
   SVal getReceiverSVal() const;
 
+  /// \brief Return the value of 'self' if available.
+  SVal getSelfSVal() const;
+
   /// \brief Get the interface for the receiver.
   ///
   /// This works whether this is an instance message or a class message.
@@ -818,6 +821,9 @@ public:
     return getOriginExpr()->getReceiverInterface();
   }
 
+  /// \brief Checks if the receiver refers to 'self' or 'super'.
+  bool isReceiverSelfOrSuper() const;
+
   /// Returns how the message was written in the source (property access,
   /// subscript, or explicit message send).
   ObjCMessageKind getMessageKind() const;
diff --git a/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
index 03956bb2930..ab8acb18318 100644
--- a/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
@@ -930,6 +930,35 @@ void RetainSummaryManager::updateSummaryForCall(const RetainSummary *&S,
 
     S = getPersistentSummary(RE, RecEffect, DefEffect);
   }
+
+  // Special case '[super init];' and '[self init];'
+  //
+  // Even though calling '[super init]' without assigning the result to self
+  // and checking if the parent returns 'nil' is a bad pattern, it is common.
+  // Additionally, our Self Init checker already warns about it. To avoid
+  // overwhelming the user with messages from both checkers, we model the case
+  // of '[super init]' in cases when it is not consumed by another expression
+  // as if the call preserves the value of 'self'; essentially, assuming it can 
+  // never fail and return 'nil'.
+  // Note, we don't want to just stop tracking the value since we want the
+  // RetainCount checker to report leaks and use-after-free if SelfInit checker
+  // is turned off.
+  if (const ObjCMethodCall *MC = dyn_cast<ObjCMethodCall>(&Call)) {
+    if (MC->getMethodFamily() == OMF_init && MC->isReceiverSelfOrSuper()) {
+
+      // Check if the message is not consumed, we know it will not be used in
+      // an assignment, ex: "self = [super init]".
+      const Expr *ME = MC->getOriginExpr();
+      const LocationContext *LCtx = MC->getLocationContext();
+      ParentMap &PM = LCtx->getAnalysisDeclContext()->getParentMap();
+      if (!PM.isConsumedExpr(ME)) {
+        RetainSummaryTemplate ModifiableSummaryTemplate(S, *this);
+        ModifiableSummaryTemplate->setReceiverEffect(DoNothing);
+        ModifiableSummaryTemplate->setRetEffect(RetEffect::MakeNoRet());
+      }
+    }
+
+  }
 }
 
 const RetainSummary *
diff --git a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
index 5345bd51704..d4ee84174e3 100644
--- a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
+++ b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -574,6 +574,14 @@ QualType ObjCMethodCall::getDeclaredResultType() const {
   return D->getResultType();
 }
 
+SVal ObjCMethodCall::getSelfSVal() const {
+  const LocationContext *LCtx = getLocationContext();
+  const ImplicitParamDecl *SelfDecl = LCtx->getSelfDecl();
+  if (!SelfDecl)
+    return SVal();
+  return getState()->getSVal(getState()->getRegion(SelfDecl, LCtx));
+}
+
 SVal ObjCMethodCall::getReceiverSVal() const {
   // FIXME: Is this the best way to handle class receivers?
   if (!isInstanceMessage())
@@ -584,10 +592,23 @@ SVal ObjCMethodCall::getReceiverSVal() const {
 
   // An instance message with no expression means we are sending to super.
   // In this case the object reference is the same as 'self'.
-  const LocationContext *LCtx = getLocationContext();
-  const ImplicitParamDecl *SelfDecl = LCtx->getSelfDecl();
-  assert(SelfDecl && "No message receiver Expr, but not in an ObjC method");
-  return getState()->getSVal(getState()->getRegion(SelfDecl, LCtx));
+  assert(getOriginExpr()->getReceiverKind() == ObjCMessageExpr::SuperInstance);
+  SVal SelfVal = getSelfSVal();
+  assert(SelfVal.isValid() && "Calling super but not in ObjC method");
+  return SelfVal;
+}
+
+bool ObjCMethodCall::isReceiverSelfOrSuper() const {
+  if (getOriginExpr()->getReceiverKind() == ObjCMessageExpr::SuperInstance ||
+      getOriginExpr()->getReceiverKind() == ObjCMessageExpr::SuperClass)
+      return true;
+
+  if (!isInstanceMessage())
+    return false;
+
+  SVal RecVal = getSVal(getOriginExpr()->getInstanceReceiver());
+
+  return (RecVal == getSelfSVal());
 }
 
 SourceRange ObjCMethodCall::getSourceRange() const {
diff --git a/clang/test/Analysis/inlining/RetainCountExamples.m b/clang/test/Analysis/inlining/RetainCountExamples.m
index 2b682c2b4bf..c48e7b068d8 100644
--- a/clang/test/Analysis/inlining/RetainCountExamples.m
+++ b/clang/test/Analysis/inlining/RetainCountExamples.m
@@ -30,4 +30,36 @@ typedef struct objc_object {
 void selfStaysLive() {
     SelfStaysLive *foo = [[SelfStaysLive alloc] init]; 
     [foo release];
-}
\ No newline at end of file
+}
+
+// Test that retain release checker warns on leaks and use-after-frees when 
+// self init is not enabled.  
+// radar://12115830
+@interface ParentOfCell : NSObject
+- (id)initWithInt: (int)inInt;
+@end
+@interface Cell : ParentOfCell{
+  int x;
+}
+- (id)initWithInt: (int)inInt;
++ (void)testOverRelease;
++ (void)testLeak;
+@property int x;
+@end
+@implementation Cell
+@synthesize x;
+- (id) initWithInt: (int)inInt {
+  [super initWithInt: inInt];
+  self.x = inInt; // no-warning 
+  return self; // Self Init checker would produce a warning here.
+}
++ (void) testOverRelease {
+  Cell *sharedCell3 = [[Cell alloc] initWithInt: 3];
+  [sharedCell3 release];
+  [sharedCell3 release]; // expected-warning {{Reference-counted object is used after it is released}}
+}
++ (void) testLeak {
+  Cell *sharedCell4 = [[Cell alloc] initWithInt: 3]; // expected-warning {{leak}}
+}
+@end
+
diff --git a/clang/test/Analysis/inlining/retain-count-self-init.m b/clang/test/Analysis/inlining/retain-count-self-init.m
new file mode 100644
index 00000000000..ee8dbe391c4
--- /dev/null
+++ b/clang/test/Analysis/inlining/retain-count-self-init.m
@@ -0,0 +1,68 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=core,osx.cocoa.RetainCount,osx.cocoa.SelfInit -analyzer-ipa=dynamic-bifurcate -verify %s
+
+typedef signed char BOOL;
+typedef struct objc_class *Class;
+typedef struct objc_object {
+    Class isa;
+} *id;
+@protocol NSObject  - (BOOL)isEqual:(id)object; @end
+@interface NSObject <NSObject> {}
++(id)alloc;
++(id)new;
+- (oneway void)release;
+-(id)init;
+-(id)autorelease;
+-(id)copy;
+- (Class)class;
+-(id)retain;
+@end
+
+// We do not want to overhelm user with error messages in case they forgot to 
+// assign to self and check that the result of [super init] is non-nil. So 
+// stop tracking the receiver of init with respect to Retain Release checker.  
+// radar://12115830
+@interface ParentOfCell : NSObject
+- (id)initWithInt: (int)inInt;
+@end
+@interface Cell : ParentOfCell{
+  int x;
+}
+- (id)init;
++ (void)test;
+@property int x;
+@end
+@implementation Cell
+@synthesize x;
+- (id) init {
+  [super init];
+  self.x = 3; // no-warning 
+  return self; // expected-warning {{Returning 'self' while it is not set to the result of '[(super or self)}} 
+}
+- (id) initWithInt: (int)inInt {
+  [super initWithInt: inInt];
+  self.x = inInt; // no-warning 
+  return self; // expected-warning {{Returning 'self' while it is not set to the result of '[(super or self)}} 
+}
+- (id) init2 {
+  [self init]; // The call [self init] is inlined. We will warn inside the inlined body.
+  self.x = 2; // no-warning 
+  return self; 
+}
+
+- (id) initWithIntGood: (int)inInt {
+    if (self = [super initWithInt: inInt]) {
+      self.x = inInt; 
+    }
+    return self; 
+}
++ (void) test {
+  Cell *sharedCell1 = [[Cell alloc] init];
+  [sharedCell1 release];
+  Cell *sharedCell2 = [[Cell alloc] initWithInt: 3];
+  [sharedCell2 release];
+  Cell *sharedCell3 = [[Cell alloc] initWithIntGood: 3];
+  [sharedCell3 release];
+}
+
+@end
+