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

[NoQ]

Summary

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".

[NoQ]

Hotfix code duplication i just noticed.

[dcoughlin]

lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp

92

"heap allocated" --> "heap-allocated"

94

It is not clear to me that this FIXME is a good idea. I would remove it so someone doesn't spend a lot of time trying to address it.

Objective-C objects don't have the strong dis-aliasing guarantee that the analyzer assumes for heap base regions. In other words, two calls to [[Foo alloc] init] may yield exactly the same instance. This is because, unlike malloc() and C++ global new, ObjC initializers can (and frequently do) return instances other than the passed-in, freshly-allocated self.

test/Analysis/dispatch-once.m

13

Should the tests for dispatch_once in unix-fns.c be moved here?

15

I think it would be good to include the full diagnostic text for the stack local variable case because there is control flow in its construction.

[NoQ]

lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp

94

Hmm, that seems to be exactly the thing i'm looking for: heap-based regions that may alias.

The property of a region's staying on the heap has little to do with the property of being able to alias.

I've a feeling that we should have avoided using C++ inheritance in the memregion hierarchy, and instead went for a bunch of constraints. Eg., memory space is essentially a constraint (it may be unknown or get known later through exploring aliasing), region's value type is essentially a constraint (as seen during dynamic type propagation, it may be unknown, it may be partially known, we may get to know it better during the analysis by observing successful dynamic casts), extent is essentially a constraint (that we currently impose on SymbolExtent), offset of a symbolic region inside its true parent region is a constraint, and so on.

But that's too vague. I've no well-defined idea how to make this better at the moment.

[zaks.anna]

Looks good overall!

[NoQ]

Consider a lot more dispatch_once_t regions: improve diagnostics for local structs containing predicates, find ivar structs with predicates.

Address a couple of review comments, discuss the rest.

test/Analysis/dispatch-once.m

13

In fact we need to de-duplicate code with unix.API's pthread_once check, which is an exact copy-paste for this checker. Not sure how to achieve that, maybe split both into a single *-once checker (and remove this checker because it becomes empty). Maybe then we'd deal with tests as well.

[dcoughlin]

lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp

57

I guess this comment needs to be updated.

94

If you feel strongly about this, I would suggest putting the FIXME in the core, perhaps in SimpleSValBuilder where the dis-aliasing assumption is introduced or perhaps in the declaration of HeapSpaceRegion. This will make it clear to future maintainers that it is not a defect in the checker.

test/Analysis/dispatch-once.m

27

Clever.

63

Interesting. Have you seen this pattern in the wild?

I think this diagnostic is a little bit confusing since the ivar itself isn't being used for the predicate value.

Maybe "... uses a subfield of the instance variable 's' for the predicate value"?

[NoQ]

Also, do not create error nodes unless we're sure we're throwing a report.

[NoQ]

test/Analysis/dispatch-once.m

63

Not yet, so this is more of a "because we can" test. That said, i'm not sure about the warning message: it might also be an array element, not necessarily a field, and not necessarily a direct subfield or direct element.

Added array tests and an attempt on a safe warning message.

[zaks.anna]

Looks great! Please, commit.

lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp

 // dispatch_once and dispatch_once_f
 //===----------------------------------------------------------------------===//
 
 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()))
+  const MemSpaceRegion *RS = R->getMemorySpace();
+  if (!R || isa<GlobalsSpaceRegion>(RS))
     return;
 
   ExplodedNode *N = C.generateErrorNode(state);
   if (!N)
     return;
 
   if (!BT_dispatchOnce)
     BT_dispatchOnce.reset(new BugType(this, "Improper use of 'dispatch_once'",
if (TrimmedFName != FName)
       FName = TrimmedFName;
   }
 
   SmallString<256> S;
   llvm::raw_svector_ostream os(S);
   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>(RS))
+    os << " heap allocated memory";
+  else if (isa<UnknownSpaceRegion>(RS)) {
+    // 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.";
   if (isa<VarRegion>(R) && isa<StackLocalsSpaceRegion>(R->getMemorySpace()))
     os << "  Perhaps you intended to declare the variable as 'static'?";
 
   auto report = llvm::make_unique<BugReport>(*BT_dispatchOnce, os.str(), N);
   report->addRange(CE->getArg(0)->getSourceRange());

test/Analysis/dispatch-once.m

+// 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.}}
+}
+
+@interface Object : NSObject {
+@public
+  dispatch_once_t once;
+}
+- (void)test_ivar_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.}}
+}
+@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.}}
+}