This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] DynamicSize: Remove 'getSizeInElements()' from store
ClosedPublic

Authored by Charusso on Oct 29 2019, 6:35 PM.

Details

Summary

This patch uses the new DynamicSize.cpp to serve dynamic information.
Previously it was static and probably imprecise data.

Diff Detail

Event Timeline

Charusso created this revision.Oct 29 2019, 6:35 PM

The [1] patch which introduced such static element-count data has only one test case in outofbound.c:

void f2() {
  int *p = malloc(12);
  p[3] = 4; // expected-warning{{Access out-of-bound array element (buffer overflow)}}
}

which probably wanted to be (int *)malloc(12), but in both ways the warning occurs, which is problematic. This is the first step to mitigate that issue.
[1] https://github.com/llvm/llvm-project/commit/228b0d4defb85b2e7acbb642b9f0b5dfc49d3fe7

NoQ added a comment.Nov 1 2019, 1:00 PM

This is the first step to mitigate that issue.

What's the issue?

clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
38–40

Even if the size is not concrete, you can ask SValBuilder to perform the division. It's going to be a symbolic expression which we won't be able to work with anyway, but these days we believe that it's still worth it, in hope that our constraint solver eventually gets better.

Charusso updated this revision to Diff 227524.Nov 1 2019, 1:32 PM
Charusso marked 2 inline comments as done.
  • Fix.
In D69599#1730707, @NoQ wrote:

This is the first step to mitigate that issue.

What's the issue?

Well, after I mentioned an issue I have realized the somewhat path-insensitive getSizeInElements() does not touch the (void *) return value. Basically the expression int *foo = malloc() could not compile, and I had felt that the assumptions about the overflow are wrong. Now I see that none of the overflow tests would compile, so I think we just bypass a cast here-and-there. Therefore there is no issue, just I was surprised.

clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
38–40

Good idea, thanks!

NoQ added inline comments.Nov 1 2019, 1:42 PM
clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
40–48

And then remove the manual division.

Charusso added inline comments.Nov 1 2019, 2:01 PM
clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
40–48

Hmpf.

Failing Tests (7):
    Clang :: Analysis/misc-ps-region-store.m
    Clang :: Analysis/mpichecker.cpp
    Clang :: Analysis/outofbound.c
    Clang :: Analysis/rdar-6541136-region.c
    Clang :: Analysis/return-ptr-range.cpp
    Clang :: Analysis/track-control-dependency-conditions.cpp
    Clang :: Analysis/uninit-vals.c

I would pick that solution because it may be a tiny-bit faster, and then later on investigate this issue when we model more about dynamic sizes.

NoQ added inline comments.Nov 1 2019, 2:31 PM
clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
40–48

Soooooo what does it tell us about the correctness of the new evalBinOp-based solution?

Charusso updated this revision to Diff 227543.Nov 1 2019, 2:57 PM
Charusso marked 4 inline comments as done.
  • Old division swapped by evalBinOp.

Thanks, now it is cool!

clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
40–48

So, when I tried to inject an APSInt it converted to 0 so division by zero made that. I felt that the implicit conversion is wonky, but dividing by 0, ugh.

NoQ accepted this revision.Nov 1 2019, 3:07 PM
NoQ added inline comments.
clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
40–48

Yay, great job figuring this out!

Also the conversion wasn't implicit; you explicitly specified llvm::APSInt(...). I agree that this constructor is evil, though.

44–47

I'd rather do a castAs here. Allocating a region of garbage size should be an immediate warning; supplying a zero-size ElementTy should be an immediate crash; in all other cases the result of division must be defined.

This revision is now accepted and ready to land.Nov 1 2019, 3:07 PM
Charusso updated this revision to Diff 227546.Nov 1 2019, 3:33 PM
Charusso marked 3 inline comments as done.
  • Done.

Thanks for the review!

clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
40–48

getQuantity() retuns a QuantityType, but now I see: typedef int64_t QuantityType, so I was fooled.

This revision was automatically updated to reflect the committed changes.

@Charusso
I think this patch may fix this bug https://bugs.llvm.org/show_bug.cgi?id=25284
Could you please verify and close it if so?
At least I couldn't reproduce it on the latest build.