Page MenuHomePhabricator

[analyzer] Ignore VLASizeChecker case that could cause crash
Needs RevisionPublic

Authored by vabridgers on Aug 27 2020, 3:30 PM.

Details

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.

Diff Detail

Event Timeline

vabridgers created this revision.Aug 27 2020, 3:30 PM
vabridgers requested review of this revision.Aug 27 2020, 3:30 PM

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.

Thanks Balazs

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.)

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.

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

Ka-Ka added a subscriber: Ka-Ka.Sep 2 2020, 10:27 PM
uabelho added a subscriber: uabelho.Sep 3 2020, 5:30 AM
NoQ added a comment.Thu, Sep 24, 12:15 AM

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.

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 added a comment.Wed, Sep 30, 9:36 AM

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.

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!

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

NoQ added a comment.Wed, Sep 30, 9:43 AM

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 requested changes to this revision.Wed, Sep 30, 9:51 AM
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.

This revision now requires changes to proceed.Wed, Sep 30, 9:51 AM

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

NoQ added a comment.Thu, Oct 1, 1:35 AM

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.