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 "clang/Analysis/CallGraph.h" #include "llvm/ADT/StringSet.h" namespace clang { @@ -31,9 +32,12 @@ void check(const ast_matchers::MatchFinder::MatchResult &Result) override; private: + bool isFunctionAsyncSafe(const FunctionDecl *FD) const; + bool isSystemCallAsyncSafe(const FunctionDecl *FD) const; void reportBug(const FunctionDecl *CalledFunction, const Expr *CallOrRef, const CallExpr *SignalCall, const FunctionDecl *HandlerDecl); - bool isSystemCallAllowed(const FunctionDecl *FD) const; + + CallGraph CG; AsyncSafeFunctionSetType AsyncSafeFunctionSet; llvm::StringSet<> &ConformingFunctions; 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 @@ -7,15 +7,9 @@ //===----------------------------------------------------------------------===// #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/DepthFirstIterator.h" #include "llvm/ADT/STLExtras.h" -#include "llvm/ADT/SmallVector.h" -#include -#include using namespace clang::ast_matchers; @@ -42,7 +36,9 @@ namespace bugprone { -static bool isSystemCall(const FunctionDecl *FD) { +namespace { + +bool isSystemCall(const FunctionDecl *FD) { // Find a possible redeclaration in system header. // FIXME: Looking at the canonical declaration is not the most exact way // to do this. @@ -73,6 +69,22 @@ FD->getCanonicalDecl()->getLocation()); } +/// Given a call graph node of a function and another one that is called from +/// this function, get a CallExpr of the corresponding function call. +/// It is unspecified which call is found if multiple calls exist, but the order +/// should be deterministic (depend only on the AST). +Expr *findCallExpr(const CallGraphNode *Caller, const CallGraphNode *Callee) { + auto FoundCallee = llvm::find_if( + Caller->callees(), [Callee](const CallGraphNode::CallRecord &Call) { + return Call.Callee == Callee; + }); + assert(FoundCallee != Caller->end() && + "Callee should be called from the caller function here."); + return FoundCallee->CallExpr; +} + +} // namespace + AST_MATCHER(FunctionDecl, isSystemCall) { return isSystemCall(&Node); } SignalHandlerCheck::SignalHandlerCheck(StringRef Name, @@ -117,68 +129,50 @@ const auto *HandlerDecl = Result.Nodes.getNodeAs("handler_decl"); const auto *HandlerExpr = Result.Nodes.getNodeAs("handler_expr"); + assert(SignalCall && HandlerDecl && HandlerExpr && + "All of these should exist in a match here."); + + if (CG.size() <= 1) { + // Call graph must be populated with the entire TU at the begin. + // (It is possible to add a single function but the functions called from it + // are not analysed in this case.) + CG.addToCallGraph(const_cast( + HandlerDecl->getTranslationUnitDecl())); + assert(CG.size() > 1 && + "There should be at least one function added to call graph."); + } - // 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; - } - - // Get the body of the encountered non-system call function. - const FunctionDecl *FBody; - if (!F->hasBody(FBody)) { - reportBug(F, CallOrRef, SignalCall, HandlerDecl); - return false; - } - - // 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); - } - - return true; - }; - - if (!ProcessFunction(HandlerDecl, HandlerExpr)) + // Check for special case when the signal handler itself is an unsafe external + // function. + if (!isFunctionAsyncSafe(HandlerDecl)) { + reportBug(HandlerDecl, HandlerExpr, SignalCall, HandlerDecl); 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; + CallGraphNode *HandlerNode = CG.getNode(HandlerDecl); + // Signal handler can be external but not unsafe, no call graph in this case. + if (!HandlerNode) + return; + // Start from signal handler and visit every function call. + for (auto Itr = llvm::df_begin(HandlerNode), ItrE = llvm::df_end(HandlerNode); + Itr != ItrE; ++Itr) { + const auto *CallF = dyn_cast((*Itr)->getDecl()); + if (CallF && !isFunctionAsyncSafe(CallF)) { + assert(Itr.getPathLength() >= 2); + reportBug(CallF, findCallExpr(Itr.getPath(Itr.getPathLength() - 2), *Itr), + SignalCall, HandlerDecl); + } } } -bool SignalHandlerCheck::isSystemCallAllowed(const FunctionDecl *FD) const { +bool SignalHandlerCheck::isFunctionAsyncSafe(const FunctionDecl *FD) const { + if (isSystemCall(FD)) + return isSystemCallAsyncSafe(FD); + // For external (not checkable) functions assume that these are unsafe. + return FD->hasBody(); +} + +bool SignalHandlerCheck::isSystemCallAsyncSafe(const FunctionDecl *FD) const { const IdentifierInfo *II = FD->getIdentifier(); // Unnamed functions are not explicitly allowed. if (!II) 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 @@ -51,6 +51,37 @@ // 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] } +void test_false_condition(int) { + if (0) + printf("1234"); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler] +} + +void test_multiple_calls(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] + 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] + f_extern(); + // first 'f_extern' call found only +} + +void f_recursive(); + +void test_recursive(int) { + f_recursive(); + printf(""); + // first 'printf' call (in other function) found only +} + +void f_recursive() { + 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] + printf(""); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler] + f_recursive(2); +} + void test() { signal(SIGINT, handler_abort); signal(SIGINT, handler_signal); @@ -66,4 +97,8 @@ signal(SIGINT, SIG_IGN); signal(SIGINT, SIG_DFL); + + signal(SIGINT, test_false_condition); + signal(SIGINT, test_multiple_calls); + signal(SIGINT, test_recursive); }