[PATCH] D45071: [analyzer] Track null or undef values through pointer arithmetic.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet.
Herald added subscribers: cfe-commits, rnkovacs, eraman.

Pointer arithmetic on null or undefined pointers results in null or undefined pointers. This is obvious for undefined pointers; for null pointers it follows from our incorrect-but-somehow-working approach that declares that `0 (Loc)` doesn't necessarily represent a pointer of numeric address value 0, but instead it represents any pointer that will cause a valid "null pointer dereference" issue when dereferenced.

For now we've been seeing through pointer arithmetic at the original dereference expression, i.e. in `bugreporter::getDerefExpr()`, but not during further investigation of the value's origins in `bugreporter::trackNullOrUndefValue()`. The patch fixes it.

Reproducers are reduced from WebKit's custom string implementations. CStringChecker doesn't call `getDerefExpr()` at all (this is //probably// because passing a pointer to `memcpy` is not an immediate null dereference - in fact i've no idea how this decision was made or how this code was actually supposed to operate). But the second test case demonstrates that simply adding `getDerefExpr()` to CStringChecker is insufficient. Also an older FIXME test gets fixed this way.


Repository:
  rC Clang

https://reviews.llvm.org/D45071

Files:
  lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  test/Analysis/inlining/inline-defensive-checks.c
  test/Analysis/null-deref-path-notes.c


Index: test/Analysis/null-deref-path-notes.c
===================================================================
--- test/Analysis/null-deref-path-notes.c
+++ test/Analysis/null-deref-path-notes.c
@@ -1,9 +1,25 @@
-// RUN: %clang_analyze_cc1 -w -x c -analyzer-checker=core -analyzer-output=text -verify %s
+// RUN: %clang_analyze_cc1 -w -x c -analyzer-checker=core,unix -analyzer-output=text -verify %s
 
 // Avoid the crash when finding the expression for tracking the origins
 // of the null pointer for path notes.
 void pr34373() {
   int *a = 0; // expected-note{{'a' initialized to a null pointer value}}
   (a + 0)[0]; // expected-warning{{Array access results in a null pointer dereference}}
               // expected-note at -1{{Array access results in a null pointer dereference}}
 }
+
+typedef __typeof(sizeof(int)) size_t;
+void *memcpy(void *dest, const void *src, unsigned long count);
+
+void f1(char *source) {
+  char *destination = 0; // expected-note{{'destination' initialized to a null pointer value}}
+  memcpy(destination + 0, source, 10); // expected-warning{{Null pointer argument in call to memory copy function}}
+                                       // expected-note at -1{{Null pointer argument in call to memory copy function}}
+}
+
+void f2(char *source) {
+  char *destination = 0; // FIXME: There should be a note here as well.
+  destination = destination + 0; // expected-note{{Null pointer value stored to 'destination'}}
+  memcpy(destination, source, 10); // expected-warning{{Null pointer argument in call to memory copy function}}
+                                   // expected-note at -1{{Null pointer argument in call to memory copy function}}
+}
Index: test/Analysis/inlining/inline-defensive-checks.c
===================================================================
--- test/Analysis/inlining/inline-defensive-checks.c
+++ test/Analysis/inlining/inline-defensive-checks.c
@@ -159,8 +159,7 @@
 void idcTrackZeroValueThroughUnaryPointerOperatorsWithOffset2(struct S *s) {
   idc(s);
   int *x = &(s->f2) - 1;
-  // FIXME: Should not warn.
-  *x = 7; // expected-warning{{Dereference of null pointer}}
+  *x = 7; // no-warning
 }
 
 void idcTrackZeroValueThroughUnaryPointerOperatorsWithAssignment(struct S *s) {
Index: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===================================================================
--- lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -75,6 +75,17 @@
   return false;
 }
 
+const Expr *peelOffPointerArithmetic(const BinaryOperator *B) {
+  if (B->isAdditiveOp() && B->getType()->isPointerType()) {
+    if (B->getLHS()->getType()->isPointerType()) {
+      return B->getLHS();
+    } else if (B->getRHS()->getType()->isPointerType()) {
+      return B->getRHS();
+    }
+  }
+  return nullptr;
+}
+
 /// Given that expression S represents a pointer that would be dereferenced,
 /// try to find a sub-expression from which the pointer came from.
 /// This is used for tracking down origins of a null or undefined value:
@@ -101,14 +112,8 @@
       E = CE->getSubExpr();
     } else if (const auto *B = dyn_cast<BinaryOperator>(E)) {
       // Pointer arithmetic: '*(x + 2)' -> 'x') etc.
-      if (B->getType()->isPointerType()) {
-        if (B->getLHS()->getType()->isPointerType()) {
-          E = B->getLHS();
-        } else if (B->getRHS()->getType()->isPointerType()) {
-          E = B->getRHS();
-        } else {
-          break;
-        }
+      if (const Expr *Inner = peelOffPointerArithmetic(B)) {
+        E = Inner;
       } else {
         // Probably more arithmetic can be pattern-matched here,
         // but for now give up.
@@ -1412,6 +1417,11 @@
       NI = NI->getFirstPred();
     } while (NI);
   }
+
+  if (auto *BO = dyn_cast<BinaryOperator>(Ex))
+    if (const Expr *SubEx = peelOffPointerArithmetic(BO))
+      return peelOffOuterExpr(SubEx, N);
+
   return Ex;
 }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D45071.140369.patch
Type: text/x-patch
Size: 3948 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180330/59a501f3/attachment.bin>