[PATCH] Division by zero

[Anders Rönnholm]

Changes according to comments. 

I'm pretty sure PVS-Studio does not have this checker, this bug has been seen in production code.

//Anders

________________________________________
Från: Jordan Rose [jordan_rose at apple.com]
Skickat: den 16 juni 2014 18:39
Till: Anders Rönnholm
Cc: cfe-commits at cs.uiuc.edu; Daniel Marjamäki
Ämne: Re: [PATCH] Division by zero

On Jun 5, 2014, at 5:16 , Anders Rönnholm <Anders.Ronnholm at evidente.se<mailto:Anders.Ronnholm at evidente.se>> wrote:

Hi Jordan,

New patch and some comments.

+inline void inline_func3(int x) {
+  var = 77 / x;
+}
+void ok_inline(int x) {
+  var = 77 / x;
+  inline_func3(x);
+  if (x==0){}
+}

This is an example where we should still be warning. Why aren't we?

It doesn't warn here because blockid is in the value and not the key as it should. I have made a set of symbol-block-frame as you suggested and that fixes this problem.


+// RUN: %clang_cc1 -analyze -analyzer-checker=core.DivideZero2 -analyzer-output=text -verify %s

We should never be running path-sensitive checks without the rest of the core checkers.

There is a small problem here. It looks like the DivideZero checker makes the assumtion that because it can't see that a variable is zero then it's not zero so all variables in our tests have the range reg_$0<x> : { [-2147483648, -1], [1, 2147483647] }. I have removed the range check in my checker to make it work otherwise i would have to change the DivideZero checker.

Hm. The important thing is that a variable is zero before the division; in the absence of this checker, it's a reasonable assumption that it's zero if we made it past the division. Would it make sense to step back through ExplodedNodes instead until we found a ProgramPoint before the division statement itself, and then look at the value of the variable in that state?


+  const ZeroState *ZS = C.getState()->get<DivZeroMap>(SR);
+  return (ZS && *ZS == ZeroState(C.getBlockID(), C.getStackFrame()));

...now I'm wondering about the wisdom of using a map for this. What if the same symbol is constrained in two stack frames? Would it make more sense to keep around a set of symbol-block-frame triples, rather than a map from symbols to block/frame pairs?

Or can this not happen because a value can only be used once along a certain path before it is constrained to be non-zero?

Changed to a set of symbol-block-frame triples to make the above example work.


+def DivZeroChecker2 : Checker<"DivideZero2">,
+  HelpText<"Check for division by variable that is later compared against 0. Either the comparison is useless or there is division by zero.">,
+  DescFile<"DivZeroChecker2.cpp">;
+

I'm not sure this is really a core checker, since it's safe to run the analyzer without it. But we don't have a category for "useful C checks that should be on by default" yet. Anton was looking at reorganizing the categories, but the problem is that everything outside of "alpha" is something people expect to be able to rely on when using scan-build.

Any ideas for this particular checker, or for the general issue?

I ok with having it as an alpha, or something else. I just put it in core since the other DivideZero checker is there.

Let's put it in alpha for now.

Code comments:

+class DivZeroChecker2
+    : public Checker<check::PreStmt<BinaryOperator>, check::BranchCondition,
+                     check::EndFunction> {

Since we're not doing the normal divide-by-zero check in this checker, let's name it something else. "TestAfterDivideChecker", maybe?


+  DivZeroMapTy DivZeroes = C.getState()->get<DivZeroMap>();
+  if (DivZeroes.isEmpty())
+    return false;
+
+  for (llvm::ImmutableSet<ZeroState>::iterator I = DivZeroes.begin(),
+                                               E = DivZeroes.end();
+       I != E; ++I) {
+    if (*I == ZeroState(SR, C.getBlockID(), C.getStackFrame()))
+      return true;
+  }
+  return false;

This whole thing can be simplified quite a bit:

ZeroState ZS(SR, C.getBlockID(), C.getStackFrame());
return C.getState()->contains<DivZeroMap>(ZS);


...and other than that it's looking pretty good! Has this checker found any real bugs that you know of? (Does PVS-Studio have a similar checker?)

Jordan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: divzero2.diff
Type: text/x-patch
Size: 18662 bytes
Desc: divzero2.diff
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140618/aa72bfdb/attachment.bin>