[analyzer] Ignore VLASizeChecker case that could cause crash

[vabridgers]

Summary

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

[balazske]

The fix is probably OK but I could not find out what causes the problem in this case and not in other (similar) ones.
Why is not possible to assume SVB.evalEQ(State, DynSize, *ArraySizeNL) to true:
DynSize: extent_$1{e}
*ArraySizeNL: 8 U64b
The problem occurs likely not at the first iteration of the loop. Probably something is "messed up" in the state.

clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp

304

If the previous assumption fails the assumptions made before it (about size of array dimensions) can be applied. Like at line 284 a transition should be added. At least if the current state indicates not a problem that makes this unnecessary.

[vabridgers]

Thanks Balazs

[balazske]

I do not know how these changes can appear:

The checker makes only 2 assumptions, about the array dimension being positive and about the size of the array and the extent. (This state transition happens when https://reviews.llvm.org/D81061 is applied, without it the state may look even worse.)

[Szelethus]

I'm not thrilled by this solution. As I understand it, the assertion was put there to enforce our ability to create a new assumption, and its great that we had it, because we managed to catch a fault in that. Seems like this patch is treating the symptoms instead of curing the illness. Given that the code would probably work fine in builds with assertions disabled, I'd prefer to take a look at where the actual problem is. If it turns out to be unsolvable or require unreasonable amount of work, we can still decide to land this as-is.

[baloghadamsoftware]

I agree here with @Szelethus. We should investigate first why the assumption fails. Then we can decide about the best possible fix.

[NoQ]

A VLA in a loop may have different size on each iteration of the loop. This looks very much related to https://bugs.llvm.org/show_bug.cgi?id=28450.

I do not know how these changes can appear

You know the node. Conditional breakpoint on the node and step-by-step debugging will give you all the answers.

// State may not be valid since constraints do not comprehend expressions
// used for VLAs.

That's not what the constraint manager does when it can't comprehend expressions. In such cases the original state would have been returned, not the null state.

[baloghadamsoftware]

Yes, @NoQ is right. If the constraint manager cannot reason about a value, then then ProgramState::assume() will return the same state with the new assumption in the constraints. Whenever ProgramState::assume() returns nullptr it means that the assumption is impossible. Thus in this case DynSize and *ArraySizeNL cannot be equal. To me this really seems to be a similar issue to Bug 28450. The last comment for that bug is D69726, but the bug is not closed to it seems to me that D69726 does not solve it, just takes a single step towards the solution.

I see that proper solution is somewhat more complicated and requires deep knowledge and takes probably much time. However, this one is a core checker, and not even alpha. Even alpha checkers should not crash at all (for me alpha means many false positives), but a crashing core checker, that is no even alpha completely undermines the users' trust towards the Analyzer. Bug 28450 is open for almost one and half years. Thus something quick should be done even before someone implements the proper solution (from @NoQ's suggestions the second one is implemented by D69726, but not the first one). A FIXME must be added in either case, and the misleading comment deleted. Or if this solution is completely unacceptable for the community then VLASizeChecker should be degraded to alpha.

[NoQ]

The last comment for that bug is D69726, but the bug is not closed to it seems to me that D69726 does not solve it, just takes a single step towards the solution.

It might as well be that the patch isn't, well, committed.

However, this one is a core checker, and not even alpha.

Yup, that's pretty bad.

[baloghadamsoftware]

In D86743#2303910, @NoQ wrote:

The last comment for that bug is D69726, but the bug is not closed to it seems to me that D69726 does not solve it, just takes a single step towards the solution.

It might as well be that the patch isn't, well, committed.

Oh! I mislooked that. I saw DynamicSize.cpp already in the repository, but that is a prerequisite, not that one. Sorry!

[baloghadamsoftware]

@vabridgers Please try to apply D69726 and check whether it solves this crash!

[NoQ]

Yup, that's pretty bad.

One slightly redeeming thing about this crash is that it's assertion-only. When built without assertions clang doesn't crash and this patch doesn't really change its behavior (adding transition to a null state is equivalent to adding no transitions at all). This means that the assertion did its job and notified us of the serious issue but simply removing the assertion doesn't bring *that* much benefit and we can probably afford to wait for a more solid fix.

[baloghadamsoftware]

In D86743#2303922, @NoQ wrote:

One slightly redeeming thing about this crash is that it's assertion-only. When built without assertions clang doesn't crash and this patch doesn't really change its behavior (adding transition to a null state is equivalent to adding no transitions at all). This means that the assertion did its job and notified us of the serious issue but simply removing the assertion doesn't bring *that* much benefit and we can probably afford to wait for a more solid fix.

Yes, that is right. CheckerContext::addTransition() accepts nullptr, thus with assertions disabled it does exactly what this patch does. Thus I think that this patch could be abandoned now, and if the bug is not exactly the same as 28450 then a new bug report should be filed in BugZilla. And maybe the review on D69726 could be continued if that is part of the proper solution you suggested.

[balazske]

I do not know if it got overlooked but there is another patch D81061 that removes the crash.

[NoQ]

But that's for a different bug, no? Even if that patch somehow dodges *this* crash, the underlying problem of us being unable to account for mutability of VLA sizes still remains to be addressed.

[chrish_ericsson_atx]

Is anything planned or in progress that would address this issue?

[vabridgers]

@baloghadamsoftware , these changes do seem to help the case described. This patch isn't quite up to date, and needs to be integrated with changes from @balazske (my integration is hacky and needs to be cleaned up). I'll continue working on this, and get back to you. These changes may also be helpful in solving https://bugs.llvm.org/show_bug.cgi?id=48136 (as I had discussed with @steakhal). Thanks for looking into these things.

[vabridgers]

I pulled https://reviews.llvm.org/D69726, brought it up to date and pushed to github at https://github.com/vabridgers/llvm-project-dev.git, branch: vla-fam-fixes. Everything seems ok on this branch (LITs pass, reproducers from https://bugs.llvm.org/show_bug.cgi?id=47272 and https://bugs.llvm.org/show_bug.cgi?id=28450 no longer crash). I think perhaps this change can be abandoned in favor of progressing https://reviews.llvm.org/D69726 ? @Charusso and @balazske , what do you think? Thanks!

[balazske]

I did not like this solution so I like it if we can omit this change and apply another fix (D69726).

clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp

if (VD) {
     // Assume that the array's size matches the region size.
     const LocationContext *LC = C.getLocationContext();
     DefinedOrUnknownSVal DynSize =
         getDynamicSize(State, State->getRegion(VD, LC), SVB);
 
     DefinedOrUnknownSVal SizeIsKnown = SVB.evalEQ(State, DynSize, *ArraySizeNL);
     State = State->assume(SizeIsKnown, true);
 
-    // Assume should not fail at this point.
-    assert(State);
+    // State may not be valid since constraints do not comprehend expressions
+    // used for VLAs. If State is null, just silently return.
+    // See https://bugs.llvm.org/show_bug.cgi?id=47272.
+    if (!State)
+      return;
   }
 
   // Remember our assumptions!
   C.addTransition(State);
 }
 
 void VLASizeChecker::checkPreStmt(const UnaryExprOrTypeTraitExpr *UETTE,
                                   CheckerContext &C) const {

clang/test/Analysis/vla.c

 void b() {
   int c = a + 1;
   for (;;) {
     int d[c];
     for (; 0 < c;)
       foo();
   }
 } // no-crash
+
+
+// https://bugs.llvm.org/show_bug.cgi?id=47272
+// similar to the above case, just different enough to have not
+// been covered.
+// Just don't crash.
+int bb;
+int c() {
+  int d = 0;
+  int sum = 0;
+  while (bb) {
+    int count = bb - d;
+    int e[count];
+    if (count > 4)
+      sum++;
+    d++;
+  }
+  return sum;
+} // no-crash