Page MenuHomePhabricator

[analyzer][UninitializedObjectChecker] Reports Loc fields pointing to themselves
ClosedPublic

Authored by Szelethus on Aug 27 2018, 7:51 AM.

Details

Summary

I've added a new functionality, the checker is now able to detect and report fields pointing to themselves. I figured this would fit well into the checker as there's no reason for a pointer to point to itself instead of being nullptr.

Diff Detail

Repository
rL LLVM

Event Timeline

Szelethus created this revision.Aug 27 2018, 7:51 AM
george.karpenkov requested changes to this revision.Aug 27 2018, 10:45 AM

I'm not sure this is a good idea. Pointing to itself may be a common pattern in circular list structures, and in any case it doesn't seem to be related to uninitialized fields.

This revision now requires changes to proceed.Aug 27 2018, 10:45 AM

Thanks for taking a look!

I'm not sure this is a good idea. Pointing to itself may be a common pattern in circular list structures,

I'm fine with not pursuing this one any further. I can see the value, but it isn't of a great importance to me. However, as far as I know, pointers may point to the node that contains them, but it doesn't make much sense for them to point to themselves -- why would anyone dereference such an object intentionally? The only use-case I could imagine is checking whether ptr == &ptr, but one could might as well store a nullptr for this reason.

and in any case it doesn't seem to be related to uninitialized fields.

You could say they are incorrectly initialized! ;) Also, I would imagine these kind of reports would have the greatest likelihood of finding an actual bug.

Szelethus added inline comments.Aug 27 2018, 1:30 PM
test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
659–691 ↗(On Diff #162676)

Pointing to itself may be a common pattern in circular list structures

One important thing I forgot to mention, there are test cases for circular lists, and this patch only aims to report only pointer arithmetic errors.

I only possible use case I can imagine is storing an extra state, like this:

  • State 1: ptr == nullptr
  • State 2: ptr == &ptr
  • State 3: otherwise

But as a best-practices checker, I think it's fine to report these cases, as I would imagine then to be very error-prone. But then again, I may be unaware of some valid use cases.

Szelethus planned changes to this revision.Sep 5 2018, 8:23 AM

I've been made aware of some valid concerns with this patch. I'll revisit this at a later time, please ignore it for now.

Szelethus updated this revision to Diff 168593.Oct 7 2018, 9:40 AM
  • Moved some functions back into the global namespace.

@whisperity told me that alignment could be an issue, where:

struct List {
  List *next;
};

List L;
L.next = &L;

In this case, it is possible (and maybe probable) that &L and L->next is equal, but I'm checking whether the regions are equal, not where they point to.

I don't believe this patch would result in any false positives, but it isn't a big deal to me if it's undesirable.

I don't have objections against this.

@Szelethus In general, have you seen this pattern, or is it just something you thought could happen?
Overall, I don't think enumerating all possible bad scenarios is a best way for writing checkers. I think it's more productive to focus on real issues you have seen,
and then iterate and be data-driven.

This revision is now accepted and ready to land.Oct 10 2018, 10:55 AM

Thank you so much! :)

[...]
@Szelethus In general, have you seen this pattern, or is it just something you thought could happen?
Overall, I don't think enumerating all possible bad scenarios is a best way for writing checkers. I think it's more productive to focus on real issues you have seen,
and then iterate and be data-driven.

This was one of the very rare occasions where I noticed this bad scenario, as you put it, before seeing it in a production code. I usually rely on data I gather from analyzing open source projects and from user feedback, but after rC339599, this was super easy to implement.

The idea came relatively naturally, as pointers pointing to themselves is a corner-case I have to cover.

But really, should this ever happen, it most definitely is a sign of a serious mess up.

This revision was automatically updated to reflect the committed changes.