donat.nagy added a comment.
The commit looks good in general, I have a few minor suggestions and a serious
question about the state transition bureaucracy.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:65-66
+ // If set to true, consider getenv calls as invalidating operations on the
+ // environment variable buffer. This is implied in the standard, but can lead
+ // to practical false positives.
+ bool InvalidatingGetEnv = false;
----------------
Nitpick: "practical false positives" sounds strange for me, consider writing
[...but] "in practice does not cause problems (in the commonly used
environments)"
or something similar.
================
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;
+
----------------
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.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:218
+ State->add<GetenvEnvPtrRegions>(Call.getReturnValue().getAsRegion());
+ C.addTransition(State);
+ }
----------------
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).
================
Comment at: clang/test/Analysis/cert/env34-c.c:6
+//
+// TODO: write test cases that follow the pattern:
+// "getenv -> store pointer -> setenv -> use stored pointer"
----------------
gamesh411 wrote:
> This test file is incomplete.
> I would welcome suggestions here as to how to test this.
> Should a new file be created for the config option with different test cases,
> or is this file to be extended?
Personally I'd prefer putting those cases into a separate files, because this
test file is already 340 lines long and it'd be difficult to understand if it
was filled with conditional checks.
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
