diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -40,6 +40,7 @@ #include "PosixReturnCheck.h" #include "RedundantBranchConditionCheck.h" #include "ReservedIdentifierCheck.h" +#include "SignalHandlerCheck.h" #include "SignedCharMisuseCheck.h" #include "SizeofContainerCheck.h" #include "SizeofExpressionCheck.h" @@ -133,6 +134,7 @@ "bugprone-posix-return"); CheckFactories.registerCheck( "bugprone-reserved-identifier"); + CheckFactories.registerCheck("bugprone-signal-handler"); CheckFactories.registerCheck( "bugprone-signed-char-misuse"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -35,6 +35,7 @@ PosixReturnCheck.cpp RedundantBranchConditionCheck.cpp ReservedIdentifierCheck.cpp + SignalHandlerCheck.cpp SignedCharMisuseCheck.cpp SizeofContainerCheck.cpp SizeofExpressionCheck.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h b/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h @@ -0,0 +1,42 @@ +//===--- SignalHandlerCheck.h - clang-tidy ----------------------*- 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 LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SIGNALHANDLERCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SIGNALHANDLERCHECK_H + +#include "../ClangTidyCheck.h" +#include "llvm/ADT/StringSet.h" + +namespace clang { +namespace tidy { +namespace bugprone { + +/// Checker for signal handler functions. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-signal-handler-check.html +class SignalHandlerCheck : public ClangTidyCheck { +public: + SignalHandlerCheck(StringRef Name, ClangTidyContext *Context); + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override; + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + +private: + void reportBug(const FunctionDecl *CalledFunction, const Expr *CallOrRef, + const CallExpr *SignalCall, const FunctionDecl *HandlerDecl); + bool isSystemCallAllowed(const FunctionDecl *FD) const; + + static llvm::StringSet<> StrictConformingFunctions; +}; + +} // namespace bugprone +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SIGNALHANDLERCHECK_H diff --git a/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp @@ -0,0 +1,186 @@ +//===--- SignalHandlerCheck.cpp - clang-tidy ------------------------------===// +// +// 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 +// +//===----------------------------------------------------------------------===// + +#include "SignalHandlerCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/AST/RecursiveASTVisitor.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Analysis/CallGraph.h" +#include "llvm/ADT/DenseSet.h" +#include "llvm/ADT/STLExtras.h" +#include "llvm/ADT/SmallVector.h" +#include +#include + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace bugprone { + +static bool isSystemCall(const FunctionDecl *FD) { + // Find a possible redeclaration in system header. + // FIXME: Looking at the canonical declaration is not the most exact way + // to do this. + + // Most common case will be inclusion directly from a header. + // This works fine by using canonical declaration. + // a.c + // #include + + // Next most common case will be extern declaration. + // Can't catch this with either approach. + // b.c + // extern void sysfunc(void); + + // Canonical declaration is the first found declaration, so this works. + // c.c + // #include + // extern void sysfunc(void); // redecl won't matter + + // This does not work with canonical declaration. + // Probably this is not a frequently used case but may happen (the first + // declaration can be in a non-system header for example). + // d.c + // extern void sysfunc(void); // Canonical declaration, not in system header. + // #include + + return FD->getASTContext().getSourceManager().isInSystemHeader( + FD->getCanonicalDecl()->getLocation()); +} + +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) {} + +bool SignalHandlerCheck::isLanguageVersionSupported( + const LangOptions &LangOpts) const { + // FIXME: Make the checker useful on C++ code. + if (LangOpts.CPlusPlus) + return false; + + return true; +} + +void SignalHandlerCheck::registerMatchers(MatchFinder *Finder) { + auto SignalFunction = functionDecl(hasAnyName("::signal", "::std::signal"), + parameterCountIs(2), isSystemCall()); + auto HandlerExpr = + declRefExpr(hasDeclaration(functionDecl().bind("handler_decl")), + unless(isExpandedFromMacro("SIG_IGN")), + unless(isExpandedFromMacro("SIG_DFL"))) + .bind("handler_expr"); + Finder->addMatcher( + callExpr(callee(SignalFunction), hasArgument(1, HandlerExpr)) + .bind("register_call"), + this); +} + +void SignalHandlerCheck::check(const MatchFinder::MatchResult &Result) { + const auto *SignalCall = Result.Nodes.getNodeAs("register_call"); + const auto *HandlerDecl = + Result.Nodes.getNodeAs("handler_decl"); + const auto *HandlerExpr = Result.Nodes.getNodeAs("handler_expr"); + + // Visit each function encountered in the callgraph only once. + llvm::DenseSet SeenFunctions; + + // The worklist of the callgraph visitation algorithm. + std::deque CalledFunctions; + + auto ProcessFunction = [&](const FunctionDecl *F, const Expr *CallOrRef) { + // Ensure that canonical declaration is used. + F = F->getCanonicalDecl(); + + // Do not visit function if already encountered. + if (!SeenFunctions.insert(F).second) + return true; + + // Check if the call is allowed. + // Non-system calls are not considered. + if (isSystemCall(F)) { + if (isSystemCallAllowed(F)) + return true; + + reportBug(F, CallOrRef, SignalCall, HandlerDecl); + + return false; + } + + // Get the body of the encountered non-system call function. + const FunctionDecl *FBody; + if (!F->hasBody(FBody)) { + reportBug(F, CallOrRef, SignalCall, HandlerDecl); + return false; + } + + // Collect all called functions. + auto Matches = match(decl(forEachDescendant(callExpr().bind("call"))), + *FBody, FBody->getASTContext()); + for (const auto &Match : Matches) { + const auto *CE = Match.getNodeAs("call"); + if (isa(CE->getCalleeDecl())) + CalledFunctions.push_back(CE); + } + + return true; + }; + + if (!ProcessFunction(HandlerDecl, HandlerExpr)) + return; + + // Visit the definition of every function referenced by the handler function. + // Check for allowed function calls. + while (!CalledFunctions.empty()) { + const CallExpr *FunctionCall = CalledFunctions.front(); + CalledFunctions.pop_front(); + // At insertion we have already ensured that only function calls are there. + const auto *F = cast(FunctionCall->getCalleeDecl()); + + if (!ProcessFunction(F, FunctionCall)) + break; + } +} + +bool SignalHandlerCheck::isSystemCallAllowed(const FunctionDecl *FD) const { + const IdentifierInfo *II = FD->getIdentifier(); + // Unnamed functions are not explicitly allowed. + if (!II) + return false; + + // FIXME: Improve for C++ (check for namespace). + if (StrictConformingFunctions.count(II->getName())) + return true; + + return false; +} + +void SignalHandlerCheck::reportBug(const FunctionDecl *CalledFunction, + const Expr *CallOrRef, + const CallExpr *SignalCall, + const FunctionDecl *HandlerDecl) { + diag(CallOrRef->getBeginLoc(), + "%0 may not be asynchronous-safe; " + "calling it from a signal handler may be dangerous") + << CalledFunction; + diag(SignalCall->getSourceRange().getBegin(), + "signal handler registered here", DiagnosticIDs::Note); + diag(HandlerDecl->getBeginLoc(), "handler function declared here", + DiagnosticIDs::Note); +} + +} // namespace bugprone +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp b/clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp --- a/clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp @@ -11,6 +11,7 @@ #include "../ClangTidyModuleRegistry.h" #include "../bugprone/BadSignalToKillThreadCheck.h" #include "../bugprone/ReservedIdentifierCheck.h" +#include "../bugprone/SignalHandlerCheck.h" #include "../bugprone/SignedCharMisuseCheck.h" #include "../bugprone/SpuriouslyWakeUpFunctionsCheck.h" #include "../bugprone/UnhandledSelfAssignmentCheck.h" @@ -109,6 +110,8 @@ // POS CheckFactories.registerCheck( "cert-pos44-c"); + // SIG + CheckFactories.registerCheck("cert-sig30-c"); // STR CheckFactories.registerCheck( "cert-str34-c"); 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 @@ -106,6 +106,18 @@ Finds condition variables in nested ``if`` statements that were also checked in the outer ``if`` statement and were not changed. +- New :doc:`bugprone-signal-handler + ` check. + + Finds functions registered as signal handlers that call non asynchronous-safe + functions. + +- New :doc:`cert-sig30-c + ` check. + + Alias to the :doc:`bugprone-signal-handler + ` check. + - New :doc:`readability-function-cognitive-complexity ` check. 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 new file mode 100644 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone-signal-handler.rst @@ -0,0 +1,20 @@ +.. title:: clang-tidy - bugprone-signal-handler + +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 +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. + +This check corresponds to the CERT C Coding Standard rule +`SIG30-C. Call only asynchronous-safe functions within signal handlers +`_. diff --git a/clang-tools-extra/docs/clang-tidy/checks/cert-sig30-c.rst b/clang-tools-extra/docs/clang-tidy/checks/cert-sig30-c.rst new file mode 100644 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/cert-sig30-c.rst @@ -0,0 +1,10 @@ +.. title:: clang-tidy - cert-sig30-c +.. meta:: + :http-equiv=refresh: 5;URL=bugprone-signal-handler.html + +cert-sig30-c +============ + +The cert-sig30-c check is an alias, please see +`bugprone-signal-handler `_ +for more information. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -77,6 +77,7 @@ `bugprone-posix-return `_, "Yes" `bugprone-redundant-branch-condition `_, "Yes" `bugprone-reserved-identifier `_, "Yes" + `bugprone-signal-handler `_, `bugprone-signed-char-misuse `_, `bugprone-sizeof-container `_, `bugprone-sizeof-expression `_, @@ -115,6 +116,7 @@ `cert-msc51-cpp `_, `cert-oop57-cpp `_, `cert-oop58-cpp `_, + `cert-sig30-c `_, `clang-analyzer-core.DynamicTypePropagation `_, `clang-analyzer-core.uninitialized.CapturedBlockVariable `_, `clang-analyzer-cplusplus.InnerPointer `_, 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 new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h @@ -0,0 +1,22 @@ +//===--- signal.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 _SIGNAL_H_ +#define _SIGNAL_H_ + +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); + +#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 new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/stdlib.h @@ -0,0 +1,18 @@ +//===--- 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. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef _STDLIB_H_ +#define _STDLIB_H_ + +void abort(void); +void _Exit(int); +void quick_exit(int); + +void other_call(int); + +#endif // _STDLIB_H_ 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 new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c @@ -0,0 +1,78 @@ +// RUN: %check_clang_tidy %s cert-sig30-c %t -- -- -isystem %S/Inputs/Headers + +#include "signal.h" +#include "stdio.h" +#include "stdlib.h" + +// The function should be classified as system call even if there is +// declaration the in source file. +// FIXME: The detection works only if the first declaration is in system +// header. +int printf(const char *, ...); +typedef void (*sighandler_t)(int); +sighandler_t signal(int signum, sighandler_t handler); + +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] +} + +void handler_signal(int) { + // FIXME: It is only OK to call signal with the current signal number. + signal(0, SIG_DFL); +} + +void f_ok() { + abort(); +} + +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] +} + +void f_extern(); + +void handler_ok(int) { + f_ok(); +} + +void handler_bad(int) { + f_bad(); +} + +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] +} + +void test() { + signal(SIGINT, handler_abort); + signal(SIGINT, handler__Exit); + signal(SIGINT, handler_quick_exit); + signal(SIGINT, handler_signal); + signal(SIGINT, handler_other); + + signal(SIGINT, handler_ok); + signal(SIGINT, handler_bad); + signal(SIGINT, handler_extern); + + signal(SIGINT, quick_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] + + signal(SIGINT, SIG_IGN); + signal(SIGINT, SIG_DFL); +}