From e2a8e43160588e12923994e2b44f67b2a7443a95 Mon Sep 17 00:00:00 2001
From: Artem Dergachev <artem.dergachev@gmail.com>
Date: Tue, 23 Apr 2019 02:50:38 +0000
Subject: [PATCH] [analyzer] PR41335: Fix crash when no-store event is in a
 body-farmed function.

Stuffing invalid source locations (such as those in functions produced by
body farms) into path diagnostics causes crashes.

Fix a typo in a nearby function name.

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

llvm-svn: 358945
---
 .../Core/BugReporter/PathDiagnostic.h         |  4 +++-
 .../Core/BugReporterVisitors.cpp              | 13 +++++++-----
 clang/test/Analysis/OSAtomic_mac.c            | 20 +++++++++++++++++++
 3 files changed, 31 insertions(+), 6 deletions(-)
 create mode 100644 clang/test/Analysis/OSAtomic_mac.c

diff --git a/clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h b/clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
index 3c8f85e26e31..547a8ca64346 100644
--- a/clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
+++ b/clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h
@@ -313,6 +313,8 @@ public:
 
   bool hasRange() const { return K == StmtK || K == RangeK || K == DeclK; }
 
+  bool hasValidLocation() const { return asLocation().isValid(); }
+
   void invalidate() {
     *this = PathDiagnosticLocation();
   }
@@ -468,7 +470,7 @@ public:
                           PathDiagnosticPiece::Kind k,
                           bool addPosRange = true)
       : PathDiagnosticPiece(s, k), Pos(pos) {
-    assert(Pos.isValid() && Pos.asLocation().isValid() &&
+    assert(Pos.isValid() && Pos.hasValidLocation() &&
            "PathDiagnosticSpotPiece's must have a valid location.");
     if (addPosRange && Pos.hasRange()) addRange(Pos.asRange());
   }
diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
index 09c0b9d171ac..af92077ef981 100644
--- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -335,7 +335,7 @@ public:
         if (RegionOfInterest->isSubRegionOf(SelfRegion) &&
             potentiallyWritesIntoIvar(Call->getRuntimeDefinition().getDecl(),
                                       IvarR->getDecl()))
-          return maybeEmitNode(R, *Call, N, {}, SelfRegion, "self",
+          return maybeEmitNote(R, *Call, N, {}, SelfRegion, "self",
                                /*FirstIsReferenceType=*/false, 1);
       }
     }
@@ -344,7 +344,7 @@ public:
       const MemRegion *ThisR = CCall->getCXXThisVal().getAsRegion();
       if (RegionOfInterest->isSubRegionOf(ThisR)
           && !CCall->getDecl()->isImplicit())
-        return maybeEmitNode(R, *Call, N, {}, ThisR, "this",
+        return maybeEmitNote(R, *Call, N, {}, ThisR, "this",
                              /*FirstIsReferenceType=*/false, 1);
 
       // Do not generate diagnostics for not modified parameters in
@@ -363,7 +363,7 @@ public:
       QualType T = PVD->getType();
       while (const MemRegion *MR = V.getAsRegion()) {
         if (RegionOfInterest->isSubRegionOf(MR) && !isPointerToConst(T))
-          return maybeEmitNode(R, *Call, N, {}, MR, ParamName,
+          return maybeEmitNote(R, *Call, N, {}, MR, ParamName,
                                ParamIsReferenceType, IndirectionLevel);
 
         QualType PT = T->getPointeeType();
@@ -371,7 +371,7 @@ public:
 
         if (const RecordDecl *RD = PT->getAsRecordDecl())
           if (auto P = findRegionOfInterestInRecord(RD, State, MR))
-            return maybeEmitNode(R, *Call, N, *P, RegionOfInterest, ParamName,
+            return maybeEmitNote(R, *Call, N, *P, RegionOfInterest, ParamName,
                                  ParamIsReferenceType, IndirectionLevel);
 
         V = State->getSVal(MR, PT);
@@ -549,7 +549,7 @@ private:
   /// \return Diagnostics piece for region not modified in the current function,
   /// if it decides to emit one.
   std::shared_ptr<PathDiagnosticPiece>
-  maybeEmitNode(BugReport &R, const CallEvent &Call, const ExplodedNode *N,
+  maybeEmitNote(BugReport &R, const CallEvent &Call, const ExplodedNode *N,
                 const RegionVector &FieldChain, const MemRegion *MatchedRegion,
                 StringRef FirstElement, bool FirstIsReferenceType,
                 unsigned IndirectionLevel) {
@@ -579,6 +579,9 @@ private:
     PathDiagnosticLocation L =
         PathDiagnosticLocation::create(N->getLocation(), SM);
 
+    if (!L.hasValidLocation())
+      return nullptr;
+
     SmallString<256> sbuf;
     llvm::raw_svector_ostream os(sbuf);
     os << "Returning without writing to '";
diff --git a/clang/test/Analysis/OSAtomic_mac.c b/clang/test/Analysis/OSAtomic_mac.c
new file mode 100644
index 000000000000..a69d98078e17
--- /dev/null
+++ b/clang/test/Analysis/OSAtomic_mac.c
@@ -0,0 +1,20 @@
+// RUN: %clang_analyze_cc1 -w -analyzer-checker=core,debug.ExprInspection \
+// RUN:                    -analyzer-output=text -verify %s
+
+int OSAtomicCompareAndSwapPtrBarrier(*, *, **);
+int OSAtomicCompareAndSwapPtrBarrier() {
+  // There is some body in the actual header,
+  // but we should trust our BodyFarm instead.
+}
+
+int *invalidSLocOnRedecl() {
+  int *b; // expected-note{{'b' declared without an initial value}}
+
+  OSAtomicCompareAndSwapPtrBarrier(0, 0, &b); // no-crash
+  // FIXME: We don't really need these notes.
+  // expected-note@-2{{Calling 'OSAtomicCompareAndSwapPtrBarrier'}}
+  // expected-note@-3{{Returning from 'OSAtomicCompareAndSwapPtrBarrier'}}
+
+  return b; // expected-warning{{Undefined or garbage value returned to caller}}
+            // expected-note@-1{{Undefined or garbage value returned to caller}}
+}