[PATCH] D124244: [analyzer] add StoreToImmutable and ModelConstQualifiedReturn checkers

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

Overall this seems to be a promising checker; I added a few minor remarks as inline comments. Unfortunately @steakhal's comment that

> The `check::PostCall` handler wants to mark some memory region immutable. Currently, the checker creates a new //symbolic// memregion, spawned into the //immutable// memory space. After this it simply re-binds the return value.
> However, only `eval::Call` handler is supposed (**must**) to bind the return value, so the current implementation cannot land.

highlights question that is as far as I see a blocking obstacle. I hope that you can find a solution for that technical difficulty.

Moreover, I agree with @martong's comment that you should probably create two commits for these two checkers. (One-way dependency between the checkers is not an obstacle: simply pay attention to merging the commits in the right order.)



================
Comment at: clang/lib/StaticAnalyzer/Checkers/StoreToImmutableChecker.cpp:68-72
+  if (const auto *BP = dyn_cast<BinaryOperator>(S))
+    if (const auto *UP = dyn_cast<UnaryOperator>(BP->getLHS())) {
+      bugreporter::trackExpressionValue(
+          Report->getErrorNode(), UP->getSubExpr()->IgnoreImpCasts(), *Report);
+    }
----------------
This code is a bit hard to understand, perhaps add a comment like
`// If the binding statement looks like "*ptr = ...", then track the pointer "ptr"`


================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/ModelConstQualifiedReturnChecker.cpp:39
+  // SEI CERT ENV30-C
+  const CallDescriptionMap<HandlerFn> ConstQualifiedReturnFunctions = {
+      {{"getenv", 1},
----------------
If you don't have concrete plans for extending this checker to cases where you need a different HandlerFn, then I strongly suggest replacing this map with a set (or a CallDescriptionMap<bool> where the bool is just a dummy placeholder). There is no reason to juggle member function pointers when they are always pointing to the same function.


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

https://reviews.llvm.org/D124244