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 @@ -10,6 +10,7 @@ #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SIGNALHANDLERCHECK_H #include "../ClangTidyCheck.h" +#include "llvm/ADT/DenseMap.h" #include "llvm/ADT/StringSet.h" namespace clang { @@ -31,8 +32,14 @@ void check(const ast_matchers::MatchFinder::MatchResult &Result) override; private: - void reportBug(const FunctionDecl *CalledFunction, const Expr *CallOrRef, - const CallExpr *SignalCall, const FunctionDecl *HandlerDecl); + using ParentMap = + llvm::DenseMap>; + + bool checkCallAllowed(const FunctionDecl *F, const Expr *CallOrRef, + const ParentMap &ParentOfCall); + void addCallStackNotes(const FunctionDecl *CheckedFunction, + const ParentMap &ParentOfCall); void reportNonExternCBug(const Expr *HandlerRef, const FunctionDecl *HandlerDecl); void reportLambdaBug(const Expr *HandlerRef); 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 @@ -129,103 +129,110 @@ return; } - const auto *SignalCall = Result.Nodes.getNodeAs("register_call"); const auto *HandlerDecl = - Result.Nodes.getNodeAs("handler_decl"); + Result.Nodes.getNodeAs("handler_decl")->getCanonicalDecl(); const auto *HandlerExpr = Result.Nodes.getNodeAs("handler_expr"); + assert(HandlerDecl && HandlerExpr && + "Handler function and reference to it should both be found."); if (!HandlerDecl->isExternC()) { reportNonExternCBug(HandlerExpr, HandlerDecl); return; } - // Visit each function encountered in the callgraph only once. - llvm::DenseSet SeenFunctions; - // The worklist of the callgraph visitation algorithm. - std::deque CalledFunctions; - - auto ProcessFunction = [&](const FunctionDecl *F, const Expr *CallOrRef) { - // Ensure that canonical declaration is used. - F = F->getCanonicalDecl(); - - // Do not visit function if already encountered. - if (!SeenFunctions.insert(F).second) - return true; - - // Check if the call is allowed. - // Non-system calls are not considered. - if (isSystemCall(F)) { - if (isSystemCallAllowed(F)) - return true; - - reportBug(F, CallOrRef, SignalCall, HandlerDecl); - - return false; - } + std::deque CallsToCheck; + // Visit each function encountered in the callgraph only once. + // Store its caller expression and the function where it appears. + ParentMap ParentOfCall; + + // For the real signal handler we have no parent call but a reference to it + // (in the call to 'signal'). + ParentOfCall[HandlerDecl] = std::make_pair(HandlerExpr, nullptr); + CallsToCheck.push_back(HandlerExpr); + + while (!CallsToCheck.empty()) { + const Expr *CallOrRef = CallsToCheck.front(); + CallsToCheck.pop_front(); + const FunctionDecl *F = + (CallOrRef == HandlerExpr) + ? HandlerDecl + : cast(cast(CallOrRef)->getCalleeDecl()) + ->getCanonicalDecl(); + + if (!checkCallAllowed(F, CallOrRef, ParentOfCall)) + continue; - // Get the body of the encountered non-system call function. const FunctionDecl *FBody; - if (!F->hasBody(FBody)) { - reportBug(F, CallOrRef, SignalCall, HandlerDecl); - return false; - } + if (!F->hasBody(FBody)) + continue; // Collect all called functions. auto Matches = match(decl(forEachDescendant(callExpr().bind("call"))), *FBody, FBody->getASTContext()); for (const auto &Match : Matches) { const auto *CE = Match.getNodeAs("call"); - if (isa(CE->getCalleeDecl())) - CalledFunctions.push_back(CE); + if (const auto *CalleeF = dyn_cast(CE->getCalleeDecl())) { + CalleeF = CalleeF->getCanonicalDecl(); + if (!ParentOfCall.count(CalleeF)) { + ParentOfCall[CalleeF] = std::make_pair(CE, F); + CallsToCheck.push_back(CE); + } + } } - - return true; - }; - - if (!ProcessFunction(HandlerDecl, HandlerExpr)) - return; - - // Visit the definition of every function referenced by the handler function. - // Check for allowed function calls. - while (!CalledFunctions.empty()) { - const CallExpr *FunctionCall = CalledFunctions.front(); - CalledFunctions.pop_front(); - // At insertion we have already ensured that only function calls are there. - const auto *F = cast(FunctionCall->getCalleeDecl()); - - if (!ProcessFunction(F, FunctionCall)) - break; } } -bool SignalHandlerCheck::isSystemCallAllowed(const FunctionDecl *FD) const { - const IdentifierInfo *II = FD->getIdentifier(); - // Can not verify if std operators are safe to call. - if (!II) - return false; - - if (!FD->isInStdNamespace() && !isInNoNamespace(FD)) - return false; +bool SignalHandlerCheck::checkCallAllowed(const FunctionDecl *F, + const Expr *CallOrRef, + const ParentMap &ParentOfCall) { + StringRef FunctionKindStr; + if (isSystemCall(F)) { + if (isSystemCallAllowed(F)) + return true; + FunctionKindStr = "system call"; + } else { + if (F->hasBody()) + return true; + FunctionKindStr = "function"; + } - if (ConformingFunctions.count(II->getName())) - return true; + if (isa(CallOrRef)) { + // Function F appears directly in 'signal' call. + diag(CallOrRef->getBeginLoc(), "%0 %1 may not be asynchronous-safe; using " + "it as signal handler may be dangerous") + << FunctionKindStr << F; + } else { + // Function F is called from another function. + diag(CallOrRef->getBeginLoc(), + "%0 %1 may not be asynchronous-safe; calling it from a signal handler " + "may be dangerous") + << FunctionKindStr << F; + addCallStackNotes(F, ParentOfCall); + } 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::addCallStackNotes(const FunctionDecl *F, + const ParentMap &ParentOfCall) { + auto I = ParentOfCall.find(F); + while (I != ParentOfCall.end()) { + auto Caller = I->second; + if (const auto *HandlerRef = dyn_cast(Caller.first)) { + diag(HandlerRef->getBeginLoc(), + "function %0 registered here as signal handler", DiagnosticIDs::Note) + << F; + break; + } else { + const auto *Call = cast(Caller.first); + diag(Call->getBeginLoc(), "function %0 called here from %1", + DiagnosticIDs::Note) + << F << Caller.second; + } + F = Caller.second; + I = ParentOfCall.find(F); + } } void SignalHandlerCheck::reportNonExternCBug(const Expr *HandlerRef, @@ -242,6 +249,21 @@ "lambda expression is not allowed as signal handler"); } +bool SignalHandlerCheck::isSystemCallAllowed(const FunctionDecl *FD) const { + const IdentifierInfo *II = FD->getIdentifier(); + // Can not verify if std operators are safe to call. + if (!II) + return false; + + if (!FD->isInStdNamespace() && !isInNoNamespace(FD)) + return false; + + if (ConformingFunctions.count(II->getName())) + return true; + + return false; +} + // 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/bugprone-signal-handler-minimal.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler-minimal.c --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler-minimal.c +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler-minimal.c @@ -10,12 +10,12 @@ void handler_bad1(int) { _exit(0); - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: '_exit' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler] + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: system call '_exit' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler] } void handler_bad2(void *dst, const void *src) { memcpy(dst, src, 10); - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'memcpy' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler] + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: system call 'memcpy' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler] } void handler_good(int) { diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler-posix.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler-posix.c --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler-posix.c +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler-posix.c @@ -11,7 +11,7 @@ void handler_bad(int) { printf("1234"); - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler] + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: system call 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler] } void handler_good(int) { diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c @@ -19,7 +19,7 @@ void handler_other(int) { printf("1234"); - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler] + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: system call 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler] } void handler_signal(int) { @@ -33,7 +33,7 @@ void f_bad() { printf("1234"); - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler] + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: system call 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler] } void f_extern(); @@ -48,7 +48,7 @@ void handler_extern(int) { f_extern(); - // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'f_extern' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler] + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function 'f_extern' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler] } void test() { @@ -62,7 +62,7 @@ signal(SIGINT, _Exit); signal(SIGINT, other_call); - // 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] + // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: system call 'other_call' may not be asynchronous-safe; using it as signal handler may be dangerous [bugprone-signal-handler] signal(SIGINT, SIG_IGN); signal(SIGINT, SIG_DFL); 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 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.cpp @@ -19,11 +19,11 @@ 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] + // CHECK-MESSAGES-NOT: :[[@LINE-2]]:5: warning: system call '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] + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: system call 'other_call' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler] } }; @@ -31,14 +31,14 @@ 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] + // CHECK-MESSAGES-NOT: :[[@LINE-2]]:5: warning: system call '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] + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: system call 'other_call' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler] } void testDefaultInit(int I = defaultInit()) { @@ -70,13 +70,13 @@ 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] + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: system call '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] + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: system call 'operator<<' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler] } void handler_bad3(int) { @@ -98,7 +98,7 @@ 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] + // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: system call 'other_call' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler] } } @@ -110,16 +110,23 @@ 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);