[analyzer] MacOSXAPIChecker: Disallow dispatch_once_t in ivars and heap.

Unlike global/static variables, calloc etc. functions that allocate ObjC
objects behave differently in terms of memory barriers, and hacks that make
dispatch_once as fast as it possibly could be start failing.

Differential Revision: https://reviews.llvm.org/D25909


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

lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp

@@ -33,6 +33,8 @@ namespace {
 class MacOSXAPIChecker : public Checker< check::PreStmt<CallExpr> > {
   mutable std::unique_ptr<BugType> BT_dispatchOnce;
 
+  static const ObjCIvarRegion *getParentIvarRegion(const MemRegion *R);
+
 public:
   void checkPreStmt(const CallExpr *CE, CheckerContext &C) const;
 
@@ -49,27 +51,34 @@ public:
 // dispatch_once and dispatch_once_f
 //===----------------------------------------------------------------------===//
 
+const ObjCIvarRegion *
+MacOSXAPIChecker::getParentIvarRegion(const MemRegion *R) {
+  const SubRegion *SR = dyn_cast<SubRegion>(R);
+  while (SR) {
+    if (const ObjCIvarRegion *IR = dyn_cast<ObjCIvarRegion>(SR))
+      return IR;
+    SR = dyn_cast<SubRegion>(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<StackSpaceRegion>(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<GlobalsSpaceRegion>(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 @@ void MacOSXAPIChecker::CheckDispatchOnce(CheckerContext &C, const CallExpr *CE,
 
   SmallString<256> S;
   llvm::raw_svector_ostream os(S);
+  bool SuggestStatic = false;
   os << "Call to '" << FName << "' uses";
-  if (const VarRegion *VR = dyn_cast<VarRegion>(R))
+  if (const VarRegion *VR = dyn_cast<VarRegion>(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<HeapSpaceRegion>(RS)) {
+    os << " heap-allocated memory";
+  } else if (isa<UnknownSpaceRegion>(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<VarRegion>(R) && isa<StackLocalsSpaceRegion>(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<BugReport>(*BT_dispatchOnce, os.str(), N);
   report->addRange(CE->getArg(0)->getSourceRange());
   C.emitReport(std::move(report));

lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp

@@ -753,6 +753,12 @@ SVal SimpleSValBuilder::evalBinOpLL(ProgramStateRef state,
     // 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<SymbolicRegion>(LeftBase) && !isa<SymbolicRegion>(RightBase)) ||
          (isa<HeapSpaceRegion>(LeftMS) || isa<HeapSpaceRegion>(RightMS))) ){

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.}}
+}