[PATCH] D69318: [analyzer] Add SufficientSizeArrayIndexingChecker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

I suspect that with this check by choosing path-sensitivity you gain less than you lose. That is, checking how the array pointer is passed around may be nice, but if you sacrificed that, you could have turned this check into a compiler warning which would have had an effect on much larger //audience//.



================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:622
+
 } // end: "alpha.cplusplus"
 
----------------
What makes this checker C++-specific? All your tests are in plain C.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SufficientSizeArrayIndexingChecker.cpp:93
+  const auto *BaseSubRegion = dyn_cast<SubRegion>(BaseMemRegion);
+  if (!BaseSubRegion)
+    return;
----------------
This is always true (we should probably tell `SVal.getAsRegion()` to return a `const SubRegion *`).


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SufficientSizeArrayIndexingChecker.cpp:100
+  // array indexing situation.
+  if (BaseMemRegion == SuperMemRegion)
+    return;
----------------
This never happens. The chain of superregions never has loops; it is always finite.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SufficientSizeArrayIndexingChecker.cpp:103
+  const auto *SuperSubRegion = dyn_cast<SubRegion>(SuperMemRegion);
+  // The checker has to access the extent of both the sub and the superregion.
+  if (!SuperSubRegion)
----------------
This deserves comments on what kinds of regions do you expect to see here. Do i understand correctly that you expect `BaseMemRegion` to be an `ElementRegion` and its superregion to be the whole array? 'Cause the former is super unobvious and most likely //shouldn't// be that way.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/SufficientSizeArrayIndexingChecker.cpp:153-155
+  auto R = std::make_unique<PathSensitiveBugReport>(BT, BT.getDescription(), N);
+  R->addRange(Index->getSourceRange());
+  C.emitReport(std::move(R));
----------------
I recommend also adding `trackExpressionValue()` for the array expression. In your "symbolic index handling" tests it would hopefully* allow you to see the execution path within `f()` as part of the report, which is essential.

__
*If it doesn't work out of the box, it's most likely a bug in `trackExpressionValue()`. But generally such tracking needs to be implemented in order to move the checker out of alpha, because path-sensitive reports are worthless when you don't see visitor notes on all the interesting events on the path.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69318/new/

https://reviews.llvm.org/D69318