balazske updated this revision to Diff 421103.
balazske added a comment.
Removed a left comment and improved another comment.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D118996/new/
https://reviews.llvm.org/D118996
Files:
clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h
clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/bugprone-signal-handler.rst
clang-tools-extra/docs/clang-tidy/checks/cert-msc54-cpp.rst
clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h
clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-signal-handler/stdcpp.h
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
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.cpp
@@ -0,0 +1,228 @@
+// 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"
+
+// Functions called "signal" that are different from the system version.
+typedef void (*callback_t)(int);
+void signal(int, callback_t, int);
+namespace ns {
+void signal(int, callback_t);
+}
+
+extern "C" void handler_unsafe(int) {
+ printf("xxx");
+}
+
+extern "C" void handler_unsafe_1(int) {
+ printf("xxx");
+}
+
+namespace test_invalid_handler {
+
+void handler_non_extern_c(int) {
+ printf("xxx");
+}
+
+struct A {
+ static void handler_member(int) {
+ printf("xxx");
+ }
+};
+
+void test() {
+ std::signal(SIGINT, handler_unsafe_1);
+ // CHECK-MESSAGES: :[[@LINE-17]]:3: warning: standard function 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+ // CHECK-MESSAGES: :[[@LINE-2]]:23: note: function 'handler_unsafe_1' registered here as signal handler
+
+ std::signal(SIGINT, handler_non_extern_c);
+ // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: functions with other than C linkage are not allowed as signal handler (until C++17) [bugprone-signal-handler]
+ std::signal(SIGINT, A::handler_member);
+ // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: functions with other than C linkage are not allowed as signal handler (until C++17) [bugprone-signal-handler]
+ std::signal(SIGINT, [](int) { printf("xxx"); });
+ // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: lambda function is not allowed as signal handler (until C++17) [bugprone-signal-handler]
+
+ // This case is (deliberately) not found by the checker.
+ std::signal(SIGINT, [](int) -> callback_t { return &handler_unsafe; }(1));
+}
+
+} // namespace test_invalid_handler
+
+namespace test_non_standard_signal_call {
+
+struct Signal {
+ static void signal(int, callback_t);
+};
+
+void test() {
+ // No diagnostics here. All these signal calls differ from the standard system one.
+ signal(SIGINT, handler_unsafe, 1);
+ ns::signal(SIGINT, handler_unsafe);
+ Signal::signal(SIGINT, handler_unsafe);
+ system_other::signal(SIGINT, handler_unsafe);
+}
+
+} // namespace test_non_standard_signal_call
+
+namespace test_cpp_construct_in_handler {
+
+struct Struct {
+ virtual ~Struct() {}
+ void f1();
+ int *begin();
+ int *end();
+ static void f2();
+};
+struct Derived : public Struct {
+};
+
+struct X {
+ X(int, float);
+};
+
+Struct *S_Global;
+const Struct *S_GlobalConst;
+
+void f_non_extern_c() {
+}
+
+void f_default_arg(int P1 = 0) {
+}
+
+extern "C" void handler_cpp(int) {
+ using namespace ::test_cpp_construct_in_handler;
+
+ // These calls are not found as problems.
+ // (Called functions are not analyzed if the current function has already
+ // other problems.)
+ f_non_extern_c();
+ Struct::f2();
+ // 'auto' is not disallowed
+ auto Auto = 28u;
+
+ Struct S;
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: C++-only construct is not allowed in signal handler (until C++17) [bugprone-signal-handler]
+ // CHECK-MESSAGES: :[[@LINE-2]]:10: remark: internally, the statement is parsed as a 'CXXConstructExpr'
+ // CHECK-MESSAGES: :198:23: note: function 'handler_cpp' registered here as signal handler
+ S_Global->f1();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: C++-only construct is not allowed in signal handler (until C++17) [bugprone-signal-handler]
+ // CHECK-MESSAGES: :[[@LINE-2]]:3: remark: internally, the statement is parsed as a 'CXXMemberCallExpr'
+ // CHECK-MESSAGES: :198:23: note: function 'handler_cpp' registered here as signal handler
+ const Struct &SRef = Struct();
+ // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: C++-only construct is not allowed in signal handler (until C++17) [bugprone-signal-handler]
+ // CHECK-MESSAGES: :[[@LINE-2]]:24: remark: internally, the statement is parsed as a 'CXXBindTemporaryExpr'
+ // CHECK-MESSAGES: :198:23: note: function 'handler_cpp' registered here as signal handler
+ X(3, 4.4);
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: C++-only construct is not allowed in signal handler (until C++17) [bugprone-signal-handler]
+ // CHECK-MESSAGES: :[[@LINE-2]]:3: remark: internally, the statement is parsed as a 'CXXTemporaryObjectExpr'
+ // CHECK-MESSAGES: :198:23: note: function 'handler_cpp' registered here as signal handler
+
+ auto L = [](int i) { printf("%d", i); };
+ // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: C++-only construct is not allowed in signal handler (until C++17) [bugprone-signal-handler]
+ // CHECK-MESSAGES: :[[@LINE-2]]:12: remark: internally, the statement is parsed as a 'CXXConstructExpr'
+ // CHECK-MESSAGES: :198:23: note: function 'handler_cpp' registered here as signal handler
+ L(2);
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: C++-only construct is not allowed in signal handler (until C++17) [bugprone-signal-handler]
+ // CHECK-MESSAGES: :[[@LINE-2]]:3: remark: internally, the statement is parsed as a 'CXXOperatorCallExpr'
+ // CHECK-MESSAGES: :198:23: note: function 'handler_cpp' registered here as signal handler
+
+ try {
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: C++-only construct is not allowed in signal handler (until C++17) [bugprone-signal-handler]
+ // CHECK-MESSAGES: :[[@LINE-2]]:3: remark: internally, the statement is parsed as a 'CXXTryStmt'
+ // CHECK-MESSAGES: :198:23: note: function 'handler_cpp' registered here as signal handler
+ int A;
+ } catch (int) {
+ };
+ // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: C++-only construct is not allowed in signal handler (until C++17) [bugprone-signal-handler]
+ // CHECK-MESSAGES: :[[@LINE-3]]:5: remark: internally, the statement is parsed as a 'CXXCatchStmt'
+ // CHECK-MESSAGES: :198:23: note: function 'handler_cpp' registered here as signal handler
+
+ throw(12);
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: C++-only construct is not allowed in signal handler (until C++17) [bugprone-signal-handler]
+ // CHECK-MESSAGES: :[[@LINE-2]]:3: remark: internally, the statement is parsed as a 'CXXThrowExpr'
+ // CHECK-MESSAGES: :198:23: note: function 'handler_cpp' registered here as signal handler
+
+ for (int I : S) {
+ }
+ // CHECK-MESSAGES: :[[@LINE-2]]:3: warning: C++-only construct is not allowed in signal handler (until C++17) [bugprone-signal-handler]
+ // CHECK-MESSAGES: :[[@LINE-3]]:3: remark: internally, the statement is parsed as a 'CXXForRangeStmt'
+ // CHECK-MESSAGES: :198:23: note: function 'handler_cpp' registered here as signal handler
+ // CHECK-MESSAGES: :[[@LINE-5]]:14: warning: C++-only construct is not allowed in signal handler (until C++17) [bugprone-signal-handler]
+ // CHECK-MESSAGES: :[[@LINE-6]]:14: remark: internally, the statement is parsed as a 'CXXMemberCallExpr'
+ // CHECK-MESSAGES: :198:23: note: function 'handler_cpp' registered here as signal handler
+
+ int Int = *(reinterpret_cast<int *>(&S));
+ // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: C++-only construct is not allowed in signal handler (until C++17) [bugprone-signal-handler]
+ // CHECK-MESSAGES: :[[@LINE-2]]:15: remark: internally, the statement is parsed as a 'CXXReinterpretCastExpr'
+ // CHECK-MESSAGES: :198:23: note: function 'handler_cpp' registered here as signal handler
+ Int = static_cast<int>(12.34);
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: C++-only construct is not allowed in signal handler (until C++17) [bugprone-signal-handler]
+ // CHECK-MESSAGES: :[[@LINE-2]]:9: remark: internally, the statement is parsed as a 'CXXStaticCastExpr'
+ // CHECK-MESSAGES: :198:23: note: function 'handler_cpp' registered here as signal handler
+ Derived *Der = dynamic_cast<Derived *>(S_Global);
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: C++-only construct is not allowed in signal handler (until C++17) [bugprone-signal-handler]
+ // CHECK-MESSAGES: :[[@LINE-2]]:18: remark: internally, the statement is parsed as a 'CXXDynamicCastExpr'
+ // CHECK-MESSAGES: :198:23: note: function 'handler_cpp' registered here as signal handler
+ Struct *SPtr = const_cast<Struct *>(S_GlobalConst);
+ // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: C++-only construct is not allowed in signal handler (until C++17) [bugprone-signal-handler]
+ // CHECK-MESSAGES: :[[@LINE-2]]:18: remark: internally, the statement is parsed as a 'CXXConstCastExpr'
+ // CHECK-MESSAGES: :198:23: note: function 'handler_cpp' registered here as signal handler
+ Int = int(12.34);
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: C++-only construct is not allowed in signal handler (until C++17) [bugprone-signal-handler]
+ // CHECK-MESSAGES: :[[@LINE-2]]:9: remark: internally, the statement is parsed as a 'CXXFunctionalCastExpr'
+ // CHECK-MESSAGES: :198:23: note: function 'handler_cpp' registered here as signal handler
+
+ int *IPtr = new int[10];
+ // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: C++-only construct is not allowed in signal handler (until C++17) [bugprone-signal-handler]
+ // CHECK-MESSAGES: :[[@LINE-2]]:15: remark: internally, the statement is parsed as a 'CXXNewExpr'
+ // CHECK-MESSAGES: :198:23: note: function 'handler_cpp' registered here as signal handler
+ delete[] IPtr;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: C++-only construct is not allowed in signal handler (until C++17) [bugprone-signal-handler]
+ // CHECK-MESSAGES: :[[@LINE-2]]:3: remark: internally, the statement is parsed as a 'CXXDeleteExpr'
+ // CHECK-MESSAGES: :198:23: note: function 'handler_cpp' registered here as signal handler
+ IPtr = nullptr;
+ // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: C++-only construct is not allowed in signal handler (until C++17) [bugprone-signal-handler]
+ // CHECK-MESSAGES: :[[@LINE-2]]:10: remark: internally, the statement is parsed as a 'CXXNullPtrLiteralExpr'
+ // CHECK-MESSAGES: :198:23: note: function 'handler_cpp' registered here as signal handler
+ bool Bool = true;
+ // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: C++-only construct is not allowed in signal handler (until C++17) [bugprone-signal-handler]
+ // CHECK-MESSAGES: :[[@LINE-2]]:15: remark: internally, the statement is parsed as a 'CXXBoolLiteralExpr'
+ // CHECK-MESSAGES: :198:23: note: function 'handler_cpp' registered here as signal handler
+ f_default_arg();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: C++-only construct is not allowed in signal handler (until C++17) [bugprone-signal-handler]
+ // CHECK-MESSAGES: :[[@LINE-2]]:3: remark: internally, the statement is parsed as a 'CXXDefaultArgExpr'
+ // CHECK-MESSAGES: :198:23: note: function 'handler_cpp' registered here as signal handler
+}
+
+void test() {
+ std::signal(SIGINT, handler_cpp);
+}
+
+} // namespace test_cpp_construct_in_handler
+
+namespace test_cpp_indirect {
+
+void non_extern_c() {
+ int *P = nullptr;
+}
+
+extern "C" void call_cpp_indirect() {
+ int *P = nullptr;
+ // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: C++-only construct is not allowed in signal handler (until C++17) [bugprone-signal-handler]
+ // CHECK-MESSAGES: :[[@LINE-2]]:12: remark: internally, the statement is parsed as a 'CXXNullPtrLiteralExpr'
+ // CHECK-MESSAGES: :[[@LINE+8]]:3: note: function 'call_cpp_indirect' called here from 'handler_cpp_indirect'
+ // CHECK-MESSAGES: :[[@LINE+11]]:23: note: function 'handler_cpp_indirect' registered here as signal handler
+}
+
+extern "C" void handler_cpp_indirect(int) {
+ non_extern_c();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: functions with other than C linkage are not allowed as signal handler (until C++17) [bugprone-signal-handler]
+ // CHECK-MESSAGES: :[[@LINE+5]]:23: note: function 'handler_cpp_indirect' registered here as signal handler
+ call_cpp_indirect();
+}
+
+void test() {
+ std::signal(SIGINT, handler_cpp_indirect);
+}
+
+} // namespace test_cpp_indirect
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
@@ -18,7 +18,6 @@
void handler_printf(int) {
printf("1234");
// CHECK-NOTES: :[[@LINE-1]]:3: warning: standard function '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
}
@@ -29,7 +28,6 @@
void handler_extern(int) {
f_extern();
// CHECK-NOTES: :[[@LINE-1]]:3: warning: cannot verify that external function 'f_extern' is 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
}
@@ -52,7 +50,6 @@
void f_bad() {
printf("1234");
// CHECK-NOTES: :[[@LINE-1]]:3: warning: standard function '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
}
@@ -68,7 +65,6 @@
void f_bad1() {
printf("1234");
// CHECK-NOTES: :[[@LINE-1]]:3: warning: standard function '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
@@ -100,7 +96,6 @@
if (0)
printf("1234");
// CHECK-NOTES: :[[@LINE-1]]:5: warning: standard function '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
}
@@ -111,11 +106,9 @@
void handler_multiple_calls(int) {
f_extern();
// CHECK-NOTES: :[[@LINE-1]]:3: warning: cannot verify that external function 'f_extern' is 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
+ // CHECK-NOTES: :[[@LINE+9]]:18: note: function 'handler_multiple_calls' registered here as signal handler
printf("1234");
// CHECK-NOTES: :[[@LINE-1]]:3: warning: standard function '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
@@ -136,13 +129,11 @@
void f_recursive() {
f_extern();
// CHECK-NOTES: :[[@LINE-1]]:3: warning: cannot verify that external function 'f_extern' is 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
+ // CHECK-NOTES: :[[@LINE-8]]:3: note: function 'f_recursive' called here from 'handler_recursive'
+ // CHECK-NOTES: :[[@LINE+9]]:18: note: function 'handler_recursive' registered here as signal handler
printf("");
// CHECK-NOTES: :[[@LINE-1]]:3: warning: standard function '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-12]]: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);
}
@@ -154,7 +145,6 @@
void f_multiple_paths() {
printf("");
// CHECK-NOTES: :[[@LINE-1]]:3: warning: standard function '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
}
Index: clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-signal-handler/stdcpp.h
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-signal-handler/stdcpp.h
+++ clang-tools-extra/test/clang-tidy/checkers/Inputs/bugprone-signal-handler/stdcpp.h
@@ -1,4 +1,4 @@
-//===--- signal.h - Stub header for tests -----------------------*- C++ -*-===//
+//===--- 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.
@@ -6,18 +6,33 @@
//
//===----------------------------------------------------------------------===//
-#ifndef _SIGNAL_H_
-#define _SIGNAL_H_
+#define SIGINT 1
+#define SIG_IGN std::_sig_ign
+#define SIG_DFL std::_sig_dfl
-typedef void (*sighandler_t)(int);
+namespace std {
void _sig_ign(int);
void _sig_dfl(int);
-#define SIGINT 1
-#define SIG_IGN _sig_ign
-#define SIG_DFL _sig_dfl
+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);
-#endif // _SIGNAL_H_
+} // namespace system_other
Index: clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h
+++ clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h
@@ -13,10 +13,12 @@
void _sig_ign(int);
void _sig_dfl(int);
+void _sig_err(int);
#define SIGINT 1
#define SIG_IGN _sig_ign
#define SIG_DFL _sig_dfl
+#define SIG_ERR _sig_err
sighandler_t signal(int, sighandler_t);
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,10 @@
+.. title:: clang-tidy - cert-msc54-cpp
+.. meta::
+ :http-equiv=refresh: 5;URL=bugprone-signal-handler.html
+
+cert-msc54-cpp
+==============
+
+The cert-msc54-cpp check is an alias, please see
+`bugprone-signal-handler <bugprone-signal-handler.html>`_
+for more information.
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone-signal-handler.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/bugprone-signal-handler.rst
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone-signal-handler.rst
@@ -3,20 +3,37 @@
bugprone-signal-handler
=======================
-Finds functions registered as signal handlers that call non asynchronous-safe
-functions. Any function that cannot be determined to be an asynchronous-safe
+Finds functions registered as signal handlers that contain specific
+constructs that can cause undefined behavior. The rules for what is
+allowed are different for various C++ language versions.
+
+For C it is checked if non-asynchronous-safe functions are called.
+
+For C++ until (and including) C++14 such code constructs are found
+that are C++-specific and therefore not recommended in signal handlers.
+Additionally the calls to non-asynchronous-safe functions are found
+too and a signal handler or any function called from it is allowed
+to have only extern C linkage. Restrictions related to atomic
+lock-free operations are not checked.
+In C++17 there are more clarified rules but these are not
+implemented yet in this check. The check does not run on C++17 code.
+
+Any function that cannot be determined to be an asynchronous-safe
function call is assumed to be non-asynchronous-safe by the checker,
including user functions for which only the declaration is visible.
User function calls with visible definition are checked recursively.
-The check handles only C code. Only the function names are considered and the
-fact that the function is a system-call, but no other restrictions on the
-arguments passed to the functions (the ``signal`` call is allowed without
+Only the function names are considered and the fact that the function
+is a system-call, but no other restrictions on the arguments passed
+to the functions (the ``signal`` call is allowed without
restrictions).
-This check corresponds to the CERT C Coding Standard rule
+This check implements the CERT C Coding Standard rule
`SIG30-C. Call only asynchronous-safe functions within signal handlers
<https://www.securecoding.cert.org/confluence/display/c/SIG30-C.+Call+only+asynchronous-safe+functions+within+signal+handlers>`_
-and has an alias name ``cert-sig30-c``.
+and the rule
+`MSC54-CPP. A signal handler must be a plain old function
+<https://wiki.sei.cmu.edu/confluence/display/cplusplus/MSC54-CPP.+A+signal+handler+must+be+a+plain+old+function>`_.
+It has alias names ``cert-sig30-c`` and ``cert-msc54-cpp``.
.. option:: AsyncSafeFunctionSet
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -106,9 +106,19 @@
New check aliases
^^^^^^^^^^^^^^^^^
+- New alias :doc:`cert-msc54-cpp
+ <clang-tidy/checks/cert-msc54-cpp>` to
+ :doc:`bugprone-signal-handler
+ <clang-tidy/checks/bugprone-signal-handler>` was added.
+
Changes in existing checks
^^^^^^^^^^^^^^^^^^^^^^^^^^
+- :doc:`bugprone-signal-handler
+ <clang-tidy/checks/bugprone-signal-handler>` check now partially
+ supports signal handler rules for C++14. Bug report generation is
+ improved.
+
Removed checks
^^^^^^^^^^^^^^
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
@@ -265,6 +265,8 @@
CheckFactories.registerCheck<LimitedRandomnessCheck>("cert-msc50-cpp");
CheckFactories.registerCheck<ProperlySeededRandomGeneratorCheck>(
"cert-msc51-cpp");
+ CheckFactories.registerCheck<bugprone::SignalHandlerCheck>(
+ "cert-msc54-cpp");
// OOP
CheckFactories.registerCheck<performance::MoveConstructorInitCheck>(
"cert-oop11-cpp");
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
@@ -37,13 +37,24 @@
/// Should test the properties of the function, and check in the code body.
/// Should not check function calls in the code (this part is done by the call
/// graph scan).
- /// @param FD The function to check. It may or may not have a definition.
- /// @param CallOrRef Location of the call to this function (in another
+ /// Bug reports are generated for the whole code body (no stop at the first
+ /// found issue). For issues that are not in the code body, only one
+ /// bug report is generated.
+ /// \param FD The function to check. It may or may not have a definition.
+ /// \param CallOrRef Location of the call to this function (in another
/// function) or the reference to the function (if it is used as a registered
/// signal handler). This is the location where diagnostics are to be placed.
- /// @return Returns true if a diagnostic was emitted for this function.
- bool checkFunction(const FunctionDecl *FD, const Expr *CallOrRef);
- /// Returns true if a standard library function is considered as
+ /// \param ChainReporter A function that adds bug report notes to display the
+ /// chain of called functions from signal handler registration to the current
+ /// function. This is called at every generated bug report.
+ /// The bool parameter is used like \c SkipPathEnd in \c reportHandlerChain .
+ /// \return Returns true if a diagnostic was emitted for this function.
+ bool checkFunction(const FunctionDecl *FD, const Expr *CallOrRef,
+ std::function<void(bool)> ChainReporter);
+ /// Similar as \c checkFunction but only check for C++14 rules.
+ bool checkFunctionCPP14(const FunctionDecl *FD, const Expr *CallOrRef,
+ std::function<void(bool)> ChainReporter);
+ /// Returns true if a standard library function is considered
/// asynchronous-safe.
bool isStandardFunctionAsyncSafe(const FunctionDecl *FD) const;
/// Add diagnostic notes to show the call chain of functions from a signal
@@ -54,8 +65,10 @@
/// function call.
/// @param HandlerRef Reference to the signal handler function where it is
/// registered as signal handler.
+ /// @param SkipPathEnd If true the last item of the call chain (farthest away
+ /// from the \c signal call) is omitted from note generation.
void reportHandlerChain(const llvm::df_iterator<clang::CallGraphNode *> &Itr,
- const DeclRefExpr *HandlerRef);
+ const DeclRefExpr *HandlerRef, bool SkipPathEnd);
clang::CallGraph CG;
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
@@ -291,6 +291,18 @@
return FoundCallee->CallExpr;
}
+SourceRange getSourceRangeOfStmt(const Stmt *S, ASTContext &Ctx) {
+ ParentMapContext &PM = Ctx.getParentMapContext();
+ DynTypedNode P = DynTypedNode::create(*S);
+ while (P.getSourceRange().isInvalid()) {
+ DynTypedNodeList PL = PM.getParents(P);
+ if (PL.size() != 1)
+ return {};
+ P = PL[0];
+ }
+ return P.getSourceRange();
+}
+
} // namespace
AST_MATCHER(FunctionDecl, isStandardFunction) {
@@ -313,11 +325,7 @@
bool SignalHandlerCheck::isLanguageVersionSupported(
const LangOptions &LangOpts) const {
- // FIXME: Make the checker useful on C++ code.
- if (LangOpts.CPlusPlus)
- return false;
-
- return true;
+ return LangOpts.CPlusPlus17 == 0;
}
void SignalHandlerCheck::registerMatchers(MatchFinder *Finder) {
@@ -328,13 +336,23 @@
unless(isExpandedFromMacro("SIG_IGN")),
unless(isExpandedFromMacro("SIG_DFL")))
.bind("handler_expr");
- Finder->addMatcher(
- callExpr(callee(SignalFunction), hasArgument(1, HandlerExpr))
- .bind("register_call"),
- this);
+ auto HandlerLambda = cxxMemberCallExpr(
+ on(expr(ignoringParenImpCasts(lambdaExpr().bind("handler_lambda")))));
+ Finder->addMatcher(callExpr(callee(SignalFunction),
+ hasArgument(1, anyOf(HandlerExpr, HandlerLambda)))
+ .bind("register_call"),
+ this);
}
void SignalHandlerCheck::check(const MatchFinder::MatchResult &Result) {
+ if (const auto *HandlerLambda =
+ Result.Nodes.getNodeAs<LambdaExpr>("handler_lambda")) {
+ diag(HandlerLambda->getBeginLoc(),
+ "lambda function is not allowed as signal handler (until C++17)")
+ << HandlerLambda->getSourceRange();
+ return;
+ }
+
const auto *SignalCall = Result.Nodes.getNodeAs<CallExpr>("register_call");
const auto *HandlerDecl =
Result.Nodes.getNodeAs<FunctionDecl>("handler_decl");
@@ -353,32 +371,48 @@
}
if (!HandlerDecl->hasBody()) {
- (void)checkFunction(HandlerDecl, HandlerExpr);
- // Here checkFunction will put the messages to HandlerExpr.
- // No need to show a call chain.
- // Without code body there is nothing more to check.
+ // Check the handler function.
+ // The warning is placed to the signal handler registration.
+ // No need to display a call chain and no need for more checks.
+ (void)checkFunction(HandlerDecl, HandlerExpr, {});
return;
}
+ // FIXME: Update CallGraph::getNode to use canonical decl?
CallGraphNode *HandlerNode = CG.getNode(HandlerDecl->getCanonicalDecl());
assert(HandlerNode &&
"Handler with body should be present in the call graph.");
// Start from signal handler and visit every function call.
- for (auto Itr = llvm::df_begin(HandlerNode), ItrE = llvm::df_end(HandlerNode);
- Itr != ItrE; ++Itr) {
- if (const auto *CallF = dyn_cast<FunctionDecl>((*Itr)->getDecl())) {
- unsigned int PathL = Itr.getPathLength();
- const Expr *CallOrRef = (PathL == 1)
- ? HandlerExpr
- : findCallExpr(Itr.getPath(PathL - 2), *Itr);
- if (checkFunction(CallF, CallOrRef))
- reportHandlerChain(Itr, HandlerExpr);
+ auto Itr = llvm::df_begin(HandlerNode), ItrE = llvm::df_end(HandlerNode);
+ while (Itr != ItrE) {
+ const auto *CallF = dyn_cast<FunctionDecl>((*Itr)->getDecl());
+ unsigned int PathL = Itr.getPathLength();
+ if (CallF) {
+ // A signal handler or a function transitively reachable from the signal
+ // handler was found to be unsafe.
+ // Generate notes for the whole call chain (including the signal handler
+ // registration).
+ const Expr *CallOrRef = (PathL > 1)
+ ? findCallExpr(Itr.getPath(PathL - 2), *Itr)
+ : HandlerExpr;
+ auto ChainReporter = [this, &Itr, HandlerExpr](bool SkipPathEnd) {
+ reportHandlerChain(Itr, HandlerExpr, SkipPathEnd);
+ };
+ // If problems were found in a function (`CallF`), skip the analysis of
+ // functions that are called from it.
+ if (checkFunction(CallF, CallOrRef, ChainReporter))
+ Itr.skipChildren();
+ else
+ ++Itr;
+ } else {
+ ++Itr;
}
}
}
-bool SignalHandlerCheck::checkFunction(const FunctionDecl *FD,
- const Expr *CallOrRef) {
+bool SignalHandlerCheck::checkFunction(
+ const FunctionDecl *FD, const Expr *CallOrRef,
+ std::function<void(bool)> ChainReporter) {
bool FunctionIsCalled = isa<CallExpr>(CallOrRef);
if (isStandardFunction(FD)) {
@@ -387,7 +421,9 @@
"asynchronous-safe; "
"%select{using it as|calling it from}1 "
"a signal handler may be dangerous")
- << FD << FunctionIsCalled;
+ << FD << FunctionIsCalled << CallOrRef->getSourceRange();
+ if (ChainReporter)
+ ChainReporter(/*SkipPathEnd=*/true);
return true;
}
return false;
@@ -398,23 +434,75 @@
"asynchronous-safe; "
"%select{using it as|calling it from}1 "
"a signal handler may be dangerous")
- << FD << FunctionIsCalled;
+ << FD << FunctionIsCalled << CallOrRef->getSourceRange();
+ if (ChainReporter)
+ ChainReporter(/*SkipPathEnd=*/true);
return true;
}
+ if (getLangOpts().CPlusPlus)
+ return checkFunctionCPP14(FD, CallOrRef, ChainReporter);
+
return false;
}
+bool SignalHandlerCheck::checkFunctionCPP14(
+ const FunctionDecl *FD, const Expr *CallOrRef,
+ std::function<void(bool)> ChainReporter) {
+ if (!FD->isExternC()) {
+ diag(CallOrRef->getBeginLoc(),
+ "functions with other than C linkage are not allowed as signal "
+ "handler (until C++17)");
+ if (ChainReporter)
+ ChainReporter(/*SkipPathEnd=*/true);
+ return true;
+ }
+
+ const FunctionDecl *FBody;
+ const Stmt *BodyS = FD->getBody(FBody);
+ if (!BodyS)
+ return false;
+
+ bool StmtProblemsFound = false;
+ ASTContext &Ctx = FBody->getASTContext();
+ auto Matches =
+ match(decl(forEachDescendant(stmt().bind("stmt"))), *FBody, Ctx);
+ for (const auto &Match : Matches) {
+ const auto *FoundS = Match.getNodeAs<Stmt>("stmt");
+ StringRef Name = FoundS->getStmtClassName();
+ if (Name.startswith("CXX")) {
+ SourceRange R = getSourceRangeOfStmt(FoundS, Ctx);
+ if (R.isInvalid())
+ continue;
+ diag(R.getBegin(),
+ "C++-only construct is not allowed in signal handler (until C++17)")
+ << R;
+ diag(R.getBegin(), "internally, the statement is parsed as a '%0'",
+ DiagnosticIDs::Remark)
+ << Name;
+ if (ChainReporter)
+ ChainReporter(/*SkipPathEnd=*/false);
+ StmtProblemsFound = true;
+ }
+ }
+
+ return StmtProblemsFound;
+}
+
bool SignalHandlerCheck::isStandardFunctionAsyncSafe(
const FunctionDecl *FD) const {
assert(isStandardFunction(FD));
const IdentifierInfo *II = FD->getIdentifier();
// Unnamed functions are not explicitly allowed.
+ // C++ std operators may be unsafe and not within the
+ // "common subset of C and C++".
if (!II)
return false;
- // FIXME: Improve for C++ (check for namespace).
+ if (!FD->isInStdNamespace() && !FD->isGlobal())
+ return false;
+
if (ConformingFunctions.count(II->getName()))
return true;
@@ -423,7 +511,7 @@
void SignalHandlerCheck::reportHandlerChain(
const llvm::df_iterator<clang::CallGraphNode *> &Itr,
- const DeclRefExpr *HandlerRef) {
+ const DeclRefExpr *HandlerRef, bool SkipPathEnd) {
int CallLevel = Itr.getPathLength() - 2;
assert(CallLevel >= -1 && "Empty iterator?");
@@ -432,16 +520,21 @@
Callee = Caller;
Caller = Itr.getPath(CallLevel);
const Expr *CE = findCallExpr(Caller, Callee);
- diag(CE->getBeginLoc(), "function %0 called here from %1",
- DiagnosticIDs::Note)
- << cast<FunctionDecl>(Callee->getDecl())
- << cast<FunctionDecl>(Caller->getDecl());
+ if (SkipPathEnd)
+ SkipPathEnd = false;
+ else
+ diag(CE->getBeginLoc(), "function %0 called here from %1",
+ DiagnosticIDs::Note)
+ << cast<FunctionDecl>(Callee->getDecl())
+ << cast<FunctionDecl>(Caller->getDecl());
--CallLevel;
}
- diag(HandlerRef->getBeginLoc(),
- "function %0 registered here as signal handler", DiagnosticIDs::Note)
- << cast<FunctionDecl>(Caller->getDecl()) << HandlerRef->getSourceRange();
+ if (!SkipPathEnd)
+ diag(HandlerRef->getBeginLoc(),
+ "function %0 registered here as signal handler", DiagnosticIDs::Note)
+ << cast<FunctionDecl>(Caller->getDecl())
+ << HandlerRef->getSourceRange();
}
} // namespace bugprone
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
