[Analyzer] Crash fix for FindLastStoreBRVisitor

FindLastStoreBRVisitor tries to find the first node in the exploded graph where
the current value was assigned to a region. This node is called the "store
site". It is identified by a pair of Pred and Succ nodes where Succ already has
the binding for the value while Pred does not have it. However the visitor
mistakenly identifies a node pair as the store site where the value is a
`LazyCompoundVal` and `Pred` does not have a store yet but `Succ` has it. In
this case the `LazyCompoundVal` is different in the `Pred` node because it also
contains the store which is different in the two nodes. This error may lead to
crashes (a declaration is cast to a parameter declaration without check) or
misleading bug path notes.

In this patch we fix this problem by checking for unequal `LazyCompoundVals`: if
their region is equal, and their store is the same as the store of their nodes
we consider them as equal when looking for the "store site". This is an
approximation because we do not check for differences of the subvalues
(structure members or array elements) in the stores.

Differential Revision: https://reviews.llvm.org/D58067

llvm-svn: 353943

3 files changed, 48 insertions, 5 deletions

diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index 00dfd0257f81..49de53e53b0b 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -153,6 +153,32 @@ const Expr *bugreporter::getDerefExpr(const Stmt *S) {
   return E;
 }
 
+/// Comparing internal representations of symbolic values (via
+/// SVal::operator==()) is a valid way to check if the value was updated,
+/// unless it's a LazyCompoundVal that may have a different internal
+/// representation every time it is loaded from the state. In this function we
+/// do an approximate comparison for lazy compound values, checking that they
+/// are the immediate snapshots of the tracked region's bindings within the
+/// node's respective states but not really checking that these snapshots
+/// actually contain the same set of bindings.
+bool hasVisibleUpdate(const ExplodedNode *LeftNode, SVal LeftVal,
+                      const ExplodedNode *RightNode, SVal RightVal) {
+  if (LeftVal == RightVal)
+    return true;
+
+  const auto LLCV = LeftVal.getAs<nonloc::LazyCompoundVal>();
+  if (!LLCV)
+    return false;
+
+  const auto RLCV = RightVal.getAs<nonloc::LazyCompoundVal>();
+  if (!RLCV)
+    return false;
+
+  return LLCV->getRegion() == RLCV->getRegion() &&
+    LLCV->getStore() == LeftNode->getState()->getStore() &&
+    RLCV->getStore() == RightNode->getState()->getStore();
+}
+
 //===----------------------------------------------------------------------===//
 // Definitions for bug reporter visitors.
 //===----------------------------------------------------------------------===//
@@ -1177,7 +1203,7 @@ FindLastStoreBRVisitor::VisitNode(const ExplodedNode *Succ,
     if (Succ->getState()->getSVal(R) != V)
       return nullptr;
 
-    if (Pred->getState()->getSVal(R) == V) {
+    if (hasVisibleUpdate(Pred, Pred->getState()->getSVal(R), Succ, V)) {
       Optional<PostStore> PS = Succ->getLocationAs<PostStore>();
       if (!PS || PS->getLocationValue() != R)
         return nullptr;
@@ -1198,6 +1224,7 @@ FindLastStoreBRVisitor::VisitNode(const ExplodedNode *Succ,
     // UndefinedVal.)
     if (Optional<CallEnter> CE = Succ->getLocationAs<CallEnter>()) {
       if (const auto *VR = dyn_cast<VarRegion>(R)) {
+
         const auto *Param = cast<ParmVarDecl>(VR->getDecl());
 
         ProgramStateManager &StateMgr = BRC.getStateManager();
diff --git a/clang/test/Analysis/PR40625.cpp b/clang/test/Analysis/PR40625.cpp
new file mode 100644
index 000000000000..6cc27d39b6c0
--- /dev/null
+++ b/clang/test/Analysis/PR40625.cpp
@@ -0,0 +1,16 @@
+// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,alpha.core.CallAndMessageUnInitRefArg  %s -verify
+
+void f(const int *end);
+
+void g(const int (&arrr)[10]) {
+  f(arrr+sizeof(arrr)); // expected-warning{{1st function call argument is a pointer to uninitialized value}}
+  // FIXME: This is a false positive that should be fixed. Until then this
+  //        tests the crash fix in FindLastStoreBRVisitor (beside
+  //        uninit-vals.m).
+}
+
+void h() {
+  int arr[10];
+
+  g(arr);
+}
diff --git a/clang/test/Analysis/uninit-vals.m b/clang/test/Analysis/uninit-vals.m
index f97af1a6633c..33352122ca5a 100644
--- a/clang/test/Analysis/uninit-vals.m
+++ b/clang/test/Analysis/uninit-vals.m
@@ -394,11 +394,11 @@ void testSmallStructBitfieldsFirstUnnamed() {
   struct {
     int : 4;
     int y : 4;
-  } a, b, c;
+  } a, b, c; // expected-note{{'c' initialized here}}
 
   a.y = 2;
 
-  b = a; // expected-note{{Value assigned to 'c'}}
+  b = a;
   clang_analyzer_eval(b.y == 2); // expected-warning{{TRUE}}
                                  // expected-note@-1{{TRUE}}
 
@@ -411,11 +411,11 @@ void testSmallStructBitfieldsSecondUnnamed() {
   struct {
     int x : 4;
     int : 4;
-  } a, b, c;
+  } a, b, c; // expected-note{{'c' initialized here}}
 
   a.x = 1;
 
-  b = a; // expected-note{{Value assigned to 'c'}}
+  b = a;
   clang_analyzer_eval(b.x == 1); // expected-warning{{TRUE}}
                                  // expected-note@-1{{TRUE}}