From: Jordan Rose <jordan_rose@apple.com>
Date: Thu, 6 Sep 2012 23:44:36 +0000 (+0000)
Subject: [analyzer] Don't crash if we cache out while evaluating an ObjC message.
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=5601c9aac3bf7be5e1ea8a76149090933d2d3c78;p=clang

[analyzer] Don't crash if we cache out while evaluating an ObjC message.

A bizarre series of coincidences led us to generate a previously-seen
node in the middle of processing an Objective-C message, where we assume
the receiver is non-nil. We were assuming that such an assumption would
never "cache out" like this, and blithely went on using a null ExplodedNode
as the predecessor for the next step in evaluation.

Although the test case committed here is complicated, this could in theory
happen in other ways as well, so the correct fix is just to test if the
non-nil assumption results in an ExplodedNode we've seen before.

<rdar://problem/12243648>

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@163361 91177308-0d34-0410-b5e6-96231b3b80d8
---

diff --git a/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp b/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
index 2b787b64f9..abe18bf835 100644
--- a/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
+++ b/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
@@ -245,8 +245,9 @@ void ExprEngine::VisitObjCMessage(const ObjCMessageExpr *ME,
       }
     }
 
-    // Evaluate the call.
-    defaultEvalCall(Bldr, Pred, *UpdatedMsg);
+    // Evaluate the call if we haven't cached out.
+    if (Pred)
+      defaultEvalCall(Bldr, Pred, *UpdatedMsg);
   }
   
   ExplodedNodeSet dstPostvisit;
diff --git a/test/Analysis/retain-release-crashes.m b/test/Analysis/retain-release-crashes.m
new file mode 100644
index 0000000000..40171a39c9
--- /dev/null
+++ b/test/Analysis/retain-release-crashes.m
@@ -0,0 +1,62 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -analyze -analyzer-checker=core,osx.cocoa.RetainCount,osx.cocoa.SelfInit,debug.ExprInspection -verify -w %s
+
+// This file contains crash regression tests; please do not remove any checkers
+// from the RUN line because they may have been necessary to produce the crash.
+// (Adding checkers should be fine.)
+
+void clang_analyzer_eval(int);
+
+@interface NSObject
+- (id)init;
+@end
+
+@interface Foo : NSObject
+@end
+
+void touch(id, SEL);
+id getObject();
+int getInt();
+
+
+@implementation Foo
+// Bizarre crash related to the ExprEngine reaching a previously-seen
+// ExplodedNode /during/ the processing of a message. Removing any
+// parts of this test case seem not to trigger the crash any longer.
+// <rdar://problem/12243648>
+- (id)init {
+  // Step 0: properly call the superclass's initializer
+  self = [super init];
+  if (!self) return self;
+
+  // Step 1: Perturb the state with a new conjured symbol.
+  int value = getInt();
+
+  // Step 2: Loop. Some loops seem to trigger this, some don't.
+  // The original used a for-in loop.
+  while (--value) {
+    // Step 3: Make it impossible to retain-count 'self' by calling
+    // a function that takes a "callback" (in this case, a selector).
+    // Note that this does not trigger the crash if you use a message!
+    touch(self, @selector(hi));
+  }
+
+  // Step 4: Use 'self', so that we know it's non-nil.
+  [self bar];
+
+  // Step 5: Once again, make it impossible to retain-count 'self'...
+  // ...while letting ObjCSelfInitChecker mark this as an interesting
+  // message, since 'self' is an argument...
+  // ...but this time do it in such a way that we'll also assume that
+  // 'other' is non-nil. Once we've made the latter assumption, we
+  // should cache out.
+  id other = getObject();
+  [other use:self withSelector:@selector(hi)];
+
+  // Step 6: Check that we did, in fact, keep the assumptions about 'self'
+  // and 'other' being non-nil.
+  clang_analyzer_eval(other != 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(self != 0); // expected-warning{{TRUE}}
+
+  return self;
+}
+@end