[PATCH] D81061: [Analyzer][VLASizeChecker] Fix problem with zero index assumption.

[Balázs Kéri via Phabricator via cfe-commits]

balazske created this revision.
Herald added subscribers: cfe-commits, ASDenysPetrov, martong, Charusso, gamesh411, dkrupp, donat.nagy, Szelethus, arphaman, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun.
Herald added a reviewer: Szelethus.
Herald added a project: clang.
balazske added reviewers: vabridgers, NoQ.
balazske added a comment.

There may be still a problem somewhere else. I think assume of "a" and "a==0" should have the same results.


See https://bugs.llvm.org/show_bug.cgi?id=46128
The crash was caused by incorect assumptions on `a` and `a + 1`
that resulted in knowing something about `a` (that it is exactly -1)
and only knowing about `a + 1` that it is non-zero.

  "constraints": [
    { "symbol": "reg_$0<int a>", "range": "{ [-1, -1] }" },
    { "symbol": "(reg_$0<int a>) + 1", "range": "{ [-2147483648, -1], [1, 2147483647] }" }
  ],

The problem was fixed by replacing plain assume on symbol with assume on
binary operator (test for "a==0" instead of "a").
An additional check was inserted at place of the assertion to prevent
similar crashes.

A test that triggers this last case is missing now.
The added test causes no problem because at start of the second loop
"c" can not be zero because the previous assumption made by the checker
(that did not work correct before the fix).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D81061

Files:
  clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
  clang/test/Analysis/vla.c


Index: clang/test/Analysis/vla.c
===================================================================
--- clang/test/Analysis/vla.c
+++ clang/test/Analysis/vla.c
@@ -137,3 +137,17 @@
   clang_analyzer_eval(clang_analyzer_getExtent(&vla3m) == 2 * x * 4 * sizeof(int));
   // expected-warning at -1{{TRUE}}
 }
+
+// https://bugs.llvm.org/show_bug.cgi?id=46128
+// Analyzer doesn't handle more than simple symbolic expressions correct.
+// Just don't crash.
+extern void foo(void);
+int a;
+void b() {
+  int c = a + 1;
+  for (;;) {
+    int d[c];
+    for (; 0 < c;)
+      foo();
+  }
+} // no-crash
Index: clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
@@ -122,11 +122,19 @@
       return State;
 
     if (const llvm::APSInt *IndexLVal = SVB.getKnownValue(State, IndexLength)) {
+      uint64_t IndexL = IndexLVal->getZExtValue();
+      if (IndexL == 0) {
+        // Despite the previous assumptions for non-zero and positiveness,
+        // this value might be zero or negative.
+        // At least check for zero again.
+        // Assume that this is a more exact fact than the previous assumptions
+        // (in checkVLAIndexSize), so report error too.
+        reportBug(VLA_Zero, SizeE, State, C);
+        return nullptr;
+      }
       // Check if the array size will overflow.
       // Size overflow check does not work with symbolic expressions because a
       // overflow situation can not be detected easily.
-      uint64_t IndexL = IndexLVal->getZExtValue();
-      assert(IndexL > 0 && "Index length should have been checked for zero.");
       if (KnownSize <= SizeMax / IndexL) {
         KnownSize *= IndexL;
       } else {
@@ -166,33 +174,33 @@
     return nullptr;
   }
 
-  // Check if the size is zero.
+  QualType SizeTy = SizeE->getType();
   DefinedSVal SizeD = SizeV.castAs<DefinedSVal>();
+  SValBuilder &SVB = C.getSValBuilder();
+  DefinedOrUnknownSVal Zero = SVB.makeZeroVal(SizeTy);
+
+  // Check if the size is zero.
 
-  ProgramStateRef StateNotZero, StateZero;
-  std::tie(StateNotZero, StateZero) = State->assume(SizeD);
+  SVal IsZeroVal = SVB.evalBinOp(State, BO_EQ, SizeD, Zero, SizeTy);
+  if (Optional<DefinedSVal> IsZeroDVal = IsZeroVal.getAs<DefinedSVal>()) {
+    ProgramStateRef StateZero, StateNotZero;
 
-  if (StateZero && !StateNotZero) {
-    reportBug(VLA_Zero, SizeE, StateZero, C);
-    return nullptr;
+    std::tie(StateZero, StateNotZero) = State->assume(*IsZeroDVal);
+    if (StateZero && !StateNotZero) {
+      reportBug(VLA_Zero, SizeE, State, C);
+      return nullptr;
+    }
+    State = StateNotZero;
   }
 
-  // From this point on, assume that the size is not zero.
-  State = StateNotZero;
-
   // Check if the size is negative.
-  SValBuilder &SVB = C.getSValBuilder();
-
-  QualType SizeTy = SizeE->getType();
-  DefinedOrUnknownSVal Zero = SVB.makeZeroVal(SizeTy);
 
   SVal LessThanZeroVal = SVB.evalBinOp(State, BO_LT, SizeD, Zero, SizeTy);
   if (Optional<DefinedSVal> LessThanZeroDVal =
           LessThanZeroVal.getAs<DefinedSVal>()) {
-    ConstraintManager &CM = C.getConstraintManager();
     ProgramStateRef StatePos, StateNeg;
 
-    std::tie(StateNeg, StatePos) = CM.assumeDual(State, *LessThanZeroDVal);
+    std::tie(StateNeg, StatePos) = State->assume(*LessThanZeroDVal);
     if (StateNeg && !StatePos) {
       reportBug(VLA_Negative, SizeE, State, C);
       return nullptr;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D81061.268078.patch
Type: text/x-patch
Size: 3559 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200603/63026f06/attachment.bin>