steakhal requested changes to this revision.
steakhal added a comment.
This revision now requires changes to proceed.
Thanks for the ping. I have some concerns inline.
================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:1000-1008
+ CheckerOptions<[
+ CmdLineOption<Boolean,
+ "InvalidatingGetEnv",
+ "Regard getenv as an invalidating call (as per POSIX "
+ "standard), which can lead to false positives depending on "
+ "implementation.",
+ "false",
----------------
I think we should mention this flag in the docs, and an example.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:115
+ auto PlaceInvalidationNote = [&C, FunctionName,
+ &State](const MemRegion *Region,
+ StringRef Message, ExplodedNode *Pred)
{
----------------
I'd prefer an explicit out parameter instead of capturing `&State` here.
This way we only capture "immutable" stuff.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:124
+ PathSensitiveBugReport &BR, llvm::raw_ostream &Out) {
+ if (!BR.isInteresting(Region))
+ return;
----------------
To me, it feels like all the messages we emit from this NoteTag, are specific
to this particular checker.
if that's true, checking interestingness is not enough, and we should also make
sure that the BugType is from this checker.
Otherwise, this note could appear for any other reason when the region is
marked as interesting.
I also have the feeling that it should mark uninteresting the region once it
puts a message there - which should stop other notes placed for the same reason
for other - basically unrelated env invalidations.
Could you verify this with a test?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:138-141
+ for (const MemRegion *EnvPtr : State->get<GetenvEnvPtrRegions>())
+ CurrentChainEnd = PlaceInvalidationNote(
+ EnvPtr, "call may invalidate the environment returned by getenv",
+ CurrentChainEnd);
----------------
I'd prefer if we wouldn't put N separate NoteTags, but rather iterate over this
set of regions inside the NoteTag.
That way here you don't need the loop and play with Pred nodes.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:219-225
+ if (GetEnvCall.matches(Call)) {
+ const MemRegion *Region = Call.getReturnValue().getAsRegion();
+ if (Region) {
+ State = State->add<GetenvEnvPtrRegions>(Region);
+ C.addTransition(State);
+ }
+ }
----------------
================
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))
----------------
gamesh411 wrote:
> steakhal wrote:
> > `FunctionName` and `Message` will dangle inside the NoteTag.
> Good catch, thanks! Fixed this with a lambda capture initializer.
On second thought, I'm wrong. It won't dangle, because the
StringRef(FunctionName) is owned by the identifier of the function, and thus
lives as long as the ASTContext.
But `Message` would dangle :D
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D154603/new/
https://reviews.llvm.org/D154603
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
