gamesh411 marked 8 inline comments as done.
gamesh411 added inline comments.
================
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))
----------------
steakhal wrote:
> `FunctionName` and `Message` will dangle inside the NoteTag.
Good catch, thanks! Fixed this with a lambda capture initializer.
================
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;
+
----------------
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).
================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:218
+ State->add<GetenvEnvPtrRegions>(Call.getReturnValue().getAsRegion());
+ C.addTransition(State);
+ }
----------------
steakhal wrote:
> 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.
(the line number of this comment desync-ed)
I agree, that the addTransition API is easy to misuse, and I would welcome a
more streamlined approach.
I tried to pay attention to "build" the state and the Exploded Graph by always
providing the Exploded Node (second parameter), and this seems fine.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:44
+ // environment variable buffer. This is implied in the standard, but in
+ // practice does not cause problems (in the commonly used environments).
+ bool InvalidatingGetEnv = false;
----------------
Reworded the message here
================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:48
+ // GetEnv can be treated invalidating and non-invalidating as well.
+ const CallDescription GetEnvCall{{"getenv"}, 1};
+
----------------
Hoisted here
================
Comment at: clang/test/Analysis/invalid-ptr-checker.c:51
+
+ *envp;
+ // expected-warning@-1 2 {{dereferencing an invalid pointer}}
----------------
This gives 2 warnings. One for subexpression `envp`, and one for the whole
statement `*envp`. This is the current behaviour ( check
clang/test/Analysis/cert/env31-c.c ), and this patch does not change it.
However, I would like to devise a solution for this in a different patch.
One option would be to make the error of this checker Fatal, so only one would
appear, or refine the checkLocation callback to only consider one of these 2
cases for reporting.
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
