[analyzer][UninitializedObjectChecker] Fixed dereferencing

iThis patch aims to fix derefencing, which has been debated for months now.

Instead of working with SVals, the function now relies on TypedValueRegion.

Differential Revision: https://reviews.llvm.org/D51057


git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@342213 91177308-0d34-0410-b5e6-96231b3b80d8

lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h

@@ -87,7 +87,7 @@ public:
 
 /// Returns with Field's name. This is a helper function to get the correct name
 /// even if Field is a captured lambda variable.
-StringRef getVariableName(const FieldDecl *Field);
+std::string getVariableName(const FieldDecl *Field);
 
 /// Represents a field chain. A field chain is a vector of fields where the
 /// first element of the chain is the object under checking (not stored), and
@@ -255,7 +255,13 @@ private:
 /// ease. This also helps ensuring that every special field type is handled
 /// correctly.
 inline bool isPrimitiveType(const QualType &T) {
-  return T->isBuiltinType() || T->isEnumeralType() || T->isMemberPointerType();
+  return T->isBuiltinType() || T->isEnumeralType() ||
+         T->isMemberPointerType() || T->isBlockPointerType() ||
+         T->isFunctionType();
+}
+
+inline bool isDereferencableType(const QualType &T) {
+  return T->isAnyPointerType() || T->isReferenceType();
 }
 
 // Template method definitions.

lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp

@@ -252,9 +252,12 @@ bool FindUninitializedFields::isNonUnionUninit(const TypedValueRegion *R,
          !R->getValueType()->isUnionType() &&
          "This method only checks non-union record objects!");
 
-  const RecordDecl *RD =
-      R->getValueType()->getAs<RecordType>()->getDecl()->getDefinition();
-  assert(RD && "Referred record has no definition");
+  const RecordDecl *RD = R->getValueType()->getAsRecordDecl()->getDefinition();
+
+  if (!RD) {
+    IsAnyFieldInitialized = true;
+    return true;
+  }
 
   bool ContainsUninitField = false;
 
@@ -292,8 +295,7 @@ bool FindUninitializedFields::isNonUnionUninit(const TypedValueRegion *R,
       continue;
     }
 
-    if (T->isAnyPointerType() || T->isReferenceType() ||
-        T->isBlockPointerType()) {
+    if (isDereferencableType(T)) {
       if (isPointerOrReferenceUninit(FR, LocalChain))
         ContainsUninitField = true;
       continue;
@@ -487,7 +489,7 @@ static bool willObjectBeAnalyzedLater(const CXXConstructorDecl *Ctor,
   return false;
 }
 
-StringRef clang::ento::getVariableName(const FieldDecl *Field) {
+std::string clang::ento::getVariableName(const FieldDecl *Field) {
   // If Field is a captured lambda variable, Field->getName() will return with
   // an empty string. We can however acquire it's name from the lambda's
   // captures.
@@ -496,7 +498,16 @@ StringRef clang::ento::getVariableName(const FieldDecl *Field) {
   if (CXXParent && CXXParent->isLambda()) {
     assert(CXXParent->captures_begin());
     auto It = CXXParent->captures_begin() + Field->getFieldIndex();
-    return It->getCapturedVar()->getName();
+
+    if (It->capturesVariable())
+      return llvm::Twine("/*captured variable*/" +
+                         It->getCapturedVar()->getName())
+          .str();
+
+    if (It->capturesThis())
+      return "/*'this' capture*/";
+
+    llvm_unreachable("No other capture type is expected!");
   }
 
   return Field->getName();

lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp

@@ -95,11 +95,13 @@ public:
 /// known, and thus FD can not be analyzed.
 static bool isVoidPointer(QualType T);
 
-/// Dereferences \p V and returns the value and dynamic type of the pointee, as
-/// well as whether \p FR needs to be casted back to that type. If for whatever
-/// reason dereferencing fails, returns with None.
-static llvm::Optional<std::tuple<SVal, QualType, bool>>
-dereference(ProgramStateRef State, const FieldRegion *FR);
+using DereferenceInfo = std::pair<const TypedValueRegion *, bool>;
+
+/// 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);
 
 //===----------------------------------------------------------------------===//
 //                   Methods for FindUninitializedFields.
@@ -110,10 +112,8 @@ dereference(ProgramStateRef State, const FieldRegion *FR);
 bool FindUninitializedFields::isPointerOrReferenceUninit(
     const FieldRegion *FR, FieldChainInfo LocalChain) {
 
-  assert((FR->getDecl()->getType()->isAnyPointerType() ||
-          FR->getDecl()->getType()->isReferenceType() ||
-          FR->getDecl()->getType()->isBlockPointerType()) &&
-         "This method only checks pointer/reference objects!");
+  assert(isDereferencableType(FR->getDecl()->getType()) &&
+         "This method only checks dereferencable objects!");
 
   SVal V = State->getSVal(FR);
 
@@ -134,54 +134,47 @@ bool FindUninitializedFields::isPointerOrReferenceUninit(
 
   // At this point the pointer itself is initialized and points to a valid
   // location, we'll now check the pointee.
-  llvm::Optional<std::tuple<SVal, QualType, bool>> DerefInfo =
-      dereference(State, FR);
+  llvm::Optional<DereferenceInfo> DerefInfo = dereference(State, FR);
   if (!DerefInfo) {
     IsAnyFieldInitialized = true;
     return false;
   }
 
-  V = std::get<0>(*DerefInfo);
-  QualType DynT = std::get<1>(*DerefInfo);
-  bool NeedsCastBack = std::get<2>(*DerefInfo);
+  const TypedValueRegion *R = DerefInfo->first;
+  const bool NeedsCastBack = DerefInfo->second;
 
-  // If FR is a pointer pointing to a non-primitive type.
-  if (Optional<nonloc::LazyCompoundVal> RecordV =
-          V.getAs<nonloc::LazyCompoundVal>()) {
+  QualType DynT = R->getLocationType();
+  QualType PointeeT = DynT->getPointeeType();
 
-    const TypedValueRegion *R = RecordV->getRegion();
+  if (PointeeT->isStructureOrClassType()) {
+    if (NeedsCastBack)
+      return isNonUnionUninit(R, LocalChain.add(NeedsCastLocField(FR, DynT)));
+    return isNonUnionUninit(R, LocalChain.add(LocField(FR)));
+  }
 
-    if (DynT->getPointeeType()->isStructureOrClassType()) {
+  if (PointeeT->isUnionType()) {
+    if (isUnionUninit(R)) {
       if (NeedsCastBack)
-        return isNonUnionUninit(R, LocalChain.add(NeedsCastLocField(FR, DynT)));
-      return isNonUnionUninit(R, LocalChain.add(LocField(FR)));
-    }
-
-    if (DynT->getPointeeType()->isUnionType()) {
-      if (isUnionUninit(R)) {
-        if (NeedsCastBack)
-          return addFieldToUninits(LocalChain.add(NeedsCastLocField(FR, DynT)));
-        return addFieldToUninits(LocalChain.add(LocField(FR)));
-      } else {
-        IsAnyFieldInitialized = true;
-        return false;
-      }
-    }
-
-    if (DynT->getPointeeType()->isArrayType()) {
+        return addFieldToUninits(LocalChain.add(NeedsCastLocField(FR, DynT)));
+      return addFieldToUninits(LocalChain.add(LocField(FR)));
+    } else {
       IsAnyFieldInitialized = true;
       return false;
     }
-
-    llvm_unreachable("All cases are handled!");
   }
 
-  assert((isPrimitiveType(DynT->getPointeeType()) || DynT->isAnyPointerType() ||
-          DynT->isReferenceType()) &&
+  if (PointeeT->isArrayType()) {
+    IsAnyFieldInitialized = true;
+    return false;
+  }
+
+  assert((isPrimitiveType(PointeeT) || isDereferencableType(PointeeT)) &&
          "At this point FR must either have a primitive dynamic type, or it "
          "must be a null, undefined, unknown or concrete pointer!");
 
-  if (isPrimitiveUninit(V)) {
+  SVal PointeeV = State->getSVal(R);
+
+  if (isPrimitiveUninit(PointeeV)) {
     if (NeedsCastBack)
       return addFieldToUninits(LocalChain.add(NeedsCastLocField(FR, DynT)));
     return addFieldToUninits(LocalChain.add(LocField(FR)));
@@ -204,47 +197,46 @@ static bool isVoidPointer(QualType T) {
   return false;
 }
 
-static llvm::Optional<std::tuple<SVal, QualType, bool>>
-dereference(ProgramStateRef State, const FieldRegion *FR) {
+static llvm::Optional<DereferenceInfo> dereference(ProgramStateRef State,
+                                                   const FieldRegion *FR) {
 
-  DynamicTypeInfo DynTInfo;
-  QualType DynT;
+  llvm::SmallSet<const TypedValueRegion *, 5> VisitedRegions;
 
   // If the static type of the field is a void pointer, we need to cast it back
   // to the dynamic type before dereferencing.
   bool NeedsCastBack = isVoidPointer(FR->getDecl()->getType());
 
   SVal V = State->getSVal(FR);
-  assert(V.getAs<loc::MemRegionVal>() && "V must be loc::MemRegionVal!");
+  assert(V.getAsRegion() && "V must have an underlying region!");
 
-  // If V is multiple pointer value, we'll dereference it again (e.g.: int** ->
-  // int*).
-  // TODO: Dereference according to the dynamic type to avoid infinite loop for
-  // these kind of fields:
-  //   int **ptr = reinterpret_cast<int **>(&ptr);
-  while (auto Tmp = V.getAs<loc::MemRegionVal>()) {
-    // We can't reason about symbolic regions, assume its initialized.
-    // Note that this also avoids a potential infinite recursion, because
-    // constructors for list-like classes are checked without being called, and
-    // the Static Analyzer will construct a symbolic region for Node *next; or
-    // similar code snippets.
-    if (Tmp->getRegion()->getSymbolicBase()) {
+  // The region we'd like to acquire.
+  const auto *R = V.getAsRegion()->getAs<TypedValueRegion>();
+  if (!R)
+    return None;
+
+  VisitedRegions.insert(R);
+
+  // We acquire the dynamic type of R,
+  QualType DynT = R->getLocationType();
+
+  while (const MemRegion *Tmp = State->getSVal(R, DynT).getAsRegion()) {
+
+    R = Tmp->getAs<TypedValueRegion>();
+
+    if (!R)
       return None;
-    }
 
-    DynTInfo = getDynamicTypeInfo(State, Tmp->getRegion());
-    if (!DynTInfo.isValid()) {
+    // We found a cyclic pointer, like int *ptr = (int *)&ptr.
+    // TODO: Report these fields too.
+    if (!VisitedRegions.insert(R).second)
       return None;
-    }
 
-    DynT = DynTInfo.getType();
-
-    if (isVoidPointer(DynT)) {
-      return None;
-    }
-
-    V = State->getSVal(*Tmp, DynT);
+    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;
   }
 
-  return std::make_tuple(V, DynT, NeedsCastBack);
+  return std::make_pair(R, NeedsCastBack);
 }

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

@@ -45,6 +45,50 @@ void fNullPtrTest() {
   NullPtrTest();
 }
 
+//===----------------------------------------------------------------------===//
+// Alloca tests.
+//===----------------------------------------------------------------------===//
+
+struct UntypedAllocaTest {
+  void *allocaPtr;
+  int dontGetFilteredByNonPedanticMode = 0;
+
+  UntypedAllocaTest() : allocaPtr(__builtin_alloca(sizeof(int))) {
+    // All good!
+  }
+};
+
+void fUntypedAllocaTest() {
+  UntypedAllocaTest();
+}
+
+struct TypedAllocaTest1 {
+  int *allocaPtr; // expected-note{{uninitialized pointee 'this->allocaPtr'}}
+  int dontGetFilteredByNonPedanticMode = 0;
+
+  TypedAllocaTest1() // expected-warning{{1 uninitialized field}}
+      : allocaPtr(static_cast<int *>(__builtin_alloca(sizeof(int)))) {}
+};
+
+void fTypedAllocaTest1() {
+  TypedAllocaTest1();
+}
+
+struct TypedAllocaTest2 {
+  int *allocaPtr;
+  int dontGetFilteredByNonPedanticMode = 0;
+
+  TypedAllocaTest2()
+      : allocaPtr(static_cast<int *>(__builtin_alloca(sizeof(int)))) {
+    *allocaPtr = 55555;
+    // All good!
+  }
+};
+
+void fTypedAllocaTest2() {
+  TypedAllocaTest2();
+}
+
 //===----------------------------------------------------------------------===//
 // Heap pointer tests.
 //===----------------------------------------------------------------------===//
@@ -203,18 +247,14 @@ void fCyclicPointerTest1() {
   CyclicPointerTest1();
 }
 
-// TODO: Currently, the checker ends up in an infinite loop for the following
-// test case.
-/*
 struct CyclicPointerTest2 {
-  int **pptr;
+  int **pptr; // no-crash
   CyclicPointerTest2() : pptr(reinterpret_cast<int **>(&pptr)) {}
 };
 
 void fCyclicPointerTest2() {
   CyclicPointerTest2();
 }
-*/
 
 //===----------------------------------------------------------------------===//
 // Void pointer tests.
@@ -470,6 +510,39 @@ void fMultiPointerTest3() {
   MultiPointerTest3(mptr, int()); // '**mptr' uninitialized
 }
 
+//===----------------------------------------------------------------------===//
+// Incomplete pointee tests.
+//===----------------------------------------------------------------------===//
+
+class IncompleteType;
+
+struct IncompletePointeeTypeTest {
+  IncompleteType *pImpl; //no-crash
+  int dontGetFilteredByNonPedanticMode = 0;
+
+  IncompletePointeeTypeTest(IncompleteType *A) : pImpl(A) {}
+};
+
+void fIncompletePointeeTypeTest(void *ptr) {
+  IncompletePointeeTypeTest(reinterpret_cast<IncompleteType *>(ptr));
+}
+
+//===----------------------------------------------------------------------===//
+// Function pointer tests.
+//===----------------------------------------------------------------------===//
+
+struct FunctionPointerWithDifferentDynTypeTest {
+  using Func1 = void *(*)();
+  using Func2 = int *(*)();
+
+  Func1 f; // no-crash
+  FunctionPointerWithDifferentDynTypeTest(Func2 f) : f((Func1)f) {}
+};
+
+// Note that there isn't a function calling the constructor of
+// FunctionPointerWithDifferentDynTypeTest, because a crash could only be
+// reproduced without it.
+
 //===----------------------------------------------------------------------===//
 // Member pointer tests.
 //===----------------------------------------------------------------------===//
@@ -645,6 +718,15 @@ void fCyclicList() {
   CyclicList(&n1, int());
 }
 
+struct RingListTest {
+  RingListTest *next; // no-crash
+  RingListTest() : next(this) {}
+};
+
+void fRingListTest() {
+  RingListTest();
+}
+
 //===----------------------------------------------------------------------===//
 // Tests for classes containing references.
 //===----------------------------------------------------------------------===//

test/Analysis/cxx-uninitialized-object.cpp

@@ -1,11 +1,11 @@
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.cplusplus.UninitializedObject \
 // RUN:   -analyzer-config alpha.cplusplus.UninitializedObject:Pedantic=true -DPEDANTIC \
 // RUN:   -analyzer-config alpha.cplusplus.UninitializedObject:CheckPointeeInitialization=true \
-// RUN:   -std=c++11 -verify  %s
+// RUN:   -std=c++14 -verify  %s
 
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.cplusplus.UninitializedObject \
 // RUN:   -analyzer-config alpha.cplusplus.UninitializedObject:CheckPointeeInitialization=true \
-// RUN:   -std=c++11 -verify  %s
+// RUN:   -std=c++14 -verify  %s
 
 //===----------------------------------------------------------------------===//
 // Default constructor test.
@@ -781,7 +781,7 @@ struct LambdaTest2 {
 
 void fLambdaTest2() {
   int b;
-  auto equals = [&b](int a) { return a == b; }; // expected-note{{uninitialized pointee 'this->functor.b'}}
+  auto equals = [&b](int a) { return a == b; }; // expected-note{{uninitialized pointee 'this->functor./*captured variable*/b'}}
   LambdaTest2<decltype(equals)>(equals, int());
 }
 #else
@@ -803,8 +803,8 @@ void fLambdaTest2() {
 namespace LT3Detail {
 
 struct RecordType {
-  int x; // expected-note{{uninitialized field 'this->functor.rec1.x'}}
-  int y; // expected-note{{uninitialized field 'this->functor.rec1.y'}}
+  int x; // expected-note{{uninitialized field 'this->functor./*captured variable*/rec1.x'}}
+  int y; // expected-note{{uninitialized field 'this->functor./*captured variable*/rec1.y'}}
 };
 
 } // namespace LT3Detail
@@ -857,8 +857,8 @@ struct MultipleLambdaCapturesTest1 {
 
 void fMultipleLambdaCapturesTest1() {
   int b1, b2 = 3, b3;
-  auto equals = [&b1, &b2, &b3](int a) { return a == b1 == b2 == b3; }; // expected-note{{uninitialized pointee 'this->functor.b1'}}
-  // expected-note@-1{{uninitialized pointee 'this->functor.b3'}}
+  auto equals = [&b1, &b2, &b3](int a) { return a == b1 == b2 == b3; }; // expected-note{{uninitialized pointee 'this->functor./*captured variable*/b1'}}
+  // expected-note@-1{{uninitialized pointee 'this->functor./*captured variable*/b3'}}
   MultipleLambdaCapturesTest1<decltype(equals)>(equals, int());
 }
 
@@ -872,10 +872,35 @@ struct MultipleLambdaCapturesTest2 {
 
 void fMultipleLambdaCapturesTest2() {
   int b1, b2 = 3, b3;
-  auto equals = [b1, &b2, &b3](int a) { return a == b1 == b2 == b3; }; // expected-note{{uninitialized pointee 'this->functor.b3'}}
+  auto equals = [b1, &b2, &b3](int a) { return a == b1 == b2 == b3; }; // expected-note{{uninitialized pointee 'this->functor./*captured variable*/b3'}}
   MultipleLambdaCapturesTest2<decltype(equals)>(equals, int());
 }
 
+struct LambdaWrapper {
+  void *func; // no-crash
+  int dontGetFilteredByNonPedanticMode = 0;
+
+  LambdaWrapper(void *ptr) : func(ptr) {} // expected-warning{{1 uninitialized field}}
+};
+
+struct ThisCapturingLambdaFactory {
+  int a; // expected-note{{uninitialized field 'static_cast<decltype(a.ret()) *>(this->func)->/*'this' capture*/->a'}}
+
+  auto ret() {
+    return [this] { (void)this; };
+  }
+};
+
+void fLambdaFieldWithInvalidThisCapture() {
+  void *ptr;
+  {
+    ThisCapturingLambdaFactory a;
+    decltype(a.ret()) lambda = a.ret();
+    ptr = &lambda;
+  }
+  LambdaWrapper t(ptr);
+}
+
 //===----------------------------------------------------------------------===//
 // System header tests.
 //===----------------------------------------------------------------------===//

test/Analysis/objcpp-uninitialized-object.mm

@@ -4,7 +4,7 @@ typedef void (^myBlock) ();
 
 struct StructWithBlock {
   int a;
-  myBlock z; // expected-note{{uninitialized pointer 'this->z'}}
+  myBlock z; // expected-note{{uninitialized field 'this->z'}}
 
   StructWithBlock() : a(0), z(^{}) {}