[analyzer] PR43551: Do not dereferce void* in UndefOrNullArgVisitor

[Szelethus]

Summary

Exactly what it says on the tin!

[NoQ]

Thanks!!

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

2037–2040

Aha, so you're basically propagating this FIXME instead of addressing it. When it was originally added, i vaguely recall that the pointer that we were trying to dereference did not really need to be tracked to begin with. I'm really curious if that's still the case in this example.

clang/test/Analysis/novoidtypecrash.c

1–9

So does the warning get actually emitted, or is it marked as invalid? Maybe it's worth it to add -verify (and maybe even -analyzer-output=text) and assess how good the report actually is.

[NoQ]

I think it's worth it to commit the patch as-is, because the crash seems to be fairly popular.

[NoQ]

Decided to commit myself in order to hurry things up^^
Also rC375328.

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

bool bugreporter::trackExpressionValue(const ExplodedNode *InputNode,
   // track the constraints on its contents.
   SVal V = LVState->getSValAsScalarOrLoc(Inner, LVNode->getLocationContext());
 
   ReturnVisitor::addVisitorIfNecessary(
     LVNode, Inner, report, EnableNullFPSuppression, TKind);
 
   // Is it a symbolic value?
   if (auto L = V.getAs<loc::MemRegionVal>()) {
-    report.addVisitor(std::make_unique<UndefOrNullArgVisitor>(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 CanDereference = true;
     if (const auto *SR = L->getRegionAs<SymbolicRegion>()) {
       if (SR->getSymbol()->getType()->getPointeeType()->isVoidType())
         CanDereference = false;
     } else if (L->getRegionAs<AllocaRegion>())
       CanDereference = false;
 
     // At this point we are dealing with the region's LValue.
     // 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 (ExplodedGraph::isInterestingLValueExpr(Inner))
       RVal = LVState->getRawSVal(L.getValue(), Inner->getType());
     else if (CanDereference)
       RVal = LVState->getSVal(L->getRegion());
 
-    if (CanDereference)
+    if (CanDereference) {
+      report.addVisitor(
+          std::make_unique<UndefOrNullArgVisitor>(L->getRegion()));
+
       if (auto KV = RVal.getAs<KnownSVal>())
         report.addVisitor(std::make_unique<FindLastStoreBRVisitor>(
             *KV, L->getRegion(), EnableNullFPSuppression, TKind, SFC));
+    }
 
     const MemRegion *RegionRVal = RVal.getAsRegion();
     if (RegionRVal && isa<SymbolicRegion>(RegionRVal)) {
       report.markInteresting(RegionRVal, TKind);
       report.addVisitor(std::make_unique<TrackConstraintBRVisitor>(
             loc::MemRegionVal(RegionRVal), /*assumption=*/false));
     }
   }

clang/test/Analysis/novoidtypecrash.c

 // RUN: %clang_analyze_cc1 -analyzer-checker=core %s
+x;
+y(void **z) { // no-crash
+  *z = x;
+  int *w;
+  y(&w);
+  *w;
+}
+
 a;
-b(void **c) { // no-crash
-  *c = a;
-  int *d;
-  b(&d);
-  *d;
+b(*c) {}
+e(*c) {
+  void *d = f();
+  b(d);
+  *c = d;
+}
+void *g() {
+  e(&a);
+  return a;
+}
+j() {
+  int h;
+  char i = g();
+  if (i)
+    for (; h;)
+      ;
 }