Page MenuHomePhabricator

[analyzer] Quickfix: do not overflow in calculating offset in RegionManager
ClosedPublic

Authored by george.karpenkov on Feb 12 2018, 6:32 PM.

Details

Summary

Addresses https://bugs.llvm.org/show_bug.cgi?id=36206

rdar://37159026

A proper fix would be much harder, and would involve changing the appropriate code in ExprEngine to be aware of the size limitations of the type used for addressing.

Diff Detail

Repository
rC Clang

Event Timeline

dcoughlin added inline comments.Feb 12 2018, 9:39 PM
lib/StaticAnalyzer/Core/MemRegion.cpp
1219

I'd really like to minimize this kind of debug logging. The comment is sufficient here.

lib/StaticAnalyzer/Core/MemRegion.cpp
1219

@dcoughlin it would help the next person to debug a weird issue, as analyzer does many relaxing assertions in many places. If anything, we almost never use it as the enabling flag is quite bizarre.

We have a comment documenting the decision. Adding a printf doesn't seem like it would help very much here and adds untested code. I don't think we should have a policy of adding these printfs on every early return. Since these printfs make it harder to read the code and are duplicated with the comments, I think we should avoid adding one until we know it is actually generally useful.

Here is what I propose:

  • When you are debugging an issue and found a printf helpful, only commit the DEBUG printf the *second* time you found it helpful. This will avoid prematurely littering the code with printfs and ensure that the ones that are committed are generally useful rather only helpful for debugging one specific issue.
  • Add lit FileCheck tests that exercise the debug configuration and test the output. This will make sure that future refactorings and changes in behavior don't unexpectedly break the DEBUG code.
NoQ added a comment.Feb 16 2018, 6:42 PM

Dunno, it'd be great to come up with some systematic approach to those debug prints. Having some of them is really nice sometimes.

I also wish the informal // no-crash thingy was specifically saying that it's about not causing a UBSan error, not just a simple assertion failure or segfault that the reader would expect. Because we don't have many such tests, so the reader doesn't expect that, and a quick glimpse at the differential revision may not be enough to figure this out.

NoQ accepted this revision.Feb 16 2018, 6:42 PM

LGTM otherwise!

This revision is now accepted and ready to land.Feb 16 2018, 6:42 PM

...turns out the previous version had a bug. Re-introduced logging, as I've needed that for the second time for debugging.

This version is more robust, and is more obvious, but function is considerably more generic, except I don't know a good place to put it (and it can't be MathExtras, since APInt depends on that).

@vsk on the other hand if circular dependency between MathExtras and APInt is OK, I guess the code could be moved there.

vsk added a comment.Feb 23 2018, 4:57 PM

@vsk on the other hand if circular dependency between MathExtras and APInt is OK, I guess the code could be moved there.

Sure. As I mentioned off-list, I think it would be cleaner to implement these checked operations as overloads of SaturatingAdd, SaturatingMultiply, etc. I don't anticipate a build dependency issue there.

vsk added inline comments.Feb 23 2018, 5:02 PM
lib/StaticAnalyzer/Core/MemRegion.cpp
1216

Oh, aren't sizes non-negative? Why not just cast to unsigned and use SaturatingMultiplyAdd, or something?

lib/StaticAnalyzer/Core/MemRegion.cpp
1216

IIRC array offsets can be negative (yay, C), and in this case i can be negative. I suppose two calls can also do, but then if it fits in unsigned it doesn't mean it fits in signed, since the underlying type is signed.

...turns out the previous version had a bug. Re-introduced logging, as I've needed that for the second time for debugging.

Please make sure to add a test for the debugging output so it doesn't regress.

This revision was automatically updated to reflect the committed changes.