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

[Donát Nagy via Phabricator via cfe-commits]

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