From b088237f08c4c48be64890a9418ae88532075888 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Bal=C3=A1zs=20K=C3=A9ri?= <1.int32@gmail.com> Date: Mon, 31 Jan 2022 09:47:49 +0100 Subject: [PATCH] [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 --- .../clang-tidy/bugprone/SignalHandlerCheck.cpp | 42 ++++-- .../clang-tidy/bugprone/SignalHandlerCheck.h | 9 +- .../clang-tidy/checkers/bugprone-signal-handler.c | 163 ++++++++++++++++----- 3 files changed, 163 insertions(+), 51 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp index 1fc19a8..983a7c0 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 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(Callee->getDecl()) + << cast(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 9ac77a5..dbc7ac8 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 @@ 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 DirectHandler); + void reportHandlerCommon(llvm::df_iterator 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 ac9b731..16ceacc 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); } -- 2.7.4