[analyzer] NoStoreFuncVisitor: Suppress reports with no-store in syst…

…em headers.

The idea behind this heuristic is that normally the visitor is there to
inform the user that a certain function may fail to initialize a certain
out-parameter. For system header functions this is usually dictated by the
contract, and it's unlikely that the header function has accidentally
forgot to put the value into the out-parameter; it's more likely
that the user has intentionally skipped the error check.

Warnings on skipped error checks are more like security warnings;
they aren't necessarily useful for all users, and they should instead
be introduced on a per-API basis.

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

llvm-svn: 357810

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

@@ -306,9 +306,14 @@ class NoStoreFuncVisitor final : public BugReporterVisitor {
     ID.AddPointer(RegionOfInterest);
   }
 
+  void *getTag() const {
+    static int Tag = 0;
+    return static_cast<void *>(&Tag);
+  }
+
   std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N,
                                                  BugReporterContext &BR,
-                                                 BugReport &) override {
+                                                 BugReport &R) override {
 
     const LocationContext *Ctx = N->getLocationContext();
     const StackFrameContext *SCtx = Ctx->getStackFrame();
@@ -322,9 +327,6 @@ class NoStoreFuncVisitor final : public BugReporterVisitor {
     CallEventRef<> Call =
         BR.getStateManager().getCallEventManager().getCaller(SCtx, State);
 
-    if (Call->isInSystemHeader())
-      return nullptr;
-
     // Region of interest corresponds to an IVar, exiting a method
     // which could have written into that IVar, but did not.
     if (const auto *MC = dyn_cast<ObjCMethodCall>(Call)) {
@@ -333,17 +335,17 @@ class NoStoreFuncVisitor final : public BugReporterVisitor {
         if (RegionOfInterest->isSubRegionOf(SelfRegion) &&
             potentiallyWritesIntoIvar(Call->getRuntimeDefinition().getDecl(),
                                       IvarR->getDecl()))
-          return notModifiedDiagnostics(N, {}, SelfRegion, "self",
-                                        /*FirstIsReferenceType=*/false, 1);
+          return maybeEmitNode(R, *Call, N, {}, SelfRegion, "self",
+                               /*FirstIsReferenceType=*/false, 1);
       }
     }
 
     if (const auto *CCall = dyn_cast<CXXConstructorCall>(Call)) {
       const MemRegion *ThisR = CCall->getCXXThisVal().getAsRegion();
       if (RegionOfInterest->isSubRegionOf(ThisR)
           && !CCall->getDecl()->isImplicit())
-        return notModifiedDiagnostics(N, {}, ThisR, "this",
-                                      /*FirstIsReferenceType=*/false, 1);
+        return maybeEmitNode(R, *Call, N, {}, ThisR, "this",
+                             /*FirstIsReferenceType=*/false, 1);
 
       // Do not generate diagnostics for not modified parameters in
       // constructors.
@@ -353,27 +355,26 @@ class NoStoreFuncVisitor final : public BugReporterVisitor {
     ArrayRef<ParmVarDecl *> parameters = getCallParameters(Call);
     for (unsigned I = 0; I < Call->getNumArgs() && I < parameters.size(); ++I) {
       const ParmVarDecl *PVD = parameters[I];
-      SVal S = Call->getArgSVal(I);
+      SVal V = Call->getArgSVal(I);
       bool ParamIsReferenceType = PVD->getType()->isReferenceType();
       std::string ParamName = PVD->getNameAsString();
 
       int IndirectionLevel = 1;
       QualType T = PVD->getType();
-      while (const MemRegion *R = S.getAsRegion()) {
-        if (RegionOfInterest->isSubRegionOf(R) && !isPointerToConst(T))
-          return notModifiedDiagnostics(N, {}, R, ParamName,
-                                        ParamIsReferenceType, IndirectionLevel);
+      while (const MemRegion *MR = V.getAsRegion()) {
+        if (RegionOfInterest->isSubRegionOf(MR) && !isPointerToConst(T))
+          return maybeEmitNode(R, *Call, N, {}, MR, ParamName,
+                               ParamIsReferenceType, IndirectionLevel);
 
         QualType PT = T->getPointeeType();
         if (PT.isNull() || PT->isVoidType()) break;
 
         if (const RecordDecl *RD = PT->getAsRecordDecl())
-          if (auto P = findRegionOfInterestInRecord(RD, State, R))
-            return notModifiedDiagnostics(N, *P, RegionOfInterest, ParamName,
-                                          ParamIsReferenceType,
-                                          IndirectionLevel);
+          if (auto P = findRegionOfInterestInRecord(RD, State, MR))
+            return maybeEmitNode(R, *Call, N, *P, RegionOfInterest, ParamName,
+                                 ParamIsReferenceType, IndirectionLevel);
 
-        S = State->getSVal(R, PT);
+        V = State->getSVal(MR, PT);
         T = PT;
         IndirectionLevel++;
       }
@@ -543,11 +544,37 @@ class NoStoreFuncVisitor final : public BugReporterVisitor {
            Ty->getPointeeType().getCanonicalType().isConstQualified();
   }
 
-  /// \return Diagnostics piece for region not modified in the current function.
+  /// Consume the information on the no-store stack frame in order to
+  /// either emit a note or suppress the report enirely.
+  /// \return Diagnostics piece for region not modified in the current function,
+  /// if it decides to emit one.
   std::shared_ptr<PathDiagnosticPiece>
-  notModifiedDiagnostics(const ExplodedNode *N, const RegionVector &FieldChain,
-                         const MemRegion *MatchedRegion, StringRef FirstElement,
-                         bool FirstIsReferenceType, unsigned IndirectionLevel) {
+  maybeEmitNode(BugReport &R, const CallEvent &Call, const ExplodedNode *N,
+                const RegionVector &FieldChain, const MemRegion *MatchedRegion,
+                StringRef FirstElement, bool FirstIsReferenceType,
+                unsigned IndirectionLevel) {
+    // Optimistically suppress uninitialized value bugs that result
+    // from system headers having a chance to initialize the value
+    // but failing to do so. It's too unlikely a system header's fault.
+    // It's much more likely a situation in which the function has a failure
+    // mode that the user decided not to check. If we want to hunt such
+    // omitted checks, we should provide an explicit function-specific note
+    // describing the precondition under which the function isn't supposed to
+    // initialize its out-parameter, and additionally check that such
+    // precondition can actually be fulfilled on the current path.
+    if (Call.isInSystemHeader()) {
+      // We make an exception for system header functions that have no branches,
+      // i.e. have exactly 3 CFG blocks: begin, all its code, end. Such
+      // functions unconditionally fail to initialize the variable.
+      // If they call other functions that have more paths within them,
+      // this suppression would still apply when we visit these inner functions.
+      // One common example of a standard function that doesn't ever initialize
+      // its out parameter is operator placement new; it's up to the follow-up
+      // constructor (if any) to initialize the memory.
+      if (N->getStackFrame()->getCFG()->size() > 3)
+        R.markInvalid(getTag(), nullptr);
+      return nullptr;
+    }
 
     PathDiagnosticLocation L =
         PathDiagnosticLocation::create(N->getLocation(), SM);

clang/test/Analysis/Inputs/no-store-suppression.h

@@ -0,0 +1,17 @@
+#pragma clang system_header
+
+namespace std {
+class istream {
+public:
+  bool is_eof();
+  char get_char();
+};
+
+istream &operator>>(istream &is, char &c) {
+  if (is.is_eof())
+    return;
+  c = is.get_char();
+}
+
+extern istream cin;
+};

clang/test/Analysis/no-store-suppression.cpp

@@ -0,0 +1,22 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+
+// expected-no-diagnostics
+
+#include "Inputs/no-store-suppression.h"
+
+using namespace std;
+
+namespace value_uninitialized_after_stream_shift {
+void use(char c);
+
+// Technically, it is absolutely necessary to check the status of cin after
+// read before using the value that just read from it. Practically, we don't
+// really care unless we eventually come up with a special security check
+// for just that purpose. Static Analyzer shouldn't be yelling at every person's
+// third program in their C++ 101.
+void foo() {
+  char c;
+  std::cin >> c;
+  use(c); // no-warning
+}
+} // namespace value_uninitialized_after_stream_shift