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

[Endre Fülöp via Phabricator via cfe-commits]

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 at -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