[analyzer] Implement handling array subscript into null pointer, improve null dereference checks for array subscripts

[NoQ]

Summary

When encountering an array-to-pointer-decay and the array base is null (or any other concrete pointer value) (eg. it's a member array in a structure, and the structure pointer is null; of course it wouldn't happen to stack-based or global arrays), do not yield UnknownVal; instead, yield that concrete value.

While obvious, this change now triggers false positives because our suppression for inlined defensive checks was not prepared for dealing with array subscripts (the idcTrackZeroValueThroughUnaryPointerOperatorsWithArrayField test in inlining/inline-defensive-checks.cpp starts failing). So i additionally improve the suppression.

As discussed in D31982, which added the aforementioned test case, bugreporter::getDerefExpr() should have been used (we only used to match member expressions earlier, but now that we encountered arrays, we could use all the features it function can offer). Now that the code uses that function, and a few issues within that function were further fixed in order to support the new use case and avoid regressions.

[xazax.hun]

LGTM!

[zaks.anna]

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

80

Not sure what this does, but looks like we are stricter here now.
Also, since you are changing bug reporter visitor, shouldn't there be tests for diagnostic paths?

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

if (const BinaryOperator *B = dyn_cast<BinaryOperator>(E)) {
       E = B->getLHS()->IgnoreParenCasts();
       continue;
     }
     else if (const UnaryOperator *U = dyn_cast<UnaryOperator>(E)) {
       if (U->getOpcode() == UO_Deref)
         return U->getSubExpr()->IgnoreParenCasts();
     }
     else if (const MemberExpr *ME = dyn_cast<MemberExpr>(E)) {
-      if (ME->isArrow() || isDeclRefExprToReference(ME->getBase())) {
+      if (ME->isImplicitAccess()) {
+        return ME;
+      } else if (ME->isArrow() || isDeclRefExprToReference(ME->getBase())) {
         return ME->getBase()->IgnoreParenCasts();
       } else {
         // If we have a member expr with a dot, the base must have been
         // dereferenced.
         return getDerefExpr(ME->getBase());
       }
     }
     else if (const ObjCIvarRefExpr *IvarRef = dyn_cast<ObjCIvarRefExpr>(E)) {
       return IvarRef->getBase()->IgnoreParenCasts();
     }
     else if (const ArraySubscriptExpr *AE = dyn_cast<ArraySubscriptExpr>(E)) {
-      return AE->getBase();
+      return getDerefExpr(AE->getBase());
     }
-    else if (isDeclRefExprToReference(E)) {
+    else if (isa<DeclRefExpr>(E)) {
       return E;
     }
     break;
   }
 
   return nullptr;
 }
 
if (const Expr *Ex = dyn_cast<Expr>(S)) {
     // field offsets around null pointers as if they are always equal to 0.
     // The idea here is to report accesses to fields as null dereferences
     // even though the pointer value that's being dereferenced is actually
     // the offset of the field rather than exactly 0.
     // See the FIXME in StoreManager's getLValueFieldOrIvar() method.
     // This code interacts heavily with this hack; otherwise the value
     // would not be null at all for most fields, so we'd be unable to track it.
     if (const auto *Op = dyn_cast<UnaryOperator>(Ex))
-      if (Op->getOpcode() == UO_AddrOf && Op->getSubExpr()->isLValue()) {
-        Ex = Op->getSubExpr()->IgnoreParenCasts();
-        while (const auto *ME = dyn_cast<MemberExpr>(Ex)) {
-          Ex = ME->getBase()->IgnoreParenCasts();
-        }
-      }
+      if (Op->getOpcode() == UO_AddrOf && Op->getSubExpr()->isLValue())
+        if (const Expr *DerefEx = getDerefExpr(Op->getSubExpr()))
+          Ex = DerefEx;
 
-    if (ExplodedGraph::isInterestingLValueExpr(Ex) || CallEvent::isCallStmt(Ex))
+    if (Ex && (ExplodedGraph::isInterestingLValueExpr(Ex) || CallEvent::isCallStmt(Ex)))
       Inner = Ex;
   }
 
   if (IsArg && !Inner) {
     assert(N->getLocation().getAs<CallEnter>() && "Tracking arg but not at call");
   } else {
     // Walk through nodes until we get one that matches the statement exactly.
     // Alternately, if we hit a known lvalue for the statement, we know we've

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

 
 /// ArrayToPointer - Emulates the "decay" of an array to a pointer
 ///  type.  'Array' represents the lvalue of the array being decayed
 ///  to a pointer, and the returned SVal represents the decayed
 ///  version of that lvalue (i.e., a pointer to the first element of
 ///  the array).  This is called by ExprEngine when evaluating casts
 ///  from arrays to pointers.
 SVal RegionStoreManager::ArrayToPointer(Loc Array, QualType T) {
+  if (Array.getAs<loc::ConcreteInt>())
+    return Array;
+
   if (!Array.getAs<loc::MemRegionVal>())
     return UnknownVal();
 
   const SubRegion *R =
       cast<SubRegion>(Array.castAs<loc::MemRegionVal>().getRegion());
   NonLoc ZeroIdx = svalBuilder.makeZeroArrayIndex();
   return loc::MemRegionVal(MRMgr.getElementRegion(T, ZeroIdx, R, Ctx));
 }

cfe/trunk/test/Analysis/null-deref-offsets.c

 // RUN: %clang_analyze_cc1 -w -triple i386-apple-darwin10 -analyzer-checker=core,debug.ExprInspection -verify %s
 
 void clang_analyzer_eval(int);
 
 struct S {
   int x, y;
   int z[2];
 };
 
-void testOffsets(struct S *s) {
+void testOffsets(struct S *s, int coin) {
   if (s != 0)
     return;
 
   // FIXME: Here we are testing the hack that computes offsets to null pointers
   // as 0 in order to find null dereferences of not-exactly-null pointers,
   // such as &(s->y) below, which is equal to 4 rather than 0 in run-time.
 
   // These are indeed null.
   clang_analyzer_eval(s == 0); // expected-warning{{TRUE}}
   clang_analyzer_eval(&(s->x) == 0); // expected-warning{{TRUE}}
 
   // FIXME: These should ideally be true.
   clang_analyzer_eval(&(s->y) == 4); // expected-warning{{FALSE}}
-  clang_analyzer_eval(&(s->z[0]) == 8); // expected-warning{{UNKNOWN}}
-  clang_analyzer_eval(&(s->z[1]) == 12); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(&(s->z[0]) == 8); // expected-warning{{FALSE}}
+  clang_analyzer_eval(&(s->z[1]) == 12); // expected-warning{{FALSE}}
 
   // FIXME: These should ideally be false.
   clang_analyzer_eval(&(s->y) == 0); // expected-warning{{TRUE}}
-  clang_analyzer_eval(&(s->z[0]) == 0); // expected-warning{{UNKNOWN}}
-  clang_analyzer_eval(&(s->z[1]) == 0); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(&(s->z[0]) == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(&(s->z[1]) == 0); // expected-warning{{TRUE}}
 
-  // But this should still be a null dereference.
+  // But these should still be reported as null dereferences.
+  if (coin)
     s->y = 5; // expected-warning{{Access to field 'y' results in a dereference of a null pointer (loaded from variable 's')}}
+  else
+    s->z[1] = 6; // expected-warning{{Array access (via field 'z') results in a null pointer dereference}}
 }