From 6667b4e8ebef009497a0c211e94efbd35531b21e Mon Sep 17 00:00:00 2001 From: Kristof Umann Date: Fri, 14 Sep 2018 08:58:21 +0000 Subject: [PATCH] [analyzer][UninitializedObjectChecker] Fixed dereferencing iThis patch aims to fix derefencing, which has been debated for months now. Instead of working with SVals, the function now relies on TypedValueRegion. Differential Revision: https://reviews.llvm.org/D51057 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@342213 91177308-0d34-0410-b5e6-96231b3b80d8 --- .../UninitializedObject/UninitializedObject.h | 10 +- .../UninitializedObjectChecker.cpp | 25 +++- .../UninitializedPointee.cpp | 128 ++++++++---------- .../cxx-uninitialized-object-ptr-ref.cpp | 92 ++++++++++++- test/Analysis/cxx-uninitialized-object.cpp | 41 ++++-- test/Analysis/objcpp-uninitialized-object.mm | 2 +- 6 files changed, 207 insertions(+), 91 deletions(-) diff --git a/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h b/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h index 257186e270..3733914a89 100644 --- a/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h +++ b/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h @@ -87,7 +87,7 @@ public: /// Returns with Field's name. This is a helper function to get the correct name /// even if Field is a captured lambda variable. -StringRef getVariableName(const FieldDecl *Field); +std::string getVariableName(const FieldDecl *Field); /// Represents a field chain. A field chain is a vector of fields where the /// first element of the chain is the object under checking (not stored), and @@ -255,7 +255,13 @@ private: /// ease. This also helps ensuring that every special field type is handled /// correctly. inline bool isPrimitiveType(const QualType &T) { - return T->isBuiltinType() || T->isEnumeralType() || T->isMemberPointerType(); + return T->isBuiltinType() || T->isEnumeralType() || + T->isMemberPointerType() || T->isBlockPointerType() || + T->isFunctionType(); +} + +inline bool isDereferencableType(const QualType &T) { + return T->isAnyPointerType() || T->isReferenceType(); } // Template method definitions. diff --git a/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp b/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp index d6ede81623..84c18869d7 100644 --- a/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp @@ -252,9 +252,12 @@ bool FindUninitializedFields::isNonUnionUninit(const TypedValueRegion *R, !R->getValueType()->isUnionType() && "This method only checks non-union record objects!"); - const RecordDecl *RD = - R->getValueType()->getAs()->getDecl()->getDefinition(); - assert(RD && "Referred record has no definition"); + const RecordDecl *RD = R->getValueType()->getAsRecordDecl()->getDefinition(); + + if (!RD) { + IsAnyFieldInitialized = true; + return true; + } bool ContainsUninitField = false; @@ -292,8 +295,7 @@ bool FindUninitializedFields::isNonUnionUninit(const TypedValueRegion *R, continue; } - if (T->isAnyPointerType() || T->isReferenceType() || - T->isBlockPointerType()) { + if (isDereferencableType(T)) { if (isPointerOrReferenceUninit(FR, LocalChain)) ContainsUninitField = true; continue; @@ -487,7 +489,7 @@ static bool willObjectBeAnalyzedLater(const CXXConstructorDecl *Ctor, return false; } -StringRef clang::ento::getVariableName(const FieldDecl *Field) { +std::string clang::ento::getVariableName(const FieldDecl *Field) { // If Field is a captured lambda variable, Field->getName() will return with // an empty string. We can however acquire it's name from the lambda's // captures. @@ -496,7 +498,16 @@ StringRef clang::ento::getVariableName(const FieldDecl *Field) { if (CXXParent && CXXParent->isLambda()) { assert(CXXParent->captures_begin()); auto It = CXXParent->captures_begin() + Field->getFieldIndex(); - return It->getCapturedVar()->getName(); + + if (It->capturesVariable()) + return llvm::Twine("/*captured variable*/" + + It->getCapturedVar()->getName()) + .str(); + + if (It->capturesThis()) + return "/*'this' capture*/"; + + llvm_unreachable("No other capture type is expected!"); } return Field->getName(); diff --git a/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp b/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp index 72e256b717..22e3cbdb10 100644 --- a/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp +++ b/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedPointee.cpp @@ -95,11 +95,13 @@ public: /// 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 whether \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); +using DereferenceInfo = std::pair; + +/// Dereferences \p FR and returns with the pointee's region, and whether it +/// needs to be casted back to it's location type. If for whatever reason +/// dereferencing fails, returns with None. +static llvm::Optional dereference(ProgramStateRef State, + const FieldRegion *FR); //===----------------------------------------------------------------------===// // Methods for FindUninitializedFields. @@ -110,10 +112,8 @@ dereference(ProgramStateRef State, const FieldRegion *FR); bool FindUninitializedFields::isPointerOrReferenceUninit( const FieldRegion *FR, FieldChainInfo LocalChain) { - assert((FR->getDecl()->getType()->isAnyPointerType() || - FR->getDecl()->getType()->isReferenceType() || - FR->getDecl()->getType()->isBlockPointerType()) && - "This method only checks pointer/reference objects!"); + assert(isDereferencableType(FR->getDecl()->getType()) && + "This method only checks dereferencable objects!"); SVal V = State->getSVal(FR); @@ -134,54 +134,47 @@ bool FindUninitializedFields::isPointerOrReferenceUninit( // 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); + llvm::Optional DerefInfo = dereference(State, FR); if (!DerefInfo) { IsAnyFieldInitialized = true; return false; } - V = std::get<0>(*DerefInfo); - QualType DynT = std::get<1>(*DerefInfo); - bool NeedsCastBack = std::get<2>(*DerefInfo); + const TypedValueRegion *R = DerefInfo->first; + const bool NeedsCastBack = DerefInfo->second; - // If FR is a pointer pointing to a non-primitive type. - if (Optional RecordV = - V.getAs()) { + QualType DynT = R->getLocationType(); + QualType PointeeT = DynT->getPointeeType(); - const TypedValueRegion *R = RecordV->getRegion(); + if (PointeeT->isStructureOrClassType()) { + if (NeedsCastBack) + return isNonUnionUninit(R, LocalChain.add(NeedsCastLocField(FR, DynT))); + return isNonUnionUninit(R, LocalChain.add(LocField(FR))); + } - if (DynT->getPointeeType()->isStructureOrClassType()) { + if (PointeeT->isUnionType()) { + if (isUnionUninit(R)) { if (NeedsCastBack) - return isNonUnionUninit(R, LocalChain.add(NeedsCastLocField(FR, DynT))); - return isNonUnionUninit(R, LocalChain.add(LocField(FR))); - } - - if (DynT->getPointeeType()->isUnionType()) { - if (isUnionUninit(R)) { - if (NeedsCastBack) - return addFieldToUninits(LocalChain.add(NeedsCastLocField(FR, DynT))); - return addFieldToUninits(LocalChain.add(LocField(FR))); - } else { - IsAnyFieldInitialized = true; - return false; - } - } - - if (DynT->getPointeeType()->isArrayType()) { + return addFieldToUninits(LocalChain.add(NeedsCastLocField(FR, DynT))); + return addFieldToUninits(LocalChain.add(LocField(FR))); + } else { IsAnyFieldInitialized = true; return false; } + } - llvm_unreachable("All cases are handled!"); + if (PointeeT->isArrayType()) { + IsAnyFieldInitialized = true; + return false; } - assert((isPrimitiveType(DynT->getPointeeType()) || DynT->isAnyPointerType() || - DynT->isReferenceType()) && + assert((isPrimitiveType(PointeeT) || isDereferencableType(PointeeT)) && "At this point FR must either have a primitive dynamic type, or it " "must be a null, undefined, unknown or concrete pointer!"); - if (isPrimitiveUninit(V)) { + SVal PointeeV = State->getSVal(R); + + if (isPrimitiveUninit(PointeeV)) { if (NeedsCastBack) return addFieldToUninits(LocalChain.add(NeedsCastLocField(FR, DynT))); return addFieldToUninits(LocalChain.add(LocField(FR))); @@ -204,47 +197,46 @@ static bool isVoidPointer(QualType T) { return false; } -static llvm::Optional> -dereference(ProgramStateRef State, const FieldRegion *FR) { +static llvm::Optional dereference(ProgramStateRef State, + const FieldRegion *FR) { - DynamicTypeInfo DynTInfo; - QualType DynT; + llvm::SmallSet VisitedRegions; // 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; - } + assert(V.getAsRegion() && "V must have an underlying region!"); - DynTInfo = getDynamicTypeInfo(State, Tmp->getRegion()); - if (!DynTInfo.isValid()) { - return None; - } + // The region we'd like to acquire. + const auto *R = V.getAsRegion()->getAs(); + if (!R) + return None; + + VisitedRegions.insert(R); - DynT = DynTInfo.getType(); + // We acquire the dynamic type of R, + QualType DynT = R->getLocationType(); - if (isVoidPointer(DynT)) { + while (const MemRegion *Tmp = State->getSVal(R, DynT).getAsRegion()) { + + R = Tmp->getAs(); + + if (!R) + return None; + + // We found a cyclic pointer, like int *ptr = (int *)&ptr. + // TODO: Report these fields too. + if (!VisitedRegions.insert(R).second) return None; - } - V = State->getSVal(*Tmp, DynT); + DynT = R->getLocationType(); + // In order to ensure that this loop terminates, we're also checking the + // dynamic type of R, since type hierarchy is finite. + if (isDereferencableType(DynT->getPointeeType())) + break; } - return std::make_tuple(V, DynT, NeedsCastBack); + return std::make_pair(R, NeedsCastBack); } diff --git a/test/Analysis/cxx-uninitialized-object-ptr-ref.cpp b/test/Analysis/cxx-uninitialized-object-ptr-ref.cpp index f214a86adf..4ee113c9e8 100644 --- a/test/Analysis/cxx-uninitialized-object-ptr-ref.cpp +++ b/test/Analysis/cxx-uninitialized-object-ptr-ref.cpp @@ -45,6 +45,50 @@ void fNullPtrTest() { NullPtrTest(); } +//===----------------------------------------------------------------------===// +// Alloca tests. +//===----------------------------------------------------------------------===// + +struct UntypedAllocaTest { + void *allocaPtr; + int dontGetFilteredByNonPedanticMode = 0; + + UntypedAllocaTest() : allocaPtr(__builtin_alloca(sizeof(int))) { + // All good! + } +}; + +void fUntypedAllocaTest() { + UntypedAllocaTest(); +} + +struct TypedAllocaTest1 { + int *allocaPtr; // expected-note{{uninitialized pointee 'this->allocaPtr'}} + int dontGetFilteredByNonPedanticMode = 0; + + TypedAllocaTest1() // expected-warning{{1 uninitialized field}} + : allocaPtr(static_cast(__builtin_alloca(sizeof(int)))) {} +}; + +void fTypedAllocaTest1() { + TypedAllocaTest1(); +} + +struct TypedAllocaTest2 { + int *allocaPtr; + int dontGetFilteredByNonPedanticMode = 0; + + TypedAllocaTest2() + : allocaPtr(static_cast(__builtin_alloca(sizeof(int)))) { + *allocaPtr = 55555; + // All good! + } +}; + +void fTypedAllocaTest2() { + TypedAllocaTest2(); +} + //===----------------------------------------------------------------------===// // Heap pointer tests. //===----------------------------------------------------------------------===// @@ -203,18 +247,14 @@ void fCyclicPointerTest1() { CyclicPointerTest1(); } -// TODO: Currently, the checker ends up in an infinite loop for the following -// test case. -/* struct CyclicPointerTest2 { - int **pptr; + int **pptr; // no-crash CyclicPointerTest2() : pptr(reinterpret_cast(&pptr)) {} }; void fCyclicPointerTest2() { CyclicPointerTest2(); } -*/ //===----------------------------------------------------------------------===// // Void pointer tests. @@ -470,6 +510,39 @@ void fMultiPointerTest3() { MultiPointerTest3(mptr, int()); // '**mptr' uninitialized } +//===----------------------------------------------------------------------===// +// Incomplete pointee tests. +//===----------------------------------------------------------------------===// + +class IncompleteType; + +struct IncompletePointeeTypeTest { + IncompleteType *pImpl; //no-crash + int dontGetFilteredByNonPedanticMode = 0; + + IncompletePointeeTypeTest(IncompleteType *A) : pImpl(A) {} +}; + +void fIncompletePointeeTypeTest(void *ptr) { + IncompletePointeeTypeTest(reinterpret_cast(ptr)); +} + +//===----------------------------------------------------------------------===// +// Function pointer tests. +//===----------------------------------------------------------------------===// + +struct FunctionPointerWithDifferentDynTypeTest { + using Func1 = void *(*)(); + using Func2 = int *(*)(); + + Func1 f; // no-crash + FunctionPointerWithDifferentDynTypeTest(Func2 f) : f((Func1)f) {} +}; + +// Note that there isn't a function calling the constructor of +// FunctionPointerWithDifferentDynTypeTest, because a crash could only be +// reproduced without it. + //===----------------------------------------------------------------------===// // Member pointer tests. //===----------------------------------------------------------------------===// @@ -645,6 +718,15 @@ void fCyclicList() { CyclicList(&n1, int()); } +struct RingListTest { + RingListTest *next; // no-crash + RingListTest() : next(this) {} +}; + +void fRingListTest() { + RingListTest(); +} + //===----------------------------------------------------------------------===// // Tests for classes containing references. //===----------------------------------------------------------------------===// diff --git a/test/Analysis/cxx-uninitialized-object.cpp b/test/Analysis/cxx-uninitialized-object.cpp index e9079d99af..07006bea47 100644 --- a/test/Analysis/cxx-uninitialized-object.cpp +++ b/test/Analysis/cxx-uninitialized-object.cpp @@ -1,11 +1,11 @@ // RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.cplusplus.UninitializedObject \ // RUN: -analyzer-config alpha.cplusplus.UninitializedObject:Pedantic=true -DPEDANTIC \ // RUN: -analyzer-config alpha.cplusplus.UninitializedObject:CheckPointeeInitialization=true \ -// RUN: -std=c++11 -verify %s +// RUN: -std=c++14 -verify %s // RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.cplusplus.UninitializedObject \ // RUN: -analyzer-config alpha.cplusplus.UninitializedObject:CheckPointeeInitialization=true \ -// RUN: -std=c++11 -verify %s +// RUN: -std=c++14 -verify %s //===----------------------------------------------------------------------===// // Default constructor test. @@ -781,7 +781,7 @@ struct LambdaTest2 { void fLambdaTest2() { int b; - auto equals = [&b](int a) { return a == b; }; // expected-note{{uninitialized pointee 'this->functor.b'}} + auto equals = [&b](int a) { return a == b; }; // expected-note{{uninitialized pointee 'this->functor./*captured variable*/b'}} LambdaTest2(equals, int()); } #else @@ -803,8 +803,8 @@ void fLambdaTest2() { namespace LT3Detail { struct RecordType { - int x; // expected-note{{uninitialized field 'this->functor.rec1.x'}} - int y; // expected-note{{uninitialized field 'this->functor.rec1.y'}} + int x; // expected-note{{uninitialized field 'this->functor./*captured variable*/rec1.x'}} + int y; // expected-note{{uninitialized field 'this->functor./*captured variable*/rec1.y'}} }; } // namespace LT3Detail @@ -857,8 +857,8 @@ struct MultipleLambdaCapturesTest1 { void fMultipleLambdaCapturesTest1() { int b1, b2 = 3, b3; - auto equals = [&b1, &b2, &b3](int a) { return a == b1 == b2 == b3; }; // expected-note{{uninitialized pointee 'this->functor.b1'}} - // expected-note@-1{{uninitialized pointee 'this->functor.b3'}} + auto equals = [&b1, &b2, &b3](int a) { return a == b1 == b2 == b3; }; // expected-note{{uninitialized pointee 'this->functor./*captured variable*/b1'}} + // expected-note@-1{{uninitialized pointee 'this->functor./*captured variable*/b3'}} MultipleLambdaCapturesTest1(equals, int()); } @@ -872,10 +872,35 @@ struct MultipleLambdaCapturesTest2 { void fMultipleLambdaCapturesTest2() { int b1, b2 = 3, b3; - auto equals = [b1, &b2, &b3](int a) { return a == b1 == b2 == b3; }; // expected-note{{uninitialized pointee 'this->functor.b3'}} + auto equals = [b1, &b2, &b3](int a) { return a == b1 == b2 == b3; }; // expected-note{{uninitialized pointee 'this->functor./*captured variable*/b3'}} MultipleLambdaCapturesTest2(equals, int()); } +struct LambdaWrapper { + void *func; // no-crash + int dontGetFilteredByNonPedanticMode = 0; + + LambdaWrapper(void *ptr) : func(ptr) {} // expected-warning{{1 uninitialized field}} +}; + +struct ThisCapturingLambdaFactory { + int a; // expected-note{{uninitialized field 'static_cast(this->func)->/*'this' capture*/->a'}} + + auto ret() { + return [this] { (void)this; }; + } +}; + +void fLambdaFieldWithInvalidThisCapture() { + void *ptr; + { + ThisCapturingLambdaFactory a; + decltype(a.ret()) lambda = a.ret(); + ptr = λ + } + LambdaWrapper t(ptr); +} + //===----------------------------------------------------------------------===// // System header tests. //===----------------------------------------------------------------------===// diff --git a/test/Analysis/objcpp-uninitialized-object.mm b/test/Analysis/objcpp-uninitialized-object.mm index c1afb72638..8ea4b56998 100644 --- a/test/Analysis/objcpp-uninitialized-object.mm +++ b/test/Analysis/objcpp-uninitialized-object.mm @@ -4,7 +4,7 @@ typedef void (^myBlock) (); struct StructWithBlock { int a; - myBlock z; // expected-note{{uninitialized pointer 'this->z'}} + myBlock z; // expected-note{{uninitialized field 'this->z'}} StructWithBlock() : a(0), z(^{}) {} -- 2.40.0