[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 ↗

(On Diff #75589)

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

94 ↗

(On Diff #75589)

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 ↗

(On Diff #75589)

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

15 ↗

(On Diff #75589)

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 ↗

(On Diff #75589)

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 ↗

(On Diff #75589)

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

70 ↗

(On Diff #75739)

I guess this comment needs to be updated.

94 ↗

(On Diff #75589)

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

26 ↗

(On Diff #75739)

Clever.

62 ↗

(On Diff #75739)

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

62 ↗

(On Diff #75739)

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.

cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp

 
 using namespace clang;
 using namespace ento;
 
 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;
 
   void CheckDispatchOnce(CheckerContext &C, const CallExpr *CE,
                          StringRef FName) const;
 
   typedef void (MacOSXAPIChecker::*SubChecker)(CheckerContext &,
                                                const CallExpr *,
                                                StringRef FName) const;
 };
 } //end anonymous namespace
 
 //===----------------------------------------------------------------------===//
 // 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.
   // Users do not care; they just want the warning at the top-level call.
   if (CE->getLocStart().isMacroID()) {
     StringRef TrimmedFName = FName.ltrim('_');
     if (TrimmedFName != FName)
       FName = TrimmedFName;
   }
 
   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));
 }
 
 //===----------------------------------------------------------------------===//
 // Central dispatch function.
 //===----------------------------------------------------------------------===//

cfe/trunk/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp

if (LeftMS != RightMS &&
         return makeTruthVal(true, resultTy);
       }
     }
 
     // If both values wrap regions, see if they're from different base regions.
     // 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))) ){
       switch (op) {
       default:
         return UnknownVal();
       case BO_EQ:
         return makeTruthVal(false, resultTy);

cfe/trunk/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.  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.}}
+}