[clang][analyzer] Improve PointerSubChecker #96501
Conversation
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.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-static-analyzer-1 Author: Balázs Kéri (balazske) ChangesThe 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. Full diff: https://github.com/llvm/llvm-project/pull/96501.diff 2 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/PointerSubChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/PointerSubChecker.cpp
index eea93a41f1384..63ed4df67d6d5 100644
--- a/clang/lib/StaticAnalyzer/Checkers/PointerSubChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/PointerSubChecker.cpp
@@ -49,12 +49,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,7 +80,6 @@ bool PointerSubChecker::checkArrayBounds(CheckerContext &C, const Expr *E,
};
ProgramStateRef State = C.getState();
- const MemRegion *SuperReg = ElemReg->getSuperRegion();
SValBuilder &SVB = C.getSValBuilder();
if (SuperReg == Reg) {
@@ -121,8 +136,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);
diff --git a/clang/test/Analysis/pointer-sub.c b/clang/test/Analysis/pointer-sub.c
index 88e6dec2d172f..404b8530b89c0 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,17 @@ void f7(int *p) {
}
void f8(int n) {
- int a[10];
+ int a[10] = {1};
int d = a[n] - a[0];
}
+
+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
+}
|
There was a problem hiding this comment.
LGTM.
I'm a bit surprised to see that this checker duplicates the logic of the array bounds checkers (in the special case when the indexing operation is within a pointer subtraction). Right now this is OK but we'll need to delete those parts of this checker once ArrayBoundV2 becomes stable.
|
These results look correct according to the checker, but I am not sure if such results are useful or really invalid: |
The current behavior is good in this area, you don't need to spend time on it. This trickery basically reimplements |
|
The warning message may be still misleading if the LHS or RHS "arrays" are non-array variables. Is it better to improve the messages in this case (or detect if |
I think that the warning message is OK: "Subtraction of two pointers that do not point into the same array is undefined behavior." -- this also covers the case when one or both of the pointers do not point to arrays. (It doesn't mention the corner case that it's also valid to subtract two identical pointers that point to a non-array value, but that's completely irrelevant in practice, so wouldn't be a helpful suggestion.) However, it would be helpful, if you changed the notes: you should keep the current "Array at the left-hand side of subtraction" when the declaration does declare an array, but use "Object" or "Variable" instead of "Array" when it isn't an array.
I think that would be a waste of time, because it's very rare that a project manually reimplements |
|
If the array bounds checker does the same job then the array bounds check it is not needed in this checker. Specially if it makes no difference if the indexing is used at pointer subtraction. |
I still think that this |
I agree with @NagyDonat that we don't need special handling of this case in the code, however I think the checker documentation should be extended with the description of this special case as it may be a surprising warning from the checker with an example. Specifically that it warns for cases where two pointers are subtracted which point to members of the same struct and suggest the usage of the standard compliant solution: offsetof. I think it should be described which pointer subtractions the checker accepts and which don't (with examples) and a reference to the standard where it describes the undefined behaviour. |
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.