[analyzer] Prevent crashes in FindLastStoreBRVisitor

[george.karpenkov]

Summary

This patch is a band-aid. A proper solution would be too change trackNullOrUndefValue to only try to dereference the pointer when it is relevant to the problem.

[NoQ]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

1742 ↗

(On Diff #166749)

I almost want to make Optional<Loc> non-convertible to bool, because it's almost always a bad idea to make decisions based on Loc-ness of SVal, as a lot of code assumes that Loc means lvalue, which is completely incorrect.

[Szelethus]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

1758 ↗

(On Diff #166749)

I think ShouldFindLastStore would be more descriptive.

1760 ↗

(On Diff #166749)

Does this also work for void **?

clang/test/Analysis/diagnostics/find_last_store.c

1 ↗

(On Diff #166749)

Space after //

[george.karpenkov]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

1760 ↗

(On Diff #166749)

The problem is FindLastStoreBRVisitor will later crash without this branch when attempting to dereference the region.
This problem will not occur with void** because it's OK to dereference that.

[Szelethus]

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

1760 ↗

(On Diff #166749)

Oh right, thanks!

cfe/trunk/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h

public:
   virtual void Profile(llvm::FoldingSetNodeID &ID) const = 0;
 
   /// Generates the default final diagnostic piece.
   static std::shared_ptr<PathDiagnosticPiece>
   getDefaultEndPath(BugReporterContext &BRC, const ExplodedNode *N,
                     BugReport &BR);
 };
 
+/// Finds last store into the given region,
+/// which is different from a given symbolic value.
 class FindLastStoreBRVisitor final : public BugReporterVisitor {
   const MemRegion *R;
   SVal V;
   bool Satisfied = false;
 
   /// If the visitor is tracking the value directly responsible for the
   /// bug, we are going to employ false positive suppression.
   bool EnableNullFPSuppression;

cfe/trunk/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

if (DS) {
                                      "Captured by block as ";
       if (VR) {
         // See if we can get the BlockVarRegion.
         ProgramStateRef State = StoreSite->getState();
         SVal V = StoreSite->getSVal(S);
         if (const auto *BDR =
               dyn_cast_or_null<BlockDataRegion>(V.getAsRegion())) {
           if (const VarRegion *OriginalR = BDR->getOriginalRegion(VR)) {
-            if (Optional<KnownSVal> KV =
-                State->getSVal(OriginalR).getAs<KnownSVal>())
+            if (auto KV = State->getSVal(OriginalR).getAs<KnownSVal>())
               BR.addVisitor(llvm::make_unique<FindLastStoreBRVisitor>(
                   *KV, OriginalR, EnableNullFPSuppression));
           }
         }
       }
     }
     if (action)
       showBRDiagnostics(action, os, R, V, DS);
if (auto L = V.getAs<loc::MemRegionVal>()) {
     // However, if the rvalue is a symbolic region, we should track it as well.
     // Try to use the correct type when looking up the value.
     SVal RVal;
     if (const auto *E = dyn_cast<Expr>(S))
       RVal = state->getRawSVal(L.getValue(), E->getType());
     else
       RVal = state->getSVal(L->getRegion());
 
+    // FIXME: this is a hack for fixing a later crash when attempting to
+    // dereference a void* pointer.
+    // We should not try to dereference pointers at all when we don't care
+    // what is written inside the pointer.
+    bool ShouldFindLastStore = true;
+    if (const auto *SR = dyn_cast<SymbolicRegion>(L->getRegion()))
+      if (SR->getSymbol()->getType()->getPointeeType()->isVoidType())
+        ShouldFindLastStore = false;
+
+    if (ShouldFindLastStore)
       if (auto KV = RVal.getAs<KnownSVal>())
         report.addVisitor(llvm::make_unique<FindLastStoreBRVisitor>(
             *KV, L->getRegion(), EnableNullFPSuppression));
 
     const MemRegion *RegionRVal = RVal.getAsRegion();
     if (RegionRVal && isa<SymbolicRegion>(RegionRVal)) {
       report.markInteresting(RegionRVal);
       report.addVisitor(llvm::make_unique<TrackConstraintBRVisitor>(
             loc::MemRegionVal(RegionRVal), false));
     }

cfe/trunk/test/Analysis/diagnostics/find_last_store.c

+// RUN: %clang_analyze_cc1 -analyzer-checker=core -analyzer-output=text -verify %s
+typedef struct { float b; } c;
+void *a();
+void *d() {
+  return a(); // expected-note{{Returning pointer}}
+}
+
+void no_find_last_store() {
+  c *e = d(); // expected-note{{Calling 'd'}}
+              // expected-note@-1{{Returning from 'd'}}
+              // expected-note@-2{{'e' initialized here}}
+
+  (void)(e || e->b); // expected-note{{Assuming 'e' is null}}
+      // expected-note@-1{{Left side of '||' is false}}
+      // expected-note@-2{{Access to field 'b' results in a dereference of a null pointer (loaded from variable 'e')}}
+      // expected-warning@-3{{Access to field 'b' results in a dereference of a null pointer (loaded from variable 'e')}}
+}