diff --git a/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h b/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h --- a/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h +++ b/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 diff --git a/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp --- a/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp +++ b/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> + 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,211 @@ 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 +// 'quick_exit' is added to the set additionally because it looks like the +// mentioned POSIX specification was not updated after 'quick_exit' appeared +// in the C11 standard. +// Also, we want to keep the "minimal set" a subset of the "POSIX set". +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", + "quick_exit", + "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 diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -87,6 +87,14 @@ :doc:`concurrency-thread-canceltype-asynchronous ` was added. +Changes in existing checks +^^^^^^^^^^^^^^^^^^^^^^^^^^ + +- Improved :doc:`bugprone-signal-handler + ` check. + + Added an option to choose the set of allowed functions. + Improvements to include-fixer ----------------------------- diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone-signal-handler.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone-signal-handler.rst --- a/clang-tools-extra/docs/clang-tidy/checks/bugprone-signal-handler.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone-signal-handler.rst @@ -8,13 +8,30 @@ 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 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. +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). This check corresponds to the CERT C Coding Standard rule `SIG30-C. Call only asynchronous-safe functions within signal handlers -`_. +`_ +and has an alias name ``cert-sig30-c``. + +.. option:: AsyncSafeFunctionSet + + 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 + `_ + for more information). + The function ``quick_exit`` is not included in the shown list. It is + assumable that the reason is that the list was not updated for C11. + The checker includes ``quick_exit`` in the set of safe functions. + Functions registered as exit handlers are not checked. + + Default is ``POSIX``. + diff --git a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h --- a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h +++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h @@ -9,6 +9,8 @@ #ifndef _SIGNAL_H_ #define _SIGNAL_H_ +typedef void (*sighandler_t)(int); + void _sig_ign(int); void _sig_dfl(int); @@ -16,7 +18,6 @@ #define SIG_IGN _sig_ign #define SIG_DFL _sig_dfl -typedef void (*sighandler_t)(int); sighandler_t signal(int, sighandler_t); #endif // _SIGNAL_H_ diff --git a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h --- a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h +++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h @@ -1,4 +1,4 @@ -//===--- stdlib.h - Stub header for tests -----------------------*- C++ -*-===// +//===--- stdlib.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. @@ -13,6 +13,4 @@ void _Exit(int); void quick_exit(int); -void other_call(int); - #endif // _STDLIB_H_ diff --git a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string.h copy from clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h copy to clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string.h --- a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h +++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/string.h @@ -1,4 +1,4 @@ -//===--- stdlib.h - Stub header for tests -----------------------*- C++ -*-===// +//===--- string.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 _STRING_H_ +#define _STRING_H_ -void abort(void); -void _Exit(int); -void quick_exit(int); +typedef __typeof__(sizeof(0)) size_t; -void other_call(int); +void *memcpy(void *dest, const void *src, size_t n); -#endif // _STDLIB_H_ +#endif // _STRING_H_ diff --git a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/system-other.h copy from clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h copy to clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/system-other.h --- a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h +++ b/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_ diff --git a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/unistd.h copy from clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h copy to clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/unistd.h --- a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h +++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/unistd.h @@ -1,4 +1,4 @@ -//===--- stdlib.h - Stub header for tests -----------------------*- C++ -*-===// +//===--- unistd.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,9 @@ // //===----------------------------------------------------------------------===// -#ifndef _STDLIB_H_ -#define _STDLIB_H_ +#ifndef _UNISTD_H_ +#define _UNISTD_H_ -void abort(void); -void _Exit(int); -void quick_exit(int); +void _exit(int status); -void other_call(int); - -#endif // _STDLIB_H_ +#endif // _UNISTD_H_ diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler-minimal.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler-minimal.c new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler-minimal.c @@ -0,0 +1,32 @@ +// 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 "signal.h" +#include "stdlib.h" +#include "string.h" +#include "unistd.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(void *dst, const void *src) { + memcpy(dst, src, 10); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'memcpy' 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_good); +} diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler-posix.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler-posix.c new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler-posix.c @@ -0,0 +1,29 @@ +// 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 "signal.h" +#include "stdlib.h" +#include "stdio.h" +#include "string.h" +#include "unistd.h" + +void handler_bad(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_good(int) { + abort(); + _Exit(0); + _exit(0); + quick_exit(0); + signal(0, SIG_DFL); + memcpy((void*)10, (const void*)20, 1); +} + +void test() { + signal(SIGINT, handler_good); + signal(SIGINT, handler_bad); +} diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c @@ -1,8 +1,9 @@ -// 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 "stdio.h" +#include "system-other.h" // The function should be classified as system call even if there is // declaration the in source file. @@ -16,17 +17,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 +33,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 +48,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 +60,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);