[PATCH] D97699: [analyzer] Add InvalidPtrChecker

[Balázs Kéri via Phabricator via cfe-commits]

balazske added a comment.

I have not too deep knowledge about checker development but still found (hopefully valid) issues, even if only a part of them.



================
Comment at: clang/docs/analyzer/checkers.rst:2117
+        // envp may no longer point to the current environment
+        // this program has unanticipated behavior, since envp
+        // does not reflect changes made by setenv function.
----------------
>From my understanding the "unanticipated behavior" here is clearly a bug: We do not know if the `envp` points to any valid location. It is not guaranteed that the original value is preserved (in this case it would have sense to use it). And it is not known if it will point to the new and correct array. Unless we know how the internal implementation exactly works.


================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:947
+
+} // end "alpha.cert.env"
+
----------------
I have multiple issues with the documentation texts but they look not final anyway. And the package and checker names could be better. There are already checkers for CERT rules that do not reside in the cert package, it is not good to have some checkers in a `cert` package and some not. It is likely that checkers will check for more rules or parts of the rules. Checker aliases are a better solution for the cert checker names. (And now the `cert` would contain checker with name not matching a rule: `InvalidPtr`.) The name `InvalidPtr` sounds too general, even if in an `env˙ package.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:28
+// pointer. These functions include: getenv, localeconv, asctime, setlocale,
+// strerror
+//===----------------------------------------------------------------------===//
----------------
I think we do not need to repeat the documentation here.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:180
+// Note: This pointer has type 'const MemRegion *'
+REGISTER_TRAIT_WITH_PROGRAMSTATE(EnvPtrRegion, const void *)
+
----------------
Is it not possible to use `const MemRegion *` then?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:192
+  return false;
+}
+
----------------
`evalCall` should be used only if the called function is exclusively modeled by this checker which is not the case here. The `checkPostCall` should be sufficient instead of `evalCall`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:227
+  if (const auto *Reg =
+          State->get<PreviousCallResultMap>(FunctionName.data())) {
+    const auto *PrevReg = *Reg;
----------------
`auto` is not to be used if the type is not clearly visible in the context. And this would be exactly `auto **` here. (But better is `const MemRegion **`.) Makes the later statement (assign to `PrevReg`) "messy" (and `auto` is bad there too).


================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:227
+  if (const auto *Reg =
+          State->get<PreviousCallResultMap>(FunctionName.data())) {
+    const auto *PrevReg = *Reg;
----------------
balazske wrote:
> `auto` is not to be used if the type is not clearly visible in the context. And this would be exactly `auto **` here. (But better is `const MemRegion **`.) Makes the later statement (assign to `PrevReg`) "messy" (and `auto` is bad there too).
`.data` is unnecessary here?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:319
+        C.emitReport(std::move(Report));
+        C.addTransition(State);
+      }
----------------
`addTransition` is not needed here, the state was not modified.
Save the error node returned by `generateNonFatalErrorNode` and pass it to the next call of `generateNonFatalErrorNode` as the `Pred` parameter.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:341
+  State = State->set<EnvPtrRegion>(
+      reinterpret_cast<void *>(const_cast<MemRegion *>(EnvpReg)));
+  C.addTransition(State);
----------------
Are these casts really needed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97699