[clang-tidy] Extending bugprone-signal-handler with POSIX functions.

An option is added to the check to select wich set of functions is
defined as asynchronous-safe functions.

Reviewed By: aaron.ballman

Differential Revision: https://reviews.llvm.org/D90851
This commit is contained in:
Balázs Kéri 2021-02-22 17:16:51 +01:00
parent 6c06b0aa5a
commit 2c54b29337
12 changed files with 394 additions and 36 deletions

View File

@ -21,6 +21,25 @@ using namespace clang::ast_matchers;
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 @@ static bool isSystemCall(const FunctionDecl *FD) {
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 @@ bool SignalHandlerCheck::isSystemCallAllowed(const FunctionDecl *FD) const {
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 @@ void SignalHandlerCheck::reportBug(const FunctionDecl *CalledFunction,
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

View File

@ -22,7 +22,10 @@ namespace bugprone {
/// 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 @@ private:
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

View File

@ -87,6 +87,14 @@ New check aliases
:doc:`concurrency-thread-canceltype-asynchronous
<clang-tidy/checks/concurrency-thread-canceltype-asynchronous>` was added.
Changes in existing checks
^^^^^^^^^^^^^^^^^^^^^^^^^^
- Improved :doc:`bugprone-signal-handler
<clang-tidy/checks/bugprone-signal-handler>` check.
Added an option to choose the set of allowed functions.
Improvements to include-fixer
-----------------------------

View File

@ -8,13 +8,30 @@ 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 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
<https://www.securecoding.cert.org/confluence/display/c/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``.
.. 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
<https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_04_03>`_
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``.

View File

@ -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 @@ void _sig_dfl(int);
#define SIG_IGN _sig_ign
#define SIG_DFL _sig_dfl
typedef void (*sighandler_t)(int);
sighandler_t signal(int, sighandler_t);
#endif // _SIGNAL_H_

View File

@ -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 abort(void);
void _Exit(int);
void quick_exit(int);
void other_call(int);
#endif // _STDLIB_H_

View File

@ -0,0 +1,16 @@
//===--- 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.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//
#ifndef _STRING_H_
#define _STRING_H_
typedef __typeof__(sizeof(0)) size_t;
void *memcpy(void *dest, const void *src, size_t n);
#endif // _STRING_H_

View File

@ -0,0 +1,16 @@
//===--- 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.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//
#ifndef _SYSTEM_OTHER_H_
#define _SYSTEM_OTHER_H_
// Special system calls.
void other_call();
#endif // _SYSTEM_OTHER_H_

View File

@ -0,0 +1,14 @@
//===--- 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.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//
#ifndef _UNISTD_H_
#define _UNISTD_H_
void _exit(int status);
#endif // _UNISTD_H_

View File

@ -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);
}

View File

@ -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);
}

View File

@ -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 @@ void handler_abort(int) {
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_ok() {
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_bad(int) {
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 @@ void test() {
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);