Page MenuHomePhabricator

[analyzer] Be more careful with destructors of non-regions.
ClosedPublic

Authored by NoQ on Jul 26 2019, 2:04 PM.

Details

Summary

It turns out that we crash all over the place when we try to evaluate destructors over concrete-int-but-not-null locations.

Add some defensive code.

Diff Detail

Repository
rL LLVM

Event Timeline

NoQ created this revision.Jul 26 2019, 2:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 26 2019, 2:04 PM

Yes, it crashes! :-) I tried it because I did not believe it, but it does. Is there any real-world use-case for casting concrete integers to class instances? How did you find this crashing case?

clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
1055 ↗(On Diff #212004)

Why do we rename it from FieldVal to FieldLoc? Are we sure that this is a Loc? If so, then we should apply castAs<Loc>() and change the type to Loc as well. (Or auto since castAs<Loc>() is enough according to the LLVM coding standards.) If we are not sure, then I think it is better to leave it FieldVal.

clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
627 ↗(On Diff #212004)

Could you please add some lines of comments to this code block?

NoQ added a comment.Jul 29 2019, 3:19 PM

Is there any real-world use-case for casting concrete integers to class instances? How did you find this crashing case?

I think in original code this value was produced by doing pointer arithmetic over a null pointer. Which is kinda weird because we normally mis-model such arithmetic as resulting in a null pointer, so that to treat dereferences of such pointers as null dereferences (and abort the analysis immediately, never reaching the destructor). See also D37478.

Also it seems that this bug has just been independently reported as https://bugs.llvm.org/show_bug.cgi?id=42816.

This revision is now accepted and ready to land.Jul 29 2019, 8:35 PM
NoQ updated this revision to Diff 214972.Aug 13 2019, 4:28 PM
NoQ marked 2 inline comments as done.

Fxd!

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 20 2019, 2:40 PM