[clang][analyzer] Improved PointerSubChecker

clang/lib/StaticAnalyzer/Checkers/PointerSubChecker.cpp

@@ -17,6 +17,7 @@
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicExtent.h"
 #include "llvm/ADT/StringRef.h"
 
 using namespace clang;
@@ -26,16 +27,88 @@ namespace {
 class PointerSubChecker
   : public Checker< check::PreStmt<BinaryOperator> > {
   const BugType BT{this, "Pointer subtraction"};
+  const llvm::StringLiteral Msg_MemRegionDifferent =
+      "Subtraction of two pointers that do not point into the same array "
+      "is undefined behavior.";
+  const llvm::StringLiteral Msg_LargeArrayIndex =
+      "Using an array index greater than the array size at pointer subtraction "
+      "is undefined behavior.";
+  const llvm::StringLiteral Msg_NegativeArrayIndex =
+      "Using a negative array index at pointer subtraction "
+      "is undefined behavior.";
+  const llvm::StringLiteral Msg_BadVarIndex =
+      "Indexing the address of a variable with other than 1 at this place "
+      "is undefined behavior.";
+
+  bool checkArrayBounds(CheckerContext &C, const Expr *E,
+                        const ElementRegion *ElemReg,
+                        const MemRegion *Reg) const;
+  void reportBug(CheckerContext &C, const Expr *E,
+                 const llvm::StringLiteral &Msg) const;
 
 public:
   void checkPreStmt(const BinaryOperator *B, CheckerContext &C) const;
 };
 }
 
+bool PointerSubChecker::checkArrayBounds(CheckerContext &C, const Expr *E,
+                                         const ElementRegion *ElemReg,
+                                         const MemRegion *Reg) const {
+  if (!ElemReg)
+    return true;
+
+  ProgramStateRef State = C.getState();
+  const MemRegion *SuperReg = ElemReg->getSuperRegion();
+  SValBuilder &SVB = C.getSValBuilder();
+
+  if (SuperReg == Reg) {
+    if (const llvm::APSInt *I = SVB.getKnownValue(State, ElemReg->getIndex());
+        I && (!I->isOne() && !I->isZero()))
+      reportBug(C, E, Msg_BadVarIndex);
+    return false;
+  }
+
+  DefinedOrUnknownSVal ElemCount =
+      getDynamicElementCount(State, SuperReg, SVB, ElemReg->getElementType());
+  auto IndexTooLarge = SVB.evalBinOp(C.getState(), BO_GT, ElemReg->getIndex(),
+                                     ElemCount, SVB.getConditionType())
+                           .getAs<DefinedOrUnknownSVal>();
+  if (IndexTooLarge) {
+    ProgramStateRef S1, S2;
+    std::tie(S1, S2) = C.getState()->assume(*IndexTooLarge);
+    if (S1 && !S2) {
+      reportBug(C, E, Msg_LargeArrayIndex);
+      return false;
+    }
+  }
+  auto IndexTooSmall = SVB.evalBinOp(State, BO_LT, ElemReg->getIndex(),
+                                     SVB.makeZeroVal(SVB.getArrayIndexType()),
+                                     SVB.getConditionType())
+                           .getAs<DefinedOrUnknownSVal>();
+  if (IndexTooSmall) {
+    ProgramStateRef S1, S2;
+    std::tie(S1, S2) = State->assume(*IndexTooSmall);
+    if (S1 && !S2) {
+      reportBug(C, E, Msg_NegativeArrayIndex);
+      return false;
+    }
+  }
+  return true;
+}
+
+void PointerSubChecker::reportBug(CheckerContext &C, const Expr *E,
+                                  const llvm::StringLiteral &Msg) const {
+  if (ExplodedNode *N = C.generateNonFatalErrorNode()) {
+    auto R = std::make_unique<PathSensitiveBugReport>(BT, Msg, N);
+    R->addRange(E->getSourceRange());
+    C.emitReport(std::move(R));
+  }
+}
+
 void PointerSubChecker::checkPreStmt(const BinaryOperator *B,
                                      CheckerContext &C) const {
   // When doing pointer subtraction, if the two pointers do not point to the
-  // same memory chunk, emit a warning.
+  // same array, emit a warning.
   if (B->getOpcode() != BO_Sub)
     return;
 
@@ -44,28 +117,36 @@ void PointerSubChecker::checkPreStmt(const BinaryOperator *B,
 
   const MemRegion *LR = LV.getAsRegion();
   const MemRegion *RR = RV.getAsRegion();
-
-  if (!(LR && RR))
+  if (!LR || !RR)
     return;
 
-  const MemRegion *BaseLR = LR->getBaseRegion();
-  const MemRegion *BaseRR = RR->getBaseRegion();
+  // Allow subtraction of identical pointers.
+  if (LR == RR)
+    return;
 
-  if (BaseLR == BaseRR)
+  // No warning if one operand is unknown.
+  if (isa<SymbolicRegion>(LR) || isa<SymbolicRegion>(RR))
     return;
 
-  // Allow arithmetic on different symbolic regions.
-  if (isa<SymbolicRegion>(BaseLR) || isa<SymbolicRegion>(BaseRR))
+  const auto *ElemLR = dyn_cast<ElementRegion>(LR);
+  const auto *ElemRR = dyn_cast<ElementRegion>(RR);
+
+  if (!checkArrayBounds(C, B->getLHS(), ElemLR, RR))
+    return;
+  if (!checkArrayBounds(C, B->getRHS(), ElemRR, LR))
     return;
 
-  if (ExplodedNode *N = C.generateNonFatalErrorNode()) {
-    constexpr llvm::StringLiteral Msg =
-        "Subtraction of two pointers that do not point to the same memory "
-        "chunk may cause incorrect result.";
-    auto R = std::make_unique<PathSensitiveBugReport>(BT, Msg, N);
-    R->addRange(B->getSourceRange());
-    C.emitReport(std::move(R));
+  if (ElemLR && ElemRR) {
+    const MemRegion *SuperLR = ElemLR->getSuperRegion();
+    const MemRegion *SuperRR = ElemRR->getSuperRegion();
+    if (SuperLR == SuperRR)
+      return;
+    // Allow arithmetic on different symbolic regions.
+    if (isa<SymbolicRegion>(SuperLR) || isa<SymbolicRegion>(SuperRR))
+      return;
   }
+
+  reportBug(C, B, Msg_MemRegionDifferent);
 }
 
 void ento::registerPointerSubChecker(CheckerManager &mgr) {

clang/test/Analysis/casts.c

@@ -138,7 +138,9 @@ void multiDimensionalArrayPointerCasts(void) {
   clang_analyzer_eval(y1 == y2); // expected-warning{{TRUE}}
 
   // FIXME: should be FALSE (i.e. equal pointers).
+  // FIXME: pointer subtraction warning might be incorrect
   clang_analyzer_eval(y1 - y2); // expected-warning{{UNKNOWN}}
+  // expected-warning@-1{{Subtraction of two pointers that do not point into the same array is undefined behavior}}
   // FIXME: should be TRUE (i.e. same symbol).
   clang_analyzer_eval(*y1 == *y2); // expected-warning{{UNKNOWN}}
 
@@ -147,7 +149,9 @@ void multiDimensionalArrayPointerCasts(void) {
   clang_analyzer_eval(y1 == y3); // expected-warning{{TRUE}}
 
   // FIXME: should be FALSE (i.e. equal pointers).
+  // FIXME: pointer subtraction warning might be incorrect
   clang_analyzer_eval(y1 - y3); // expected-warning{{UNKNOWN}}
+  // expected-warning@-1{{Subtraction of two pointers that do not point into the same array is undefined behavior}}
   // FIXME: should be TRUE (i.e. same symbol).
   clang_analyzer_eval(*y1 == *y3); // expected-warning{{UNKNOWN}}
 

clang/test/Analysis/pointer-sub.c

@@ -0,0 +1,120 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.core.PointerSub -verify %s
+
+void f1(void) {
+  int x, y, z[10];
+  int d = &y - &x; // expected-warning{{Subtraction of two pointers that do not point into the same array is undefined behavior}}
+  d = z - &y; // expected-warning{{Subtraction of two pointers that do not point into the same array is undefined behavior}}
+  d = &x - &x; // no-warning (subtraction of any two identical pointers is allowed)
+  d = (long *)&x - (long *)&x;
+  d = (&x + 1) - &x; // no-warning ('&x' is like a single-element array)
+  d = &x - (&x + 1); // no-warning
+  d = (&x + 0) - &x; // no-warning
+  d = (&x - 1) - &x; // expected-warning{{Indexing the address of a variable with other than 1 at this place is undefined behavior}}
+  d = (&x + 2) - &x; // expected-warning{{Indexing the address of a variable with other than 1 at this place is undefined behavior}}
+
+  d = (z + 9) - z; // no-warning (pointers to same array)
+  d = (z + 10) - z; // no-warning (pointer to "one after the end")
+  d = (z + 11) - z; // expected-warning{{Using an array index greater than the array size at pointer subtraction is undefined behavior}}
+  d = (z - 1) - z; // expected-warning{{Using a negative array index at pointer subtraction is undefined behavior}}
+}
+
+void f2(void) {
+  int a[10], b[10], c;
+  int *p = &a[2];
+  int *q = &a[8];
+  int d = q - p; // no-warning (pointers into the same array)
+
+  q = &b[3];
+  d = q - p; // expected-warning{{Subtraction of two pointers that}}
+
+  q = a + 10;
+  d = q - p; // no warning (use of pointer to one after the end is allowed)
+  q = a + 11;
+  d = q - a; // expected-warning{{Using an array index greater than the array size at pointer subtraction is undefined behavior}}
+
+  d = &a[4] - a; // no-warning
+  d = &a[2] - p; // no-warning
+  d = &c - p; // expected-warning{{Subtraction of two pointers that}}
+
+  d = (int *)((char *)(&a[4]) + sizeof(int)) - &a[4]; // no-warning (pointers into the same array data)
+  d = (int *)((char *)(&a[4]) + 1) - &a[4]; // expected-warning{{Subtraction of two pointers that}}
+}
+
+void f3(void) {
+  int a[3][4];
+  int d;
+
+  d = &(a[2]) - &(a[1]);
+  d = a[2] - a[1]; // expected-warning{{Subtraction of two pointers that}}
+  d = a[1] - a[1];
+  d = &(a[1][2]) - &(a[1][0]);
+  d = &(a[1][2]) - &(a[0][0]); // expected-warning{{Subtraction of two pointers that}}
+
+  d = (int *)((char *)(&a[2][2]) + sizeof(int)) - &a[2][2]; // expected-warning{{Subtraction of two pointers that}}
+  d = (int *)((char *)(&a[2][2]) + 1) - &a[2][2]; // expected-warning{{Subtraction of two pointers that}}
+  d = (int (*)[4])((char *)&a[2] + sizeof(int (*)[4])) - &a[2]; // expected-warning{{Subtraction of two pointers that}}
+  d = (int (*)[4])((char *)&a[2] + 1) - &a[2]; // expected-warning{{Subtraction of two pointers that}}
+}
+
+void f4(void) {
+  int n = 4, m = 3;
+  int a[n][m];
+  int (*p)[m] = a; // p == &a[0]
+  p += 1; // p == &a[1]
+
+  // FIXME: This is a known problem with -Wpointer-arith
+  int d = p - a; // d == 1 // expected-warning{{subtraction of pointers to type 'int[m]' of zero size has undefined behavior}}
+
+  // FIXME: This is a known problem with -Wpointer-arith
+  d = &(a[2]) - &(a[1]); // expected-warning{{subtraction of pointers to type 'int[m]' of zero size has undefined behavior}}
+
+  d = a[2] - a[1]; // expected-warning{{Subtraction of two pointers that}}
+}
+
+typedef struct {
+  int a;
+  int b;
+  int c[10];
+  int d[10];
+} S;
+
+void f5(void) {
+  S s;
+  int y;
+  int d;
+
+  d = &s.b - &s.a; // expected-warning{{Subtraction of two pointers that}}
+  d = &s.c[0] - &s.a; // expected-warning{{Subtraction of two pointers that}}
+  d = &s.b - &y; // expected-warning{{Subtraction of two pointers that}}
+  d = &s.c[3] - &s.c[2];
+  d = &s.d[3] - &s.c[2]; // expected-warning{{Subtraction of two pointers that}}
+  d = s.d - s.c; // expected-warning{{Subtraction of two pointers that}}
+
+  S sa[10];
+  d = &sa[2] - &sa[1];
+  d = &sa[2].a - &sa[1].b; // expected-warning{{Subtraction of two pointers that}}
+}
+
+void f6(void) {
+  long long l;
+  char *a1 = (char *)&l;
+  int d = a1[3] - l;
+
+  long long la1[3];
+  long long la2[3];
+  char *pla1 = (char *)la1;
+  char *pla2 = (char *)la2;
+  d = pla1[1] - pla1[0];
+  d = (long long *)&pla1[1] - &l; // expected-warning{{Subtraction of two pointers that}}
+  d = &pla2[3] - &pla1[3]; // expected-warning{{Subtraction of two pointers that}}
+}
+
+void f7(int *p) {
+  int a[10];
+  int d = &a[10] - p; // no-warning ('p' is unknown, even if it cannot point into 'a')
+}
+
+void f8(int n) {
+  int a[10];
+  int d = a[n] - a[0];
+}

clang/test/Analysis/ptr-arith.c

@@ -1,5 +1,5 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.core.FixedAddr,alpha.core.PointerArithm,alpha.core.PointerSub,debug.ExprInspection -Wno-pointer-to-int-cast -verify -triple x86_64-apple-darwin9 -Wno-tautological-pointer-compare -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.core.FixedAddr,alpha.core.PointerArithm,alpha.core.PointerSub,debug.ExprInspection -Wno-pointer-to-int-cast -verify -triple i686-apple-darwin9 -Wno-tautological-pointer-compare -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.core.FixedAddr,alpha.core.PointerArithm,debug.ExprInspection -Wno-pointer-to-int-cast -verify -triple x86_64-apple-darwin9 -Wno-tautological-pointer-compare -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.core.FixedAddr,alpha.core.PointerArithm,debug.ExprInspection -Wno-pointer-to-int-cast -verify -triple i686-apple-darwin9 -Wno-tautological-pointer-compare -analyzer-config eagerly-assume=false %s
 
 void clang_analyzer_eval(int);
 void clang_analyzer_dump(int);
@@ -35,16 +35,6 @@ domain_port (const char *domain_b, const char *domain_e,
   return port;
 }
 
-void f3(void) {
-  int x, y;
-  int d = &y - &x; // expected-warning{{Subtraction of two pointers that do not point to the same memory chunk may cause incorrect result}}
-
-  int a[10];
-  int *p = &a[2];
-  int *q = &a[8];
-  d = q-p; // no-warning
-}
-
 void f4(void) {
   int *p;
   p = (int*) 0x10000; // expected-warning{{Using a fixed address is not portable because that address will probably not be valid in all environments or platforms}}