[PATCH] Extend PointerSubChecker to detect more bugs
Matthew Dempsky
matthew at dempsky.org
Sat Jun 22 19:18:26 PDT 2013
On Sat, Jun 22, 2013 at 12:53:05PM -0700, Matthew Dempsky wrote:
> 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.
Revised diff below. I realized the original diff was matching
expressions like "p - 0", which was causing some funny consequences.
("p - 0" is a pointer minus offset expression, not a pointer minus
pointer expression.)
Also, I realized the original diff was forcing the program to branch
down into two separate "p == 0 && q == 0" and "p != 0 && q != 0"
program states, when what I really wanted was a single state for both
of these. I had earlier tried matching "(p == 0) == (q == 0)" like
below now does, but ran into problems (e.g., my evalBinOpLN diff from
just a little bit ago); however, I now think those were simply because
of the "p - 0" matching issue above.
Finally, I also tweaked PointerArithChecker to not warn about adding
or subtracting 0 or 1, so it doesn't generate any false positives
about legitimate one-past pointers. I can split this into a separate
patch if desirable.
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,60 @@
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 the arguments are actually pointers.
+ 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) {
+ SValBuilder &SVB = C.getSValBuilder();
+ Loc Null = SVB.makeNull();
+
+ DefinedOrUnknownSVal NullityMatched = SVB.evalEQ(state,
+ SVB.evalEQ(state, *DLV, Null), SVB.evalEQ(state, *DRV, Null));
+
+ ProgramStateRef StMatch = state->assume(NullityMatched, true);
+ if (!StMatch) {
+ 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) {
+ C.addTransition(StMatch);
+ }
+ }
+
const MemRegion *LR = LV.getAsRegion();
const MemRegion *RR = RV.getAsRegion();
@@ -61,11 +105,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/lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp (revision 184494)
+++ clang/lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp (working copy)
@@ -46,6 +46,9 @@
if (!LR || !RV.isConstant())
return;
+ if (RV.isZeroConstant() || RV.isConstant(1))
+ return;
+
// If pointer arithmetic is done on variables of non-array type, this often
// means behavior rely on memory organization, which is dangerous.
if (isa<VarRegion>(LR) || isa<CodeTextRegion>(LR) ||
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,23 @@
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;
+ d = q - p; // TODO: warn
+ 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() {
@@ -50,12 +62,23 @@
}
void f5() {
- int x, y;
+ int x;
int *p;
- p = &x + 1; // expected-warning{{Pointer arithmetic done on non-array variables means reliance on memory layout, which is dangerous}}
+ p = &x + 1; // no-warning
+ p = p - 1; // no-warning
+ p = &x + 2; // expected-warning{{Pointer arithmetic done on non-array variables means reliance on memory layout, which is dangerous}}
+ p = &x - 1; // TODO: warn
+ p = &x + 1;
+ p = p + 1; // TODO: warn
+
int a[10];
p = a + 1; // no-warning
+ p = a + 10; // no-warning
+ p = a + 11; // TODO: warn
+ p = a - 1; // TODO: warn
+ p = a + 7;
+ p = a + 7; // TODO: warn
}
// Allow arithmetic on different symbolic regions.
@@ -67,21 +90,21 @@
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); // 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}}
// 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}}
// 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}}
@@ -141,8 +164,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 +174,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}}
}
More information about the cfe-commits
mailing list