Index: cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h =================================================================== --- cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h +++ cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h @@ -34,6 +34,12 @@ // `-analyzer-config \ // alpha.cplusplus.UninitializedObject:CheckPointeeInitialization=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 +// references it, and we wouldn't generate multiple report on the same +// pointee. +// // - "IgnoreRecordsWithField" (string). If supplied, the checker will not // analyze structures that have a field with a name or type name that // matches the given pattern. Defaults to "". @@ -41,11 +47,12 @@ // `-analyzer-config \ // alpha.cplusplus.UninitializedObject:IgnoreRecordsWithField="[Tt]ag|[Kk]ind"`. // -// 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 -// references it, and we wouldn't generate multiple report on the same -// pointee. +// - "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`. // // Most of the following methods as well as the checker itself is defined in // UninitializedObjectChecker.cpp. @@ -68,6 +75,7 @@ bool ShouldConvertNotesToWarnings = false; bool CheckPointeeInitialization = false; std::string IgnoredRecordsWithFieldPattern; + bool IgnoreGuardedFields = false; }; /// A lightweight polymorphic wrapper around FieldRegion *. We'll use this Index: cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp =================================================================== --- cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp +++ cfe/trunk/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp @@ -19,6 +19,7 @@ #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" #include "UninitializedObject.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" @@ -26,6 +27,7 @@ using namespace clang; using namespace clang::ento; +using namespace clang::ast_matchers; /// We'll mark fields (and pointee of fields) that are confirmed to be /// uninitialized as already analyzed. @@ -118,6 +120,16 @@ /// \p Pattern. static bool shouldIgnoreRecord(const RecordDecl *RD, StringRef Pattern); +/// 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, ProgramStateRef State); + //===----------------------------------------------------------------------===// // Methods for UninitializedObjectChecker. //===----------------------------------------------------------------------===// @@ -234,6 +246,13 @@ "One must also pass the pointee region as a parameter for " "dereferenceable fields!"); + if (State->getStateManager().getContext().getSourceManager().isInSystemHeader( + FR->getDecl()->getLocation())) + return false; + + if (Opts.IgnoreGuardedFields && !hasUnguardedAccess(FR->getDecl(), State)) + return false; + if (State->contains(FR)) return false; @@ -246,13 +265,10 @@ State = State->add(FR); - if (State->getStateManager().getContext().getSourceManager().isInSystemHeader( - FR->getDecl()->getLocation())) - return false; - UninitFieldMap::mapped_type NoteMsgBuf; llvm::raw_svector_ostream OS(NoteMsgBuf); Chain.printNoteMsg(OS); + return UninitFields.insert({FR, std::move(NoteMsgBuf)}).second; } @@ -441,8 +457,8 @@ getConstructedRegion(const CXXConstructorDecl *CtorDecl, CheckerContext &Context) { - Loc ThisLoc = Context.getSValBuilder().getCXXThis(CtorDecl, - Context.getStackFrame()); + Loc ThisLoc = + Context.getSValBuilder().getCXXThis(CtorDecl, Context.getStackFrame()); SVal ObjectV = Context.getState()->getSVal(ThisLoc); @@ -495,6 +511,75 @@ return false; } +static const Stmt *getMethodBody(const CXXMethodDecl *M) { + if (isa(M)) + return nullptr; + + if (!M->isDefined()) + return nullptr; + + return M->getDefinition()->getBody(); +} + +static bool hasUnguardedAccess(const FieldDecl *FD, ProgramStateRef State) { + + if (FD->getAccess() == AccessSpecifier::AS_public) + return true; + + const auto *Parent = dyn_cast(FD->getParent()); + + if (!Parent) + return true; + + Parent = Parent->getDefinition(); + assert(Parent && "The record's definition must be avaible if an uninitialized" + " field of it was found!"); + + ASTContext &AC = State->getStateManager().getContext(); + + auto FieldAccessM = memberExpr(hasDeclaration(equalsNode(FD))).bind("access"); + + auto AssertLikeM = callExpr(callee(functionDecl( + anyOf(hasName("exit"), hasName("panic"), hasName("error"), + hasName("Assert"), hasName("assert"), hasName("ziperr"), + hasName("assfail"), hasName("db_error"), hasName("__assert"), + hasName("__assert2"), hasName("_wassert"), hasName("__assert_rtn"), + hasName("__assert_fail"), hasName("dtrace_assfail"), + hasName("yy_fatal_error"), hasName("_XCAssertionFailureHandler"), + hasName("_DTAssertionFailureHandler"), + hasName("_TSAssertionFailureHandler"))))); + + auto NoReturnFuncM = callExpr(callee(functionDecl(isNoReturn()))); + + auto GuardM = + stmt(anyOf(ifStmt(), switchStmt(), conditionalOperator(), AssertLikeM, + NoReturnFuncM)) + .bind("guard"); + + for (const CXXMethodDecl *M : Parent->methods()) { + const Stmt *MethodBody = getMethodBody(M); + if (!MethodBody) + continue; + + auto Accesses = match(stmt(hasDescendant(FieldAccessM)), *MethodBody, AC); + if (Accesses.empty()) + continue; + const auto *FirstAccess = Accesses[0].getNodeAs("access"); + assert(FirstAccess); + + auto Guards = match(stmt(hasDescendant(GuardM)), *MethodBody, AC); + if (Guards.empty()) + return true; + const auto *FirstGuard = Guards[0].getNodeAs("guard"); + assert(FirstGuard); + + if (FirstAccess->getBeginLoc() < FirstGuard->getBeginLoc()) + return true; + } + + return false; +} + 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 @@ -528,12 +613,16 @@ ChOpts.IsPedantic = AnOpts.getCheckerBooleanOption("Pedantic", /*DefaultVal*/ false, Chk); ChOpts.ShouldConvertNotesToWarnings = - AnOpts.getCheckerBooleanOption("NotesAsWarnings", /*DefaultVal*/ false, Chk); + AnOpts.getCheckerBooleanOption("NotesAsWarnings", + /*DefaultVal*/ false, Chk); ChOpts.CheckPointeeInitialization = AnOpts.getCheckerBooleanOption( "CheckPointeeInitialization", /*DefaultVal*/ false, Chk); ChOpts.IgnoredRecordsWithFieldPattern = AnOpts.getCheckerStringOption("IgnoreRecordsWithField", - /*DefaultVal*/ "", Chk); + /*DefaultVal*/ "", Chk); + ChOpts.IgnoreGuardedFields = + AnOpts.getCheckerBooleanOption("IgnoreGuardedFields", + /*DefaultVal*/ false, Chk); } bool ento::shouldRegisterUninitializedObjectChecker(const LangOptions &LO) { Index: cfe/trunk/test/Analysis/cxx-uninitialized-object-unguarded-access.cpp =================================================================== --- cfe/trunk/test/Analysis/cxx-uninitialized-object-unguarded-access.cpp +++ cfe/trunk/test/Analysis/cxx-uninitialized-object-unguarded-access.cpp @@ -0,0 +1,440 @@ +// 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. +//===----------------------------------------------------------------------===// + +[[noreturn]] void halt(); + +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 NoUngardedFieldsNoReturnFuncCalledTest { +public: + enum Kind { + V, + A + }; + +private: + int Volume, Area; + Kind K; + +public: + NoUngardedFieldsNoReturnFuncCalledTest(Kind K) : K(K) { + switch (K) { + case V: + Volume = 0; + break; + case A: + Area = 0; + break; + } + } + + void operator-() { + halt(); + (void)Area; + } + + void operator+() { + halt(); + (void)Volume; + } +}; + +void fNoUngardedFieldsNoReturnFuncCalledTest() { + NoUngardedFieldsNoReturnFuncCalledTest + T1(NoUngardedFieldsNoReturnFuncCalledTest::Kind::A); + NoUngardedFieldsNoReturnFuncCalledTest + T2(NoUngardedFieldsNoReturnFuncCalledTest::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; + } + + // We're checking method definitions for guards, so this is a no-crash test + // whether we handle methods without definitions. + void methodWithoutDefinition(); +}; + +void fNoUnguardedFieldsWithUndefMethodTest() { + NoUnguardedFieldsWithUndefMethodTest + T1(NoUnguardedFieldsWithUndefMethodTest::Kind::A); + NoUnguardedFieldsWithUndefMethodTest + T2(NoUnguardedFieldsWithUndefMethodTest::Kind::V); +} + +class UnguardedFieldThroughMethodTest { +public: + enum Kind { + V, + A + }; + +private: + 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 + }; + +public: + // Note that fields are public. + 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); +} + +//===----------------------------------------------------------------------===// +// Tests for other guards. These won't be as thorough, as other guards are +// matched the same way as asserts, so if they are recognized, they are expected +// to work as well as asserts do. +// +// None of these tests expect warnings, since the flag works correctly if these +// fields are regarded properly guarded. +//===----------------------------------------------------------------------===// + +class IfGuardedFieldsTest { +public: + enum Kind { + V, + A + }; + +private: + int Volume, Area; + Kind K; + +public: + IfGuardedFieldsTest(Kind K) : K(K) { + switch (K) { + case V: + Volume = 0; + break; + case A: + Area = 0; + break; + } + } + + void operator-() { + if (K != Kind::A) + return; + (void)Area; + } + + void operator+() { + if (K != Kind::V) + return; + (void)Volume; + } +}; + +void fIfGuardedFieldsTest() { + IfGuardedFieldsTest T1(IfGuardedFieldsTest::Kind::A); + IfGuardedFieldsTest T2(IfGuardedFieldsTest::Kind::V); +} + +class SwitchGuardedFieldsTest { +public: + enum Kind { + V, + A + }; + +private: + int Volume, Area; + Kind K; + +public: + SwitchGuardedFieldsTest(Kind K) : K(K) { + switch (K) { + case V: + Volume = 0; + break; + case A: + Area = 0; + break; + } + } + + int operator-() { + switch (K) { + case Kind::A: + return Area; + case Kind::V: + return -1; + } + } + + int operator+() { + switch (K) { + case Kind::A: + return Area; + case Kind::V: + return -1; + } + } +}; + +void fSwitchGuardedFieldsTest() { + SwitchGuardedFieldsTest T1(SwitchGuardedFieldsTest::Kind::A); + SwitchGuardedFieldsTest T2(SwitchGuardedFieldsTest::Kind::V); +} + +class ConditionalOperatorGuardedFieldsTest { +public: + enum Kind { + V, + A + }; + +private: + int Volume, Area; + Kind K; + +public: + ConditionalOperatorGuardedFieldsTest(Kind K) : K(K) { + switch (K) { + case V: + Volume = 0; + break; + case A: + Area = 0; + break; + } + } + + int operator-() { + return K == Kind::A ? Area : -1; + } + + int operator+() { + return K == Kind::V ? Volume : -1; + } +}; + +void fConditionalOperatorGuardedFieldsTest() { + ConditionalOperatorGuardedFieldsTest + T1(ConditionalOperatorGuardedFieldsTest::Kind::A); + ConditionalOperatorGuardedFieldsTest + T2(ConditionalOperatorGuardedFieldsTest::Kind::V); +}