From 5601c9aac3bf7be5e1ea8a76149090933d2d3c78 Mon Sep 17 00:00:00 2001 From: Jordan Rose Date: Thu, 6 Sep 2012 23:44:36 +0000 Subject: [PATCH] [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. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@163361 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/StaticAnalyzer/Core/ExprEngineObjC.cpp | 5 +- test/Analysis/retain-release-crashes.m | 62 ++++++++++++++++++++++ 2 files changed, 65 insertions(+), 2 deletions(-) create mode 100644 test/Analysis/retain-release-crashes.m 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. +// +- (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 -- 2.40.0