[analyzer] Ignore calculated indices of <= 0 in VLASizeChecker

[vabridgers]

Summary

See https://bugs.llvm.org/show_bug.cgi?id=46128. The checker does not
yet comprehend constraints involving multiple symbols, so it's possible
to calculate a VLA size that's negative or 0. A LIT is added to catch
regressions, and this change simply bails if a VLA size of 0 or less is
calculated.

[vabridgers]

Not sure this is "correct", but it passes LITs with the new case, and it will at least get the discussion started :)

[vabridgers]

Address some typos

[NoQ]

clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp

114–115

Do i understand correctly that this cast is the only difference between the value that has been checked and the value on which the assertion is asserted?

[vabridgers]

clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp

114–115

Yes, looks that way to me. Let's see if Balasz, Gabor, Adam or Kristof responds in the next day or two? Thanks Artem!

[balazske]

clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp

114–115

Yes the cast is the difference. Even if this problem is fixed (cast included in checkVLAIndexSize) the same problem happens.
The following code (in checkVLAIndexSize) prints (reg_$0<int a>) + 1 0 before the crash. So there is some difference between assume and getKnownValue. It can be better to include check of getKnownValue in checkVLAIndexSize (at least for zero value).

// Convert the array length to size_t.
NonLoc SizeNL =
    SVB.evalCast(SizeD, SizeTy, SizeE->getType()).castAs<NonLoc>();

// Check if the size is zero.
ProgramStateRef StateNotZero, StateZero;
std::tie(StateNotZero, StateZero) = State->assume(SizeNL);

if (StateZero && !StateNotZero) {
  reportBug(VLA_Zero, SizeE, StateZero, C);
  return nullptr;
}

// From this point on, assume that the size is not zero.
State = StateNotZero;

if (const llvm::APSInt *IndexLVal = SVB.getKnownValue(State, SizeNL)) {
  uint64_t L = IndexLVal->getZExtValue();
  llvm::errs() << SizeNL << " " << L << "\n";
  assert(L != 0);
}

[NoQ]

clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp

114–115

It might be that simplifySVal isn't applied consistently. Like, assume() fails to apply it so it fails to see that the value is in fact concrete zero.

Let's remove the assertion for now and add a FIXME to investigate why can't we add it back. Like, there's nothing preventing us from investigating it now but it will take some time whereas crashes are already there and we should fix them to unblock other people.

[balazske]

It is no problem to return instead of the assert. I could fix the problem by using

SVal LessThanZeroVal = SVB.evalBinOp(State, BO_LE, SizeD, Zero, SizeTy);

in checkVLAIndexSize (BO_LT is used before). Still the proposed return makes the code more safe.

clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp

129–134

The assert does not look good if there is already the check before.

130

I do not know if this is accurate reason for the problem, a more general text is better? And should start with uppercase.

131

Can be ==.

[vabridgers]

Address comments from Artem and Balazs. This change just avoids the crash for now until a proper fix can be made.

[balazske]

At least as fast bugfix it is acceptable.

[NoQ]

Thank you! Please commit :)

[Szelethus]

LGTM! :)

[balazske]

Here is an improved fix for the problem: D81061

clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp

ProgramStateRef VLASizeChecker::checkVLA(CheckerContext &C,
   // Try to calculate the known real size of the array in KnownSize.
   uint64_t KnownSize = 0;
   if (const llvm::APSInt *KV = SVB.getKnownValue(State, ArrSize))
     KnownSize = KV->getZExtValue();
 
   for (const Expr *SizeE : VLASizes) {
     auto SizeD = C.getSVal(SizeE).castAs<DefinedSVal>();
     // Convert the array length to size_t.
     NonLoc IndexLength =
         SVB.evalCast(SizeD, SizeTy, SizeE->getType()).castAs<NonLoc>();
     // Multiply the array length by the element size.
     SVal Mul = SVB.evalBinOpNN(State, BO_Mul, ArrSize, IndexLength, SizeTy);
     if (auto MulNonLoc = Mul.getAs<NonLoc>())
       ArrSize = *MulNonLoc;
     else
       // Extent could not be determined.
       return State;
 
     if (const llvm::APSInt *IndexLVal = SVB.getKnownValue(State, IndexLength)) {
       // 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.");
+      // FIXME: See https://reviews.llvm.org/D80903 for discussion of
+      // some difference in assume and getKnownValue that leads to
+      // unexpected behavior. Just bail on IndexL == 0 at this point.
+      if (IndexL == 0)
+        return nullptr;
+
       if (KnownSize <= SizeMax / IndexL) {
         KnownSize *= IndexL;
       } else {
         // Array size does not fit into size_t.
         reportBug(VLA_Overflow, SizeE, State, C);
         return nullptr;
       }
     } else {

clang/test/Analysis/vla.c

void check_VLA_extent() {
   int vla2m[2][x];
   clang_analyzer_eval(clang_analyzer_getExtent(&vla2m) == 2 * x * sizeof(int));
   // expected-warning@-1{{TRUE}}
 
   int vla3m[2][x][4];
   clang_analyzer_eval(clang_analyzer_getExtent(&vla3m) == 2 * x * 4 * sizeof(int));
   // expected-warning@-1{{TRUE}}
 }
+
+// https://bugs.llvm.org/show_bug.cgi?id=46128
+// analyzer doesn't handle more than simple symbolic expressions.
+// 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