balazske created this revision.
Herald added subscribers: martong, gamesh411, Szelethus, dkrupp, xazax.hun,
whisperity.
balazske requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
Add a new feature to display the call chain if an unsafe function is
found in a signal handler or function called from it.
Warning messages are improved too.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D97960
Files:
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-minimal.c
clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler-posix.c
clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c
clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.cpp
+++ 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);
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c
+++ 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);
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler-posix.c
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler-posix.c
+++ 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) {
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler-minimal.c
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler-minimal.c
+++ 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) {
Index: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h
+++ 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<const FunctionDecl *,
+ std::pair<const Expr *, const FunctionDecl *>>;
+
+ 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);
Index: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
@@ -129,103 +129,110 @@
return;
}
- const auto *SignalCall = Result.Nodes.getNodeAs<CallExpr>("register_call");
const auto *HandlerDecl =
- Result.Nodes.getNodeAs<FunctionDecl>("handler_decl");
+ Result.Nodes.getNodeAs<FunctionDecl>("handler_decl")->getCanonicalDecl();
const auto *HandlerExpr = Result.Nodes.getNodeAs<DeclRefExpr>("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<const FunctionDecl *> SeenFunctions;
-
// The worklist of the callgraph visitation algorithm.
- std::deque<const CallExpr *> 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<const Expr *> 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<FunctionDecl>(cast<CallExpr>(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<CallExpr>("call");
- if (isa<FunctionDecl>(CE->getCalleeDecl()))
- CalledFunctions.push_back(CE);
+ if (const auto *CalleeF = dyn_cast<FunctionDecl>(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<FunctionDecl>(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<DeclRefExpr>(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<DeclRefExpr>(Caller.first)) {
+ diag(HandlerRef->getBeginLoc(),
+ "function %0 registered here as signal handler", DiagnosticIDs::Note)
+ << F;
+ break;
+ } else {
+ const auto *Call = cast<CallExpr>(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{
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
