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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
