[analyzer] Fix MacOSXAPIChecker fp with static locals seen from nested blocks.

This is an attempt to avoid new false positives caused by the reverted r292800,
however the scope of the fix is significantly reduced - some variables are still
in incorrect memory spaces.

Relevant test cases added.

rdar://problem/30105546
rdar://problem/30156693
Differential revision: https://reviews.llvm.org/D28946

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

diff --git a/lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp b/lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp
index 0e0f52af31653a5c7a942d801f6bd7ec571e9b12..437378e53daa054fab227953322b207329154a75 100644 (file)
--- a/lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp
+++ b/lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp
@@ -94,11 +94,18 @@ void MacOSXAPIChecker::CheckDispatchOnce(CheckerContext &C, const CallExpr *CE,
   bool SuggestStatic = false;
   os << "Call to '" << FName << "' uses";
   if (const VarRegion *VR = dyn_cast<VarRegion>(RB)) {
+    const VarDecl *VD = VR->getDecl();
+    // FIXME: These should have correct memory space and thus should be filtered
+    // out earlier. This branch only fires when we're looking from a block,
+    // which we analyze as a top-level declaration, onto a static local
+    // in a function that contains the block.
+    if (VD->isStaticLocal())
+      return;
     // We filtered out globals earlier, so it must be a local variable
     // or a block variable which is under UnknownSpaceRegion.
     if (VR != R)
       os << " memory within";
-    if (VR->getDecl()->hasAttr<BlocksAttr>())
+    if (VD->hasAttr<BlocksAttr>())
       os << " the block variable '";
     else
       os << " the local variable '";

diff --git a/lib/StaticAnalyzer/Core/MemRegion.cpp b/lib/StaticAnalyzer/Core/MemRegion.cpp
index c4ba2ae199f883cfb745943e064f574f53aea669..d6e8fe5b51b3698ee928d33ef29b7715652e03ac 100644 (file)
--- a/lib/StaticAnalyzer/Core/MemRegion.cpp
+++ b/lib/StaticAnalyzer/Core/MemRegion.cpp
@@ -816,9 +816,11 @@ const VarRegion* MemRegionManager::getVarRegion(const VarDecl *D,
 
     const StackFrameContext *STC = V.get<const StackFrameContext*>();
 
-    if (!STC)
+    if (!STC) {
+      // FIXME: Assign a more sensible memory space to static locals
+      // we see from within blocks that we analyze as top-level declarations.
       sReg = getUnknownRegion();
-    else {
+    } else {
       if (D->hasLocalStorage()) {
         sReg = isa<ParmVarDecl>(D) || isa<ImplicitParamDecl>(D)
                ? static_cast<const MemRegion*>(getStackArgumentsRegion(STC))

diff --git a/lib/StaticAnalyzer/Core/RegionStore.cpp b/lib/StaticAnalyzer/Core/RegionStore.cpp
index 15ca2c14f9440b6fd13c6a47ab1c0d13b5f6780b..934cc5cd3ac466716c120f93ae9e3a5185871c79 100644 (file)
--- a/lib/StaticAnalyzer/Core/RegionStore.cpp
+++ b/lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -1849,6 +1849,8 @@ SVal RegionStoreManager::getBindingForVar(RegionBindingsConstRef B,
 
     // Function-scoped static variables are default-initialized to 0; if they
     // have an initializer, it would have been processed by now.
+    // FIXME: This is only true when we're starting analysis from main().
+    // We're losing a lot of coverage here.
     if (isa<StaticGlobalSpaceRegion>(MS))
       return svalBuilder.makeZeroVal(T);
 

diff --git a/test/Analysis/dispatch-once.m b/test/Analysis/dispatch-once.m
index 7d54147aebe24db392a41dfefbefe4111b166dac..2f82718663e5bb7b99d957cced6294fb6b336403 100644 (file)
--- a/test/Analysis/dispatch-once.m
+++ b/test/Analysis/dispatch-once.m
@@ -107,3 +107,10 @@ void test_block_var_from_outside_block() {
   };
   dispatch_once(&once, ^{}); // expected-warning{{Call to 'dispatch_once' uses the block variable 'once' for the predicate value.}}
 }
+
+void test_static_var_from_outside_block() {
+  static dispatch_once_t once;
+  ^{
+    dispatch_once(&once, ^{}); // no-warning
+  };
+}

diff --git a/test/Analysis/null-deref-static.m b/test/Analysis/null-deref-static.m
new file mode 100644 (file)
index 0000000..887bea2
--- /dev/null
+++ b/test/Analysis/null-deref-static.m
@@ -0,0 +1,35 @@
+// RUN: %clang_cc1 -w -fblocks -analyze -analyzer-checker=core,deadcode,alpha.core,debug.ExprInspection -verify %s
+
+void *malloc(unsigned long);
+void clang_analyzer_warnIfReached();
+
+void test_static_from_block() {
+  static int *x;
+  ^{
+    *x; // no-warning
+  };
+}
+
+void test_static_within_block() {
+  ^{
+    static int *x;
+    *x; // expected-warning{{Dereference of null pointer}}
+  };
+}
+
+void test_static_control_flow(int y) {
+  static int *x;
+  if (x) {
+    // FIXME: Should be reachable.
+    clang_analyzer_warnIfReached(); // no-warning
+  }
+  if (y) {
+    // We are not sure if this branch is possible, because the developer
+    // may argue that function is always called with y == 1 for the first time.
+    // In this case, we can only advise the developer to add assertions
+    // for suppressing such path.
+    *x; // expected-warning{{Dereference of null pointer}}
+  } else {
+    x = malloc(1);
+  }
+}