[analyzer][UninitializedObjectChecker] New flag to ignore guarded uni…

…nitialized 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

llvm-svn: 352959

clang/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObject.h

@@ -34,18 +34,25 @@
 //     `-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 "".
 //
 //     `-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

clang/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp

@@ -19,13 +19,15 @@
 
 #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"
 #include "clang/StaticAnalyzer/Core/PathSensitive/DynamicTypeMap.h"
 
 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) {