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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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);
+    }};
----------------
balazske wrote:
> 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?
Given that we want it in the CERT module, we should try to ensure it follows the rule as closely as we can. I went and checked what the C++ rules say about this and... it was interesting to notice that SIG30-C is not one of the C rules included by reference in C++ (https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=88046336).

It's not clear to me that this rule was accidentally tagged as `not-for-cpp` or not, so I'd say it's fine to ignore lambdas for the moment but we may have some follow-up work if CERT changes the rule to be included in C++. My recommendation is: make the check a C-only check for now, document it as such, and I'll ping the folks at CERT to see if this rule was mistagged or not. WDYT?


================
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);
----------------
balazske wrote:
> 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.
Let's punt on this until we hear back from CERT on whether this rule should be supported in C++.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449