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

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

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