[PATCH] D87449: [clang-tidy] Add new check for SEI CERT rule SIG30-C

[Balázs Kéri via Phabricator via cfe-commits]

balazske marked 3 inline comments as done.
balazske added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:117-118
+    FunctionCallCollector Collector{[&CalledFunctions](const CallExpr *CE) {
+      if (isa<FunctionDecl>(CE->getCalleeDecl()))
+        CalledFunctions.push_back(CE);
+    }};
----------------
aaron.ballman wrote:
> For correctness, I think you need to handle more than just calls to function declarations -- for instance, this should be just as problematic:
> ```
> void some_signal_handler(int sig) {
>   []{ puts("this should not be an escape hatch for the check); }();
> }
> ```
> even though the call expression in the signal handler doesn't resolve back to a function declaration. (Similar for blocks instead of lambdas.) WDYT?
I do not know how many other cases could be there. Probably this can be left for future  improvement, the checker is mainly usable for C code then. There is a `clang::CallGraph` functionality that could be used instead of `FunctionCallCollector` but the `CallExpr` for the calls is not provided by it so it does not work for this case. Maybe there is other similar functionality that is usable?


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cert-sig30-c.cpp:52
+void test() {
+  std::signal(SIGINT, handler_abort);
+  std::signal(SIGINT, handler__Exit);
----------------
aaron.ballman wrote:
> I'd also like to see a test case where the handler to `signal` call is itself not a function call:
> ```
> std::signal(SIGINT, [](int sig) {
>   puts("I did the bad thing this way"); // should be diagnosed, yes?
> });
> ```
This is again a new case to handle. A new matcher must be added to detect this. But I am not sure how many other cases are there, or is it worth to handle all of them.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87449/new/

https://reviews.llvm.org/D87449