[analyzer][UninitializedObjectChecker] Pointer/reference objects are dereferenced...

This patch fixed an issue where the dynamic type of pointer/reference
object was known by the analyzer, but wasn't obtained in the checker,
which resulted in false negatives. This should also increase reliability
of the checker, as derefencing is always done now according to the
dynamic type (even if that happens to be the same as the static type).

Special thanks to Artem Degrachev for setting me on the right track.

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

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

diff --git a/lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp b/lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
index 4e6ccd9da31bc01efd7fb0aaab6f0b8aa8b9fffd..6aead3f676e1fcae250f57cd6d1a01bb079b1aa6 100644 (file)
--- a/lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
+++ b/lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
@@ -44,7 +44,7 @@
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
-#include <algorithm>
+#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicTypeMap.h"
 
 using namespace clang;
 using namespace clang::ento;
@@ -236,10 +236,10 @@ getObjectVal(const CXXConstructorDecl *CtorDecl, CheckerContext &Context);
 static bool willObjectBeAnalyzedLater(const CXXConstructorDecl *Ctor,
                                CheckerContext &Context);
 
-/// Returns whether FD can be (transitively) dereferenced to a void pointer type
+/// Returns whether T can be (transitively) dereferenced to a void pointer type
 /// (void*, void**, ...). The type of the region behind a void pointer isn't
 /// known, and thus FD can not be analyzed.
-static bool isVoidPointer(const FieldDecl *FD);
+static bool isVoidPointer(QualType T);
 
 /// Returns true if T is a primitive type. We defined this type so that for
 /// objects that we'd only like analyze as much as checking whether their
@@ -483,7 +483,7 @@ bool FindUninitializedFields::isPointerOrReferenceUninit(
 
   SVal V = State->getSVal(FR);
 
-  if (V.isUnknown() || V.isZeroConstant()) {
+  if (V.isUnknown() || V.getAs<loc::ConcreteInt>()) {
     IsAnyFieldInitialized = true;
     return false;
   }
@@ -497,48 +497,70 @@ bool FindUninitializedFields::isPointerOrReferenceUninit(
     return false;
   }
 
-  const FieldDecl *FD = FR->getDecl();
+  assert(V.getAs<loc::MemRegionVal>() &&
+         "At this point V must be loc::MemRegionVal!");
+  auto L = V.castAs<loc::MemRegionVal>();
 
-  // TODO: The dynamic type of a void pointer may be retrieved with
-  // `getDynamicTypeInfo`.
-  if (isVoidPointer(FD)) {
+  // 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 (L.getRegion()->getSymbolicBase()) {
     IsAnyFieldInitialized = true;
     return false;
   }
 
-  assert(V.getAs<Loc>() && "V should be Loc at this point!");
+  DynamicTypeInfo DynTInfo = getDynamicTypeInfo(State, L.getRegion());
+  if (!DynTInfo.isValid()) {
+    IsAnyFieldInitialized = true;
+    return false;
+  }
 
-  // At this point the pointer itself is initialized and points to a valid
-  // location, we'll now check the pointee.
-  SVal DerefdV = State->getSVal(V.castAs<Loc>());
+  QualType DynT = DynTInfo.getType();
 
-  // TODO: Dereferencing should be done according to the dynamic type.
-  while (Optional<Loc> L = DerefdV.getAs<Loc>()) {
-    DerefdV = State->getSVal(*L);
+  if (isVoidPointer(DynT)) {
+    IsAnyFieldInitialized = true;
+    return false;
   }
 
-  // If V is a pointer pointing to a record type.
-  if (Optional<nonloc::LazyCompoundVal> RecordV =
-          DerefdV.getAs<nonloc::LazyCompoundVal>()) {
+  // At this point the pointer itself is initialized and points to a valid
+  // location, we'll now check the pointee.
+  SVal DerefdV = State->getSVal(V.castAs<Loc>(), DynT);
 
-    const TypedValueRegion *R = RecordV->getRegion();
+  // If DerefdV is still a pointer value, we'll dereference it again (e.g.:
+  // int** -> int*).
+  while (auto Tmp = DerefdV.getAs<loc::MemRegionVal>()) {
+    if (Tmp->getRegion()->getSymbolicBase()) {
+      IsAnyFieldInitialized = true;
+      return false;
+    }
+
+    DynTInfo = getDynamicTypeInfo(State, Tmp->getRegion());
+    if (!DynTInfo.isValid()) {
+      IsAnyFieldInitialized = true;
+      return false;
+    }
 
-    // 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 (R->getSymbolicBase()) {
+    DynT = DynTInfo.getType();
+    if (isVoidPointer(DynT)) {
       IsAnyFieldInitialized = true;
       return false;
     }
 
-    const QualType T = R->getValueType();
+    DerefdV = State->getSVal(*Tmp, DynT);
+  }
 
-    if (T->isStructureOrClassType())
+  // If FR is a pointer pointing to a non-primitive type.
+  if (Optional<nonloc::LazyCompoundVal> RecordV =
+          DerefdV.getAs<nonloc::LazyCompoundVal>()) {
+
+    const TypedValueRegion *R = RecordV->getRegion();
+
+    if (DynT->getPointeeType()->isStructureOrClassType())
       return isNonUnionUninit(R, {LocalChain, FR});
 
-    if (T->isUnionType()) {
+    if (DynT->getPointeeType()->isUnionType()) {
       if (isUnionUninit(R)) {
         return addFieldToUninits({LocalChain, FR, /*IsDereferenced*/ true});
       } else {
@@ -547,7 +569,7 @@ bool FindUninitializedFields::isPointerOrReferenceUninit(
       }
     }
 
-    if (T->isArrayType()) {
+    if (DynT->getPointeeType()->isArrayType()) {
       IsAnyFieldInitialized = true;
       return false;
     }
@@ -555,8 +577,10 @@ bool FindUninitializedFields::isPointerOrReferenceUninit(
     llvm_unreachable("All cases are handled!");
   }
 
-  // TODO: If possible, it should be asserted that the DerefdV at this point is
-  // primitive.
+  assert((isPrimitiveType(DynT->getPointeeType()) || DynT->isPointerType() ||
+          DynT->isReferenceType()) &&
+         "At this point FR must either have a primitive dynamic type, or it "
+         "must be a null, undefined, unknown or concrete pointer!");
 
   if (isPrimitiveUninit(DerefdV))
     return addFieldToUninits({LocalChain, FR, /*IsDereferenced*/ true});
@@ -600,6 +624,25 @@ const FieldDecl *FieldChainInfo::getEndOfChain() const {
   return (*Chain.begin())->getDecl();
 }
 
+// TODO: This function constructs an incorrect string if a void pointer is a
+// part of the chain:
+//
+//   struct B { int x; }
+//
+//   struct A {
+//     void *vptr;
+//     A(void* vptr) : vptr(vptr) {}
+//   };
+//
+//   void f() {
+//     B b;
+//     A a(&b);
+//   }
+//
+// The note message will be "uninitialized field 'this->vptr->x'", even though
+// void pointers can't be dereferenced. This should be changed to "uninitialized
+// field 'static_cast<B*>(this->vptr)->x'".
+//
 // TODO: This function constructs an incorrect fieldchain string in the
 // following case:
 //
@@ -640,9 +683,7 @@ void FieldChainInfo::printTail(
 //                           Utility functions.
 //===----------------------------------------------------------------------===//
 
-static bool isVoidPointer(const FieldDecl *FD) {
-  QualType T = FD->getType();
-
+static bool isVoidPointer(QualType T) {
   while (!T.isNull()) {
     if (T->isVoidPointerType())
       return true;

diff --git a/test/Analysis/cxx-uninitialized-object-inheritance.cpp b/test/Analysis/cxx-uninitialized-object-inheritance.cpp
index 9f6d2a0e71de5a92a1c2b2313fc052ac1cf83cfb..8616dd3836b9f8e85b4f3c7a9cf88ec2f72ce7e9 100644 (file)
--- a/test/Analysis/cxx-uninitialized-object-inheritance.cpp
+++ b/test/Analysis/cxx-uninitialized-object-inheritance.cpp
@@ -776,3 +776,26 @@ public:
 void fVirtualDiamondInheritanceTest3() {
   VirtualDiamondInheritanceTest3();
 }
+
+//===----------------------------------------------------------------------===//
+// Dynamic type test.
+//===----------------------------------------------------------------------===//
+
+struct DynTBase {};
+struct DynTDerived : DynTBase {
+  // TODO: we'd expect the note: {{uninitialized field 'this->x'}}
+  int x; // no-note
+};
+
+struct DynamicTypeTest {
+  DynTBase *bptr;
+  int i = 0;
+
+  // TODO: we'd expect the warning: {{1 uninitialized field}}
+  DynamicTypeTest(DynTBase *bptr) : bptr(bptr) {} // no-warning
+};
+
+void f() {
+  DynTDerived d;
+  DynamicTypeTest t(&d);
+};

diff --git a/test/Analysis/cxx-uninitialized-object-ptr-ref.cpp b/test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
index bfffc800bc46372f39613d9977fa2f970f9e6485..2d5ebff53618eb2021c191ac6fff6cbb3301ed96 100644 (file)
--- a/test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
+++ b/test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
@@ -196,7 +196,7 @@ void fCharPointerTest() {
 
 struct CyclicPointerTest {
   int *ptr;
-  CyclicPointerTest() : ptr(reinterpret_cast<int*>(&ptr)) {}
+  CyclicPointerTest() : ptr(reinterpret_cast<int *>(&ptr)) {}
 };
 
 void fCyclicPointerTest() {
@@ -285,13 +285,62 @@ struct CyclicVoidPointerTest {
   void *vptr; // no-crash
 
   CyclicVoidPointerTest() : vptr(&vptr) {}
-
 };
 
 void fCyclicVoidPointerTest() {
   CyclicVoidPointerTest();
 }
 
+struct IntDynTypedVoidPointerTest1 {
+  void *vptr; // expected-note{{uninitialized pointee 'this->vptr'}}
+  int dontGetFilteredByNonPedanticMode = 0;
+
+  IntDynTypedVoidPointerTest1(void *vptr) : vptr(vptr) {} // expected-warning{{1 uninitialized field}}
+};
+
+void fIntDynTypedVoidPointerTest1() {
+  int a;
+  IntDynTypedVoidPointerTest1 tmp(&a);
+}
+
+struct RecordDynTypedVoidPointerTest {
+  struct RecordType {
+    int x; // expected-note{{uninitialized field 'this->vptr->x'}}
+    int y; // expected-note{{uninitialized field 'this->vptr->y'}}
+  };
+
+  void *vptr;
+  int dontGetFilteredByNonPedanticMode = 0;
+
+  RecordDynTypedVoidPointerTest(void *vptr) : vptr(vptr) {} // expected-warning{{2 uninitialized fields}}
+};
+
+void fRecordDynTypedVoidPointerTest() {
+  RecordDynTypedVoidPointerTest::RecordType a;
+  RecordDynTypedVoidPointerTest tmp(&a);
+}
+
+struct NestedNonVoidDynTypedVoidPointerTest {
+  struct RecordType {
+    int x;      // expected-note{{uninitialized field 'this->vptr->x'}}
+    int y;      // expected-note{{uninitialized field 'this->vptr->y'}}
+    void *vptr; // expected-note{{uninitialized pointee 'this->vptr->vptr'}}
+  };
+
+  void *vptr;
+  int dontGetFilteredByNonPedanticMode = 0;
+
+  NestedNonVoidDynTypedVoidPointerTest(void *vptr, void *c) : vptr(vptr) {
+    static_cast<RecordType *>(vptr)->vptr = c; // expected-warning{{3 uninitialized fields}}
+  }
+};
+
+void fNestedNonVoidDynTypedVoidPointerTest() {
+  NestedNonVoidDynTypedVoidPointerTest::RecordType a;
+  char c;
+  NestedNonVoidDynTypedVoidPointerTest tmp(&a, &c);
+}
+
 //===----------------------------------------------------------------------===//
 // Multipointer tests.
 //===----------------------------------------------------------------------===//