[PATCH] D25909: [analyzer] MacOSXApiChecker: Disallow dispatch_once predicates on heap and in ivars.

[Artem Dergachev via cfe-commits]

NoQ created this revision.
NoQ added reviewers: zaks.anna, dcoughlin.
NoQ added a subscriber: cfe-commits.

As documentation in https://developer.apple.com/reference/dispatch/dispatch_once_t says, only global or static variables should have type `dispatch_once_t`, otherwise the magic with fast memory barriers doesn't work, and using dispatch_once() would cause hard-to-catch errors.

There's already a check in `MacOSXApiChecker` that disallows stack variables here. The check is extended to warn upon heap and ivar predicates of type `dispatch_once_t`.

While ivars could have been handled on the AST level, heap variables could not.

Currently the analyzer core does not realize that all Objective-C objects always reside on the heap. I thought of stating that, say, any `SymbolRegionValue` of `ObjCObjectPointerType` type should produce a heap-based symbolic region. However, if i do this, it would no longer be true that different heap-based symbolic regions never alias. So a more complicated solution is necessary here. So for this checker i'm settling on the solution of "treat all ivars as heap regions in this checker".


https://reviews.llvm.org/D25909

Files:
  lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp
  test/Analysis/dispatch-once.m


Index: test/Analysis/dispatch-once.m
===================================================================
--- /dev/null
+++ test/Analysis/dispatch-once.m
@@ -0,0 +1,63 @@
+// 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.}}
+}
+
+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));
+  dispatch_once(once, ^{}); // expected-warning{{Call to 'dispatch_once' uses heap allocated memory for the predicate value.}}
+}
+
+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_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.}}
+}
+
+ at interface Object : NSObject {
+ at public
+  dispatch_once_t once;
+}
+- (void)test_ivar_from_inside;
+ at end
+
+ at 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.}}
+}
+ at 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_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.}}
+}
Index: lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp
@@ -59,7 +59,8 @@
   ProgramStateRef state = C.getState();
   const MemRegion *R =
     state->getSVal(CE->getArg(0), C.getLocationContext()).getAsRegion();
-  if (!R || !isa<StackSpaceRegion>(R->getMemorySpace()))
+  const MemSpaceRegion *RS = R->getMemorySpace();
+  if (!R || isa<GlobalsSpaceRegion>(RS))
     return;
 
   ExplodedNode *N = C.generateErrorNode(state);
@@ -85,7 +86,16 @@
   os << "Call to '" << FName << "' uses";
   if (const VarRegion *VR = dyn_cast<VarRegion>(R))
     os << " the local variable '" << VR->getDecl()->getName() << '\'';
-  else
+  else if (const ObjCIvarRegion *IVR = dyn_cast<ObjCIvarRegion>(R))
+    os << " the instance variable '" << IVR->getDecl()->getName() << '\'';
+  else if (isa<HeapSpaceRegion>(R->getMemorySpace()))
+    os << " heap allocated memory";
+  else if (isa<UnknownSpaceRegion>(R->getMemorySpace())) {
+    // FIXME: Presence of an IVar region has priority over this branch, because
+    // ObjC objects are on the heap even if the core doesn't realize this.
+    // Make core realize that all ObjC objects are on the heap.
+    return;
+  } else
     os << " stack allocated memory";
   os << " for the predicate value.  Using such transient memory for "
         "the predicate is potentially dangerous.";


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D25909.75587.patch
Type: text/x-patch
Size: 3859 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20161024/62d30dfa/attachment.bin>