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. //===----------------------------------------------------------------------===// _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
