bruntib updated this revision to Diff 288777.
bruntib added a comment.
Note texts are now describing what kind of C++ constructs were used. The
warning message also explains better the reason of the issue: "signal handlers
must be plain old functions, C++-only constructs are not allowed"
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D33825/new/
https://reviews.llvm.org/D33825
Files:
clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
clang-tools-extra/clang-tidy/cert/CMakeLists.txt
clang-tools-extra/clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp
clang-tools-extra/clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/cert-msc54-cpp.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cert-signal-handler-must-be-plain-old-function.cpp
@@ -0,0 +1,134 @@
+// RUN: %check_clang_tidy %s cert-msc54-cpp %t -- -- -std=c++11
+
+namespace std {
+extern "C" using signalhandler = void(int);
+int signal(int sig, signalhandler *func);
+}
+
+#define SIG_ERR -1
+#define SIGTERM 15
+
+static void sig_handler(int sig) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: signal handlers must be 'extern "C"' [cert-msc54-cpp]
+// CHECK-MESSAGES: :[[@LINE+3]]:39: note: given as a signal parameter here
+
+void install_signal_handler() {
+ if (SIG_ERR == std::signal(SIGTERM, sig_handler))
+ return;
+}
+
+extern "C" void cpp_signal_handler(int sig) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: signal handlers must be plain old functions, C++-only constructs are not allowed [cert-msc54-cpp]
+ // CHECK-MESSAGES: :[[@LINE+1]]:3: note: throw statement used here
+ throw "error message";
+}
+
+void install_cpp_signal_handler() {
+ if (SIG_ERR == std::signal(SIGTERM, cpp_signal_handler))
+ return;
+}
+
+void indirect_recursion();
+
+void cpp_like(){
+ try {
+ cpp_signal_handler(SIG_ERR);
+ } catch(...) {
+ // handle error
+ }
+}
+
+void no_body();
+
+void recursive_function() {
+ indirect_recursion();
+ cpp_like();
+ no_body();
+ recursive_function();
+}
+
+void indirect_recursion() {
+ recursive_function();
+}
+
+extern "C" void signal_handler_with_cpp_function_call(int sig) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: do not call functions with C++ constructs in signal handlers [cert-msc54-cpp]
+ // CHECK-MESSAGES: :[[@LINE-11]]:3: note: function called here
+ // CHECK-MESSAGES: :[[@LINE-23]]:3: note: try statement used here
+ recursive_function();
+}
+
+void install_signal_handler_with_cpp_function_call() {
+ if (SIG_ERR == std::signal(SIGTERM, signal_handler_with_cpp_function_call))
+ return;
+}
+
+class TestClass {
+public:
+ static void static_function() {}
+};
+
+extern "C" void signal_handler_with_cpp_static_method_call(int sig) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: do not call functions with C++ constructs in signal handlers [cert-msc54-cpp]
+ // CHECK-MESSAGES: :[[@LINE+2]]:3: note: function called here
+ // CHECK-MESSAGES: TestClass::static_function();
+ TestClass::static_function();
+}
+
+void install_signal_handler_with_cpp_static_method_call() {
+ if (SIG_ERR == std::signal(SIGTERM, signal_handler_with_cpp_static_method_call))
+ return;
+}
+
+
+// Something that does not trigger the check:
+
+extern "C" void c_sig_handler(int sig) {
+#define cbrt(X) _Generic((X), long double \
+ : cbrtl, default \
+ : cbrt)(X)
+ auto char c = '1';
+ extern _Thread_local double _Complex d;
+ static const unsigned long int i = sizeof(float);
+ void f();
+ volatile struct c_struct {
+ enum e {};
+ union u;
+ };
+ typedef bool boolean;
+label:
+ switch (c) {
+ case ('1'):
+ break;
+ default:
+ d = 1.2;
+ }
+ goto label;
+ for (; i < 42;) {
+ if (d == 1.2)
+ continue;
+ else
+ return;
+ }
+ do {
+ _Atomic int v = _Alignof(char);
+ _Static_assert(42 == 42, "True");
+ } while (c == 42);
+}
+
+void install_c_sig_handler() {
+ if (SIG_ERR == std::signal(SIGTERM, c_sig_handler)) {
+ // Handle error
+ }
+}
+
+extern "C" void signal_handler_with_function_pointer(int sig) {
+ void (*funp) (void);
+ funp = &cpp_like;
+ funp();
+}
+
+void install_signal_handler_with_function_pointer() {
+ if (SIG_ERR == std::signal(SIGTERM, signal_handler_with_function_pointer))
+ return;
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -111,6 +111,7 @@
`cert-mem57-cpp <cert-mem57-cpp.html>`_,
`cert-msc50-cpp <cert-msc50-cpp.html>`_,
`cert-msc51-cpp <cert-msc51-cpp.html>`_,
+ `cert-msc54-cpp <cert-msc54-cpp.html>`_,
`cert-oop57-cpp <cert-oop57-cpp.html>`_,
`cert-oop58-cpp <cert-oop58-cpp.html>`_,
`clang-analyzer-core.DynamicTypePropagation <clang-analyzer-core.DynamicTypePropagation.html>`_,
Index: clang-tools-extra/docs/clang-tidy/checks/cert-msc54-cpp.rst
===================================================================
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/cert-msc54-cpp.rst
@@ -0,0 +1,34 @@
+.. title:: clang-tidy - cert-msc54-cpp
+
+cert-msc54-cpp
+==============
+
+This check will give a warning if a signal handler is not defined
+as an `extern "C"` function or if the declaration of the function
+contains C++ representation.
+
+Here's an example:
+
+ .. code-block:: c++
+
+ static void sig_handler(int sig) {}
+ // warning: use 'external C' prefix for signal handlers
+
+ void install_signal_handler() {
+ if (SIG_ERR == std::signal(SIGTERM, sig_handler))
+ return;
+ }
+
+ extern "C" void cpp_signal_handler(int sig) {
+ // warning: do not use C++ constructs in signal handlers
+ throw "error message";
+ }
+
+ void install_cpp_signal_handler() {
+ if (SIG_ERR == std::signal(SIGTERM, cpp_signal_handler))
+ return;
+ }
+
+This check corresponds to the CERT C++ Coding Standard rule
+`MSC54-CPP. A signal handler must be a plain old function
+<https://www.securecoding.cert.org/confluence/display/cplusplus/MSC54-CPP.+A+signal+handler+must+be+a+plain+old+function>`_.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -76,6 +76,14 @@
Added an option `GetConfigPerFile` to support including files which use
different naming styles.
+New checks
+^^^^^^^^^^
+
+- New `cert-msc54-cpp
+ <http://clang.llvm.org/extra/clang-tidy/checks/cert-msc54-cpp.html>`_ check
+
+ Checks if a signal handler is an 'extern \"C\" function without C++ constructs.
+
Improvements to include-fixer
-----------------------------
Index: clang-tools-extra/clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.h
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.h
@@ -0,0 +1,34 @@
+//===--- CERTTidyModule.cpp - clang-tidy ----------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_SIGNAL_HANDLER_MUST_BE_PLAIN_OLD_FUNCTION_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_SIGNAL_HANDLER_MUST_BE_PLAIN_OLD_FUNCTION_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace cert {
+
+/// A signal handler must be a plain old function.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cert-msc54-cpp.html
+class SignalHandlerMustBePlainOldFunctionCheck : public ClangTidyCheck {
+public:
+ SignalHandlerMustBePlainOldFunctionCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace cert
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_SIGNAL_HANDLER_MUST_BE_PLAIN_OLD_FUNCTION_H
Index: clang-tools-extra/clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/cert/SignalHandlerMustBePlainOldFunctionCheck.cpp
@@ -0,0 +1,234 @@
+//===--- CERTTidyModule.cpp - clang-tidy ----------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#include "SignalHandlerMustBePlainOldFunctionCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include <queue>
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace cert {
+
+namespace {
+internal::Matcher<Decl> hasCxxStmt() {
+ return hasDescendant(
+ stmt(anyOf(cxxBindTemporaryExpr(), cxxBoolLiteral(), cxxCatchStmt(),
+ cxxConstCastExpr(), cxxConstructExpr(), cxxDefaultArgExpr(),
+ cxxDeleteExpr(), cxxDynamicCastExpr(), cxxForRangeStmt(),
+ cxxFunctionalCastExpr(), cxxMemberCallExpr(), cxxNewExpr(),
+ cxxNullPtrLiteralExpr(), cxxOperatorCallExpr(),
+ cxxReinterpretCastExpr(), cxxStaticCastExpr(),
+ cxxTemporaryObjectExpr(), cxxThisExpr(), cxxThrowExpr(),
+ cxxTryStmt(), cxxUnresolvedConstructExpr()))
+ .bind("cxx_stmt"));
+}
+
+internal::Matcher<Decl> hasCxxDecl() {
+ return hasDescendant(
+ decl(anyOf(cxxConstructorDecl(), cxxConversionDecl(), cxxDestructorDecl(),
+ cxxMethodDecl(),
+ cxxRecordDecl(unless(anyOf(isStruct(), isUnion())))))
+ .bind("cxx_decl"));
+}
+
+const char *cxxStmtText(const Stmt *stmt) {
+ if (llvm::isa<CXXBindTemporaryExpr>(stmt))
+ return "Binding temporary C++ expression here";
+ else if (llvm::isa<CXXBoolLiteralExpr>(stmt))
+ return "C++ boolean literal used here";
+ else if (llvm::isa<CXXCatchStmt>(stmt))
+ return "catch statement used here";
+ else if (llvm::isa<CXXConstCastExpr>(stmt))
+ return "const_cast statement used here";
+ else if (llvm::isa<CXXConstructExpr>(stmt))
+ return "Constructor called here";
+ else if (llvm::isa<CXXDefaultArgExpr>(stmt))
+ return "Default function argument used here";
+ else if (llvm::isa<CXXDeleteExpr>(stmt))
+ return "delete statement used here";
+ else if (llvm::isa<CXXDynamicCastExpr>(stmt))
+ return "dynamic_cast statement used here";
+ else if (llvm::isa<CXXForRangeStmt>(stmt))
+ return "for range loop used here";
+ else if (llvm::isa<CXXFunctionalCastExpr>(stmt))
+ return "C++-style cast expression used here";
+ else if (llvm::isa<CXXMemberCallExpr>(stmt))
+ return "Member function called here";
+ else if (llvm::isa<CXXNewExpr>(stmt))
+ return "new statement used here";
+ else if (llvm::isa<CXXNullPtrLiteralExpr>(stmt))
+ return "nullptr used here";
+ else if (llvm::isa<CXXOperatorCallExpr>(stmt))
+ return "C++ operator used here";
+ else if (llvm::isa<CXXReinterpretCastExpr>(stmt))
+ return "reinterpret_cast statement used here";
+ else if (llvm::isa<CXXStaticCastExpr>(stmt))
+ return "static_cast statement used here";
+ else if (llvm::isa<CXXTemporaryObjectExpr>(stmt))
+ return "Temporary object created here";
+ else if (llvm::isa<CXXThisExpr>(stmt))
+ return "\"this\" object used here";
+ else if (llvm::isa<CXXThrowExpr>(stmt))
+ return "throw statement used here";
+ else if (llvm::isa<CXXTryStmt>(stmt))
+ return "try statement used here";
+ else if (llvm::isa<CXXUnresolvedConstructExpr>(stmt))
+ return "Construction of an unresolved type here";
+ else
+ // Shouldn't reach this point.
+ return "C++ construct used here";
+}
+
+const char *cxxDeclText(const Decl *decl) {
+ if (llvm::isa<CXXConstructorDecl>(decl))
+ return "Constructor declared here";
+ else if (llvm::isa<CXXConversionDecl>(decl))
+ return "Conversion operator declared here";
+ else if (llvm::isa<CXXDestructorDecl>(decl))
+ return "Destructor declared here";
+ else if (llvm::isa<CXXMethodDecl>(decl))
+ return "Method declared here";
+ else if (llvm::isa<CXXRecordDecl>(decl))
+ return "C++ record declared here";
+ else
+ // Shouldn't reach this point.
+ return "C++ construct used here";
+}
+
+class CallGraphCheck {
+ std::queue<const FunctionDecl *> UncheckedCalls;
+ llvm::DenseSet<const FunctionDecl *> CheckedCalls;
+ SourceLocation CxxRepresentation;
+ SourceLocation FunctionCall;
+ ASTContext *Context;
+ const char *CxxReprNote;
+
+ bool hasCxxRepr(const FunctionDecl *Func) {
+ auto MatchesCxxStmt = match(hasCxxStmt(), *Func, *Context);
+ if (!MatchesCxxStmt.empty()) {
+ const Stmt *stmt = MatchesCxxStmt[0].getNodeAs<Stmt>("cxx_stmt");
+ CxxRepresentation = stmt->getBeginLoc();
+ CxxReprNote = cxxStmtText(stmt);
+ return true;
+ }
+
+ auto MatchesCxxDecl = match(hasCxxDecl(), *Func, *Context);
+ if (!MatchesCxxDecl.empty()) {
+ const Decl *decl = MatchesCxxDecl[0].getNodeAs<Decl>("cxx_decl");
+ CxxRepresentation = decl->getBeginLoc();
+ CxxReprNote = cxxDeclText(decl);
+ return true;
+ }
+ return false;
+ }
+
+ bool callExprContainsCxxRepr(SmallVectorImpl<BoundNodes> &Calls) {
+ for (const auto &Match : Calls) {
+ const auto *Call = Match.getNodeAs<CallExpr>("call");
+ const FunctionDecl *Func = Call->getDirectCallee();
+ if (!Func || !Func->isDefined())
+ continue;
+ auto MatchesCxxMethod = match(decl(cxxMethodDecl()), *Func, *Context);
+ if (hasCxxRepr(Func->getDefinition()) || !MatchesCxxMethod.empty()) {
+ FunctionCall = Call->getBeginLoc();
+ return true;
+ }
+ if (!CheckedCalls.count(Func))
+ UncheckedCalls.push(Func);
+ }
+ return false;
+ }
+
+public:
+ CallGraphCheck(const FunctionDecl *SignalHandler, ASTContext *ResultContext)
+ : Context(ResultContext) {
+ UncheckedCalls.push(SignalHandler);
+ }
+ const SourceLocation callLoc() { return FunctionCall; }
+ const SourceLocation cxxRepLoc() { return CxxRepresentation; }
+ const char *cxxRepNoteText() { return CxxReprNote; }
+
+ bool checkAllCall() {
+ while (!UncheckedCalls.empty()) {
+ CheckedCalls.insert(UncheckedCalls.front());
+ auto Matches = match(decl(forEachDescendant(callExpr().bind("call"))),
+ *UncheckedCalls.front()->getDefinition(), *Context);
+ UncheckedCalls.pop();
+ if (callExprContainsCxxRepr(Matches))
+ return true;
+ }
+ return false;
+ }
+};
+} // namespace
+
+void SignalHandlerMustBePlainOldFunctionCheck::registerMatchers(
+ MatchFinder *Finder) {
+ auto SignalHas = [](const internal::Matcher<Decl> &SignalHandler) {
+ return callExpr(hasDeclaration(functionDecl(hasName("signal"))),
+ hasArgument(1, declRefExpr(hasDeclaration(SignalHandler))
+ .bind("signal_argument")));
+ };
+ auto SignalHandler = [](const internal::Matcher<Decl> &HasCxxRepr) {
+ return functionDecl(isExternC(), HasCxxRepr)
+ .bind("signal_handler_with_cxx_repr");
+ };
+ Finder->addMatcher(
+ SignalHas(functionDecl(unless(isExternC())).bind("signal_handler")),
+ this);
+ Finder->addMatcher(SignalHas(SignalHandler(hasCxxStmt())), this);
+ Finder->addMatcher(SignalHas(SignalHandler(hasCxxDecl())), this);
+ Finder->addMatcher(SignalHas(functionDecl(hasDescendant(callExpr()))
+ .bind("signal_handler_with_call_expr")),
+ this);
+}
+
+void SignalHandlerMustBePlainOldFunctionCheck::check(
+ const MatchFinder::MatchResult &Result) {
+ if (const auto *SignalHandler =
+ Result.Nodes.getNodeAs<FunctionDecl>("signal_handler")) {
+ diag(SignalHandler->getLocation(),
+ "signal handlers must be 'extern \"C\"'");
+ diag(Result.Nodes.getNodeAs<DeclRefExpr>("signal_argument")->getLocation(),
+ "given as a signal parameter here", DiagnosticIDs::Note);
+ }
+
+ if (const auto *SingalHandler = Result.Nodes.getNodeAs<FunctionDecl>(
+ "signal_handler_with_cxx_repr")) {
+ diag(SingalHandler->getLocation(),
+ "signal handlers must be plain old functions, "
+ "C++-only constructs are not allowed");
+ if (const auto *CxxStmt = Result.Nodes.getNodeAs<Stmt>("cxx_stmt"))
+ diag(CxxStmt->getBeginLoc(), cxxStmtText(CxxStmt), DiagnosticIDs::Note);
+ else {
+ const auto *CxxDecl = Result.Nodes.getNodeAs<Decl>("cxx_decl");
+ diag(CxxDecl->getBeginLoc(), cxxDeclText(CxxDecl), DiagnosticIDs::Note);
+ }
+ }
+
+ if (const auto *SignalHandler = Result.Nodes.getNodeAs<FunctionDecl>(
+ "signal_handler_with_call_expr")) {
+ CallGraphCheck CallChain(SignalHandler, Result.Context);
+ if (CallChain.checkAllCall()) {
+ diag(SignalHandler->getLocation(), "do not call functions with C++ "
+ "constructs in signal "
+ "handlers");
+ diag(CallChain.callLoc(), "function called here", DiagnosticIDs::Note);
+ if (CallChain.cxxRepLoc().isValid())
+ diag(CallChain.cxxRepLoc(), CallChain.cxxRepNoteText(),
+ DiagnosticIDs::Note);
+ }
+ }
+}
+
+} // namespace cert
+} // namespace tidy
+} // namespace clang
Index: clang-tools-extra/clang-tidy/cert/CMakeLists.txt
===================================================================
--- clang-tools-extra/clang-tidy/cert/CMakeLists.txt
+++ clang-tools-extra/clang-tidy/cert/CMakeLists.txt
@@ -15,6 +15,7 @@
PostfixOperatorCheck.cpp
ProperlySeededRandomGeneratorCheck.cpp
SetLongJmpCheck.cpp
+ SignalHandlerMustBePlainOldFunctionCheck.cpp
StaticObjectExceptionCheck.cpp
StrToNumCheck.cpp
ThrownExceptionTypeCheck.cpp
Index: clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
===================================================================
--- clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
+++ clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
@@ -31,6 +31,7 @@
#include "PostfixOperatorCheck.h"
#include "ProperlySeededRandomGeneratorCheck.h"
#include "SetLongJmpCheck.h"
+#include "SignalHandlerMustBePlainOldFunctionCheck.h"
#include "StaticObjectExceptionCheck.h"
#include "StrToNumCheck.h"
#include "ThrownExceptionTypeCheck.h"
@@ -74,6 +75,8 @@
CheckFactories.registerCheck<LimitedRandomnessCheck>("cert-msc50-cpp");
CheckFactories.registerCheck<ProperlySeededRandomGeneratorCheck>(
"cert-msc51-cpp");
+ CheckFactories.registerCheck<SignalHandlerMustBePlainOldFunctionCheck>(
+ "cert-msc54-cpp");
// OOP
CheckFactories.registerCheck<performance::MoveConstructorInitCheck>(
"cert-oop11-cpp");
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
