[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,12 +27,84 @@ 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
@@ -57,6 +130,12 @@ void PointerSubChecker::checkPreStmt(const BinaryOperator *B,
 
   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 (ElemLR && ElemRR) {
     const MemRegion *SuperLR = ElemLR->getSuperRegion();
     const MemRegion *SuperRR = ElemRR->getSuperRegion();
@@ -67,14 +146,7 @@ void PointerSubChecker::checkPreStmt(const BinaryOperator *B,
       return;
   }
 
-  if (ExplodedNode *N = C.generateNonFatalErrorNode()) {
-    constexpr llvm::StringLiteral Msg =
-        "Subtraction of two pointers that do not point into the same array "
-        "is undefined behavior.";
-    auto R = std::make_unique<PathSensitiveBugReport>(BT, Msg, N);
-    R->addRange(B->getSourceRange());
-    C.emitReport(std::move(R));
-  }
+  reportBug(C, B, Msg_MemRegionDifferent);
 }
 
 void ento::registerPointerSubChecker(CheckerManager &mgr) {

clang/test/Analysis/pointer-sub.c

@@ -6,7 +6,16 @@ void f1(void) {
   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; // expected-warning{{Subtraction of two pointers that do not point into the same array is undefined behavior}}
+  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) {
@@ -21,7 +30,7 @@ void f2(void) {
   q = a + 10;
   d = q - p; // no warning (use of pointer to one after the end is allowed)
   q = a + 11;
-  d = q - a; // no-warning (no check for past-the-end array pointers in this checker)
+  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
@@ -53,10 +62,10 @@ void f4(void) {
   int (*p)[m] = a; // p == &a[0]
   p += 1; // p == &a[1]
 
-  // FIXME: This warning is not needed
+  // 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 warning is not needed
+  // 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}}
@@ -104,3 +113,8 @@ 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];
+}