Index: cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp =================================================================== --- cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp +++ cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp @@ -33,6 +33,8 @@ class MacOSXAPIChecker : public Checker< check::PreStmt > { mutable std::unique_ptr BT_dispatchOnce; + static const ObjCIvarRegion *getParentIvarRegion(const MemRegion *R); + public: void checkPreStmt(const CallExpr *CE, CheckerContext &C) const; @@ -49,27 +51,34 @@ // dispatch_once and dispatch_once_f //===----------------------------------------------------------------------===// +const ObjCIvarRegion * +MacOSXAPIChecker::getParentIvarRegion(const MemRegion *R) { + const SubRegion *SR = dyn_cast(R); + while (SR) { + if (const ObjCIvarRegion *IR = dyn_cast(SR)) + return IR; + SR = dyn_cast(SR->getSuperRegion()); + } + return nullptr; +} + void MacOSXAPIChecker::CheckDispatchOnce(CheckerContext &C, const CallExpr *CE, StringRef FName) const { if (CE->getNumArgs() < 1) return; - // Check if the first argument is stack allocated. If so, issue a warning - // because that's likely to be bad news. - ProgramStateRef state = C.getState(); - const MemRegion *R = - state->getSVal(CE->getArg(0), C.getLocationContext()).getAsRegion(); - if (!R || !isa(R->getMemorySpace())) + // Check if the first argument is improperly allocated. If so, issue a + // warning because that's likely to be bad news. + const MemRegion *R = C.getSVal(CE->getArg(0)).getAsRegion(); + if (!R) return; - ExplodedNode *N = C.generateErrorNode(state); - if (!N) + // Global variables are fine. + const MemRegion *RB = R->getBaseRegion(); + const MemSpaceRegion *RS = RB->getMemorySpace(); + if (isa(RS)) return; - if (!BT_dispatchOnce) - BT_dispatchOnce.reset(new BugType(this, "Improper use of 'dispatch_once'", - "API Misuse (Apple)")); - // Handle _dispatch_once. In some versions of the OS X SDK we have the case // that dispatch_once is a macro that wraps a call to _dispatch_once. // _dispatch_once is then a function which then calls the real dispatch_once. @@ -82,16 +91,40 @@ SmallString<256> S; llvm::raw_svector_ostream os(S); + bool SuggestStatic = false; os << "Call to '" << FName << "' uses"; - if (const VarRegion *VR = dyn_cast(R)) + if (const VarRegion *VR = dyn_cast(RB)) { + // We filtered out globals earlier, so it must be a local variable. + if (VR != R) + os << " memory within"; os << " the local variable '" << VR->getDecl()->getName() << '\''; - else + SuggestStatic = true; + } else if (const ObjCIvarRegion *IVR = getParentIvarRegion(R)) { + if (IVR != R) + os << " memory within"; + os << " the instance variable '" << IVR->getDecl()->getName() << '\''; + } else if (isa(RS)) { + os << " heap-allocated memory"; + } else if (isa(RS)) { + // Presence of an IVar superregion has priority over this branch, because + // ObjC objects are on the heap even if the core doesn't realize this. + return; + } else { os << " stack allocated memory"; + } os << " for the predicate value. Using such transient memory for " "the predicate is potentially dangerous."; - if (isa(R) && isa(R->getMemorySpace())) + if (SuggestStatic) os << " Perhaps you intended to declare the variable as 'static'?"; + ExplodedNode *N = C.generateErrorNode(); + if (!N) + return; + + if (!BT_dispatchOnce) + BT_dispatchOnce.reset(new BugType(this, "Improper use of 'dispatch_once'", + "API Misuse (Apple)")); + auto report = llvm::make_unique(*BT_dispatchOnce, os.str(), N); report->addRange(CE->getArg(0)->getSourceRange()); C.emitReport(std::move(report)); Index: cfe/trunk/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp =================================================================== --- cfe/trunk/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp +++ cfe/trunk/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp @@ -753,6 +753,12 @@ // Note, heap base symbolic regions are assumed to not alias with // each other; for example, we assume that malloc returns different address // on each invocation. + // FIXME: ObjC object pointers always reside on the heap, but currently + // we treat their memory space as unknown, because symbolic pointers + // to ObjC objects may alias. There should be a way to construct + // possibly-aliasing heap-based regions. For instance, MacOSXApiChecker + // guesses memory space for ObjC object pointers manually instead of + // relying on us. if (LeftBase != RightBase && ((!isa(LeftBase) && !isa(RightBase)) || (isa(LeftMS) || isa(RightMS))) ){ Index: cfe/trunk/test/Analysis/dispatch-once.m =================================================================== --- cfe/trunk/test/Analysis/dispatch-once.m +++ cfe/trunk/test/Analysis/dispatch-once.m @@ -0,0 +1,92 @@ +// RUN: %clang_cc1 -w -fblocks -analyze -analyzer-checker=core,osx.API,unix.Malloc -verify %s +// RUN: %clang_cc1 -w -fblocks -fobjc-arc -analyze -analyzer-checker=core,osx.API,unix.Malloc -verify %s + +#include "Inputs/system-header-simulator-objc.h" + +typedef unsigned long size_t; +void *calloc(size_t nmemb, size_t size); + +typedef void (^dispatch_block_t)(void); +typedef long dispatch_once_t; +void dispatch_once(dispatch_once_t *predicate, dispatch_block_t block); + +void test_stack() { + dispatch_once_t once; + dispatch_once(&once, ^{}); // expected-warning{{Call to 'dispatch_once' uses the local variable 'once' for the predicate value. Using such transient memory for the predicate is potentially dangerous. Perhaps you intended to declare the variable as 'static'?}} +} + +void test_static_local() { + static dispatch_once_t once; + dispatch_once(&once, ^{}); // no-warning +} + +void test_heap_var() { + dispatch_once_t *once = calloc(1, sizeof(dispatch_once_t)); + // Use regexps to check that we're NOT suggesting to make this static. + dispatch_once(once, ^{}); // expected-warning-re{{{{^Call to 'dispatch_once' uses heap-allocated memory for the predicate value. Using such transient memory for the predicate is potentially dangerous$}}}} +} + +void test_external_pointer(dispatch_once_t *once) { + // External pointer does not necessarily point to the heap. + dispatch_once(once, ^{}); // no-warning +} + +typedef struct { + dispatch_once_t once; +} Struct; + +void test_local_struct() { + Struct s; + dispatch_once(&s.once, ^{}); // expected-warning{{Call to 'dispatch_once' uses memory within the local variable 's' for the predicate value.}} +} + +void test_heap_struct() { + Struct *s = calloc(1, sizeof(Struct)); + dispatch_once(&s->once, ^{}); // expected-warning{{Call to 'dispatch_once' uses heap-allocated memory for the predicate value.}} +} + +@interface Object : NSObject { +@public + dispatch_once_t once; + Struct s; + dispatch_once_t once_array[2]; +} +- (void)test_ivar_from_inside; +- (void)test_ivar_struct_from_inside; +@end + +@implementation Object +- (void)test_ivar_from_inside { + dispatch_once(&once, ^{}); // expected-warning{{Call to 'dispatch_once' uses the instance variable 'once' for the predicate value.}} +} +- (void)test_ivar_struct_from_inside { + dispatch_once(&s.once, ^{}); // expected-warning{{Call to 'dispatch_once' uses memory within the instance variable 's' for the predicate value.}} +} +- (void)test_ivar_array_from_inside { + dispatch_once(&once_array[1], ^{}); // expected-warning{{Call to 'dispatch_once' uses memory within the instance variable 'once_array' for the predicate value.}} +} +@end + +void test_ivar_from_alloc_init() { + Object *o = [[Object alloc] init]; + dispatch_once(&o->once, ^{}); // expected-warning{{Call to 'dispatch_once' uses the instance variable 'once' for the predicate value.}} +} +void test_ivar_struct_from_alloc_init() { + Object *o = [[Object alloc] init]; + dispatch_once(&o->s.once, ^{}); // expected-warning{{Call to 'dispatch_once' uses memory within the instance variable 's' for the predicate value.}} +} +void test_ivar_array_from_alloc_init() { + Object *o = [[Object alloc] init]; + dispatch_once(&o->once_array[1], ^{}); // expected-warning{{Call to 'dispatch_once' uses memory within the instance variable 'once_array' for the predicate value.}} +} + +void test_ivar_from_external_obj(Object *o) { + // ObjC object pointer always points to the heap. + dispatch_once(&o->once, ^{}); // expected-warning{{Call to 'dispatch_once' uses the instance variable 'once' for the predicate value.}} +} +void test_ivar_struct_from_external_obj(Object *o) { + dispatch_once(&o->s.once, ^{}); // expected-warning{{Call to 'dispatch_once' uses memory within the instance variable 's' for the predicate value.}} +} +void test_ivar_array_from_external_obj(Object *o) { + dispatch_once(&o->once_array[1], ^{}); // expected-warning{{Call to 'dispatch_once' uses memory within the instance variable 'once_array' for the predicate value.}} +}