Index: lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h =================================================================== --- lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h +++ lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h @@ -35,6 +35,13 @@ // `-analyzer-config \ // alpha.cplusplus.UninitializedObject:CheckPointeeInitialization=true`. // +// - "IgnoreGuardedFields" (boolean). If set to true, the checker will analyze +// _syntactically_ whether the found uninitialized object is used without a +// preceding assert call. Defaults to false. +// +// `-analyzer-config \ +// alpha.cplusplus.UninitializedObject:IgnoreGuardedFields=true`. +// // TODO: With some clever heuristics, some pointers should be dereferenced // by default. For example, if the pointee is constructed within the // constructor call, it's reasonable to say that no external object @@ -61,6 +68,7 @@ bool IsPedantic; bool ShouldConvertNotesToWarnings; bool CheckPointeeInitialization; + bool IgnoreGuardedFields; }; /// Represent a single field. This is only an interface to abstract away special Index: lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp =================================================================== --- lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp +++ lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp @@ -20,6 +20,7 @@ #include "../ClangSACheckers.h" #include "UninitializedObject.h" +#include "clang/AST/StmtVisitor.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" @@ -107,6 +108,16 @@ static bool willObjectBeAnalyzedLater(const CXXConstructorDecl *Ctor, CheckerContext &Context); +/// Checks _syntactically_ whether it is possible to access FD from the record +/// that contains it without a preceding assert (even if that access happens +/// inside a method). This is mainly used for records that act like unions, like +/// having multiple bit fields, with only a fraction being properly initialized. +/// If these fields are properly guarded with asserts, this method returns +/// false. +/// +/// Since this check is done syntactically, this method could be inaccurate. +static bool hasUnguardedAccess(const FieldDecl *FD); + //===----------------------------------------------------------------------===// // Methods for UninitializedObjectChecker. //===----------------------------------------------------------------------===// @@ -201,16 +212,19 @@ } bool FindUninitializedFields::addFieldToUninits(FieldChainInfo Chain) { + const FieldRegion *FR = Chain.getUninitRegion(); + if (State->getStateManager().getContext().getSourceManager().isInSystemHeader( - Chain.getUninitRegion()->getDecl()->getLocation())) + FR->getDecl()->getLocation())) + return false; + + if (Opts.IgnoreGuardedFields && !hasUnguardedAccess(FR->getDecl())) return false; UninitFieldMap::mapped_type NoteMsgBuf; llvm::raw_svector_ostream OS(NoteMsgBuf); Chain.printNoteMsg(OS); - return UninitFields - .insert(std::make_pair(Chain.getUninitRegion(), std::move(NoteMsgBuf))) - .second; + return UninitFields.insert(std::make_pair(FR, std::move(NoteMsgBuf))).second; } bool FindUninitializedFields::isNonUnionUninit(const TypedValueRegion *R, @@ -454,6 +468,94 @@ return false; } +namespace { + +/// Checks whether the body of a method contains a use of a preselected field +/// without a previous assert. +class UnguardedAccessVerifier + : public ConstStmtVisitor { + + const FieldDecl *UninitField; + bool HadAssertCall = false; + bool HadUnguardedAccess = false; + +public: + UnguardedAccessVerifier(const FieldDecl *FD, const CXXMethodDecl *M) + : UninitField(FD) { + + if (isa(M)) + return; + + if (!M->isDefined()) + return; + + const Stmt *MethodBody = M->getDefinition()->getBody(); + + if (!MethodBody) + return; + + VisitStmt(MethodBody); + } + + bool hadUnguardedAccess() { return HadUnguardedAccess; } + + // Implementation methods. + void VisitStmt(const Stmt *S) { VisitChildren(S); } + + void VisitChildren(const Stmt *S) { + for (Stmt::const_child_iterator It = S->child_begin(), End = S->child_end(); + It != End; ++It) { + if (const Stmt *Child = *It) + Visit(Child); + } + } + + void VisitCallExpr(const CallExpr *C) { + if (const FunctionDecl *FD = C->getDirectCallee()) { + if (const IdentifierInfo *II = FD->getIdentifier()) { + if (II->isStr("assert")) + HadAssertCall = true; + } + } + } + + void VisitMemberExpr(const MemberExpr *ME) { + const FieldDecl *ReferencedFD = dyn_cast(ME->getMemberDecl()); + assert(ReferencedFD); + + if (UninitField == ReferencedFD) { + if (!HadAssertCall) + HadUnguardedAccess = true; + } + } +}; + +} // end of anonymous namespace + +static bool hasUnguardedAccess(const FieldDecl *FD) { + + if (FD->getAccess() == AccessSpecifier::AS_public) + return true; + + const auto *Parent = dyn_cast(FD->getParent()); + + // This field is in a C-like struct. + if (!Parent) + return true; + + Parent = Parent->getDefinition(); + assert(Parent && "The record's definition must be avaible if an uninitialized" + " field of it was found!"); + + for (const CXXMethodDecl *M : Parent->methods()) { + UnguardedAccessVerifier U(FD, M); + if (U.hadUnguardedAccess()) + return true; + } + + return false; +} + StringRef 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 @@ -480,4 +582,6 @@ "NotesAsWarnings", /*DefaultVal*/ false, Chk); Opts.CheckPointeeInitialization = Mgr.getAnalyzerOptions().getBooleanOption( "CheckPointeeInitialization", /*DefaultVal*/ false, Chk); + Opts.IgnoreGuardedFields = Mgr.getAnalyzerOptions().getBooleanOption( + "IgnoreGuardedFields", /*DefaultVal*/ false, Chk); } Index: test/Analysis/cxx-uninitialized-object-unguarded-access.cpp =================================================================== --- /dev/null +++ test/Analysis/cxx-uninitialized-object-unguarded-access.cpp @@ -0,0 +1,257 @@ +// 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:IgnoreGuardedFields=true \ +// RUN: -std=c++11 -verify %s + +//===----------------------------------------------------------------------===// +// Helper functions for tests. +//===----------------------------------------------------------------------===// +void halt() __attribute__((__noreturn__)); +void assert(int b) { + if (!b) + halt(); +} + +int rand(); + +//===----------------------------------------------------------------------===// +// Tests for fields properly guarded by asserts. +//===----------------------------------------------------------------------===// + +class NoUnguardedFieldsTest { +public: + enum Kind { + V, + A + }; + +private: + int Volume, Area; + Kind K; + +public: + NoUnguardedFieldsTest(Kind K) : K(K) { + switch (K) { + case V: + Volume = 0; + break; + case A: + Area = 0; + break; + } + } + + void operator-() { + assert(K == Kind::A); + (void)Area; + } + + void operator+() { + assert(K == Kind::V); + (void)Volume; + } +}; + +void fNoUnguardedFieldsTest() { + NoUnguardedFieldsTest T1(NoUnguardedFieldsTest::Kind::A); + NoUnguardedFieldsTest T2(NoUnguardedFieldsTest::Kind::V); +} + +class NoUnguardedFieldsWithUndefMethodTest { +public: + enum Kind { + V, + A + }; + +private: + int Volume, Area; + Kind K; + +public: + NoUnguardedFieldsWithUndefMethodTest(Kind K) : K(K) { + switch (K) { + case V: + Volume = 0; + break; + case A: + Area = 0; + break; + } + } + + void operator-() { + assert(K == Kind::A); + (void)Area; + } + + void operator+() { + assert(K == Kind::V); + (void)Volume; + } + + void methodWithoutDefinition(); +}; + +void fNoUnguardedFieldsWithUndefMethodTest() { + NoUnguardedFieldsWithUndefMethodTest + T1(NoUnguardedFieldsWithUndefMethodTest::Kind::A); + NoUnguardedFieldsWithUndefMethodTest + T2(NoUnguardedFieldsWithUndefMethodTest::Kind::V); +} + +class UnguardedFieldThroughMethodTest { +public: + enum Kind { + V, + A + }; + +public: + int Volume, Area; // expected-note {{uninitialized field 'this->Volume'}} + Kind K; + +public: + UnguardedFieldThroughMethodTest(Kind K) : K(K) { + switch (K) { + case V: + Volume = 0; + break; + case A: + Area = 0; // expected-warning {{1 uninitialized field}} + break; + } + } + + void operator-() { + assert(K == Kind::A); + (void)Area; + } + + void operator+() { + (void)Volume; + } +}; + +void fUnguardedFieldThroughMethodTest() { + UnguardedFieldThroughMethodTest T1(UnguardedFieldThroughMethodTest::Kind::A); +} + +class UnguardedPublicFieldsTest { +public: + enum Kind { + V, + A + }; + + int Volume, Area; // expected-note {{uninitialized field 'this->Volume'}} + Kind K; + +public: + UnguardedPublicFieldsTest(Kind K) : K(K) { + switch (K) { + case V: + Volume = 0; + break; + case A: + Area = 0; // expected-warning {{1 uninitialized field}} + break; + } + } + + void operator-() { + assert(K == Kind::A); + (void)Area; + } + + void operator+() { + assert(K == Kind::V); + (void)Volume; + } +}; + +void fUnguardedPublicFieldsTest() { + UnguardedPublicFieldsTest T1(UnguardedPublicFieldsTest::Kind::A); +} + +//===----------------------------------------------------------------------===// +// Highlights of some false negatives due to syntactic checking. +//===----------------------------------------------------------------------===// + +class UnguardedFalseNegativeTest1 { +public: + enum Kind { + V, + A + }; + +private: + int Volume, Area; + Kind K; + +public: + UnguardedFalseNegativeTest1(Kind K) : K(K) { + switch (K) { + case V: + Volume = 0; + break; + case A: + Area = 0; + break; + } + } + + void operator-() { + if (rand()) + assert(K == Kind::A); + (void)Area; + } + + void operator+() { + if (rand()) + assert(K == Kind::V); + (void)Volume; + } +}; + +void fUnguardedFalseNegativeTest1() { + UnguardedFalseNegativeTest1 T1(UnguardedFalseNegativeTest1::Kind::A); +} + +class UnguardedFalseNegativeTest2 { +public: + enum Kind { + V, + A + }; + +private: + int Volume, Area; + Kind K; + +public: + UnguardedFalseNegativeTest2(Kind K) : K(K) { + switch (K) { + case V: + Volume = 0; + break; + case A: + Area = 0; + break; + } + } + + void operator-() { + assert(rand()); + (void)Area; + } + + void operator+() { + assert(rand()); + (void)Volume; + } +}; + +void fUnguardedFalseNegativeTest2() { + UnguardedFalseNegativeTest2 T1(UnguardedFalseNegativeTest2::Kind::A); +}