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

[Szelethus]

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.

[george.karpenkov]

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.

[Szelethus]

Thanks for taking a look!

In D51305#1214733, @george.karpenkov wrote:

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]

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]

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]

• 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.

[george.karpenkov]

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.

[Szelethus]

Thank you so much! :)

In D51305#1260752, @george.karpenkov wrote:

[...]
@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.

cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp

virtual void printNode(llvm::raw_ostream &Out) const override {
     Out << getVariableName(getDecl()) << ')';
   }
 
   virtual void printSeparator(llvm::raw_ostream &Out) const override {
     Out << "->";
   }
 };
 
+/// Represents a Loc field that points to itself.
+class CyclicLocField final : public FieldNode {
+
+public:
+  CyclicLocField(const FieldRegion *FR) : FieldNode(FR) {}
+
+  virtual void printNoteMsg(llvm::raw_ostream &Out) const override {
+    Out << "object references itself ";
+  }
+
+  virtual void printPrefix(llvm::raw_ostream &Out) const override {}
+
+  virtual void printNode(llvm::raw_ostream &Out) const override {
+    Out << getVariableName(getDecl());
+  }
+
+  virtual void printSeparator(llvm::raw_ostream &Out) const override {
+    llvm_unreachable("CyclicLocField objects must be the last node of the "
+                     "fieldchain!");
+  }
+};
+
 } // end of anonymous namespace
 
 // Utility function declarations.
 
-/// Returns whether \p T can be (transitively) dereferenced to a void pointer
-/// type (void*, void**, ...).
-static bool isVoidPointer(QualType T);
-
-using DereferenceInfo = std::pair<const TypedValueRegion *, bool>;
+struct DereferenceInfo {
+  const TypedValueRegion *R;
+  const bool NeedsCastBack;
+  const bool IsCyclic;
+  DereferenceInfo(const TypedValueRegion *R, bool NCB, bool IC)
+      : R(R), NeedsCastBack(NCB), IsCyclic(IC) {}
+};
 
 /// Dereferences \p FR and returns with the pointee's region, and whether it
 /// needs to be casted back to it's location type. If for whatever reason
 /// dereferencing fails, returns with None.
 static llvm::Optional<DereferenceInfo> dereference(ProgramStateRef State,
                                                    const FieldRegion *FR);
 
+/// Returns whether \p T can be (transitively) dereferenced to a void pointer
+/// type (void*, void**, ...).
+static bool isVoidPointer(QualType T);
+
 //===----------------------------------------------------------------------===//
 //                   Methods for FindUninitializedFields.
 //===----------------------------------------------------------------------===//
 
 bool FindUninitializedFields::isDereferencableUninit(
     const FieldRegion *FR, FieldChainInfo LocalChain) {
 
   SVal V = State->getSVal(FR);
bool FindUninitializedFields::isDereferencableUninit(
   // At this point the pointer itself is initialized and points to a valid
   // location, we'll now check the pointee.
   llvm::Optional<DereferenceInfo> DerefInfo = dereference(State, FR);
   if (!DerefInfo) {
     IsAnyFieldInitialized = true;
     return false;
   }
 
-  const TypedValueRegion *R = DerefInfo->first;
-  const bool NeedsCastBack = DerefInfo->second;
+  if (DerefInfo->IsCyclic)
+    return addFieldToUninits(LocalChain.add(CyclicLocField(FR)));
+
+  const TypedValueRegion *R = DerefInfo->R;
+  const bool NeedsCastBack = DerefInfo->NeedsCastBack;
 
   QualType DynT = R->getLocationType();
   QualType PointeeT = DynT->getPointeeType();
 
   if (PointeeT->isStructureOrClassType()) {
     if (NeedsCastBack)
       return isNonUnionUninit(R, LocalChain.add(NeedsCastLocField(FR, DynT)));
     return isNonUnionUninit(R, LocalChain.add(LocField(FR)));
bool FindUninitializedFields::isDereferencableUninit(
   IsAnyFieldInitialized = true;
   return false;
 }
 
 //===----------------------------------------------------------------------===//
 //                           Utility functions.
 //===----------------------------------------------------------------------===//
 
-static bool isVoidPointer(QualType T) {
-  while (!T.isNull()) {
-    if (T->isVoidPointerType())
-      return true;
-    T = T->getPointeeType();
-  }
-  return false;
-}
-
 static llvm::Optional<DereferenceInfo> dereference(ProgramStateRef State,
                                                    const FieldRegion *FR) {
 
   llvm::SmallSet<const TypedValueRegion *, 5> VisitedRegions;
 
   SVal V = State->getSVal(FR);
   assert(V.getAsRegion() && "V must have an underlying region!");
 
static llvm::Optional<DereferenceInfo> dereference(ProgramStateRef State,
 
   while (const MemRegion *Tmp = State->getSVal(R, DynT).getAsRegion()) {
 
     R = Tmp->getAs<TypedValueRegion>();
     if (!R)
       return None;
 
     // We found a cyclic pointer, like int *ptr = (int *)&ptr.
-    // TODO: Should we report these fields too?
     if (!VisitedRegions.insert(R).second)
-      return None;
+      return DereferenceInfo{R, NeedsCastBack, /*IsCyclic*/ true};
 
     DynT = R->getLocationType();
     // In order to ensure that this loop terminates, we're also checking the
     // dynamic type of R, since type hierarchy is finite.
     if (isDereferencableType(DynT->getPointeeType()))
       break;
   }
 
   while (R->getAs<CXXBaseObjectRegion>()) {
     NeedsCastBack = true;
 
     if (!isa<TypedValueRegion>(R->getSuperRegion()))
       break;
     R = R->getSuperRegion()->getAs<TypedValueRegion>();
   }
 
-  return std::make_pair(R, NeedsCastBack);
+  return DereferenceInfo{R, NeedsCastBack, /*IsCyclic*/ false};
+}
+
+static bool isVoidPointer(QualType T) {
+  while (!T.isNull()) {
+    if (T->isVoidPointerType())
+      return true;
+    T = T->getPointeeType();
+  }
+  return false;
 }

cfe/trunk/test/Analysis/cxx-uninitialized-object-ptr-ref.cpp

struct CharPointerTest {
   CharPointerTest() : str("") {}
 };
 
 void fCharPointerTest() {
   CharPointerTest();
 }
 
 struct CyclicPointerTest1 {
-  int *ptr;
-  CyclicPointerTest1() : ptr(reinterpret_cast<int *>(&ptr)) {}
+  int *ptr; // expected-note{{object references itself 'this->ptr'}}
+  int dontGetFilteredByNonPedanticMode = 0;
+
+  CyclicPointerTest1() : ptr(reinterpret_cast<int *>(&ptr)) {} // expected-warning{{1 uninitialized field}}
 };
 
 void fCyclicPointerTest1() {
   CyclicPointerTest1();
 }
 
 struct CyclicPointerTest2 {
-  int **pptr; // no-crash
-  CyclicPointerTest2() : pptr(reinterpret_cast<int **>(&pptr)) {}
+  int **pptr; // expected-note{{object references itself 'this->pptr'}}
+  int dontGetFilteredByNonPedanticMode = 0;
+
+  CyclicPointerTest2() : pptr(reinterpret_cast<int **>(&pptr)) {} // expected-warning{{1 uninitialized field}}
 };
 
 void fCyclicPointerTest2() {
   CyclicPointerTest2();
 }
 
 //===----------------------------------------------------------------------===//
 // Void pointer tests.

 };
 
 void fVoidPointerLRefTest() {
   void *vptr = malloc(sizeof(int));
   VoidPointerLRefTest(vptr, char());
 }
 
 struct CyclicVoidPointerTest {
-  void *vptr; // no-crash
+  void *vptr; // expected-note{{object references itself 'this->vptr'}}
+  int dontGetFilteredByNonPedanticMode = 0;
 
-  CyclicVoidPointerTest() : vptr(&vptr) {}
+  CyclicVoidPointerTest() : vptr(&vptr) {} // expected-warning{{1 uninitialized field}}
 };
 
 void fCyclicVoidPointerTest() {
   CyclicVoidPointerTest();
 }
 
 struct IntDynTypedVoidPointerTest1 {
   void *vptr; // expected-note{{uninitialized pointee 'static_cast<int *>(this->vptr)'}}