balazske created this revision.
Herald added subscribers: cfe-commits, martong, gamesh411, Szelethus, dkrupp,
xazax.hun, whisperity.
Herald added a project: clang.
balazske requested review of this revision.
Check for namespaces and lambda signal handler was added.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D91164
Files:
clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h
clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-signal-handler/stdcpp.h
clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.cpp
@@ -0,0 +1,142 @@
+// RUN: %check_clang_tidy -std=c++14 %s bugprone-signal-handler %t -- -- -isystem %S/Inputs/Headers -isystem %S/Inputs/bugprone-signal-handler
+
+#include "stdcpp.h"
+#include "stdio.h"
+
+// Function called "signal" that is not to be recognized by the checker.
+typedef void (*callback_t)(int);
+void signal(int, callback_t, int);
+
+namespace ns {
+void signal(int, callback_t);
+}
+
+struct S {
+ static void signal(int, callback_t);
+ static void handler(int);
+
+ S() = default;
+ 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]
+ };
+ 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]
+ }
+};
+
+struct TestMemberInit {
+ 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]
+ }
+ 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]
+}
+
+void testDefaultInit(int I = defaultInit()) {
+}
+
+void S::handler(int) {
+ std::other_call();
+}
+
+extern "C" {
+
+void handler_abort(int) {
+ std::abort();
+}
+
+void handler__Exit(int) {
+ std::_Exit(0);
+}
+
+void handler_quick_exit(int) {
+ std::quick_exit(0);
+}
+
+void handler_signal(int) {
+ // FIXME: It is only OK to call signal with the current signal number.
+ std::signal(0, SIG_DFL);
+ std::signal(0, SIG_IGN);
+}
+
+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]
+}
+
+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]
+}
+
+void handler_bad3(int) {
+ S S1(0);
+}
+
+void handler_bad4(int) {
+ S S1;
+ -S1;
+}
+
+void handler_bad5(int) {
+ TestMemberInit MI;
+}
+
+void handler_bad6(int) {
+ testDefaultInit();
+}
+
+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]
+}
+}
+
+void handler_noexternc(int) {
+ std::other_call();
+}
+
+void test() {
+ 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);
+
+ std::signal(SIGINT, handler_noexternc);
+ // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: function 'handler_noexternc' should have C language linkage if used as signal handler [bugprone-signal-handler]
+ // make a ExprWithCleanups
+ std::signal(-S(), handler_noexternc);
+ // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: function 'handler_noexternc' should have C language linkage if used as signal handler [bugprone-signal-handler]
+ std::signal(SIGINT, S::handler);
+ // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: function 'handler' should have C language linkage if used as signal handler [bugprone-signal-handler]
+ std::signal(SIGINT, [](int) { std::other_call(); });
+ // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: lambda expression is not allowed as signal handler [bugprone-signal-handler]
+ std::signal(SIGINT, [](int) -> callback_t { return &handler_bad1; }(1));
+
+ // Do not find problems here.
+ signal(SIGINT, handler_abort, 1);
+ ns::signal(SIGINT, handler_abort);
+ S::signal(SIGINT, handler_abort);
+ system_other::signal(SIGINT, handler_abort);
+}
Index: clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-signal-handler/stdcpp.h
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-signal-handler/stdcpp.h
@@ -0,0 +1,38 @@
+//===--- Header for test bugprone-signal-handler.cpp ------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#define SIGINT 1
+#define SIG_IGN std::_sig_ign
+#define SIG_DFL std::_sig_dfl
+
+namespace std {
+
+void _sig_ign(int);
+void _sig_dfl(int);
+
+typedef void (*sighandler_t)(int);
+sighandler_t signal(int, sighandler_t);
+
+void abort();
+void _Exit(int);
+void quick_exit(int);
+
+void other_call();
+
+struct SysStruct {
+ void operator<<(int);
+};
+
+} // namespace std
+
+namespace system_other {
+
+typedef void (*sighandler_t)(int);
+sighandler_t signal(int, sighandler_t);
+
+} // namespace system_other
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
@@ -33,6 +33,10 @@
private:
void reportBug(const FunctionDecl *CalledFunction, const Expr *CallOrRef,
const CallExpr *SignalCall, const FunctionDecl *HandlerDecl);
+ void reportNonExternCBug(const Expr *HandlerRef,
+ const FunctionDecl *HandlerDecl);
+ void reportLambdaBug(const Expr *HandlerRef);
+
bool isSystemCallAllowed(const FunctionDecl *FD) const;
AsyncSafeFunctionSetType AsyncSafeFunctionSet;
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
@@ -10,12 +10,10 @@
#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/STLExtras.h"
#include "llvm/ADT/SmallVector.h"
-#include <iterator>
-#include <queue>
+#include <deque>
using namespace clang::ast_matchers;
@@ -73,6 +71,14 @@
FD->getCanonicalDecl()->getLocation());
}
+static bool isInNoNamespace(const FunctionDecl *FD) {
+ const DeclContext *DC = FD->getDeclContext();
+ while (DC && DC->isTransparentContext())
+ DC = DC->getParent();
+ assert(DC && "Function should have a declaration context.");
+ return DC->isTranslationUnit();
+}
+
AST_MATCHER(FunctionDecl, isSystemCall) { return isSystemCall(&Node); }
SignalHandlerCheck::SignalHandlerCheck(StringRef Name,
@@ -91,8 +97,8 @@
bool SignalHandlerCheck::isLanguageVersionSupported(
const LangOptions &LangOpts) const {
- // FIXME: Make the checker useful on C++ code.
- if (LangOpts.CPlusPlus)
+ // FIXME: Improve C++ support.
+ if (LangOpts.CPlusPlus17)
return false;
return true;
@@ -106,18 +112,33 @@
unless(isExpandedFromMacro("SIG_IGN")),
unless(isExpandedFromMacro("SIG_DFL")))
.bind("handler_expr");
+ auto HandlerLambda = cxxMemberCallExpr(
+ on(expr(ignoringParenImpCasts(lambdaExpr().bind("lambda")))));
Finder->addMatcher(
callExpr(callee(SignalFunction), hasArgument(1, HandlerExpr))
.bind("register_call"),
this);
+ Finder->addMatcher(
+ callExpr(callee(SignalFunction), hasArgument(1, HandlerLambda)), this);
}
void SignalHandlerCheck::check(const MatchFinder::MatchResult &Result) {
+ const auto *LambdaHandler = Result.Nodes.getNodeAs<LambdaExpr>("lambda");
+ if (LambdaHandler) {
+ reportLambdaBug(LambdaHandler);
+ return;
+ }
+
const auto *SignalCall = Result.Nodes.getNodeAs<CallExpr>("register_call");
const auto *HandlerDecl =
Result.Nodes.getNodeAs<FunctionDecl>("handler_decl");
const auto *HandlerExpr = Result.Nodes.getNodeAs<DeclRefExpr>("handler_expr");
+ if (!HandlerDecl->isExternC()) {
+ reportNonExternCBug(HandlerExpr, HandlerDecl);
+ return;
+ }
+
// Visit each function encountered in the callgraph only once.
llvm::DenseSet<const FunctionDecl *> SeenFunctions;
@@ -180,11 +201,13 @@
bool SignalHandlerCheck::isSystemCallAllowed(const FunctionDecl *FD) const {
const IdentifierInfo *II = FD->getIdentifier();
- // Unnamed functions are not explicitly allowed.
+ // Can not verify if std operators are safe to call.
if (!II)
return false;
- // FIXME: Improve for C++ (check for namespace).
+ if (!FD->isInStdNamespace() && !isInNoNamespace(FD))
+ return false;
+
if (ConformingFunctions.count(II->getName()))
return true;
@@ -205,6 +228,20 @@
DiagnosticIDs::Note);
}
+void SignalHandlerCheck::reportNonExternCBug(const Expr *HandlerRef,
+ const FunctionDecl *HandlerDecl) {
+ diag(HandlerRef->getBeginLoc(),
+ "function %0 should have C language linkage if used as signal handler")
+ << HandlerDecl;
+ diag(HandlerDecl->getBeginLoc(), "handler function declared here",
+ DiagnosticIDs::Note);
+}
+
+void SignalHandlerCheck::reportLambdaBug(const Expr *HandlerRef) {
+ diag(HandlerRef->getBeginLoc(),
+ "lambda expression is not allowed as signal handler");
+}
+
// 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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
