steakhal added a comment.
I'm sorry starting the review of this one only now, but I'm quite booked.
Is it still relevant? If so, I'll continue.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:116-117
+ const NoteTag *Note =
+ C.getNoteTag([Region, FunctionName, Message](PathSensitiveBugReport
&BR,
+ llvm::raw_ostream &Out) {
+ if (!BR.isInteresting(Region))
----------------
`FunctionName` and `Message` will dangle inside the NoteTag.
================
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;
+
----------------
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`?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:214
+ // Model 'getenv' calls
+ CallDescription GetEnvCall{{"getenv"}, 1};
+ if (GetEnvCall.matches(Call)) {
----------------
We should hoist this into a field, to only construct it once.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:216-217
+ if (GetEnvCall.matches(Call)) {
+ State =
+ State->add<GetenvEnvPtrRegions>(Call.getReturnValue().getAsRegion());
+ C.addTransition(State);
----------------
What ensures that `Call.getReturnValue().getAsRegion()` is not null?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:218
+ State->add<GetenvEnvPtrRegions>(Call.getReturnValue().getAsRegion());
+ C.addTransition(State);
+ }
----------------
donat.nagy wrote:
> I fear that this state transition will go "sideways" and the later state
> transitions (which add the note tags) will branch off instead of building
> onto this. IIUC calling `CheckerContext::addTransition` registers the
> transition without updating the "current ExplodedNode" field of
> `CheckerContext`, so you need to explicitly store and pass around the
> ExplodedNode returned by it if you want to build on it.
>
> This is an ugly and counter-intuitive API, and I also ran into a very similar
> issue a few weeks ago (@Szelethus helped me).
I think the usage here is correct.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D154603/new/
https://reviews.llvm.org/D154603
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
