[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

llvm-svn: 285605

1 files changed, 48 insertions, 15 deletions

diff --git a/clang/lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp
index c038a2649e1..c2a54baa927 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp
+++ b/clang/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));