Page MenuHomePhabricator

[analyzer] Fix assertion in SVals.h
ClosedPublic

Authored by vabridgers on Apr 30 2021, 7:45 AM.

Details

Summary

Fix assertion in SVals.h apparently caused by
https://reviews.llvm.org/D89055.

clang:clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h:596:
clang::ento::loc::MemRegionVal::MemRegionVal(const clang::ento::MemRegion *):

Assertion `r' failed.

Backtrace:
...

clang/include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h:597:3
clang::QualType, clang::QualType)
clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:773:18
clang::QualType, clang::QualType)
clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:612:12
clang::QualType) clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:587:12
namespace)::RegionBindingsRef const&, clang::ento::Loc, clang::QualType)
clang/lib/StaticAnalyzer/Core/RegionStore.cpp:1510:24

...

Diff Detail

Event Timeline

vabridgers created this revision.Apr 30 2021, 7:45 AM
vabridgers requested review of this revision.Apr 30 2021, 7:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 30 2021, 7:45 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
ASDenysPetrov accepted this revision.Apr 30 2021, 8:48 AM

@vabridgers
Thank you for a good catch! The fix looks fairly reasonable for me!

This revision is now accepted and ready to land.Apr 30 2021, 8:48 AM

I don't know how did we miss this. I run your patch on several projects and it seemed good. Does anyone have an idea how to prevent such a silly mistake from happening again? I was thinking of coverage data, but that wouldn't be enough for this example.

clang/test/Analysis/casts.c
254–268

Please, @ASDenysPetrov investigate this.
I also think that this test case could be simplified, and a no-crash comment would be also appreciated at the corresponding line.

NoQ added a comment.May 3 2021, 1:18 AM

Does anyone have an idea how to prevent such a silly mistake from happening again?

Maybe use more optionals? I.e., castRegion may fail, so let's change it to return Optional<const MemRegion *> where the pointer is always non-null when present? In many cases it's probably unnecessary but when a function can return null for poorly predictable reasons (eg., like in this case, the region turning out to have symbolic offset after a potentially unlimited number of unwraps) it's probably worth it.