This is an archive of the discontinued LLVM Phabricator instance.

[analyzer][UninitializedObjectChecker] Pointer/reference objects are dereferenced according to dynamic type
ClosedPublic

Authored by Szelethus on Jul 11 2018, 11:49 AM.

Details

Summary

This diff fixes a long debated issues with pointers/references not being dereferenced according to their dynamic type. This also fixed an issue where regions that were pointed to by void pointers were not analyzed.

Note how I also added a test case about inheritance, clearly showing that it doesn't find an uninitialized field that it should. Because base class handling is still being discussed, I'm planning to fix that issue in a different patch. The reason why it's still in this patch is that it might be closely tied to these changes too.

Thumbs up to @NoQ for setting me on the right track!

Diff Detail

Event Timeline

Szelethus created this revision.Jul 11 2018, 11:49 AM
Szelethus added inline comments.Jul 11 2018, 12:20 PM
lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
598–616

I already have a fix for this, but I want to be extra sure, so I'm running the checker on LLVM to see whether anything breaks.

This is also important for nonloc::LocAsInteger, which I'm going add fix in yet another patch.

Rebased to the latest trunk.

george.karpenkov requested changes to this revision.Jul 17 2018, 11:12 AM

Cf. my comments to https://reviews.llvm.org/D49437: is it possible to separate pointer-chasing from the rest of the checker?

This revision now requires changes to proceed.Jul 17 2018, 11:12 AM
george.karpenkov accepted this revision.Aug 2 2018, 2:35 PM
This revision is now accepted and ready to land.Aug 2 2018, 2:35 PM

@NoQ, we had quite a few conversations about this fix. How do you feel about this implementation?

NoQ accepted this revision.Aug 7 2018, 1:31 PM

This looks roughly correct, but at the same time none of the tests actually exercise the dynamic type propagation. In these tests all the necessary information is obtained from the structure of the MemRegion (directly or via the initial StripCasts), not from the dynamic type map that is an additional layer of metadata over the program state. The actual test would assume, as an example, chasing undefined values through a symbolic pointer produced by operator new() - which is a symbolic void pointer, but it points to a well-defined type of object. Because we skip symbolic pointers for now, i guess you cannot really write such tests. But at the same time chasing through heap symbolic pointers (i.e., pointers in the heap memory space) should be safe (so safe that they shouldn't really have been implemented as symbolic pointers in the first place).

Thanks! I plan on revisiting how heap allocated regions are handled in the future.

Szelethus updated this revision to Diff 159699.Aug 8 2018, 6:14 AM

Rebased to latest trunk.

This revision was automatically updated to reflect the committed changes.