bruntib updated this revision to Diff 288606.
bruntib added a comment.
Herald added a subscriber: mgehre.
I rebased the patch so it compiles with master version of LLVM/Clang. I did no
other change, so I would like if this patch would be committed on behalf of
@NorenaLeonetti if the patch is acceptable.
I would kindly ask the reviewers to give some comments if any additional
modification is needed. I run this checker on DuckDB project and this checker
gave two reports on functions which shouldn't be used as signal handler:
duckdb-0.2.0/third_party/sqlsmith/sqlsmith.cc:38:17: do not use C++ constructs
in signal handlers [cert-msc54-cpp]
https://github.com/cwida/duckdb/blob/master/third_party/sqlsmith/sqlsmith.cc#L38
duckdb-0.2.0/tools/shell/shell.c:8828:13: signal handlers must be 'extern "C"'
[cert-msc54-cpp]
https://github.com/cwida/duckdb/blob/master/tools/shell/shell.c#L8828
I haven't found other C++ projects which use signals. However, this checker
reports on the non-compliant code fragments of the corresponding SEI-CERT rule:
https://wiki.sei.cmu.edu/confluence/display/cplusplus/MSC54-CPP.+A+signal+handler+must+be+a+plain+old+function.
The two findings above also seem to be true positive.
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,133 @@
+// 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: do not use C++ constructs in signal handlers [cert-msc54-cpp]
+ // CHECK-MESSAGES: :[[@LINE+1]]:3: note: C++ construct 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: C++ construct 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+1]]:3: note: function called here
+ 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++ representations 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,35 @@
+//===--- SignalHandlerMustBePlainOldFunctionCheck.h - clang-tidy-*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#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,166 @@
+//===--- SignalHandlerMustBePlainOldFunctionCheck.cpp - clang-tidy---------===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#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"));
+}
+
+class CallGraphCheck {
+ std::queue<const FunctionDecl *> UncheckedCalls;
+ llvm::DenseSet<const FunctionDecl *> CheckedCalls;
+ SourceLocation CxxRepresentation;
+ SourceLocation FunctionCall;
+ ASTContext *Context;
+
+ bool hasCxxRepr(const FunctionDecl *Func) {
+ auto MatchesCxxStmt = match(hasCxxStmt(), *Func, *Context);
+ if (!MatchesCxxStmt.empty()) {
+ CxxRepresentation =
+ MatchesCxxStmt[0].getNodeAs<Stmt>("cxx_stmt")->getBeginLoc();
+ return true;
+ }
+
+ auto MatchesCxxDecl = match(hasCxxDecl(), *Func, *Context);
+ if (!MatchesCxxDecl.empty()) {
+ CxxRepresentation =
+ MatchesCxxDecl[0].getNodeAs<Decl>("cxx_decl")->getBeginLoc();
+ 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; }
+
+ 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(),
+ "do not use C++ constructs in signal handlers");
+ if (const auto *CxxStmt = Result.Nodes.getNodeAs<Stmt>("cxx_stmt"))
+ diag(CxxStmt->getBeginLoc(), "C++ construct used here",
+ DiagnosticIDs::Note);
+ else
+ diag(Result.Nodes.getNodeAs<Decl>("cxx_decl")->getBeginLoc(),
+ "C++ construct used here", 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(), "C++ construct used here",
+ 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
