Page MenuHomePhabricator

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

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

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
80–83

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
80–83

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.