<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div>Hi, Matthew. Thanks for working on this -- these are definitely useful checks! One concern I have, though, is that plenty of people assume that comparisons between different objects are not just undefined or unspecified but actually consistent. It might be nice to have a separate checker flag for the comparison checks, though the implementation can still be shared in one Checker class. You can see how this works in CStringChecker or IvarInvalidationChecker.</div><div><br></div><div>Here are some comments on the patch itself:</div><div><br></div><br><div><div>On Jun 22, 2013, at 19:18 , Matthew Dempsky <<a href="mailto:matthew@dempsky.org">matthew@dempsky.org</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite">On Sat, Jun 22, 2013 at 12:53:05PM -0700, Matthew Dempsky wrote:<br><blockquote type="cite">As suggested in <a href="http://llvm.org/bugs/show_bug.cgi?id=16380">http://llvm.org/bugs/show_bug.cgi?id=16380</a>, I wanted<br>to implement a clang analyzer check to detect comparisons of pointers<br>to different memory ranges. It turns out there's already a check for<br>pointer subtraction, which has very similar restrictions, so it's easy<br>to extend to warn about comparisons too by just matching more binops.<br><br>Notably, pointer subtraction is stricter and is only valid for<br>pointers to elements of the same array, whereas pointer comparison is<br>also valid for pointers to member of the same struct. The current<br>checker, however, doesn't distinguish these cases anyway, so no<br>problem here. Just the same, I added a no-warning test case to make<br>sure this doesn't regress in the future.<br><br>Lastly, there's a distinction between C and C++ here: C says<br>comparisons between pointers to different memory chunks is undefined,<br>whereas C++ says it's only unspecified. (Both agree subtraction<br>between pointers to different chunks is undefined.) I tried to<br>account for this, but I'm not sure I did so correctly.<br></blockquote><br>Revised diff below. I realized the original diff was matching<br>expressions like "p - 0", which was causing some funny consequences.<br>("p - 0" is a pointer minus offset expression, not a pointer minus<br>pointer expression.)<br><br>Also, I realized the original diff was forcing the program to branch<br>down into two separate "p == 0 && q == 0" and "p != 0 && q != 0"<br>program states, when what I really wanted was a single state for both<br>of these. I had earlier tried matching "(p == 0) == (q == 0)" like<br>below now does, but ran into problems (e.g., my evalBinOpLN diff from<br>just a little bit ago); however, I now think those were simply because<br>of the "p - 0" matching issue above.<br><br>Finally, I also tweaked PointerArithChecker to not warn about adding<br>or subtracting 0 or 1, so it doesn't generate any false positives<br>about legitimate one-past pointers. I can split this into a separate<br>patch if desirable.<br><br><br>Index: clang/lib/StaticAnalyzer/Checkers/PointerSubChecker.cpp<br>===================================================================<br>--- clang/lib/StaticAnalyzer/Checkers/PointerSubChecker.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 184494)<br>+++ clang/lib/StaticAnalyzer/Checkers/PointerSubChecker.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)<br>@@ -25,7 +25,7 @@<br> namespace {<br> class PointerSubChecker <br> : public Checker< check::PreStmt<BinaryOperator> > {<br>- mutable OwningPtr<BuiltinBug> BT;<br>+ mutable OwningPtr<BuiltinBug> BT_sub, BT_cmp, BT_nullsub, BT_nullcmp;<br><br> public:<br> void checkPreStmt(const BinaryOperator *B, CheckerContext &C) const;<br>@@ -34,16 +34,60 @@<br><br> void PointerSubChecker::checkPreStmt(const BinaryOperator *B,<br> CheckerContext &C) const {<br>- // When doing pointer subtraction, if the two pointers do not point to the<br>- // same memory chunk, emit a warning.<br>- if (B->getOpcode() != BO_Sub)<br>+ // When doing pointer subtraction or relational comparisons, if the two<br>+ // pointers do not point to the same memory chunk, emit a warning.<br>+ if (B->getOpcode() != BO_Sub &&<br>+ B->getOpcode() != BO_LT && B->getOpcode() != BO_GT &&<br>+ B->getOpcode() != BO_LE && B->getOpcode() != BO_GE)<br> return;<br></blockquote><div><br></div><div>You can simplify this with B->isRelationalOp().</div><div><br></div><br><blockquote type="cite"><br>+ // This check is only sensible if the arguments are actually pointers.<br>+ if (!B->getLHS()->getType()->isPointerType() ||<br>+ !B->getRHS()->getType()->isPointerType())<br>+ return;<br>+<br> ProgramStateRef state = C.getState();<br> const LocationContext *LCtx = C.getLocationContext();<br> SVal LV = state->getSVal(B->getLHS(), LCtx);<br> SVal RV = state->getSVal(B->getRHS(), LCtx);<br><br>+ Optional<DefinedOrUnknownSVal> DLV = LV.getAs<DefinedOrUnknownSVal>();<br>+ Optional<DefinedOrUnknownSVal> DRV = RV.getAs<DefinedOrUnknownSVal>();<br>+ if (DLV && DRV) {<br>+ SValBuilder &SVB = C.getSValBuilder();<br>+ Loc Null = SVB.makeNull();<br>+<br>+ DefinedOrUnknownSVal NullityMatched = SVB.evalEQ(state,<br>+ SVB.evalEQ(state, *DLV, Null), SVB.evalEQ(state, *DRV, Null));<br></blockquote><div><br></div><div>It would be wonderful if this worked, and indeed it will appear to work for your test cases involving a literal null. However, in the general case evalEQ just constructs an expression "a == b", and can't evaluate that on its own. The constraint manager's what actually does the work.</div><div><br></div><div>A better way to do this would be to use ProgramState::isNull on each value, and checking if one value is constrained to true while the other is false. However, it looks like isNull is actually going to give you the wrong answer for <i>non-</i>symbolic regions, so you'd have to fix that first. Barring that, you should assume() each condition individually and compare the results.</div><div><br></div><br><blockquote type="cite">+ ProgramStateRef StMatch = state->assume(NullityMatched, true);<br>+ if (!StMatch) {<br>+ if (ExplodedNode *N = C.addTransition()) {<br>+ BugReport *R;<br>+ if (B->getOpcode() == BO_Sub) {<br>+ if (!BT_nullsub)<br>+ BT_nullsub.reset(new BuiltinBug("Pointer subtraction", <br>+ "Subtraction between a null and non-null pointer "<br>+ "may cause incorrect result."));<br>+ R = new BugReport(*BT_nullsub, BT_nullsub->getDescription(), N);<br>+ } else {<br>+ if (!BT_nullcmp)<br>+ BT_nullcmp.reset(new BuiltinBug("Pointer comparison", <br>+ "Comparison between a null and non-null pointer "<br>+ "may cause incorrect result."));<br>+ R = new BugReport(*BT_nullcmp, BT_nullcmp->getDescription(), N);<br>+ }<br>+ R->addRange(B->getSourceRange());<br>+ C.emitReport(R);<br>+ }<br>+ return;<br>+ }<br></blockquote><br><blockquote type="cite">+ if (B->getOpcode() == BO_Sub || !C.getLangOpts().CPlusPlus) {<br>+ C.addTransition(StMatch);<br></blockquote><div><br></div><div>Since you have more work to do, that's actually not a good idea! Each addTransition splits the state space, and we don't actually want to do that. I suggest just updating your 'state' variable, so that the final addTransition operates on the right state.</div><div><br></div><div>Basically, you want to make sure we don't actually bifurcate the path as a result of these checks, and at the same time you want to make sure that each report happens after at least one addTransition (so they don't look they're the previous statement's fault).</div><div><br></div><div>You also are checking for C++ in determining whether or not to record the assumption, but not in whether or not to warn. As tempting as it is, I don't think we can warn in C++ mode for something that's merely unspecified.</div><br><blockquote type="cite">+ }<br>+ }<br>+<br> const MemRegion *LR = LV.getAsRegion();<br> const MemRegion *RR = RV.getAsRegion();<br><br>@@ -61,11 +105,20 @@<br> return;<br><br> if (ExplodedNode *N = C.addTransition()) {<br>- if (!BT)<br>- BT.reset(new BuiltinBug("Pointer subtraction", <br>- "Subtraction of two pointers that do not point to "<br>- "the same memory chunk may cause incorrect result."));<br>- BugReport *R = new BugReport(*BT, BT->getDescription(), N);<br>+ BugReport *R;<br>+ if (B->getOpcode() == BO_Sub) {<br>+ if (!BT_sub)<br>+ BT_sub.reset(new BuiltinBug("Pointer subtraction", <br>+ "Subtraction of two pointers that do not point to "<br>+ "the same memory chunk may cause incorrect result."));<br>+ R = new BugReport(*BT_sub, BT_sub->getDescription(), N);<br>+ } else {<br>+ if (!BT_cmp)<br>+ BT_cmp.reset(new BuiltinBug("Pointer comparison", <br>+ "Comparison of two pointers that do not point to "<br>+ "the same memory chunk may cause incorrect result."));<br>+ R = new BugReport(*BT_cmp, BT_cmp->getDescription(), N);<br>+ }<br> R->addRange(B->getSourceRange());<br> C.emitReport(R);<br> }<br>Index: clang/lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp<br>===================================================================<br>--- clang/lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 184494)<br>+++ clang/lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)<br>@@ -46,6 +46,9 @@<br> if (!LR || !RV.isConstant())<br> return;<br><br>+ if (RV.isZeroConstant() || RV.isConstant(1))<br>+ return;<br></blockquote><div><br></div><div>I think this one should be a separate patch. I see how it's useful (for pretending a single variable is an array), but it's not really part of the first set of checks, and you should get the addition vs. subtraction thing right anyway. (Your TODOs in the test file.)</div><div><br></div><div>Hopefully I'll be able to be more prompt about reviewing your next iteration!</div><div>Jordan</div><div><br></div><br><blockquote type="cite"> // If pointer arithmetic is done on variables of non-array type, this often<br> // means behavior rely on memory organization, which is dangerous.<br> if (isa<VarRegion>(LR) || isa<CodeTextRegion>(LR) || <br></blockquote></div><br></body></html>