[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:31-33
+  // This check does not work with function calls in std namespace.
+  if (!FD->isGlobal() || FD->isInStdNamespace())
+    return false;
----------------
baloghadamsoftware wrote:
> Why? In //C++// we have everything in `std` namespace, such as `std::signal()`, `std::abort()` or `std::_Exit()`. In `C++` the general rule is to use them instead of the global //C// variants.
This seems incorrect to me. `::std::quick_exit()` resolves to `::quick_exit()`, so why should that call be ignored as a system call? I would assume that anything in namespace `std` is a system call. It's a bit questionable whether a user-written template specialization in namespace `std` should be handled that way, but I think that's still reasonable to consider as a system call.


================
Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:34
+    return false;
+  // It is assumed that the function has no other re-declaration that is not
+  // in a system header. Otherwise this may produce wrong result.
----------------
re-declaration -> redeclaration


================
Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:41
+static bool isAllowedSystemCall(const FunctionDecl *FD) {
+  if (!FD->getIdentifier())
+    return true;
----------------
A function without an identifier is not a system call, so I would have expected this to return `false` based on the function name.


================
Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:43
+    return true;
+  const StringRef N = FD->getName();
+  if (N == AbortFun || N == ExitFun || N == QuickExitFun || N == SignalFun)
----------------
We don't typically use top-level `const` in the project, so this can be dropped. Same comment applies elsewhere in the patch.


================
Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:44-46
+  if (N == AbortFun || N == ExitFun || N == QuickExitFun || N == SignalFun)
+    return true;
+  return false;
----------------
baloghadamsoftware wrote:
> Maybe you could use `IdentifierInfo` instead of string comparisons.
The logic isn't quite correct here as this will claim to be an allowed system call:
```
namespace awesome {
  void quick_exit(void); // Considers this to be a system call
}
```
You should be checking the namespace as well.

Also, this list is very incomplete depending on your platform. The CERT rule lists a whole bunch of POSIX functions that are required to be async signal safe. I am guessing Microsoft likely has a list somewhere as well. I worry about the number of false positives this check will issue if we don't at least consider POSIX (where signals are much, much more useful than in strictly conforming C).


================
Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:73
+  const auto IsSignalFunction =
+      callee(functionDecl(hasName(SignalFun), parameterCountIs(2)));
+  const auto HandlerAsSecondArg = hasArgument(
----------------
Similar issue here with finding functions in the wrong namespace.


================
Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:117-118
+      diag(FunctionCall->getBeginLoc(),
+           "'%0' is considered as non asynchronous-safe and "
+           "should not be called from a signal handler")
+          << FunctionToCheck->getName();
----------------
How about: `'%0' may not be asynchronous-safe; calling it from a signal handler may be dangerous`?


================
Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:135
+        [&CalledFunctions](const FunctionDecl *FD, const CallExpr *CE) {
+          CalledFunctions.push_back(std::make_pair(FD, CE));
+        }};
----------------
`emplace_back(FD, CE)`?


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/cert-sig30-c.rst:13
+(for ``signal`` there are additional conditions that are not checked).
+Every other system call is considered as non asynchronous-safe by the checker.
+
----------------
I would document this as: `Any function that cannot be determined to be an asynchronous-safe function call is assumed to be non-asynchronous-safe by the checker, including function calls for which only the declaration of the called function is visible.`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449