Szelethus created this revision.
Szelethus added reviewers: george.karpenkov, NoQ, xazax.hun, rnkovacs.
Herald added subscribers: cfe-commits, mikhail.ramalho, a.sidorin, szepet,
whisperity.
This patch is an implementation of the ideas discussed on the mailing list
<http://lists.llvm.org/pipermail/cfe-dev/2018-September/059255.html>. It didn't
cause any crashes on the already existing test (I checked it by adding the flag
to them and run lit), but then again, I wasn't able to check it on larger code
bases just yet, and the LLVM codebase tends to have a couple tricks up it's
sleeve, so be beware of that.
Repository:
rC Clang
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,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);
+}
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<UnguardedAccessVerifier> {
+
+ const FieldDecl *UninitField;
+ bool HadAssertCall = false;
+ bool HadUnguardedAccess = false;
+
+public:
+ UnguardedAccessVerifier(const FieldDecl *FD, const CXXMethodDecl *M)
+ : UninitField(FD) {
+
+ if (isa<CXXConstructorDecl>(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<FieldDecl>(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<CXXRecordDecl>(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: 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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
