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 @@ -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 ChainReporter); + /// Similar as \c checkFunction but only check for C++14 rules. + bool checkFunctionCPP14(const FunctionDecl *FD, const Expr *CallOrRef, + std::function 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 &Itr, - const DeclRefExpr *HandlerRef); + const DeclRefExpr *HandlerRef, bool SkipPathEnd); clang::CallGraph CG; 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 @@ -22,6 +22,7 @@ // 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". +// The list is repeated in bugprone-signal-handler.rst and should be kept up to date. constexpr std::initializer_list POSIXConformingFunctions = { "_Exit", "_exit", @@ -277,6 +278,25 @@ FD->getCanonicalDecl()->getLocation()); } +/// Check if a statement is "C++-only". +/// This includes all statements that have a class name with "CXX" prefix +/// and every other statement that is declared in file ExprCXX.h. +bool isCXXOnlyStmt(const Stmt *S) { + StringRef Name = S->getStmtClassName(); + if (Name.startswith("CXX")) + return true; + // Check for all other class names in ExprCXX.h that have no 'CXX' prefix. + return isa(S); +} + /// Given a call graph node of a \p Caller function and a \p Callee that is /// called from \p Caller, get a \c CallExpr of the corresponding function call. /// It is unspecified which call is found if multiple calls exist, but the order @@ -291,6 +311,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 +345,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; } void SignalHandlerCheck::registerMatchers(MatchFinder *Finder) { @@ -328,13 +356,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("handler_lambda")) { + diag(HandlerLambda->getBeginLoc(), + "lambda function is not allowed as signal handler (until C++17)") + << HandlerLambda->getSourceRange(); + return; + } + const auto *HandlerDecl = Result.Nodes.getNodeAs("handler_decl"); const auto *HandlerExpr = Result.Nodes.getNodeAs("handler_expr"); @@ -352,32 +390,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((*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((*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 ChainReporter) { bool FunctionIsCalled = isa(CallOrRef); if (isStandardFunction(FD)) { @@ -386,7 +440,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; @@ -397,23 +453,74 @@ "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 ChainReporter) { + if (!FD->isExternC()) { + diag(CallOrRef->getBeginLoc(), + "functions without 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"); + if (isCXXOnlyStmt(FoundS)) { + 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) + << FoundS->getStmtClassName(); + 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; @@ -422,7 +529,7 @@ void SignalHandlerCheck::reportHandlerChain( const llvm::df_iterator &Itr, - const DeclRefExpr *HandlerRef) { + const DeclRefExpr *HandlerRef, bool SkipPathEnd) { int CallLevel = Itr.getPathLength() - 2; assert(CallLevel >= -1 && "Empty iterator?"); @@ -431,16 +538,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(Callee->getDecl()) - << cast(Caller->getDecl()); + if (SkipPathEnd) + SkipPathEnd = false; + else + diag(CE->getBeginLoc(), "function %0 called here from %1", + DiagnosticIDs::Note) + << cast(Callee->getDecl()) + << cast(Caller->getDecl()); --CallLevel; } - diag(HandlerRef->getBeginLoc(), - "function %0 registered here as signal handler", DiagnosticIDs::Note) - << cast(Caller->getDecl()) << HandlerRef->getSourceRange(); + if (!SkipPathEnd) + diag(HandlerRef->getBeginLoc(), + "function %0 registered here as signal handler", DiagnosticIDs::Note) + << cast(Caller->getDecl()) + << HandlerRef->getSourceRange(); } } // namespace bugprone 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 @@ -265,6 +265,8 @@ CheckFactories.registerCheck("cert-msc50-cpp"); CheckFactories.registerCheck( "cert-msc51-cpp"); + CheckFactories.registerCheck( + "cert-msc54-cpp"); // OOP CheckFactories.registerCheck( "cert-oop11-cpp"); 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 @@ -148,6 +148,11 @@ New check aliases ^^^^^^^^^^^^^^^^^ +- New alias :doc:`cert-msc54-cpp + ` to + :doc:`bugprone-signal-handler + ` was added. + - New alias :doc:`cppcoreguidelines-macro-to-enum ` to :doc:`modernize-macro-to-enum ` was added. @@ -161,10 +166,24 @@ - Fixed some false positives in :doc:`bugprone-infinite-loop ` involving dependent expressions. +- Improved :doc:`bugprone-signal-handler + ` check. Partial + support for C++14 signal handler rules was added. Bug report generation was + improved. + - Fixed a crash in :doc:`bugprone-sizeof-expression ` when `sizeof(...)` is compared against a `__int128_t`. +- Fixed bugs in :doc:`bugprone-use-after-move + `: + + - Treat a move in a lambda capture as happening in the function that defines + the lambda, not within the body of the lambda (as we were previously doing + erroneously). + + - Don't emit an erroneous warning on self-moves. + - Made :doc:`cert-oop57-cpp ` more sensitive by checking for an arbitrary expression in the second argument of ``memset``. @@ -210,6 +229,10 @@ ` to work when the vector is a member of a structure. +- Fixed a crash in :doc:`performance-unnecessary-value-param + ` when the specialization + template has an unnecessary value parameter. Removed the fix for a template. + - Fixed a crash in :doc:`readability-const-return-type ` when a pure virtual function overrided has a const return type. Removed the fix for a virtual function. @@ -225,19 +248,6 @@ ` to simplify expressions using DeMorgan's Theorem. -- Fixed a crash in :doc:`performance-unnecessary-value-param - ` when the specialization - template has an unnecessary value parameter. Removed the fix for a template. - -- Fixed bugs in :doc:`bugprone-use-after-move - `: - - - Treat a move in a lambda capture as happening in the function that defines - the lambda, not within the body of the lambda (as we were previously doing - erroneously). - - - Don't emit an erroneous warning on self-moves. - Removed checks ^^^^^^^^^^^^^^ 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 @@ -3,34 +3,95 @@ 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, +Finds specific constructs in signal handler functions that can cause undefined +behavior. The rules for what is allowed differ between C++ language versions. + +Checked signal handler rules for C: + +- Calls to non-asynchronous-safe functions are not allowed. + +Checked signal handler rules for up to and including C++14: + +- Calls to non-asynchronous-safe functions are not allowed. +- C++-specific code constructs are not allowed in signal handlers. + In other words, only the common subset of C and C++ is allowed to be used. +- Calls to functions with non-C linkage are not allowed (including the signal + handler itself). + +The check is disabled on C++17 and later. + +Asnychronous-safety is determined by comparing the function's name against a set +of known functions. In addition, the function must come from a system header +include and in a global namespace. The (possible) arguments passed to the +function are not checked. Any function that cannot be determined to be +asynchronous-safe is assumed to be non-asynchronous-safe by the check, 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 -restrictions). +Calls to user-defined functions with visible definitions are checked +recursively. -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 `_ -and has an alias name ``cert-sig30-c``. +and the rule +`MSC54-CPP. A signal handler must be a plain old function +`_. +It has the alias names ``cert-sig30-c`` and ``cert-msc54-cpp``. + +Options +------- .. option:: AsyncSafeFunctionSet Selects which 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``. + (and therefore allowed in signal handlers). It can be set to the following values: + + ``minimal`` + Selects a minimal set that is defined in the CERT SIG30-C rule. + and includes functions ``abort()``, ``_Exit()``, ``quick_exit()`` and + ``signal()``. + ``POSIX`` + Selects a larger set of functions that is listed in POSIX.1-2017 (see `this + link + `_ + for more information). The following functions are included: + ``_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`` + + The function ``quick_exit`` is not included in the POSIX list but it + is included here in the set of safe functions. + + The default value is ``POSIX``. diff --git a/clang-tools-extra/docs/clang-tidy/checks/cert/msc54-cpp.rst b/clang-tools-extra/docs/clang-tidy/checks/cert/msc54-cpp.rst new file mode 100644 --- /dev/null +++ b/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. 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 @@ -380,6 +380,7 @@ `cert-flp37-c `_, `bugprone-suspicious-memory-comparison `_, `cert-msc30-c `_, `cert-msc50-cpp `_, `cert-msc32-c `_, `cert-msc51-cpp `_, + `cert-msc54-cpp `_, `bugprone-signal-handler `_, `cert-oop11-cpp `_, `performance-move-constructor-init `_, `cert-oop54-cpp `_, `bugprone-unhandled-self-assignment `_, `cert-pos44-c `_, `bugprone-bad-signal-to-kill-thread `_, 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 @@ -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); diff --git a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/signal-handler/stdcpp.h copy from clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h copy to clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/signal-handler/stdcpp.h --- a/clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/signal.h +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/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 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 @@ -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(void) { 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(void) { 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(void) { 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(void) { 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 } diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/signal-handler.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/signal-handler.cpp new file mode 100644 --- /dev/null +++ b/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 %clang_tidy_headers -isystem %S/Inputs/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 without 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 without 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(&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(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(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(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 without 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