This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Expand conversion check to check more expressions for overflow and underflow
ClosedPublic

Authored by pfultz2 on Apr 25 2018, 2:01 PM.

Details

Summary

This expands checking for more expressions. This will check underflow and loss of precision when using call expressions like:

void foo(unsigned);
int i = -1;
foo(i);

This also includes other expressions as well, so it can catch negative indices to std::vector since it uses unsigned integers for [] and .at() function.

Diff Detail

Repository
rC Clang

Event Timeline

pfultz2 created this revision.Apr 25 2018, 2:01 PM

Looks reasonable. Have you tried on any large codebases? You relax two guards, so I would be wary of explosion in false positives.

lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
77

Sorry, I don't quite follow: why we don't want to warn when RHS is an integer constant?

While I understand extending the analyzer to cover more is a good approach, there is -Wconversion which seemingly covers this -- or at least the trivial case(?):

#include <cstdio>
#include <vector>

void foo(unsigned x)
{
  printf("%u\n", x);
}

int main()
{
  int i = -1;
  foo(i);

  std::vector<int> x;
  x[i] = 5;
}
$ clang++ -Wconversion -o/dev/null check.cpp 
check.cpp:12:7: warning: implicit conversion changes signedness: 'int' to 'unsigned int' [-Wsign-conversion]
  foo(i);
  ~~~ ^
check.cpp:15:5: warning: implicit conversion changes signedness: 'int' to 'std::vector::size_type' (aka 'unsigned long') [-Wsign-conversion]
  x[i] = 5;
  ~ ^

While I understand extending the analyzer to cover more is a good approach, there is -Wconversion which seemingly covers this -- or at least the trivial case(?):

Yes, but it won't catch something like this, which is more interesting:

void g(unsigned);
void f(int x) {
    if (x < 0) return;
    g(x - 1);
}
pfultz2 added inline comments.Apr 26 2018, 7:01 AM
lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
77

Because we want to treat unsigned i = -1 as an explicit conversion. Ideally, I would like to just check that the RHS is a literal, but this seems like the closest way to do that.

Have you tried on any large codebases?

This check is not available to the user yet. I can move it to core if you want.

So moving to core will require explicit casts in some of the tests, especially for things like: memcpy(a262.s1, input, -1). Or this could be moved to another section instead of core. In D45532 there is talk of adding a bugprone section. I think this would be good there.

pfultz2 updated this revision to Diff 144202.Apr 26 2018, 2:41 PM

So I ran this on clang/llvm code base and fixed some false positives.

NoQ accepted this revision.May 4 2018, 2:27 PM

Yep, looks good. This limitation to DeclRefExprs was super conservative.

I'll follow up regarding the bugprone category in a week or so on the mailing lists. It's been a long-standing historical decision that the Analyzer, for the sake of good user experience, should only warn on real bugs and have low false positive rates, but there definitely is a desire for more advanced users to opt-in into reasonable lint / coding standard checks, so we really need some room for them, and a way of discriminating them from checks that are simply bad due to large amounts of inevitable false positives.

While I understand extending the analyzer to cover more is a good approach, there is -Wconversion which seemingly covers this -- or at least the trivial case(?):

Yeah, this check historically was an attempt on a quieter and at the same time more powerful alternative to -Wconversion.

This revision is now accepted and ready to land.May 4 2018, 2:27 PM

To me it seems like @pfultz2 hasn't been on the site for a couple months. Is this patch going to be commited anytime?

Ping.
Shouldn't let this thing go to waste.

whisperity added a subscriber: balazske.

Pinging my people here, perhaps you being more knowledgeable in the analyser can say if this patch is salvageable in any way? Or did the state of the art moved past this in the years that went by? The patch itself doesn't look huge, but outside my direct expertise.

@whisperity Thank you for pinging! This seems still feasible. Besides, this is valuable for us, as it could pass some SEI CERT test cases, notably from INT31-C.

martong added inline comments.Nov 19 2021, 2:46 AM
lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
99

Thanks for merging as I dont have commit access.