balazske updated this revision to Diff 303356.
balazske added a comment.
Small fixes.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D90851/new/
https://reviews.llvm.org/D90851
Files:
clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h
clang-tools-extra/docs/clang-tidy/checks/bugprone-signal-handler.rst
clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h
clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h
clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/system-header-posix-api.h
clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/system-other.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
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
@@ -1,8 +1,8 @@
-// RUN: %check_clang_tidy %s cert-sig30-c %t -- -- -isystem %S/Inputs/Headers
+// RUN: %check_clang_tidy %s bugprone-signal-handler %t -- -- -isystem %S/Inputs/Headers
-#include "signal.h"
#include "stdio.h"
-#include "stdlib.h"
+#include "system-header-posix-api.h"
+#include "system-other.h"
// The function should be classified as system call even if there is
// declaration the in source file.
@@ -16,17 +16,9 @@
abort();
}
-void handler__Exit(int) {
- _Exit(0);
-}
-
-void handler_quick_exit(int) {
- quick_exit(0);
-}
-
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 [cert-sig30-c]
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
}
void handler_signal(int) {
@@ -40,7 +32,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 [cert-sig30-c]
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
}
void f_extern();
@@ -55,13 +47,11 @@
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 [cert-sig30-c]
+ // 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]
}
void test() {
signal(SIGINT, handler_abort);
- signal(SIGINT, handler__Exit);
- signal(SIGINT, handler_quick_exit);
signal(SIGINT, handler_signal);
signal(SIGINT, handler_other);
@@ -69,9 +59,9 @@
signal(SIGINT, handler_bad);
signal(SIGINT, handler_extern);
- signal(SIGINT, quick_exit);
+ 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 [cert-sig30-c]
+ // 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]
signal(SIGINT, SIG_IGN);
signal(SIGINT, SIG_DFL);
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler-posix.c
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler-posix.c
@@ -0,0 +1,36 @@
+// RUN: %check_clang_tidy %s bugprone-signal-handler %t \
+// RUN: -config='{CheckOptions: \
+// RUN: [{key: bugprone-signal-handler.AsyncSafeFunctionSet, value: "POSIX"}]}' \
+// RUN: -- -isystem %S/Inputs/Headers
+
+#include "stdio.h"
+#include "system-header-posix-api.h"
+
+void handler_bad1(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]
+}
+
+void handler_bad2(int) {
+ quick_exit(0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'quick_exit' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+}
+
+void handler_good1(int) {
+ _exit(0);
+ accept(1, 0, 0);
+ write(0, 0, 0);
+}
+
+void handler_good2(int) {
+ abort();
+ _Exit(0);
+ signal(0, SIG_DFL);
+}
+
+void test() {
+ signal(SIGINT, handler_good1);
+ signal(SIGINT, handler_good2);
+ signal(SIGINT, handler_bad1);
+ signal(SIGINT, handler_bad2);
+}
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler-minimal.c
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler-minimal.c
@@ -0,0 +1,34 @@
+// RUN: %check_clang_tidy %s bugprone-signal-handler %t \
+// RUN: -config='{CheckOptions: \
+// RUN: [{key: bugprone-signal-handler.AsyncSafeFunctionSet, value: "minimal"}]}' \
+// RUN: -- -isystem %S/Inputs/Headers
+
+#include "system-header-posix-api.h"
+
+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]
+}
+
+void handler_bad2(int) {
+ accept(1, 0, 0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'accept' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+}
+void handler_bad3(int) {
+ write(0, 0, 0);
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'write' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+}
+
+void handler_good(int) {
+ abort();
+ _Exit(0);
+ quick_exit(0);
+ signal(0, SIG_DFL);
+}
+
+void test() {
+ signal(SIGINT, handler_bad1);
+ signal(SIGINT, handler_bad2);
+ signal(SIGINT, handler_bad3);
+ signal(SIGINT, handler_good);
+}
Index: clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/system-other.h
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/system-other.h
+++ clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/system-other.h
@@ -1,4 +1,4 @@
-//===--- stdlib.h - Stub header for tests -----------------------*- C++ -*-===//
+//===--- system-other.h - Stub header for tests -----------------*- 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,13 +6,11 @@
//
//===----------------------------------------------------------------------===//
-#ifndef _STDLIB_H_
-#define _STDLIB_H_
+#ifndef _SYSTEM_OTHER_H_
+#define _SYSTEM_OTHER_H_
-void abort(void);
-void _Exit(int);
-void quick_exit(int);
+// Special system calls.
-void other_call(int);
+void other_call();
-#endif // _STDLIB_H_
+#endif // _SYSTEM_OTHER_H_
Index: clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/system-header-posix-api.h
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/system-header-posix-api.h
+++ clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/system-header-posix-api.h
@@ -1,4 +1,4 @@
-//===--- signal.h - Stub header for tests -----------------------*- C++ -*-===//
+//===--- system-header-posix-api.h - Stub header for tests ------*- 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,8 +6,16 @@
//
//===----------------------------------------------------------------------===//
-#ifndef _SIGNAL_H_
-#define _SIGNAL_H_
+#ifndef _SYSTEM_HEADER_POSIX_API_H_
+#define _SYSTEM_HEADER_POSIX_API_H_
+
+// A header intended to contain POSIX API like function
+// declarations.
+
+typedef int size_t;
+typedef int ssize_t;
+typedef int socklen_t;
+typedef void (*sighandler_t)(int);
void _sig_ign(int);
void _sig_dfl(int);
@@ -16,7 +24,12 @@
#define SIG_IGN _sig_ign
#define SIG_DFL _sig_dfl
-typedef void (*sighandler_t)(int);
+void _Exit(int);
+void _exit(int);
+void abort(void);
+int accept(int, struct sockaddr *, socklen_t *);
+void quick_exit(int);
sighandler_t signal(int, sighandler_t);
+ssize_t write(int, const void *, size_t);
-#endif // _SIGNAL_H_
+#endif // _SYSTEM_HEADER_POSIX_API_H_
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,18 +3,28 @@
bugprone-signal-handler
=======================
+This check corresponds to 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``.
+
Finds functions registered as signal handlers that call non asynchronous-safe
functions. 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.
+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
+restrictions).
-The minimal list of asynchronous-safe system functions is:
-``abort()``, ``_Exit()``, ``quick_exit()`` and ``signal()``
-(for ``signal`` there are additional conditions that are not checked).
-The check accepts only these calls as asynchronous-safe.
+.. option:: AsyncSafeFunctionSet
-This check corresponds to 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>`_.
+ Selects wich set of functions is considered as asynchronous-safe
+ (and therefore allowed in signal handlers). Value ``minimal`` selects
+ a minimal set that is defined in the CERT SIG30-C rule and includes functions
+ ``abort()``, ``_Exit()``, ``quick_exit()`` and ``signal()``. Value ``POSIX``
+ selects a larger set of functions that is listed in POSIX.1-2017 (see `this link
+ <https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_04_03>`_
+ for more information). This is not an extension of the minimal set (``quick_exit``
+ is not included). Default is ``POSIX``.
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
@@ -22,7 +22,10 @@
/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-signal-handler-check.html
class SignalHandlerCheck : public ClangTidyCheck {
public:
+ enum class AsyncSafeFunctionSetType { Minimal, POSIX };
+
SignalHandlerCheck(StringRef Name, ClangTidyContext *Context);
+ void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
bool isLanguageVersionSupported(const LangOptions &LangOpts) const override;
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
@@ -32,7 +35,11 @@
const CallExpr *SignalCall, const FunctionDecl *HandlerDecl);
bool isSystemCallAllowed(const FunctionDecl *FD) const;
- static llvm::StringSet<> StrictConformingFunctions;
+ AsyncSafeFunctionSetType AsyncSafeFunctionSet;
+ llvm::StringSet<> &ConformingFunctions;
+
+ static llvm::StringSet<> MinimalConformingFunctions;
+ static llvm::StringSet<> POSIXConformingFunctions;
};
} // namespace bugprone
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
@@ -21,6 +21,25 @@
namespace clang {
namespace tidy {
+
+template <>
+struct OptionEnumMapping<
+ bugprone::SignalHandlerCheck::AsyncSafeFunctionSetType> {
+ static llvm::ArrayRef<std::pair<
+ bugprone::SignalHandlerCheck::AsyncSafeFunctionSetType, StringRef>>
+ getEnumMapping() {
+ static constexpr std::pair<
+ bugprone::SignalHandlerCheck::AsyncSafeFunctionSetType, StringRef>
+ Mapping[] = {
+ {bugprone::SignalHandlerCheck::AsyncSafeFunctionSetType::Minimal,
+ "minimal"},
+ {bugprone::SignalHandlerCheck::AsyncSafeFunctionSetType::POSIX,
+ "POSIX"},
+ };
+ return makeArrayRef(Mapping);
+ }
+};
+
namespace bugprone {
static bool isSystemCall(const FunctionDecl *FD) {
@@ -56,14 +75,19 @@
AST_MATCHER(FunctionDecl, isSystemCall) { return isSystemCall(&Node); }
-// This is the minimal set of safe functions.
-// FIXME: Add checker option to allow a POSIX compliant extended set.
-llvm::StringSet<> SignalHandlerCheck::StrictConformingFunctions{
- "signal", "abort", "_Exit", "quick_exit"};
-
SignalHandlerCheck::SignalHandlerCheck(StringRef Name,
ClangTidyContext *Context)
- : ClangTidyCheck(Name, Context) {}
+ : ClangTidyCheck(Name, Context),
+ AsyncSafeFunctionSet(
+ Options.get("AsyncSafeFunctionSet", AsyncSafeFunctionSetType::POSIX)),
+ ConformingFunctions(AsyncSafeFunctionSet ==
+ AsyncSafeFunctionSetType::Minimal
+ ? MinimalConformingFunctions
+ : POSIXConformingFunctions) {}
+
+void SignalHandlerCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+ Options.store(Opts, "AsyncSafeFunctionSet", AsyncSafeFunctionSet);
+}
bool SignalHandlerCheck::isLanguageVersionSupported(
const LangOptions &LangOpts) const {
@@ -161,7 +185,7 @@
return false;
// FIXME: Improve for C++ (check for namespace).
- if (StrictConformingFunctions.count(II->getName()))
+ if (ConformingFunctions.count(II->getName()))
return true;
return false;
@@ -181,6 +205,207 @@
DiagnosticIDs::Note);
}
+// 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{
+ "signal", "abort", "_Exit", "quick_exit"};
+
+// The POSIX-defined set of safe functions.
+// https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_04_03
+// This list is not the same that is shown in the CERT page.
+llvm::StringSet<> SignalHandlerCheck::POSIXConformingFunctions{
+ "_Exit",
+ "_exit",
+ "abort",
+ "accept",
+ "access",
+ "aio_error",
+ "aio_return",
+ "aio_suspend",
+ "alarm",
+ "bind",
+ "cfgetispeed",
+ "cfgetospeed",
+ "cfsetispeed",
+ "cfsetospeed",
+ "chdir",
+ "chmod",
+ "chown",
+ "clock_gettime",
+ "close",
+ "connect",
+ "creat",
+ "dup",
+ "dup2",
+ "execl",
+ "execle",
+ "execv",
+ "execve",
+ "faccessat",
+ "fchdir",
+ "fchmod",
+ "fchmodat",
+ "fchown",
+ "fchownat",
+ "fcntl",
+ "fdatasync",
+ "fexecve",
+ "ffs",
+ "fork",
+ "fstat",
+ "fstatat",
+ "fsync",
+ "ftruncate",
+ "futimens",
+ "getegid",
+ "geteuid",
+ "getgid",
+ "getgroups",
+ "getpeername",
+ "getpgrp",
+ "getpid",
+ "getppid",
+ "getsockname",
+ "getsockopt",
+ "getuid",
+ "htonl",
+ "htons",
+ "kill",
+ "link",
+ "linkat",
+ "listen",
+ "longjmp",
+ "lseek",
+ "lstat",
+ "memccpy",
+ "memchr",
+ "memcmp",
+ "memcpy",
+ "memmove",
+ "memset",
+ "mkdir",
+ "mkdirat",
+ "mkfifo",
+ "mkfifoat",
+ "mknod",
+ "mknodat",
+ "ntohl",
+ "ntohs",
+ "open",
+ "openat",
+ "pause",
+ "pipe",
+ "poll",
+ "posix_trace_event",
+ "pselect",
+ "pthread_kill",
+ "pthread_self",
+ "pthread_sigmask",
+ "raise",
+ "read",
+ "readlink",
+ "readlinkat",
+ "recv",
+ "recvfrom",
+ "recvmsg",
+ "rename",
+ "renameat",
+ "rmdir",
+ "select",
+ "sem_post",
+ "send",
+ "sendmsg",
+ "sendto",
+ "setgid",
+ "setpgid",
+ "setsid",
+ "setsockopt",
+ "setuid",
+ "shutdown",
+ "sigaction",
+ "sigaddset",
+ "sigdelset",
+ "sigemptyset",
+ "sigfillset",
+ "sigismember",
+ "siglongjmp",
+ "signal",
+ "sigpause",
+ "sigpending",
+ "sigprocmask",
+ "sigqueue",
+ "sigset",
+ "sigsuspend",
+ "sleep",
+ "sockatmark",
+ "socket",
+ "socketpair",
+ "stat",
+ "stpcpy",
+ "stpncpy",
+ "strcat",
+ "strchr",
+ "strcmp",
+ "strcpy",
+ "strcspn",
+ "strlen",
+ "strncat",
+ "strncmp",
+ "strncpy",
+ "strnlen",
+ "strpbrk",
+ "strrchr",
+ "strspn",
+ "strstr",
+ "strtok_r",
+ "symlink",
+ "symlinkat",
+ "tcdrain",
+ "tcflow",
+ "tcflush",
+ "tcgetattr",
+ "tcgetpgrp",
+ "tcsendbreak",
+ "tcsetattr",
+ "tcsetpgrp",
+ "time",
+ "timer_getoverrun",
+ "timer_gettime",
+ "timer_settime",
+ "times",
+ "umask",
+ "uname",
+ "unlink",
+ "unlinkat",
+ "utime",
+ "utimensat",
+ "utimes",
+ "wait",
+ "waitpid",
+ "wcpcpy",
+ "wcpncpy",
+ "wcscat",
+ "wcschr",
+ "wcscmp",
+ "wcscpy",
+ "wcscspn",
+ "wcslen",
+ "wcsncat",
+ "wcsncmp",
+ "wcsncpy",
+ "wcsnlen",
+ "wcspbrk",
+ "wcsrchr",
+ "wcsspn",
+ "wcsstr",
+ "wcstok",
+ "wmemchr",
+ "wmemcmp",
+ "wmemcpy",
+ "wmemmove",
+ "wmemset",
+ "write"};
+
} // namespace bugprone
} // namespace tidy
} // namespace clang
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
