[cfe-commits] r163361 - in /cfe/trunk: lib/StaticAnalyzer/Core/ExprEngineObjC.cpp test/Analysis/retain-release-crashes.m

[Anna Zaks]

On Sep 6, 2012, at 4:44 PM, Jordan Rose <jordan_rose at apple.com> wrote:

> Author: jrose
> Date: Thu Sep  6 18:44:36 2012
> New Revision: 163361
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=163361&view=rev
> Log:
> [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.

Should we cash out? / Why are we cashing out?

> 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>
> 
> Added:
>    cfe/trunk/test/Analysis/retain-release-crashes.m
> Modified:
>    cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
> 
> Modified: cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp?rev=163361&r1=163360&r2=163361&view=diff
> ==============================================================================
> --- cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp (original)
> +++ cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp Thu Sep  6 18:44:36 2012
> @@ -245,8 +245,9 @@
>       }
>     }
> 
> -    // Evaluate the call.
> -    defaultEvalCall(Bldr, Pred, *UpdatedMsg);
> +    // Evaluate the call if we haven't cached out.
> +    if (Pred)
> +      defaultEvalCall(Bldr, Pred, *UpdatedMsg);
>   }
> 
>   ExplodedNodeSet dstPostvisit;
> 
> Added: cfe/trunk/test/Analysis/retain-release-crashes.m
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/retain-release-crashes.m?rev=163361&view=auto
> ==============================================================================
> --- cfe/trunk/test/Analysis/retain-release-crashes.m (added)
> +++ cfe/trunk/test/Analysis/retain-release-crashes.m Thu Sep  6 18:44:36 2012
> @@ -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);
> +
> + at interface NSObject
> +- (id)init;
> + at end
> +
> + at interface Foo : NSObject
> + at end
> +
> +void touch(id, SEL);
> +id getObject();
> +int getInt();
> +
> +
> + at 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;
> +}
> + at end
> 
> 
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits