[PATCH] Extend PointerSubChecker to detect more bugs
Matthew Dempsky
matthew at dempsky.org
Sat Jun 22 12:53:05 PDT 2013
As suggested in http://llvm.org/bugs/show_bug.cgi?id=16380, I wanted
to implement a clang analyzer check to detect comparisons of pointers
to different memory ranges. It turns out there's already a check for
pointer subtraction, which has very similar restrictions, so it's easy
to extend to warn about comparisons too by just matching more binops.
Notably, pointer subtraction is stricter and is only valid for
pointers to elements of the same array, whereas pointer comparison is
also valid for pointers to member of the same struct. The current
checker, however, doesn't distinguish these cases anyway, so no
problem here. Just the same, I added a no-warning test case to make
sure this doesn't regress in the future.
Lastly, there's a distinction between C and C++ here: C says
comparisons between pointers to different memory chunks is undefined,
whereas C++ says it's only unspecified. (Both agree subtraction
between pointers to different chunks is undefined.) I tried to
account for this, but I'm not sure I did so correctly.
Index: clang/lib/StaticAnalyzer/Checkers/PointerSubChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/PointerSubChecker.cpp (revision 184494)
+++ clang/lib/StaticAnalyzer/Checkers/PointerSubChecker.cpp (working copy)
@@ -25,7 +25,7 @@
namespace {
class PointerSubChecker
: public Checker< check::PreStmt<BinaryOperator> > {
- mutable OwningPtr<BuiltinBug> BT;
+ mutable OwningPtr<BuiltinBug> BT_sub, BT_cmp, BT_nullsub, BT_nullcmp;
public:
void checkPreStmt(const BinaryOperator *B, CheckerContext &C) const;
@@ -34,16 +34,63 @@
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.
- if (B->getOpcode() != BO_Sub)
+ // When doing pointer subtraction or relational comparisons, if the two
+ // pointers do not point to the same memory chunk, emit a warning.
+ if (B->getOpcode() != BO_Sub &&
+ B->getOpcode() != BO_LT && B->getOpcode() != BO_GT &&
+ B->getOpcode() != BO_LE && B->getOpcode() != BO_GE)
return;
+ // This check is only sensible if at least one argument is actually
+ // a pointer.
+ if (!B->getLHS()->getType()->isPointerType() &&
+ !B->getRHS()->getType()->isPointerType())
+ return;
+
ProgramStateRef state = C.getState();
const LocationContext *LCtx = C.getLocationContext();
SVal LV = state->getSVal(B->getLHS(), LCtx);
SVal RV = state->getSVal(B->getRHS(), LCtx);
+ Optional<DefinedOrUnknownSVal> DLV = LV.getAs<DefinedOrUnknownSVal>();
+ Optional<DefinedOrUnknownSVal> DRV = RV.getAs<DefinedOrUnknownSVal>();
+ if (DLV && DRV) {
+ ProgramStateRef LHSNotNull, LHSNull;
+ llvm::tie(LHSNotNull, LHSNull) = state->assume(*DLV);
+
+ ProgramStateRef BothNotNull = LHSNotNull ? LHSNotNull->assume(*DRV, true) : 0;
+ ProgramStateRef BothNull = LHSNull ? LHSNull->assume(*DRV, false) : 0;
+
+ if (!BothNotNull && !BothNull) {
+ if (ExplodedNode *N = C.addTransition()) {
+ BugReport *R;
+ if (B->getOpcode() == BO_Sub) {
+ if (!BT_nullsub)
+ BT_nullsub.reset(new BuiltinBug("Pointer subtraction",
+ "Subtraction between a null and non-null pointer "
+ "may cause incorrect result."));
+ R = new BugReport(*BT_nullsub, BT_nullsub->getDescription(), N);
+ } else {
+ if (!BT_nullcmp)
+ BT_nullcmp.reset(new BuiltinBug("Pointer comparison",
+ "Comparison between a null and non-null pointer "
+ "may cause incorrect result."));
+ R = new BugReport(*BT_nullcmp, BT_nullcmp->getDescription(), N);
+ }
+ R->addRange(B->getSourceRange());
+ C.emitReport(R);
+ }
+ return;
+ }
+
+ if (B->getOpcode() == BO_Sub || !C.getLangOpts().CPlusPlus) {
+ if (BothNull)
+ C.addTransition(BothNull);
+ if (BothNotNull)
+ C.addTransition(BothNotNull);
+ }
+ }
+
const MemRegion *LR = LV.getAsRegion();
const MemRegion *RR = RV.getAsRegion();
@@ -61,11 +108,20 @@
return;
if (ExplodedNode *N = C.addTransition()) {
- if (!BT)
- BT.reset(new BuiltinBug("Pointer subtraction",
- "Subtraction of two pointers that do not point to "
- "the same memory chunk may cause incorrect result."));
- BugReport *R = new BugReport(*BT, BT->getDescription(), N);
+ BugReport *R;
+ if (B->getOpcode() == BO_Sub) {
+ if (!BT_sub)
+ BT_sub.reset(new BuiltinBug("Pointer subtraction",
+ "Subtraction of two pointers that do not point to "
+ "the same memory chunk may cause incorrect result."));
+ R = new BugReport(*BT_sub, BT_sub->getDescription(), N);
+ } else {
+ if (!BT_cmp)
+ BT_cmp.reset(new BuiltinBug("Pointer comparison",
+ "Comparison of two pointers that do not point to "
+ "the same memory chunk may cause incorrect result."));
+ R = new BugReport(*BT_cmp, BT_cmp->getDescription(), N);
+ }
R->addRange(B->getSourceRange());
C.emitReport(R);
}
Index: clang/test/Analysis/ptr-arith.c
===================================================================
--- clang/test/Analysis/ptr-arith.c (revision 184494)
+++ clang/test/Analysis/ptr-arith.c (working copy)
@@ -37,11 +37,22 @@
void f3() {
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 e = &y < &x; // expected-warning{{Comparison 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
+ e = q<p; // no-warning
+
+ struct { int one, two; } s;
+ p = &s.one;
+ q = &s.two;
+ e = q < p; // no-warning
+
+ q = (int *)0;
+ d = p - q; // expected-warning{{Subtraction between a null and non-null pointer may cause incorrect result}}
+ e = p < q; // expected-warning{{Comparison between a null and non-null pointer may cause incorrect result}}
}
void f4() {
@@ -67,32 +78,32 @@
start:
// LHS is a label, RHS is NULL
clang_analyzer_eval(&&start != 0); // expected-warning{{TRUE}}
- clang_analyzer_eval(&&start >= 0); // expected-warning{{TRUE}}
- clang_analyzer_eval(&&start > 0); // expected-warning{{TRUE}}
- clang_analyzer_eval((&&start - 0) != 0); // expected-warning{{TRUE}}
+ clang_analyzer_eval(&&start >= 0); // expected-warning{{TRUE}} expected-warning{{Comparison between a null and non-null pointer may cause incorrect result}}
+ clang_analyzer_eval(&&start > 0); // expected-warning{{TRUE}} expected-warning{{Comparison between a null and non-null pointer may cause incorrect result}}
+ clang_analyzer_eval((&&start - 0) != 0); // expected-warning{{TRUE}} expected-warning{{Subtraction between a null and non-null pointer may cause incorrect result}}
// LHS is a non-symbolic value, RHS is NULL
clang_analyzer_eval(&a != 0); // expected-warning{{TRUE}}
- clang_analyzer_eval(&a >= 0); // expected-warning{{TRUE}}
- clang_analyzer_eval(&a > 0); // expected-warning{{TRUE}}
- clang_analyzer_eval((&a - 0) != 0); // expected-warning{{TRUE}} expected-warning{{Pointer arithmetic done on non-array variables}}
+ clang_analyzer_eval(&a >= 0); // expected-warning{{TRUE}} expected-warning{{Comparison between a null and non-null pointer may cause incorrect result}}
+ clang_analyzer_eval(&a > 0); // expected-warning{{TRUE}} expected-warning{{Comparison between a null and non-null pointer may cause incorrect result}}
+ clang_analyzer_eval((&a - 0) != 0); // expected-warning{{TRUE}} expected-warning{{Pointer arithmetic done on non-array variables}} expected-warning{{Subtraction between a null and non-null pointer may cause incorrect result}}
// LHS is NULL, RHS is non-symbolic
// The same code is used for labels and non-symbolic values.
clang_analyzer_eval(0 != &a); // expected-warning{{TRUE}}
- clang_analyzer_eval(0 <= &a); // expected-warning{{TRUE}}
- clang_analyzer_eval(0 < &a); // expected-warning{{TRUE}}
+ clang_analyzer_eval(0 <= &a); // expected-warning{{TRUE}} expected-warning{{Comparison between a null and non-null pointer may cause incorrect result}}
+ clang_analyzer_eval(0 < &a); // expected-warning{{TRUE}} expected-warning{{Comparison between a null and non-null pointer may cause incorrect result}}
// LHS is a symbolic value, RHS is NULL
clang_analyzer_eval(a != 0); // expected-warning{{UNKNOWN}}
clang_analyzer_eval(a >= 0); // expected-warning{{TRUE}}
- clang_analyzer_eval(a <= 0); // expected-warning{{UNKNOWN}}
- clang_analyzer_eval((a - 0) != 0); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(a <= 0); // expected-warning{{TRUE}}
+ clang_analyzer_eval((a - 0) != 0); // expected-warning{{FALSE}}
// LHS is NULL, RHS is a symbolic value
- clang_analyzer_eval(0 != a); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(0 != a); // expected-warning{{FALSE}}
clang_analyzer_eval(0 <= a); // expected-warning{{TRUE}}
- clang_analyzer_eval(0 < a); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(0 < a); // expected-warning{{FALSE}}
}
void const_locs() {
@@ -141,8 +152,8 @@
clang_analyzer_eval(&a.x <= &a.y); // expected-warning{{TRUE}}
clang_analyzer_eval(&a.x != &b.x); // expected-warning{{TRUE}}
- clang_analyzer_eval(&a.x > &b.x); // expected-warning{{UNKNOWN}}
- clang_analyzer_eval(&a.x >= &b.x); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(&a.x > &b.x); // expected-warning{{UNKNOWN}} expected-warning{{Comparison of two pointers that do not point to the same memory chunk may cause incorrect result}}
+ clang_analyzer_eval(&a.x >= &b.x); // expected-warning{{UNKNOWN}} expected-warning{{Comparison of two pointers that do not point to the same memory chunk may cause incorrect result}}
}
void mixed_region_types() {
@@ -151,14 +162,15 @@
void *a = &array, *b = &s;
clang_analyzer_eval(&a != &b); // expected-warning{{TRUE}}
- clang_analyzer_eval(&a > &b); // expected-warning{{UNKNOWN}}
- clang_analyzer_eval(&a >= &b); // expected-warning{{UNKNOWN}}
+ clang_analyzer_eval(&a > &b); // expected-warning{{UNKNOWN}} expected-warning{{Comparison of two pointers that do not point to the same memory chunk may cause incorrect result}}
+ clang_analyzer_eval(&a >= &b); // expected-warning{{UNKNOWN}} expected-warning{{Comparison of two pointers that do not point to the same memory chunk may cause incorrect result}}
}
void symbolic_region(int *p) {
int a;
clang_analyzer_eval(&a != p); // expected-warning{{TRUE}}
+ // TODO: These should give warnings that p points to a different memory chunk.
clang_analyzer_eval(&a > p); // expected-warning{{UNKNOWN}}
clang_analyzer_eval(&a >= p); // expected-warning{{UNKNOWN}}
}
@@ -174,7 +186,10 @@
return;
clang_analyzer_eval(lhs < rhs); // expected-warning{{FALSE}}
- clang_analyzer_eval(lhs - rhs); // expected-warning{{UNKNOWN}}
+ if (lhs)
+ clang_analyzer_eval(lhs - rhs); // expected-warning{{UNKNOWN}}
+ else
+ clang_analyzer_eval(lhs - rhs); // expected-warning{{FALSE}}
if ((lhs - rhs) != 5)
return;
clang_analyzer_eval((lhs - rhs) == 5); // expected-warning{{TRUE}}
@@ -214,7 +229,10 @@
clang_analyzer_eval(lhs <= rhs); // expected-warning{{TRUE}}
clang_analyzer_eval((rhs - lhs) >= 0); // expected-warning{{TRUE}}
- clang_analyzer_eval((rhs - lhs) > 0); // expected-warning{{UNKNOWN}}
+ if (lhs)
+ clang_analyzer_eval((rhs - lhs) > 0); // expected-warning{{UNKNOWN}}
+ else
+ clang_analyzer_eval((rhs - lhs) > 0); // expected-warning{{FALSE}}
if (lhs >= rhs) {
clang_analyzer_eval((rhs - lhs) == 0); // expected-warning{{TRUE}}
@@ -236,7 +254,10 @@
clang_analyzer_eval(lhs <= rhs); // expected-warning{{TRUE}}
clang_analyzer_eval((rhs - lhs) >= 0); // expected-warning{{TRUE}}
- clang_analyzer_eval((rhs - lhs) > 0); // expected-warning{{UNKNOWN}}
+ if (lhs)
+ clang_analyzer_eval((rhs - lhs) > 0); // expected-warning{{UNKNOWN}}
+ else
+ clang_analyzer_eval((rhs - lhs) > 0); // expected-warning{{FALSE}}
if ((rhs - lhs) <= 0) {
clang_analyzer_eval(lhs == rhs); // expected-warning{{TRUE}}
@@ -255,10 +276,15 @@
void zero_implies_reversed_equal(int *lhs, int *rhs) {
clang_analyzer_eval((rhs - lhs) == 0); // expected-warning{{UNKNOWN}}
if ((rhs - lhs) == 0) {
- // FIXME: Should be FALSE.
- clang_analyzer_eval(rhs != lhs); // expected-warning{{UNKNOWN}}
- // FIXME: Should be TRUE.
- clang_analyzer_eval(rhs == lhs); // expected-warning{{UNKNOWN}}
+ if (lhs) {
+ // FIXME: Should be FALSE.
+ clang_analyzer_eval(rhs != lhs); // expected-warning{{UNKNOWN}}
+ // FIXME: Should be TRUE.
+ clang_analyzer_eval(rhs == lhs); // expected-warning{{UNKNOWN}}
+ } else {
+ clang_analyzer_eval(rhs != lhs); // expected-warning{{FALSE}}
+ clang_analyzer_eval(rhs == lhs); // expected-warning{{TRUE}}
+ }
return;
}
clang_analyzer_eval((rhs - lhs) == 0); // expected-warning{{FALSE}}
More information about the cfe-commits
mailing list