[clang-tidy] Improve C++ support in bugprone-signal-handler.

[balazske]

Summary

Check for namespaces and lambda signal handler was added.

[njames93]

clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp

100–101

What happens differently in c++17 and later?

137

Why can't c++ linkage functions be used a signal handlers.
This is also not sticking to the documentation of this check. Ths documentation just lists that it guards against non asynchronous-safe functions being called from signal handlers.

241–242

Why can't lambdas be used as signal handlers, If they are stateless they can be implicitly converted to function pointers. Or is this something about needing C linkage?

[balazske]

clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp

100–101

The plan is to add later features to the checker that check different restrictions on the signal handler function. These restrictions may differ in C++14 and C++17 (cppreference.com tells about the difference too, and CERT rule MSC54-CPP). I do not want to make the checker valid for C++17 and later restrict it to C++14 again (if C++14 but not C++17 features are added). (It is not exactly clarified which aspects of the restrictions are to be checked in the checker.) In the current state the allowed callable functions seems to be the same for C++11 and later (this is defined by POSIX).

137

The checker documentation will be fixed. For C++14 the "common subset of C and C++" is allowed to be used as signal handler (but not for C++17) and only C linkage. For C++17 it is not clear if C++ linkage is allowed, and it is not clear if calling functions with C++ linkage is allowed from the signal handler.

241–242

I suspect that the problem is the C linkage.

[balazske]

Newer revision: D118996

clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h

public:
   void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   bool isLanguageVersionSupported(const LangOptions &LangOpts) const override;
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 
 private:
   void reportBug(const FunctionDecl *CalledFunction, const Expr *CallOrRef,
                  const CallExpr *SignalCall, const FunctionDecl *HandlerDecl);
+  void reportNonExternCBug(const Expr *HandlerRef,
+                           const FunctionDecl *HandlerDecl);
+  void reportLambdaBug(const Expr *HandlerRef);
+
   bool isSystemCallAllowed(const FunctionDecl *FD) const;
 
   AsyncSafeFunctionSetType AsyncSafeFunctionSet;
   llvm::StringSet<> &ConformingFunctions;
 
   static llvm::StringSet<> MinimalConformingFunctions;
   static llvm::StringSet<> POSIXConformingFunctions;
 };
 
 } // namespace bugprone
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SIGNALHANDLERCHECK_H

clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp

 //===--- SignalHandlerCheck.cpp - clang-tidy ------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "SignalHandlerCheck.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
-#include "clang/Analysis/CallGraph.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
-#include <iterator>
-#include <queue>
+#include <deque>
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 
 template <>
 struct OptionEnumMapping<
static bool isSystemCall(const FunctionDecl *FD) {
   // d.c
   // extern void sysfunc(void); // Canonical declaration, not in system header.
   // #include <sysheader.h>
 
   return FD->getASTContext().getSourceManager().isInSystemHeader(
       FD->getCanonicalDecl()->getLocation());
 }
 
+static bool isInNoNamespace(const FunctionDecl *FD) {
+  const DeclContext *DC = FD->getDeclContext();
+  while (DC && DC->isTransparentContext())
+    DC = DC->getParent();
+  assert(DC && "Function should have a declaration context.");
+  return DC->isTranslationUnit();
+}
+
 AST_MATCHER(FunctionDecl, isSystemCall) { return isSystemCall(&Node); }
 
 SignalHandlerCheck::SignalHandlerCheck(StringRef Name,
                                        ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       AsyncSafeFunctionSet(
           Options.get("AsyncSafeFunctionSet", AsyncSafeFunctionSetType::POSIX)),
       ConformingFunctions(AsyncSafeFunctionSet ==
                                   AsyncSafeFunctionSetType::Minimal
                               ? MinimalConformingFunctions
                               : POSIXConformingFunctions) {}
 
 void SignalHandlerCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "AsyncSafeFunctionSet", AsyncSafeFunctionSet);
 }
 
 bool SignalHandlerCheck::isLanguageVersionSupported(
     const LangOptions &LangOpts) const {
-  // FIXME: Make the checker useful on C++ code.
-  if (LangOpts.CPlusPlus)
+  // FIXME: Improve C++ support.
+  if (LangOpts.CPlusPlus17)
     return false;
 
   return true;
 }
 
 void SignalHandlerCheck::registerMatchers(MatchFinder *Finder) {
   auto SignalFunction = functionDecl(hasAnyName("::signal", "::std::signal"),
                                      parameterCountIs(2), isSystemCall());
   auto HandlerExpr =
       declRefExpr(hasDeclaration(functionDecl().bind("handler_decl")),
                   unless(isExpandedFromMacro("SIG_IGN")),
                   unless(isExpandedFromMacro("SIG_DFL")))
           .bind("handler_expr");
+  auto HandlerLambda = cxxMemberCallExpr(
+      on(expr(ignoringParenImpCasts(lambdaExpr().bind("lambda")))));
   Finder->addMatcher(
       callExpr(callee(SignalFunction), hasArgument(1, HandlerExpr))
           .bind("register_call"),
       this);
+  Finder->addMatcher(
+      callExpr(callee(SignalFunction), hasArgument(1, HandlerLambda)), this);
 }
 
 void SignalHandlerCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *LambdaHandler = Result.Nodes.getNodeAs<LambdaExpr>("lambda");
+  if (LambdaHandler) {
+    reportLambdaBug(LambdaHandler);
+    return;
+  }
+
   const auto *SignalCall = Result.Nodes.getNodeAs<CallExpr>("register_call");
   const auto *HandlerDecl =
       Result.Nodes.getNodeAs<FunctionDecl>("handler_decl");
   const auto *HandlerExpr = Result.Nodes.getNodeAs<DeclRefExpr>("handler_expr");
 
+  if (!HandlerDecl->isExternC()) {
+    reportNonExternCBug(HandlerExpr, HandlerDecl);
+    return;
+  }
+
   // Visit each function encountered in the callgraph only once.
   llvm::DenseSet<const FunctionDecl *> SeenFunctions;
 
   // The worklist of the callgraph visitation algorithm.
   std::deque<const CallExpr *> CalledFunctions;
 
   auto ProcessFunction = [&](const FunctionDecl *F, const Expr *CallOrRef) {
     // Ensure that canonical declaration is used.
while (!CalledFunctions.empty()) {
 
     if (!ProcessFunction(F, FunctionCall))
       break;
   }
 }
 
 bool SignalHandlerCheck::isSystemCallAllowed(const FunctionDecl *FD) const {
   const IdentifierInfo *II = FD->getIdentifier();
-  // Unnamed functions are not explicitly allowed.
+  // Can not verify if std operators are safe to call.
   if (!II)
     return false;
 
-  // FIXME: Improve for C++ (check for namespace).
+  if (!FD->isInStdNamespace() && !isInNoNamespace(FD))
+    return false;
+
   if (ConformingFunctions.count(II->getName()))
     return true;
 
   return false;
 }
 
 void SignalHandlerCheck::reportBug(const FunctionDecl *CalledFunction,
                                    const Expr *CallOrRef,
                                    const CallExpr *SignalCall,
                                    const FunctionDecl *HandlerDecl) {
   diag(CallOrRef->getBeginLoc(),
        "%0 may not be asynchronous-safe; "
        "calling it from a signal handler may be dangerous")
       << CalledFunction;
   diag(SignalCall->getSourceRange().getBegin(),
        "signal handler registered here", DiagnosticIDs::Note);
   diag(HandlerDecl->getBeginLoc(), "handler function declared here",
        DiagnosticIDs::Note);
 }
 
+void SignalHandlerCheck::reportNonExternCBug(const Expr *HandlerRef,
+                                             const FunctionDecl *HandlerDecl) {
+  diag(HandlerRef->getBeginLoc(),
+       "function %0 should have C language linkage if used as signal handler")
+      << HandlerDecl;
+  diag(HandlerDecl->getBeginLoc(), "handler function declared here",
+       DiagnosticIDs::Note);
+}
+
+void SignalHandlerCheck::reportLambdaBug(const Expr *HandlerRef) {
+  diag(HandlerRef->getBeginLoc(),
+       "lambda expression is not allowed as signal handler");
+}
+
 // This is the minimal set of safe functions.
 // https://wiki.sei.cmu.edu/confluence/display/c/SIG30-C.+Call+only+asynchronous-safe+functions+within+signal+handlers
 llvm::StringSet<> SignalHandlerCheck::MinimalConformingFunctions{
     "signal", "abort", "_Exit", "quick_exit"};
 
 // The POSIX-defined set of safe functions.
 // https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_04_03
 // This list is not the same that is shown in the CERT page.

clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-signal-handler/stdcpp.h

+//===--- Header for test bugprone-signal-handler.cpp ------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#define SIGINT 1
+#define SIG_IGN std::_sig_ign
+#define SIG_DFL std::_sig_dfl
+
+namespace std {
+
+void _sig_ign(int);
+void _sig_dfl(int);
+
+typedef void (*sighandler_t)(int);
+sighandler_t signal(int, sighandler_t);
+
+void abort();
+void _Exit(int);
+void quick_exit(int);
+
+void other_call();
+
+struct SysStruct {
+  void operator<<(int);
+};
+
+} // namespace std
+
+namespace system_other {
+
+typedef void (*sighandler_t)(int);
+sighandler_t signal(int, sighandler_t);
+
+} // namespace system_other

clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.cpp

+// RUN: %check_clang_tidy -std=c++14 %s bugprone-signal-handler %t -- -- -isystem %S/Inputs/Headers -isystem %S/Inputs/bugprone-signal-handler
+
+#include "stdcpp.h"
+#include "stdio.h"
+
+// Function called "signal" that is not to be recognized by the checker.
+typedef void (*callback_t)(int);
+void signal(int, callback_t, int);
+
+namespace ns {
+void signal(int, callback_t);
+}
+
+struct S {
+  static void signal(int, callback_t);
+  static void handler(int);
+
+  S() = default;
+  S(int) {
+    std::other_call();
+    // Should be fixed.
+    // CHECK-MESSAGES-NOT: :[[@LINE-2]]:5: warning: 'other_call' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+  };
+  int operator-() const {
+    std::other_call();
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'other_call' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+  }
+};
+
+struct TestMemberInit {
+  static int memberInit() {
+    std::other_call();
+    // Should be fixed.
+    // CHECK-MESSAGES-NOT: :[[@LINE-2]]:5: warning: 'other_call' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+  }
+  int M = memberInit();
+};
+
+int defaultInit() {
+  std::other_call();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'other_call' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+}
+
+void testDefaultInit(int I = defaultInit()) {
+}
+
+void S::handler(int) {
+  std::other_call();
+}
+
+extern "C" {
+
+void handler_abort(int) {
+  std::abort();
+}
+
+void handler__Exit(int) {
+  std::_Exit(0);
+}
+
+void handler_quick_exit(int) {
+  std::quick_exit(0);
+}
+
+void handler_signal(int) {
+  // FIXME: It is only OK to call signal with the current signal number.
+  std::signal(0, SIG_DFL);
+  std::signal(0, SIG_IGN);
+}
+
+void handler_bad1(int) {
+  std::other_call();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'other_call' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+}
+
+void handler_bad2(int) {
+  std::SysStruct S;
+  S << 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'operator<<' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+}
+
+void handler_bad3(int) {
+  S S1(0);
+}
+
+void handler_bad4(int) {
+  S S1;
+  -S1;
+}
+
+void handler_bad5(int) {
+  TestMemberInit MI;
+}
+
+void handler_bad6(int) {
+  testDefaultInit();
+}
+
+void handler_bad7(int) {
+  int I = []() { std::other_call(); return 2; }();
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 'other_call' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+}
+}
+
+void handler_noexternc(int) {
+  std::other_call();
+}
+
+void test() {
+  std::signal(SIGINT, handler_abort);
+  std::signal(SIGINT, handler__Exit);
+  std::signal(SIGINT, handler_signal);
+  std::signal(SIGINT, handler_bad1);
+  std::signal(SIGINT, handler_bad2);
+  // test call of user-defined operator
+  std::signal(SIGINT, handler_bad3);
+  // test call of constructor
+  std::signal(SIGINT, handler_bad4);
+  // test call of default member initializer
+  std::signal(SIGINT, handler_bad5);
+  // test call of default argument initializer
+  std::signal(SIGINT, handler_bad6);
+  // test lambda call in handler
+  std::signal(SIGINT, handler_bad7);
+
+  std::signal(SIGINT, handler_noexternc);
+  // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: function 'handler_noexternc' should have C language linkage if used as signal handler [bugprone-signal-handler]
+  // make a ExprWithCleanups
+  std::signal(-S(), handler_noexternc);
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: function 'handler_noexternc' should have C language linkage if used as signal handler [bugprone-signal-handler]
+  std::signal(SIGINT, S::handler);
+  // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: function 'handler' should have C language linkage if used as signal handler [bugprone-signal-handler]
+  std::signal(SIGINT, [](int) { std::other_call(); });
+  // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: lambda expression is not allowed as signal handler [bugprone-signal-handler]
+  std::signal(SIGINT, [](int) -> callback_t { return &handler_bad1; }(1));
+
+  // Do not find problems here.
+  signal(SIGINT, handler_abort, 1);
+  ns::signal(SIGINT, handler_abort);
+  S::signal(SIGINT, handler_abort);
+  system_other::signal(SIGINT, handler_abort);
+}