From 646019655c2e152ab0655033a33bc39b4fac6bc5 Mon Sep 17 00:00:00 2001 From: Kristof Umann Date: Tue, 21 Aug 2018 10:45:21 +0000 Subject: [PATCH] [analyzer][UninitializedObjectChecker] Refactoring p6.: Move dereferencing to a function Now that it has it's own file, it makes little sense for isPointerOrReferenceUninit to be this large, so I moved dereferencing to a separate function. Differential Revision: https://reviews.llvm.org/D50509 llvm-svn: 340265 --- .../UninitializedObjectChecker.cpp | 3 +- .../UninitializedPointee.cpp | 116 +++++++++--------- .../cxx-uninitialized-object-ptr-ref.cpp | 21 +++- 3 files changed, 80 insertions(+), 60 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp index 810e0661782b1..5e39cc18f2a8f 100644 --- a/clang/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp @@ -265,7 +265,8 @@ bool FindUninitializedFields::isNonUnionUninit(const TypedValueRegion *R, continue; } - if (T->isAnyPointerType() || T->isReferenceType() || T->isBlockPointerType()) { + if (T->isAnyPointerType() || T->isReferenceType() || + T->isBlockPointerType()) { if (isPointerOrReferenceUninit(FR, LocalChain)) ContainsUninitField = true; continue; diff --git a/clang/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp b/clang/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp index 5817ea775f205..46a70fd1e1943 100644 --- a/clang/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp @@ -95,6 +95,12 @@ class NeedsCastLocField final : public FieldNode { /// 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 wether \p FR needs to be casted back to that type. If for whatever +/// reason dereferencing fails, returns with None. +static llvm::Optional> +dereference(ProgramStateRef State, const FieldRegion *FR); + //===----------------------------------------------------------------------===// // Methods for FindUninitializedFields. //===----------------------------------------------------------------------===// @@ -126,67 +132,22 @@ bool FindUninitializedFields::isPointerOrReferenceUninit( return false; } - assert(V.getAs() && - "At this point V must be loc::MemRegionVal!"); - auto L = V.castAs(); - - // 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; - } - - DynamicTypeInfo DynTInfo = getDynamicTypeInfo(State, L.getRegion()); - if (!DynTInfo.isValid()) { - IsAnyFieldInitialized = true; - return false; - } - - QualType DynT = DynTInfo.getType(); - - // 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()); - - if (isVoidPointer(DynT)) { + // At this point the pointer itself is initialized and points to a valid + // location, we'll now check the pointee. + llvm::Optional> DerefInfo = + dereference(State, FR); + if (!DerefInfo) { 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(), DynT); - - // 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; - } - - DynT = DynTInfo.getType(); - if (isVoidPointer(DynT)) { - IsAnyFieldInitialized = true; - return false; - } - - DerefdV = State->getSVal(*Tmp, DynT); - } + V = std::get<0>(*DerefInfo); + QualType DynT = std::get<1>(*DerefInfo); + bool NeedsCastBack = std::get<2>(*DerefInfo); // If FR is a pointer pointing to a non-primitive type. if (Optional RecordV = - DerefdV.getAs()) { + V.getAs()) { const TypedValueRegion *R = RecordV->getRegion(); @@ -220,7 +181,7 @@ bool FindUninitializedFields::isPointerOrReferenceUninit( "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)) { + if (isPrimitiveUninit(V)) { if (NeedsCastBack) return addFieldToUninits(LocalChain.add(NeedsCastLocField(FR, DynT))); return addFieldToUninits(LocalChain.add(LocField(FR))); @@ -242,3 +203,48 @@ static bool isVoidPointer(QualType T) { } return false; } + +static llvm::Optional> +dereference(ProgramStateRef State, const FieldRegion *FR) { + + DynamicTypeInfo DynTInfo; + QualType DynT; + + // 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() && "V must be loc::MemRegionVal!"); + + // 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(&ptr); + while (auto Tmp = V.getAs()) { + // 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()) { + return None; + } + + DynTInfo = getDynamicTypeInfo(State, Tmp->getRegion()); + if (!DynTInfo.isValid()) { + return None; + } + + DynT = DynTInfo.getType(); + + if (isVoidPointer(DynT)) { + return None; + } + + V = State->getSVal(*Tmp, DynT); + } + + return std::make_tuple(V, DynT, NeedsCastBack); +} diff --git a/clang/test/Analysis/cxx-uninitialized-object-ptr-ref.cpp b/clang/test/Analysis/cxx-uninitialized-object-ptr-ref.cpp index eb7ac2590d07f..f214a86adfc42 100644 --- a/clang/test/Analysis/cxx-uninitialized-object-ptr-ref.cpp +++ b/clang/test/Analysis/cxx-uninitialized-object-ptr-ref.cpp @@ -194,15 +194,28 @@ void fCharPointerTest() { CharPointerTest(); } -struct CyclicPointerTest { +struct CyclicPointerTest1 { int *ptr; - CyclicPointerTest() : ptr(reinterpret_cast(&ptr)) {} + CyclicPointerTest1() : ptr(reinterpret_cast(&ptr)) {} }; -void fCyclicPointerTest() { - CyclicPointerTest(); +void fCyclicPointerTest1() { + CyclicPointerTest1(); } +// TODO: Currently, the checker ends up in an infinite loop for the following +// test case. +/* +struct CyclicPointerTest2 { + int **pptr; + CyclicPointerTest2() : pptr(reinterpret_cast(&pptr)) {} +}; + +void fCyclicPointerTest2() { + CyclicPointerTest2(); +} +*/ + //===----------------------------------------------------------------------===// // Void pointer tests. //===----------------------------------------------------------------------===//