[analyzer][UninitializedObjectChecker] New flag to ignore guarded uninitialized fields

This patch is an implementation of the ideas discussed on the mailing list[1].

The idea is to somewhat heuristically guess whether the field that was confirmed
to be uninitialized is actually guarded with ifs, asserts, switch/cases and so
on. Since this is a syntactic check, it is very much prone to drastically
reduce the amount of reports the checker emits. The reports however that do not
get filtered out though have greater likelihood of them manifesting into actual
runtime errors.

[1] http://lists.llvm.org/pipermail/cfe-dev/2018-September/059255.html

Differential Revision: https://reviews.llvm.org/D51866


git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@352959 91177308-0d34-0410-b5e6-96231b3b80d8

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 @@ struct UninitObjCheckerOptions {
   bool ShouldConvertNotesToWarnings = false;
   bool CheckPointeeInitialization = false;
   std::string IgnoredRecordsWithFieldPattern;
+  bool IgnoreGuardedFields = false;
 };
 
 /// A lightweight polymorphic wrapper around FieldRegion *. We'll use this

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 @@ static bool willObjectBeAnalyzedLater(const CXXConstructorDecl *Ctor,
 /// \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 @@ bool FindUninitializedFields::addFieldToUninits(FieldChainInfo Chain,
          "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<AnalyzedRegions>(FR))
     return false;
 
@@ -246,13 +265,10 @@ bool FindUninitializedFields::addFieldToUninits(FieldChainInfo Chain,
 
   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;
 }
 
@@ -441,8 +457,8 @@ static const TypedValueRegion *
 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 @@ static bool shouldIgnoreRecord(const RecordDecl *RD, StringRef Pattern) {
   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
@@ -528,12 +613,16 @@ void ento::registerUninitializedObjectChecker(CheckerManager &Mgr) {
   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) {

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);
+}