[PATCH] D154603: [analyzer][clangsa] Add new option to alpha.security.cert.InvalidPtrChecker
Donát Nagy via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Aug 23 07:09:05 PDT 2023
donat.nagy added a comment.
The change looks promising, I only have minor remarks.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:125
- const NoteTag *Note =
- C.getNoteTag([SymbolicEnvPtrRegion, FunctionName](
- PathSensitiveBugReport &BR, llvm::raw_ostream &Out) {
- if (!BR.isInteresting(SymbolicEnvPtrRegion))
- return;
- Out << '\'' << FunctionName
- << "' call may invalidate the environment parameter of 'main'";
- });
+ ExplodedNode *CurrentChainEnd = nullptr;
+
----------------
gamesh411 wrote:
> steakhal wrote:
> > donat.nagy wrote:
> > > Perhaps add a comment that clarifies that passing a `nullptr` as the ExplodedNode to `addTransition` is equivalent to specifying the current node. I remember this because I was studying its implementation recently, but I would've been surprised and suspicious otherwise.
> > If `nullptr` is equivalent with `C.getPredecessor()` inside `addTransition()`, why not simply initialize it to that value instead of to `nullptr`?
> I ended up using C.getPredecessor() instead of explaining; this seems a bit more intuitive (if such a thing even exists in CSA).
> if such a thing even exists in CSA
😆
================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:121-122
+ const NoteTag *Note =
+ C.getNoteTag([Region, FunctionName = SmallString<64>{FunctionName},
+ Message = SmallString<256>{Message}](
+ PathSensitiveBugReport &BR, llvm::raw_ostream &Out) {
----------------
Minor nitpick: in situations like this, when we want to save an already composed string, `std::string` is better than `SmallString` because it doesn't occupy more memory than the actual length of the string. (OTOH `SmallString` is better for buffer variables, I've seen code that creates a SmallString, composes the message in it, then converts it to a `std::string` for longer-term storage.)
Of course these are all just inconsequential micro-optimization details...
================
Comment at: clang/test/Analysis/invalid-ptr-checker.c:10
+// RUN: alpha.security.cert.env.InvalidPtr:InvalidatingGetEnv=true \
+// RUN: -analyzer-output=text -verify=pedantic -Wno-unused %s
+
----------------
Use `-verify=expected,pedantic` here and then you can eliminate the `pedantic-warning` lines that duplicate the messages of `expected-warning`s.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D154603/new/
https://reviews.llvm.org/D154603
More information about the cfe-commits
mailing list