From cab91ecffd7a6cb94fa27e7fe8cd93dfc4d9a672 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bal=C3=A1zs=20K=C3=A9ri?= Date: Thu, 1 Aug 2024 12:56:25 +0200 Subject: [PATCH] [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. --- clang/docs/analyzer/checkers.rst | 40 ++++++++++++-- .../Checkers/PointerSubChecker.cpp | 45 ++++++++++++---- clang/test/Analysis/pointer-sub.c | 54 +++++++++++++++---- 3 files changed, 117 insertions(+), 22 deletions(-) diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst index 05d3f4d4018f..55832d20bd27 100644 --- a/clang/docs/analyzer/checkers.rst +++ b/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) diff --git a/clang/lib/StaticAnalyzer/Checkers/PointerSubChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/PointerSubChecker.cpp index eea93a41f138..b856b0edc615 100644 --- a/clang/lib/StaticAnalyzer/Checkers/PointerSubChecker.cpp +++ b/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 @@ public: }; } +static bool isArrayVar(const MemRegion *R) { + while (R) { + if (isa(R)) + return true; + if (const auto *ER = dyn_cast(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(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(LR) || isa(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(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)); } diff --git a/clang/test/Analysis/pointer-sub.c b/clang/test/Analysis/pointer-sub.c index 88e6dec2d172..194c89188995 100644 --- a/clang/test/Analysis/pointer-sub.c +++ b/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) }