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,22 +33,36 @@ void check(const ast_matchers::MatchFinder::MatchResult &Result) override; private: - bool isFunctionAsyncSafe(const FunctionDecl *FD) const; + /// Check if a function is allowed as a signal handler. + /// Should test the properties of the function, and check in the code body. + /// Should not check function calls in the code (this part is done by the call + /// graph scan). + /// @param FD The function to check. It may or may not have a definition. + /// @param CallOrRef Location of the call to this function (in another + /// function) or the reference to the function (if it is used as a registered + /// signal handler). This is the location where diagnostics are to be placed. + /// @return true only if a problem was found in the function. + bool checkFunction(const FunctionDecl *FD, const Expr *CallOrRef); + /// Return if a system call function is considered as asynchronous-safe. bool isSystemCallAsyncSafe(const FunctionDecl *FD) const; - void reportBug(const FunctionDecl *CalledFunction, const Expr *CallOrRef, - bool DirectHandler); - void reportHandlerCommon(llvm::df_iterator Itr, - const CallExpr *SignalCall, - const FunctionDecl *HandlerDecl, - const Expr *HandlerRef); + /// Add bug report notes to show the call chain of functions from a signal + /// handler to an actual called function (from it). + /// @param Itr Position during a call graph depth-first iteration. It contains + /// the "path" (call chain) from the signal handler to the actual found + /// function call. + /// @param HandlerRef Reference to the signal handler function where it is + /// registered (considered as first part of the call chain). + void reportHandlerChain(const llvm::df_iterator &Itr, + const Expr *HandlerRef); clang::CallGraph CG; AsyncSafeFunctionSetType AsyncSafeFunctionSet; - llvm::StringSet<> &ConformingFunctions; + const llvm::StringSet<> &ConformingFunctions; - static llvm::StringSet<> MinimalConformingFunctions; - static llvm::StringSet<> POSIXConformingFunctions; + // FIXME: Find a better place for these string sets (TableGen?). + static const llvm::StringSet<> MinimalConformingFunctions; + static const llvm::StringSet<> POSIXConformingFunctions; }; } // namespace bugprone 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 @@ -142,38 +142,63 @@ "There should be at least one function added to call graph."); } - // Check for special case when the signal handler itself is an unsafe external - // function. - if (!isFunctionAsyncSafe(HandlerDecl)) { - reportBug(HandlerDecl, HandlerExpr, /*DirectHandler=*/true); + if (!HandlerDecl->hasBody()) { + (void)checkFunction(HandlerDecl, HandlerExpr); + // Here checkFunction will put the messages to HandlerExpr. + // No need to show a call chain. + // Without code body there is no more thing to check. return; } - CallGraphNode *HandlerNode = CG.getNode(HandlerDecl); - // Signal handler can be external but not unsafe, no call graph in this case. - if (!HandlerNode) - return; + CallGraphNode *HandlerNode = CG.getNode(HandlerDecl->getCanonicalDecl()); + assert(HandlerNode && + "Handler with body should be present in the call graph."); // 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), - /*DirectHandler=*/false); - reportHandlerCommon(Itr, SignalCall, HandlerDecl, HandlerExpr); + if (CallF) { + unsigned int PathL = Itr.getPathLength(); + const Expr *CallOrRef = (PathL == 1) + ? HandlerExpr + : findCallExpr(Itr.getPath(PathL - 2), *Itr); + if (checkFunction(CallF, CallOrRef)) + reportHandlerChain(Itr, HandlerExpr); } } } -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::checkFunction(const FunctionDecl *FD, + const Expr *CallOrRef) { + const bool FunctionIsCalled = isa(CallOrRef); + + if (isSystemCall(FD)) { + if (!isSystemCallAsyncSafe(FD)) { + diag(CallOrRef->getBeginLoc(), "system call %0 may not be " + "asynchronous-safe; " + "%select{using it as|calling it from}1 " + "a signal handler may be dangerous") + << FD << FunctionIsCalled; + return true; + } + return false; + } + + if (!FD->hasBody()) { + diag(CallOrRef->getBeginLoc(), "can not verify if external function %0 is " + "asynchronous-safe; " + "%select{using it as|calling it from}1 " + "a signal handler may be dangerous") + << FD << FunctionIsCalled; + return true; + } + + return false; } bool SignalHandlerCheck::isSystemCallAsyncSafe(const FunctionDecl *FD) const { + assert(isSystemCall(FD)); + const IdentifierInfo *II = FD->getIdentifier(); // Unnamed functions are not explicitly allowed. if (!II) @@ -186,17 +211,9 @@ return false; } -void SignalHandlerCheck::reportBug(const FunctionDecl *CalledFunction, - const Expr *CallOrRef, bool DirectHandler) { - diag(CallOrRef->getBeginLoc(), - "%0 may not be asynchronous-safe; %select{calling it from|using it as}1 " - "a signal handler may be dangerous") - << CalledFunction << DirectHandler; -} - -void SignalHandlerCheck::reportHandlerCommon( - llvm::df_iterator Itr, const CallExpr *SignalCall, - const FunctionDecl *HandlerDecl, const Expr *HandlerRef) { +void SignalHandlerCheck::reportHandlerChain( + const llvm::df_iterator &Itr, + const Expr *HandlerRef) { int CallLevel = Itr.getPathLength() - 2; assert(CallLevel >= -1 && "Empty iterator?"); @@ -214,12 +231,12 @@ diag(HandlerRef->getBeginLoc(), "function %0 registered here as signal handler", DiagnosticIDs::Note) - << HandlerDecl; + << cast(Caller->getDecl()); } // 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{ +const llvm::StringSet<> SignalHandlerCheck::MinimalConformingFunctions{ "signal", "abort", "_Exit", "quick_exit"}; // The POSIX-defined set of safe functions. @@ -228,7 +245,7 @@ // mentioned POSIX specification was not updated after 'quick_exit' appeared // in the C11 standard. // Also, we want to keep the "minimal set" a subset of the "POSIX set". -llvm::StringSet<> SignalHandlerCheck::POSIXConformingFunctions{ +const llvm::StringSet<> SignalHandlerCheck::POSIXConformingFunctions{ "_Exit", "_exit", "abort", 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 @@ -17,7 +17,7 @@ void handler_printf(int) { printf("1234"); - // CHECK-NOTES: :[[@LINE-1]]:3: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler] + // CHECK-NOTES: :[[@LINE-1]]:3: warning: system call 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler] // CHECK-NOTES: :[[@LINE-2]]:3: note: function 'printf' called here from 'handler_printf' // CHECK-NOTES: :[[@LINE+4]]:18: note: function 'handler_printf' registered here as signal handler } @@ -28,7 +28,7 @@ void handler_extern(int) { f_extern(); - // CHECK-NOTES: :[[@LINE-1]]:3: warning: 'f_extern' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler] + // CHECK-NOTES: :[[@LINE-1]]:3: warning: can not verify if external function 'f_extern' is asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler] // CHECK-NOTES: :[[@LINE-2]]:3: note: function 'f_extern' called here from 'handler_extern' // CHECK-NOTES: :[[@LINE+4]]:18: note: function 'handler_extern' registered here as signal handler } @@ -51,7 +51,7 @@ void f_bad() { printf("1234"); - // CHECK-NOTES: :[[@LINE-1]]:3: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler] + // CHECK-NOTES: :[[@LINE-1]]:3: warning: system call 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler] // CHECK-NOTES: :[[@LINE-2]]:3: note: function 'printf' called here from 'f_bad' // CHECK-NOTES: :[[@LINE+5]]:3: note: function 'f_bad' called here from 'handler_bad' // CHECK-NOTES: :[[@LINE+8]]:18: note: function 'handler_bad' registered here as signal handler @@ -67,7 +67,7 @@ void f_bad1() { printf("1234"); - // CHECK-NOTES: :[[@LINE-1]]:3: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler] + // CHECK-NOTES: :[[@LINE-1]]:3: warning: system call 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler] // CHECK-NOTES: :[[@LINE-2]]:3: note: function 'printf' called here from 'f_bad1' // CHECK-NOTES: :[[@LINE+6]]:3: note: function 'f_bad1' called here from 'f_bad2' // CHECK-NOTES: :[[@LINE+9]]:3: note: function 'f_bad2' called here from 'handler_bad1' @@ -99,7 +99,7 @@ void handler_false_condition(int) { if (0) printf("1234"); - // CHECK-NOTES: :[[@LINE-1]]:5: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler] + // CHECK-NOTES: :[[@LINE-1]]:5: warning: system call 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler] // CHECK-NOTES: :[[@LINE-2]]:5: note: function 'printf' called here from 'handler_false_condition' // CHECK-NOTES: :[[@LINE+4]]:18: note: function 'handler_false_condition' registered here as signal handler } @@ -110,11 +110,11 @@ void handler_multiple_calls(int) { f_extern(); - // CHECK-NOTES: :[[@LINE-1]]:3: warning: 'f_extern' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler] + // CHECK-NOTES: :[[@LINE-1]]:3: warning: can not verify if external function 'f_extern' is asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler] // CHECK-NOTES: :[[@LINE-2]]:3: note: function 'f_extern' called here from 'handler_multiple_calls' // CHECK-NOTES: :[[@LINE+10]]:18: note: function 'handler_multiple_calls' registered here as signal handler printf("1234"); - // CHECK-NOTES: :[[@LINE-1]]:3: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler] + // CHECK-NOTES: :[[@LINE-1]]:3: warning: system call 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler] // CHECK-NOTES: :[[@LINE-2]]:3: note: function 'printf' called here from 'handler_multiple_calls' // CHECK-NOTES: :[[@LINE+6]]:18: note: function 'handler_multiple_calls' registered here as signal handler f_extern(); @@ -135,12 +135,12 @@ void f_recursive() { f_extern(); - // CHECK-NOTES: :[[@LINE-1]]:3: warning: 'f_extern' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler] + // CHECK-NOTES: :[[@LINE-1]]:3: warning: can not verify if external function 'f_extern' is asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler] // CHECK-NOTES: :[[@LINE-2]]:3: note: function 'f_extern' called here from 'f_recursive' // CHECK-NOTES: :[[@LINE-9]]:3: note: function 'f_recursive' called here from 'handler_recursive' // CHECK-NOTES: :[[@LINE+10]]:18: note: function 'handler_recursive' registered here as signal handler printf(""); - // CHECK-NOTES: :[[@LINE-1]]:3: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler] + // CHECK-NOTES: :[[@LINE-1]]:3: warning: system call 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler] // CHECK-NOTES: :[[@LINE-2]]:3: note: function 'printf' called here from 'f_recursive' // CHECK-NOTES: :[[@LINE-14]]:3: note: function 'f_recursive' called here from 'handler_recursive' // CHECK-NOTES: :[[@LINE+5]]:18: note: function 'handler_recursive' registered here as signal handler @@ -153,7 +153,7 @@ void f_multiple_paths() { printf(""); - // CHECK-NOTES: :[[@LINE-1]]:3: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler] + // CHECK-NOTES: :[[@LINE-1]]:3: warning: system call 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler] // CHECK-NOTES: :[[@LINE-2]]:3: note: function 'printf' called here from 'f_multiple_paths' // CHECK-NOTES: :[[@LINE+5]]:3: note: function 'f_multiple_paths' called here from 'handler_multiple_paths' // CHECK-NOTES: :[[@LINE+9]]:18: note: function 'handler_multiple_paths' registered here as signal handler @@ -184,9 +184,9 @@ signal(SIGINT, _Exit); signal(SIGINT, other_call); - // CHECK-NOTES: :[[@LINE-1]]:18: warning: 'other_call' may not be asynchronous-safe; using it as a signal handler may be dangerous [bugprone-signal-handler] + // CHECK-NOTES: :[[@LINE-1]]:18: warning: system call 'other_call' may not be asynchronous-safe; using it as a signal handler may be dangerous [bugprone-signal-handler] signal(SIGINT, f_extern); - // CHECK-NOTES: :[[@LINE-1]]:18: warning: 'f_extern' may not be asynchronous-safe; using it as a signal handler may be dangerous [bugprone-signal-handler] + // CHECK-NOTES: :[[@LINE-1]]:18: warning: can not verify if external function 'f_extern' is asynchronous-safe; using it as a signal handler may be dangerous [bugprone-signal-handler] signal(SIGINT, SIG_IGN); signal(SIGINT, SIG_DFL);