r339240 - [analyzer][UninitializedObjectChecker] Pointer/reference objects are dereferenced according to dynamic type
Kristof Umann via cfe-commits
cfe-commits at lists.llvm.org
Wed Aug 8 06:18:54 PDT 2018
Author: szelethus
Date: Wed Aug 8 06:18:53 2018
New Revision: 339240
URL: http://llvm.org/viewvc/llvm-project?rev=339240&view=rev
Log:
[analyzer][UninitializedObjectChecker] Pointer/reference objects are dereferenced according to dynamic type
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
Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
cfe/trunk/test/Analysis/cxx-uninitialized-object-inheritance.cpp
cfe/trunk/test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp?rev=339240&r1=339239&r2=339240&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp Wed Aug 8 06:18:53 2018
@@ -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 *C
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::isPointerO
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::isPointerO
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>();
+
+ // 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;
+ }
- // TODO: The dynamic type of a void pointer may be retrieved with
- // `getDynamicTypeInfo`.
- if (isVoidPointer(FD)) {
+ DynamicTypeInfo DynTInfo = getDynamicTypeInfo(State, L.getRegion());
+ if (!DynTInfo.isValid()) {
IsAnyFieldInitialized = true;
return false;
}
- assert(V.getAs<Loc>() && "V should be Loc at this point!");
+ QualType DynT = DynTInfo.getType();
+
+ if (isVoidPointer(DynT)) {
+ 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>());
-
- // TODO: Dereferencing should be done according to the dynamic type.
- while (Optional<Loc> L = DerefdV.getAs<Loc>()) {
- DerefdV = State->getSVal(*L);
- }
+ SVal DerefdV = State->getSVal(V.castAs<Loc>(), DynT);
- // If V is a pointer pointing to a record type.
- if (Optional<nonloc::LazyCompoundVal> RecordV =
- DerefdV.getAs<nonloc::LazyCompoundVal>()) {
+ // 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;
+ }
- const TypedValueRegion *R = RecordV->getRegion();
+ 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 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 (T->isStructureOrClassType())
+ 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::isPointerO
}
}
- if (T->isArrayType()) {
+ if (DynT->getPointeeType()->isArrayType()) {
IsAnyFieldInitialized = true;
return false;
}
@@ -555,8 +577,10 @@ bool FindUninitializedFields::isPointerO
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::getEndO
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;
Modified: cfe/trunk/test/Analysis/cxx-uninitialized-object-inheritance.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/cxx-uninitialized-object-inheritance.cpp?rev=339240&r1=339239&r2=339240&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/cxx-uninitialized-object-inheritance.cpp (original)
+++ cfe/trunk/test/Analysis/cxx-uninitialized-object-inheritance.cpp Wed Aug 8 06:18:53 2018
@@ -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);
+};
Modified: cfe/trunk/test/Analysis/cxx-uninitialized-object-ptr-ref.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/cxx-uninitialized-object-ptr-ref.cpp?rev=339240&r1=339239&r2=339240&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/cxx-uninitialized-object-ptr-ref.cpp (original)
+++ cfe/trunk/test/Analysis/cxx-uninitialized-object-ptr-ref.cpp Wed Aug 8 06:18:53 2018
@@ -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.
//===----------------------------------------------------------------------===//
More information about the cfe-commits
mailing list