Page MenuHomePhabricator

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

Authored by NoQ on Fri, Jul 26, 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.Fri, Jul 26, 2:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Jul 26, 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.Mon, Jul 29, 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.Mon, Jul 29, 8:35 PM
NoQ updated this revision to Diff 214972.Tue, Aug 13, 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 TranscriptTue, Aug 20, 2:40 PM