Page MenuHomePhabricator

[analyzer] Add SufficientSizeArrayIndexingChecker
Needs ReviewPublic

Authored by gamesh411 on Oct 22 2019, 12:15 PM.

Details

Summary

Checks for indexing situations, where an array is indexed with a
variable of type not sufficiently large to cover the whole range of the
arrays possible index set. This check is uses path-sensitivity in order
to reason about the possible size of the array being checked.

Event Timeline

gamesh411 created this revision.Oct 22 2019, 12:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 22 2019, 12:15 PM

Please feel free to add more reviewers.

Please feel free to add more reviewers.

Here you go!

NoQ added a comment.Oct 29 2019, 2:31 PM

I suspect that with this check by choosing path-sensitivity you gain less than you lose. That is, checking how the array pointer is passed around may be nice, but if you sacrificed that, you could have turned this check into a compiler warning which would have had an effect on much larger audience.

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
622

What makes this checker C++-specific? All your tests are in plain C.

clang/lib/StaticAnalyzer/Checkers/SufficientSizeArrayIndexingChecker.cpp
93

This is always true (we should probably tell SVal.getAsRegion() to return a const SubRegion *).

100

This never happens. The chain of superregions never has loops; it is always finite.

103

This deserves comments on what kinds of regions do you expect to see here. Do i understand correctly that you expect BaseMemRegion to be an ElementRegion and its superregion to be the whole array? 'Cause the former is super unobvious and most likely shouldn't be that way.

153–155

I recommend also adding trackExpressionValue() for the array expression. In your "symbolic index handling" tests it would hopefully* allow you to see the execution path within f() as part of the report, which is essential.

__
*If it doesn't work out of the box, it's most likely a bug in trackExpressionValue(). But generally such tracking needs to be implemented in order to move the checker out of alpha, because path-sensitive reports are worthless when you don't see visitor notes on all the interesting events on the path.

NoQ added inline comments.Oct 29 2019, 2:41 PM
clang/lib/StaticAnalyzer/Checkers/SufficientSizeArrayIndexingChecker.cpp
42

The whole point of bug types is to remain the same regardless of the specific message. They're more like a category. Please use only one bug type for the whole checker (unless you really find different categories of bugs).

steakhal added inline comments.Oct 30 2019, 3:14 AM
clang/lib/StaticAnalyzer/Checkers/SufficientSizeArrayIndexingChecker.cpp
75

Shoudn't we use isa<> if we don't use the resulting pointer?

126

I think its better to stay consistent regards naming.
Use either number literals expressing numeric values, or spell out its name.
I would prefer the latter like: ConstantOne with type SVal.