Index: lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h =================================================================== --- lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h +++ lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h @@ -35,6 +35,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 "". @@ -42,11 +48,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. @@ -69,6 +76,7 @@ bool ShouldConvertNotesToWarnings = false; bool CheckPointeeInitialization = false; std::string IgnoredRecordsWithFieldPattern; + bool IgnoreGuardedFields = false; }; /// A lightweight polymorphic wrapper around FieldRegion *. We'll use this 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/ASTMatchers/ASTMatchFinder.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" @@ -27,6 +28,7 @@ using namespace clang; using namespace clang::ento; +using namespace clang::ast_matchers; namespace { @@ -113,6 +115,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. //===----------------------------------------------------------------------===// @@ -207,16 +219,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(), State)) 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, @@ -456,6 +471,61 @@ 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 AssertM = callExpr(callee(functionDecl(hasName("assert")))); + auto GuardM = + stmt(anyOf(ifStmt(), switchStmt(), conditionalOperator(), AssertM)) + .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"); + + auto Guards = match(stmt(hasDescendant(GuardM)), *MethodBody, AC); + if (Guards.empty()) + return true; + const auto *FirstGuard = Guards[0].getNodeAs("guard"); + + 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 @@ -495,4 +565,6 @@ ChOpts.IgnoredRecordsWithFieldPattern = AnOpts.getOptionAsString("IgnoreRecordsWithField", /*DefaultVal*/ "", Chk); + ChOpts.IgnoreGuardedFields = + AnOpts.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,397 @@ +// 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; + } + + // 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); +}