[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

965 ↗

(On Diff #94986)

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

[NoQ]

lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

965 ↗

(On Diff #94986)

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 ↗

(On Diff #95082)

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

977 ↗

(On Diff #95082)

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 ↗

(On Diff #95082)

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

965 ↗

(On Diff #94986)

Adding a test case for that would be good.

968 ↗

(On Diff #94986)

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.

977 ↗

(On Diff #94986)

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 ↗

(On Diff #94986)

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

409 ↗

(On Diff #94986)

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

965 ↗

(On Diff #94986)

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

440 ↗

(On Diff #95890)

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

977 ↗

(On Diff #94986)

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

440 ↗

(On Diff #95890)

This is also addressed by D32291.

cfe/trunk/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: There's a hack in our Store implementation that always computes
+    // 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 (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.

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

case loc::MemRegionValKind:
     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
+    // FIXME: What we should return is the field offset, not base. For example,
+    //  add the field offset to the integer value.  That way 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. Coming up with an approach that computes offsets
+    //  over null pointers properly while still being able to catch null
+    //  dereferences might be worth it.
     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.
+  //  value. See also the similar FIXME in getLValueFieldOrIvar().
   if (Base.isUnknownOrUndef() || Base.getAs<loc::ConcreteInt>())
     return Base;
 
   const SubRegion *BaseRegion =
       Base.castAs<loc::MemRegionVal>().getRegionAs<SubRegion>();
 
   // Pointer of any type can be cast and used as array base.
   const ElementRegion *ElemR = dyn_cast<ElementRegion>(BaseRegion);

cfe/trunk/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
+}

cfe/trunk/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;
+}

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) {
+  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}}
+
+  // 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}}
+
+  // But this should still be a null dereference.
+  s->y = 5; // expected-warning{{Access to field 'y' results in a dereference of a null pointer (loaded from variable 's')}}
+}

cfe/trunk/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}}
 }