[PATCH] D154603: [analyzer][clangsa] Add new option to alpha.security.cert.InvalidPtrChecker

[Balázs Benics via Phabricator via cfe-commits]

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