[analyzer] Improve suppression for inlined defensive checks when operator& is involved.

[NoQ]

Summary

In code

void foo(int *p) {
  if (p) {}
}
void bar(int *p) {
  foo(p);
  *p = 5;
}

we suppress the null dereference warning in *p = 5 because the state split within the inlined function is essentially irrelevant, as this is merely a defensive check that does not necessarily trigger in this caller context.

The suppression works by tracking the null pointer symbol in a bug report visitor and seeing if it was constrained to 0 inside a callee context. The fact that we have to rely on such manual suppressions is a downside of the inlining-based approach to interprocedural analysis.

The suppression gets confused when the null dereference becomes more complex, eg:

struct S {
  int x;
}
void foo(struct S *s) {
  if (s) {}
}
void bar(struct S *s) {
  foo(s);
  *(&(s->x)) = 5;
}

In this case s is &SymRegion{reg_$0<s>}, and reg_$0<s> is constrained to [0, 0], similarly to the above. However, while computing s->x and then &(s->x), the symbol is collapsed to a constant 0 (Loc), making it harder to track the symbol. The visitor is improved to handle this case.

While the fact that offset of field x in struct S in our example is equal to 0 is purely accidental, with any other offset it'd still be a null dereference, worth suppressing. How does the analyzer handle the non-zero offset case and still see a null dereference? Simply and powerfully: it mis-computes the offset, incorrectly believing that all offsets are equal to 0 (woohoo!). I add a test case to document this behavior; even though it's trivial to fix, the positive side effects of this bug are for now too useful to discard.

[alexander-shaposhnikov]

lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

987

"Address-of" operator can be overloaded,
just wondering - doest this code work correctly in that case ?

[NoQ]

lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

987

In this case we'd see a CXXOperatorCallExpr instead of UnaryOperator (all hail clang AST!).

[NoQ]

Remove accidentally added braces in unrelated code.

[xazax.hun]

lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

975

Maybe you could move this condition one level up with &&?

977

Can't you writhe this like:

while (const auto *ME = dyn_cast<MemberExpr>(Ex)) {
              Ex = ME->getBase()->IgnoreParenCasts();
      }

?

[dcoughlin]

Yay! This is going to fix some really confusing false positives.LGTM with Gabor's comments addressed.

One note. I am not a big fan of using clang_analyzer_explain() in tests for functionality. It is great for debugging, but for testing it exposes too much of the implementation details of the region hierarchy and store. Exposing this kind of state is fine in tests specific to the store/region store, but in my opinion it should be another clang_analyzer_ entry point and should be limited to regions.

Similarly we should have a clang_analyzer_ entry point for testing symbols but that doesn't expose the guts of MemRegions. And (maybe?) another one for SymExprs.

test/Analysis/explain-svals.cpp

103

It would be good to mention that this is a modeling bug and point to where it could be fixed. Otherwise the FIXME is not super helpful to future maintainers.

[zaks.anna]

Thanks!!!

lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

987

Adding a test case for that would be good.

990

Incorrect implies that there is a better "correct" model and invites a fix. Do we know what better model would be? If so, we could add that to the comment. If not, I'd prefer explaining why it works this way (similarly to how you did in the comment below). Maybe adding an example of what does not work. And you could add a FIXME to say that it's worth investigating if there is a better way of handling it. (The majority of this info should probably go to Store.cpp)

Also, maybe it's just the choice of words here. "Incorrect" sounds like something that needs to be corrected. Whereas you could use something like "is modeled imprecisely with respect to what happens at execution time", which could still mean that this is how we do want to model it going forward.

It seems that the problem with modeling this way is demonstrated with a test case in explain-svals.cpp. So the benefits of the current modeling seem to be worth it.

Can we add a note along the path saying that "p" in "p->f" is null? This would address the user confusion with imprecise modeling.

999

Why do we need the "while (true)"? Can we just use "dyn_cast<MemberExpr>(Ex)" as the loop condition?

Take a look at the getDerefExpr(const Stmt *S) and see if that would be a better place to add this code. Maybe not..

lib/StaticAnalyzer/Core/Store.cpp

405

Could you rephrase this existing comment while you are here? Using word "funny" seems content-free and a bit strange.

409

Thanks for adding the explanation!

Can you think of other cases where the same would apply? (Ex: array index)

[NoQ]

Fix comments!

[NoQ]

lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

987

Not sure. There are so many things that work differently in this scenario that i'm having troubles coming up with a test that tests exactly that and doesn't throw or not throw a warning for a dozen of other reasons. I'm even having troubles understanding what particular overload are we interested in. Did you have anything specific in mind?

lib/StaticAnalyzer/Core/Store.cpp

437

Note that this code doesn't really trigger; we return UnknownVal() somewhere above, as shown on the newly added tests. I suspect we may be missing valid null dereferences because of that; will have a look.

[NoQ]

lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

999

Accidentally clicked "Done" and forgot about the getDerefExpr() part.

I put it into a follow-up patch though: D32291, because it's quite a cascade of changes already.

lib/StaticAnalyzer/Core/Store.cpp

437

This is also addressed by D32291.

lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

if (const Expr *Ex = dyn_cast<Expr>(S)) {
     const Expr *PeeledEx = peelOffOuterExpr(Ex, N);
     if (Ex != PeeledEx)
       S = PeeledEx;
   }
 
   const Expr *Inner = nullptr;
   if (const Expr *Ex = dyn_cast<Expr>(S)) {
     Ex = Ex->IgnoreParenCasts();
+
+    // Performing operator `&' on an lvalue expression is essentially a no-op.
+    // Then, if we are taking addresses of fields or elements, these are also
+    // unlikely to matter.
+    // FIXME: Currently offsets of fields are computed incorrectly,
+    // being always equal to 0. See the FIXME in StoreManager's
+    // getLValueFieldOrIvar() method. This code interacts heavily with
+    // this incorrect computation; otherwise the value would not be null at all
+    // for most fields.
+    if (const auto *Op = dyn_cast<UnaryOperator>(Ex))
+      if (Op->getOpcode() == UO_AddrOf)
+        if (Op->getSubExpr()->isLValue()) {
+          Ex = Op->getSubExpr()->IgnoreParenCasts();
+          while (true) {
+            if (const auto *ME = dyn_cast<MemberExpr>(Ex)) {
+              Ex = ME->getBase()->IgnoreParenCasts();
+            } else {
+              break;
+            }
+          }
+        }
+
     if (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
     // gone too far (though we can likely track the lvalue better anyway).
     do {
       const ProgramPoint &pp = N->getLocation();
       if (Optional<StmtPoint> ps = pp.getAs<StmtPoint>()) {
         if (ps->getStmt() == S || ps->getStmt() == Inner)
           break;
       } else if (Optional<CallExitEnd> CEE = pp.getAs<CallExitEnd>()) {
         if (CEE->getCalleeContext()->getCallSite() == S ||
             CEE->getCalleeContext()->getCallSite() == Inner)
           break;
       }
       N = N->getFirstPred();
     } while (N);

lib/StaticAnalyzer/Core/Store.cpp

case loc::MemRegionValKind:
     BaseR = BaseL.castAs<loc::MemRegionVal>().getRegion();
     break;
 
   case loc::GotoLabelKind:
     // These are anormal cases. Flag an undefined value.
     return UndefinedVal();
 
   case loc::ConcreteIntKind:
     // While these seem funny, this can happen through casts.
     // FIXME: What we should return is the field offset.  For example,
     //  add the field offset to the integer value.  That way funny things
     //  like this work properly:  &(((struct foo *) 0xa)->f)
+    //  However, that's not easy to fix without reducing our abilities
+    //  to catch null pointer dereference. Eg., ((struct foo *)0x0)->f = 7
+    //  is a null dereference even though we're dereferencing offset of f
+    //  rather than null.
     return Base;
 
   default:
     llvm_unreachable("Unhandled Base.");
   }
 
   // NOTE: We must have this check first because ObjCIvarDecl is a subclass
   // of FieldDecl.
   if (const ObjCIvarDecl *ID = dyn_cast<ObjCIvarDecl>(D))
     return loc::MemRegionVal(MRMgr.getObjCIvarRegion(ID, BaseR));
 
   return loc::MemRegionVal(MRMgr.getFieldRegion(cast<FieldDecl>(D), BaseR));
 }
 
 SVal StoreManager::getLValueIvar(const ObjCIvarDecl *decl, SVal base) {
   return getLValueFieldOrIvar(decl, base);
 }
 
 SVal StoreManager::getLValueElement(QualType elementType, NonLoc Offset,
                                     SVal Base) {
 
   // If the base is an unknown or undefined value, just return it back.
   // FIXME: For absolute pointer addresses, we just return that value back as
   //  well, although in reality we should return the offset added to that
   //  value.
   if (Base.isUnknownOrUndef() || Base.getAs<loc::ConcreteInt>())
     return Base;
 
   const MemRegion* BaseRegion = Base.castAs<loc::MemRegionVal>().getRegion();
 
   // Pointer of any type can be cast and used as array base.
   const ElementRegion *ElemR = dyn_cast<ElementRegion>(BaseRegion);
 

test/Analysis/explain-svals.cpp

void test_4(int x, int y) {
   clang_analyzer_explain(&stat); // expected-warning-re{{{{^pointer to static local variable 'stat'$}}}}
   clang_analyzer_explain(stat_glob); // expected-warning-re{{{{^initial value of global variable 'stat_glob'$}}}}
   clang_analyzer_explain(&stat_glob); // expected-warning-re{{{{^pointer to global variable 'stat_glob'$}}}}
   clang_analyzer_explain((int[]){1, 2, 3}); // expected-warning-re{{{{^pointer to element of type 'int' with index 0 of temporary object constructed at statement '\(int \[3\]\)\{1, 2, 3\}'$}}}}
 }
 
 namespace {
 class C {
+public:
   int x[10];
+  int y;
 
-public:
   void test_5(int i) {
     clang_analyzer_explain(this); // expected-warning-re{{{{^pointer to 'this' object$}}}}
     clang_analyzer_explain(&x[i]); // expected-warning-re{{{{^pointer to element of type 'int' with index 'argument 'i'' of field 'x' of 'this' object$}}}}
     clang_analyzer_explain(__builtin_alloca(i)); // expected-warning-re{{{{^pointer to region allocated by '__builtin_alloca\(i\)'$}}}}
   }
 };
 } // end of anonymous namespace
 
 void test_6() {
   clang_analyzer_explain(conjure_S()); // expected-warning-re{{{{^lazily frozen compound value of temporary object constructed at statement 'conjure_S\(\)'$}}}}
   clang_analyzer_explain(conjure_S().z); // expected-warning-re{{{{^value derived from \(symbol of type 'struct S' conjured at statement 'conjure_S\(\)'\) for field 'z' of temporary object constructed at statement 'conjure_S\(\)'$}}}}
 }
+
+void test_7() {
+  C *c = 0;
+  // FIXME: we need to be explaining '40' rather than '0' here; not explainer bug.
+  clang_analyzer_explain(&c->y); // expected-warning-re{{{{^concrete memory address '0'$}}}}
+}

test/Analysis/inlining/inline-defensive-checks.c

 // RUN: %clang_analyze_cc1 -analyzer-checker=core -analyzer-config suppress-inlined-defensive-checks=true -verify %s
 
 // Perform inline defensive checks.
-void idc(int *p) {
+void idc(void *p) {
   if (p)
     ;
 }
 
 int test01(int *p) {
   if (p)
     ;
   return *p; // expected-warning {{Dereference of null pointer}}
int idcTriggerZeroThroughDoubleAssignemnt(int i) {
   return 5/i; // no-warning
 }
 void idcTrackZeroThroughDoubleAssignemnt(int x) {
   idcZero(x);
   int y = x;
   int z = y;
   idcTriggerZeroValueThroughCall(z);
 }
+
+struct S {
+  int f1;
+  int f2;
+};
+
+void idcTrackZeroValueThroughUnaryPointerOperators(struct S *s) {
+  idc(s);
+  *(&(s->f1)) = 7; // no-warning
+}
+
+void idcTrackZeroValueThroughUnaryPointerOperatorsWithOffset1(struct S *s) {
+  idc(s);
+  int *x = &(s->f2);
+  *x = 7; // no-warning
+}
+
+void idcTrackZeroValueThroughUnaryPointerOperatorsWithOffset2(struct S *s) {
+  idc(s);
+  int *x = &(s->f2) - 1;
+  // FIXME: Should not warn.
+  *x = 7; // expected-warning{{Dereference of null pointer}}
+}
+
+void idcTrackZeroValueThroughUnaryPointerOperatorsWithAssignment(struct S *s) {
+  idc(s);
+  int *x = &(s->f1);
+  *x = 7; // no-warning
+}
+
+
+struct S2 {
+  int a[1];
+};
+
+void idcTrackZeroValueThroughUnaryPointerOperatorsWithArrayField(struct S2 *s) {
+  idc(s);
+  *(&(s->a[0])) = 7; // no-warning
+}

test/Analysis/inlining/inline-defensive-checks.cpp

if (p3)
     ;
 }
 int *retNull() {
   return 0;
 }
 void test(int *p1, int *p2) {
   idc(p1);
   Foo f(p1);
-}
+}
+
+struct Bar {
+  int x;
+};
+void idcBar(Bar *b) {
+  if (b)
+    ;
+}
+void testRefToField(Bar *b) {
+  idcBar(b);
+  int &x = b->x; // no-warning
+  x = 5;
+}

test/Analysis/uninit-const.cpp

 }
 
 void f1(void) {
       int x_=5;
       doStuff_uninit(&x_);  // no warning
 }
 
 void f_uninit(void) {
-      int x;
+      int x;               // expected-note {{'x' declared without an initial value}}
       doStuff_uninit(&x);  // expected-warning {{1st function call argument is a pointer to uninitialized value}}
                            // expected-note@-1 {{1st function call argument is a pointer to uninitialized value}}
 }