[clang-tools-extra] b088237 - [clang-tidy] bugprone-signal-handler improvements: display call chain
Balázs Kéri via cfe-commits
cfe-commits at lists.llvm.org
Mon Jan 31 01:36:31 PST 2022
Author: Balázs Kéri
Date: 2022-01-31T10:35:23+01:00
New Revision: b088237f08c4c48be64890a9418ae88532075888
URL: https://github.com/llvm/llvm-project/commit/b088237f08c4c48be64890a9418ae88532075888
DIFF: https://github.com/llvm/llvm-project/commit/b088237f08c4c48be64890a9418ae88532075888.diff
LOG: [clang-tidy] bugprone-signal-handler improvements: display call chain
Display notes for a possible call chain if an unsafe function is found to be
called (maybe indirectly) from a signal handler.
The call chain displayed this way includes probably not the first calls of
the functions, but it is a valid possible (in non path-sensitive way) one.
Reviewed By: aaron.ballman
Differential Revision: https://reviews.llvm.org/D118224
Added:
Modified:
clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h
clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
index 1fc19a8652e3e..983a7c0610d71 100644
--- a/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
@@ -145,7 +145,7 @@ void SignalHandlerCheck::check(const MatchFinder::MatchResult &Result) {
// Check for special case when the signal handler itself is an unsafe external
// function.
if (!isFunctionAsyncSafe(HandlerDecl)) {
- reportBug(HandlerDecl, HandlerExpr, SignalCall, HandlerDecl);
+ reportBug(HandlerDecl, HandlerExpr, /*DirectHandler=*/true);
return;
}
@@ -160,7 +160,8 @@ void SignalHandlerCheck::check(const MatchFinder::MatchResult &Result) {
if (CallF && !isFunctionAsyncSafe(CallF)) {
assert(Itr.getPathLength() >= 2);
reportBug(CallF, findCallExpr(Itr.getPath(Itr.getPathLength() - 2), *Itr),
- SignalCall, HandlerDecl);
+ /*DirectHandler=*/false);
+ reportHandlerCommon(Itr, SignalCall, HandlerDecl, HandlerExpr);
}
}
}
@@ -186,17 +187,34 @@ bool SignalHandlerCheck::isSystemCallAsyncSafe(const FunctionDecl *FD) const {
}
void SignalHandlerCheck::reportBug(const FunctionDecl *CalledFunction,
- const Expr *CallOrRef,
- const CallExpr *SignalCall,
- const FunctionDecl *HandlerDecl) {
+ const Expr *CallOrRef, bool DirectHandler) {
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);
+ "%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<clang::CallGraphNode *> Itr, const CallExpr *SignalCall,
+ const FunctionDecl *HandlerDecl, const Expr *HandlerRef) {
+ int CallLevel = Itr.getPathLength() - 2;
+ assert(CallLevel >= -1 && "Empty iterator?");
+
+ const CallGraphNode *Caller = Itr.getPath(CallLevel + 1), *Callee = nullptr;
+ while (CallLevel >= 0) {
+ Callee = Caller;
+ Caller = Itr.getPath(CallLevel);
+ const Expr *CE = findCallExpr(Caller, Callee);
+ diag(CE->getBeginLoc(), "function %0 called here from %1",
+ DiagnosticIDs::Note)
+ << cast<FunctionDecl>(Callee->getDecl())
+ << cast<FunctionDecl>(Caller->getDecl());
+ --CallLevel;
+ }
+
+ diag(HandlerRef->getBeginLoc(),
+ "function %0 registered here as signal handler", DiagnosticIDs::Note)
+ << HandlerDecl;
}
// This is the minimal set of safe functions.
diff --git a/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h b/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h
index 9ac77a5cb323f..dbc7ac85f2c50 100644
--- a/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h
+++ b/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h
@@ -11,6 +11,7 @@
#include "../ClangTidyCheck.h"
#include "clang/Analysis/CallGraph.h"
+#include "llvm/ADT/DepthFirstIterator.h"
#include "llvm/ADT/StringSet.h"
namespace clang {
@@ -35,9 +36,13 @@ class SignalHandlerCheck : public ClangTidyCheck {
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 DirectHandler);
+ void reportHandlerCommon(llvm::df_iterator<clang::CallGraphNode *> Itr,
+ const CallExpr *SignalCall,
+ const FunctionDecl *HandlerDecl,
+ const Expr *HandlerRef);
- CallGraph CG;
+ clang::CallGraph CG;
AsyncSafeFunctionSetType AsyncSafeFunctionSet;
llvm::StringSet<> &ConformingFunctions;
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
index ac9b731cdc7a4..16ceacc3519d9 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c
@@ -13,62 +13,121 @@ int printf(const char *, ...);
typedef void (*sighandler_t)(int);
sighandler_t signal(int signum, sighandler_t handler);
-void handler_abort(int) {
- abort();
-}
+void f_extern();
-void handler_other(int) {
+void handler_printf(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-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-2]]:3: note: function 'printf' called here from 'handler_printf'
+ // CHECK-NOTES: :[[@LINE+4]]:18: note: function 'handler_printf' registered here as signal handler
}
-void handler_signal(int) {
- // FIXME: It is only OK to call signal with the current signal number.
- signal(0, SIG_DFL);
+void test_printf() {
+ signal(SIGINT, handler_printf);
}
-void f_ok() {
- abort();
+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-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
}
-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]
+void test_extern() {
+ signal(SIGINT, handler_extern);
}
-void f_extern();
+void f_ok() {
+ abort();
+}
void handler_ok(int) {
f_ok();
}
+void test_ok() {
+ signal(SIGINT, handler_ok);
+}
+
+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-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
+}
+
void handler_bad(int) {
f_bad();
}
-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]
+void test_bad() {
+ signal(SIGINT, handler_bad);
+}
+
+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-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'
+ // CHECK-NOTES: :[[@LINE+13]]:18: note: function 'handler_bad1' registered here as signal handler
+}
+
+void f_bad2() {
+ f_bad1();
+}
+
+void handler_bad1(int) {
+ f_bad2();
+ f_bad1();
+}
+
+void test_bad1() {
+ signal(SIGINT, handler_bad1);
}
-void test_false_condition(int) {
+void handler_abort(int) {
+ abort();
+}
+
+void handler_signal(int) {
+ // FIXME: It is only OK to call signal with the current signal number.
+ signal(0, SIG_DFL);
+}
+
+void handler_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]
+ // 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-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
+}
+
+void test_false_condition() {
+ signal(SIGINT, handler_false_condition);
}
-void test_multiple_calls(int) {
+void handler_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]
+ // 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-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-MESSAGES: :[[@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: '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();
// first 'f_extern' call found only
}
+void test_multiple_calls() {
+ signal(SIGINT, handler_multiple_calls);
+}
+
void f_recursive();
-void test_recursive(int) {
+void handler_recursive(int) {
f_recursive();
printf("");
// first 'printf' call (in other function) found only
@@ -76,29 +135,59 @@ void test_recursive(int) {
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]
+ // 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-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-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
+ handler_recursive(2);
+}
+
+void test_recursive() {
+ signal(SIGINT, handler_recursive);
+}
+
+void f_multiple_paths() {
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);
+ // 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-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
}
-void test() {
+void handler_multiple_paths(int) {
+ f_multiple_paths();
+ f_multiple_paths();
+}
+
+void test_multiple_paths() {
+ signal(SIGINT, handler_multiple_paths);
+}
+
+void handler_function_pointer(int) {
+ void (*fp)() = f_extern;
+ // Call with function pointer is not evalauted by the check.
+ (*fp)();
+}
+
+void test_function_pointer() {
+ signal(SIGINT, handler_function_pointer);
+}
+
+void test_other() {
signal(SIGINT, handler_abort);
signal(SIGINT, handler_signal);
- signal(SIGINT, handler_other);
-
- signal(SIGINT, handler_ok);
- signal(SIGINT, handler_bad);
- signal(SIGINT, handler_extern);
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-NOTES: :[[@LINE-1]]:18: warning: '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]
signal(SIGINT, SIG_IGN);
signal(SIGINT, SIG_DFL);
-
- signal(SIGINT, test_false_condition);
- signal(SIGINT, test_multiple_calls);
- signal(SIGINT, test_recursive);
}
More information about the cfe-commits
mailing list