[clang][analyzer] Improve PointerSubChecker (#96501)

The checker could report false positives if pointer arithmetic was done
on pointers to non-array data before pointer subtraction. Another
problem is fixed that could cause false positive if members of the same
structure but in different memory objects are subtracted.

Author: balazske

clang/docs/analyzer/checkers.rst

@@ -2498,15 +2498,49 @@ Check for pointer arithmetic on locations other than array elements.
 
 alpha.core.PointerSub (C)
 """""""""""""""""""""""""
-Check for pointer subtractions on two pointers pointing to different memory chunks.
+Check for pointer subtractions on two pointers pointing to different memory
+chunks. According to the C standard §6.5.6 only subtraction of pointers that
+point into (or one past the end) the same array object is valid (for this
+purpose non-array variables are like arrays of size 1).
 
 .. code-block:: c
 
  void test() {
-   int x, y;
-   int d = &y - &x; // warn
+   int a, b, c[10], d[10];
+   int x = &c[3] - &c[1];
+   x = &d[4] - &c[1]; // warn: 'c' and 'd' are different arrays
+   x = (&a + 1) - &a;
+   x = &b - &a; // warn: 'a' and 'b' are different variables
+   x = (&a + 2) - &a; // warn: for a variable it is only valid to have a pointer
+                      // to one past the address of it
+   x = &c[10] - &c[0];
+   x = &c[11] - &c[0]; // warn: index larger than one past the end
+   x = &c[-1] - &c[0]; // warn: negative index
  }
 
+ struct S {
+   int x[10];
+   int y[10];
+ };
+
+ void test1() {
+   struct S a[10];
+   struct S b;
+   int d = &a[4] - &a[6];
+   d = &a[0].x[3] - &a[0].x[1];
+   d = a[0].y - a[0].x; // warn: 'S.b' and 'S.a' are different objects
+   d = (char *)&b.y - (char *)&b.x; // warn: different members of the same object
+   d = (char *)&b.y - (char *)&b; // warn: object of type S is not the same array as a member of it
+ }
+
+There may be existing applications that use code like above for calculating
+offsets of members in a structure, using pointer subtractions. This is still
+undefined behavior according to the standard and code like this can be replaced
+with the `offsetof` macro.
+
+The checker only reports cases where stack-allocated objects are involved (no
+warnings on pointers to memory allocated by `malloc`).
+
 .. _alpha-core-StackAddressAsyncEscape:
 
 alpha.core.StackAddressAsyncEscape (C)

clang/lib/StaticAnalyzer/Checkers/PointerSubChecker.cpp

@@ -19,6 +19,7 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/DynamicExtent.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/FormatVariadic.h"
 
 using namespace clang;
 using namespace ento;
@@ -40,6 +41,11 @@ class PointerSubChecker
       "Indexing the address of a variable with other than 1 at this place "
       "is undefined behavior.";
 
+  /// Check that an array is indexed in the allowed range that is 0 to "one
+  /// after the end". The "array" can be address of a non-array variable.
+  /// @param E Expression of the pointer subtraction.
+  /// @param ElemReg An indexed region in the subtraction expression.
+  /// @param Reg Region of the other side of the expression.
   bool checkArrayBounds(CheckerContext &C, const Expr *E,
                         const ElementRegion *ElemReg,
                         const MemRegion *Reg) const;
@@ -49,12 +55,28 @@ class PointerSubChecker
 };
 }
 
+static bool isArrayVar(const MemRegion *R) {
+  while (R) {
+    if (isa<VarRegion>(R))
+      return true;
+    if (const auto *ER = dyn_cast<ElementRegion>(R))
+      R = ER->getSuperRegion();
+    else
+      return false;
+  }
+  return false;
+}
+
 bool PointerSubChecker::checkArrayBounds(CheckerContext &C, const Expr *E,
                                          const ElementRegion *ElemReg,
                                          const MemRegion *Reg) const {
   if (!ElemReg)
     return true;
 
+  const MemRegion *SuperReg = ElemReg->getSuperRegion();
+  if (!isArrayVar(SuperReg))
+    return true;
+
   auto ReportBug = [&](const llvm::StringLiteral &Msg) {
     if (ExplodedNode *N = C.generateNonFatalErrorNode()) {
       auto R = std::make_unique<PathSensitiveBugReport>(BT, Msg, N);
@@ -64,10 +86,10 @@ bool PointerSubChecker::checkArrayBounds(CheckerContext &C, const Expr *E,
   };
 
   ProgramStateRef State = C.getState();
-  const MemRegion *SuperReg = ElemReg->getSuperRegion();
   SValBuilder &SVB = C.getSValBuilder();
 
   if (SuperReg == Reg) {
+    // Case like `(&x + 1) - &x`. Only 1 or 0 is allowed as index.
     if (const llvm::APSInt *I = SVB.getKnownValue(State, ElemReg->getIndex());
         I && (!I->isOne() && !I->isZero()))
       ReportBug(Msg_BadVarIndex);
@@ -121,8 +143,9 @@ void PointerSubChecker::checkPreStmt(const BinaryOperator *B,
   if (LR == RR)
     return;
 
-  // No warning if one operand is unknown.
-  if (isa<SymbolicRegion>(LR) || isa<SymbolicRegion>(RR))
+  // No warning if one operand is unknown or resides in a region that could be
+  // equal to the other.
+  if (LR->getSymbolicBase() || RR->getSymbolicBase())
     return;
 
   const auto *ElemLR = dyn_cast<ElementRegion>(LR);
@@ -159,12 +182,16 @@ void PointerSubChecker::checkPreStmt(const BinaryOperator *B,
     //   do_something(&a.array[5] - &b.array[5]);
     // In this case don't emit notes.
     if (DiffDeclL != DiffDeclR) {
-      if (DiffDeclL)
-        R->addNote("Array at the left-hand side of subtraction",
-                   {DiffDeclL, C.getSourceManager()});
-      if (DiffDeclR)
-        R->addNote("Array at the right-hand side of subtraction",
-                   {DiffDeclR, C.getSourceManager()});
+      auto AddNote = [&R, &C](const ValueDecl *D, StringRef SideStr) {
+        if (D) {
+          std::string Msg = llvm::formatv(
+              "{0} at the {1}-hand side of subtraction",
+              D->getType()->isArrayType() ? "Array" : "Object", SideStr);
+          R->addNote(Msg, {D, C.getSourceManager()});
+        }
+      };
+      AddNote(DiffDeclL, "left");
+      AddNote(DiffDeclR, "right");
     }
     C.emitReport(std::move(R));
   }

clang/test/Analysis/pointer-sub.c

@@ -1,4 +1,4 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.core.PointerSub -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core.PointerSub -analyzer-output=text-minimal -verify %s
 
 void f1(void) {
   int x, y, z[10];
@@ -73,15 +73,15 @@ void f4(void) {
   d = a[2] - a[1]; // expected-warning{{Subtraction of two pointers that}}
 }
 
-typedef struct {
+struct S {
   int a;
   int b;
   int c[10]; // expected-note2{{Array at the right-hand side of subtraction}}
   int d[10]; // expected-note2{{Array at the left-hand side of subtraction}}
-} S;
+};
 
 void f5(void) {
-  S s;
+  struct S s;
   int y;
   int d;
 
@@ -92,18 +92,18 @@ void f5(void) {
   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];
+  struct 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;
+  long long l = 2;
   char *a1 = (char *)&l;
   int d = a1[3] - l;
 
-  long long la1[3]; // expected-note{{Array at the right-hand side of subtraction}}
-  long long la2[3]; // expected-note{{Array at the left-hand side of subtraction}}
+  long long la1[3] = {1}; // expected-note{{Array at the right-hand side of subtraction}}
+  long long la2[3] = {1}; // expected-note{{Array at the left-hand side of subtraction}}
   char *pla1 = (char *)la1;
   char *pla2 = (char *)la2;
   d = pla1[1] - pla1[0];
@@ -117,6 +117,40 @@ void f7(int *p) {
 }
 
 void f8(int n) {
-  int a[10];
-  int d = a[n] - a[0];
+  int a[10] = {1};
+  int d = a[n] - a[0]; // no-warning
+}
+
+int f9(const char *p1) {
+  const char *p2 = p1;
+  --p1;
+  ++p2;
+  return p1 - p2; // no-warning
+}
+
+int f10(struct S *p1, struct S *p2) {
+  return &p1->c[5] - &p2->c[5]; // no-warning
+}
+
+struct S1 {
+  int a;
+  int b; // expected-note{{Object at the right-hand side of subtraction}}
+};
+
+int f11() {
+  struct S1 s; // expected-note{{Object at the left-hand side of subtraction}}
+  return (char *)&s - (char *)&s.b; // expected-warning{{Subtraction of two pointers that}}
+}
+
+struct S2 {
+  char *p1;
+  char *p2;
+};
+
+void init_S2(struct S2 *);
+
+int f12() {
+  struct S2 s;
+  init_S2(&s);
+  return s.p1 - s.p2; // no-warning (pointers are unknown)
 }