diff --git a/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h b/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h --- a/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h +++ b/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h @@ -33,6 +33,10 @@ 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; diff --git a/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp --- a/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp @@ -10,12 +10,10 @@ #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 -#include +#include using namespace clang::ast_matchers; @@ -73,6 +71,14 @@ 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, @@ -91,8 +97,8 @@ 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; @@ -106,18 +112,33 @@ 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("lambda"); + if (LambdaHandler) { + reportLambdaBug(LambdaHandler); + return; + } + const auto *SignalCall = Result.Nodes.getNodeAs("register_call"); const auto *HandlerDecl = Result.Nodes.getNodeAs("handler_decl"); const auto *HandlerExpr = Result.Nodes.getNodeAs("handler_expr"); + if (!HandlerDecl->isExternC()) { + reportNonExternCBug(HandlerExpr, HandlerDecl); + return; + } + // Visit each function encountered in the callgraph only once. llvm::DenseSet SeenFunctions; @@ -180,11 +201,13 @@ 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; @@ -205,6 +228,20 @@ 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{ diff --git a/clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-signal-handler/stdcpp.h b/clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-signal-handler/stdcpp.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-signal-handler/stdcpp.h @@ -0,0 +1,38 @@ +//===--- 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 diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.cpp @@ -0,0 +1,142 @@ +// 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); +}