From 1cdeaa85d9ba70ff0794a801261876061be91926 Mon Sep 17 00:00:00 2001 From: Kristof Umann Date: Wed, 8 Aug 2018 13:18:53 +0000 Subject: [PATCH] [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 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@339240 91177308-0d34-0410-b5e6-96231b3b80d8 --- .../Checkers/UninitializedObjectChecker.cpp | 109 ++++++++++++------ .../cxx-uninitialized-object-inheritance.cpp | 23 ++++ .../cxx-uninitialized-object-ptr-ref.cpp | 53 ++++++++- 3 files changed, 149 insertions(+), 36 deletions(-) diff --git a/lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp b/lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp index 4e6ccd9da3..6aead3f676 100644 --- 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 +#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()) { IsAnyFieldInitialized = true; return false; } @@ -497,48 +497,70 @@ bool FindUninitializedFields::isPointerOrReferenceUninit( return false; } - const FieldDecl *FD = FR->getDecl(); + assert(V.getAs() && + "At this point V must be loc::MemRegionVal!"); + auto L = V.castAs(); - // 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() && "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()); + QualType DynT = DynTInfo.getType(); - // TODO: Dereferencing should be done according to the dynamic type. - while (Optional L = DerefdV.getAs()) { - DerefdV = State->getSVal(*L); + if (isVoidPointer(DynT)) { + IsAnyFieldInitialized = true; + return false; } - // If V is a pointer pointing to a record type. - if (Optional RecordV = - DerefdV.getAs()) { + // 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(), 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()) { + 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 RecordV = + DerefdV.getAs()) { + + 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(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 9f6d2a0e71..8616dd3836 100644 --- 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 bfffc800bc..2d5ebff536 100644 --- 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(&ptr)) {} + CyclicPointerTest() : ptr(reinterpret_cast(&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(vptr)->vptr = c; // expected-warning{{3 uninitialized fields}} + } +}; + +void fNestedNonVoidDynTypedVoidPointerTest() { + NestedNonVoidDynTypedVoidPointerTest::RecordType a; + char c; + NestedNonVoidDynTypedVoidPointerTest tmp(&a, &c); +} + //===----------------------------------------------------------------------===// // Multipointer tests. //===----------------------------------------------------------------------===// -- 2.40.0