Szelethus updated this revision to Diff 177080.
Szelethus added a comment.
This revision is now accepted and ready to land.
- Rebased to master.
- Adding functions calls with the `noreturn` attribute to the guards.
- Shamelessly stole all sorts of assert-like function names from
`NoReturnFunctionChecker`.
- Added new test cases accordingly.
This should be it! Sorry for being so slow with this -- I genuinely struggled a
lot with macros before ultimately giving up.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D51866/new/
https://reviews.llvm.org/D51866
Files:
lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h
lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
test/Analysis/cxx-uninitialized-object-unguarded-access.cpp
Index: test/Analysis/cxx-uninitialized-object-unguarded-access.cpp
===================================================================
--- /dev/null
+++ 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);
+}
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;
/// We'll mark fields (and pointee of fields) that are confirmed to be
/// uninitialized as already analyzed.
@@ -119,6 +121,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.
//===----------------------------------------------------------------------===//
@@ -235,6 +247,13 @@
"One must also pass the pointee region as a parameter for "
"dereferencable fields!");
+ if (State->getStateManager().getContext().getSourceManager().isInSystemHeader(
+ FR->getDecl()->getLocation()))
+ return false;
+
+ if (Opts.IgnoreGuardedFields && !hasUnguardedAccess(FR->getDecl(), State))
+ return false;
+
if (State->contains<AnalyzedRegions>(FR))
return false;
@@ -247,13 +266,10 @@
State = State->add<AnalyzedRegions>(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;
}
@@ -442,8 +458,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);
@@ -496,6 +512,75 @@
return false;
}
+static const Stmt *getMethodBody(const CXXMethodDecl *M) {
+ if (isa<CXXConstructorDecl>(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<CXXRecordDecl>(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<MemberExpr>("access");
+ assert(FirstAccess);
+
+ auto Guards = match(stmt(hasDescendant(GuardM)), *MethodBody, AC);
+ if (Guards.empty())
+ return true;
+ const auto *FirstGuard = Guards[0].getNodeAs<Stmt>("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
@@ -529,10 +614,14 @@
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);
}
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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
