]> granicus.if.org Git - clang/commitdiff
[analyzer] Improve suppression for inlined defensive checks before operator &.
authorArtem Dergachev <artem.dergachev@gmail.com>
Mon, 24 Apr 2017 19:30:33 +0000 (19:30 +0000)
committerArtem Dergachev <artem.dergachev@gmail.com>
Mon, 24 Apr 2017 19:30:33 +0000 (19:30 +0000)
Null dereferences are suppressed if the lvalue was constrained to 0 for the
first time inside a sub-function that was inlined during analysis, because
such constraint is a valid defensive check that does not, by itself,
indicate that null pointer case is anyhow special for the caller.

If further operations on the lvalue are performed, the symbolic lvalue is
collapsed to concrete null pointer, and we need to track where does the null
pointer come from.

Improve such tracking for lvalue operations involving operator &.

rdar://problem/27876009

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

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

lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
lib/StaticAnalyzer/Core/Store.cpp
test/Analysis/inlining/inline-defensive-checks.c
test/Analysis/inlining/inline-defensive-checks.cpp
test/Analysis/null-deref-offsets.c [new file with mode: 0644]
test/Analysis/uninit-const.cpp

index 7309741d22e3907a50f60cd0b08875e12318c209..ec9fd6c75e0d9847bb7fdf4bbc86f6687bd73797 100644 (file)
@@ -961,6 +961,26 @@ bool bugreporter::trackNullOrUndefValue(const ExplodedNode *N,
   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;
   }
index ba48a60d5a1ccb468f693a3625cfbceedeb12187..1af49f68cc05508174e21d9b65aefc34b927c36c 100644 (file)
@@ -404,9 +404,15 @@ SVal StoreManager::getLValueFieldOrIvar(const Decl *D, SVal Base) {
 
   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:
@@ -431,7 +437,7 @@ SVal StoreManager::getLValueElement(QualType elementType, NonLoc Offset,
   // 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;
 
index 4029da651b65f6622fd3d189be125d07fa1f5574..010d3a774756a870e4570728b1294eaff313d8d0 100644 (file)
@@ -1,7 +1,7 @@
 // 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)
                ;
 }
@@ -139,3 +139,42 @@ void idcTrackZeroThroughDoubleAssignemnt(int 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
+}
index 6a803fa695c6cd4509414ee6b14de3611dedb2a4..eaae8d2ae28f2a2ab5ab4c4c897aaae8946fd50b 100644 (file)
@@ -70,4 +70,17 @@ int *retNull() {
 void test(int *p1, int *p2) {
   idc(p1);
        Foo f(p1);
-}
\ No newline at end of file
+}
+
+struct Bar {
+  int x;
+};
+void idcBar(Bar *b) {
+  if (b)
+    ;
+}
+void testRefToField(Bar *b) {
+  idcBar(b);
+  int &x = b->x; // no-warning
+  x = 5;
+}
diff --git a/test/Analysis/null-deref-offsets.c b/test/Analysis/null-deref-offsets.c
new file mode 100644 (file)
index 0000000..567c479
--- /dev/null
@@ -0,0 +1,34 @@
+// 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')}}
+}
index 75e932a77cedbdb112ff8a82d9deaf06e700a2ce..db969bfb67d62994976a7074ab489dfceebe043e 100644 (file)
@@ -122,7 +122,7 @@ void f1(void) {
 }
 
 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}}
 }