From 8d288a0668a574863d52784084ff565c89f7366e Mon Sep 17 00:00:00 2001 From: Abel Kocsis Date: Mon, 11 Nov 2019 17:47:14 +0100 Subject: [PATCH] [clang-tidy] Add bugprone-bad-signal-to-kill-thread check and its alias cert-pos44-c --- .../bugprone/BadSignalToKillThreadCheck.cpp | 70 +++++++++++++++++++ .../bugprone/BadSignalToKillThreadCheck.h | 37 ++++++++++ .../bugprone/BugproneTidyModule.cpp | 3 + .../clang-tidy/bugprone/CMakeLists.txt | 1 + .../clang-tidy/cert/CERTTidyModule.cpp | 4 ++ clang-tools-extra/docs/ReleaseNotes.rst | 11 +++ .../bugprone-bad-signal-to-kill-thread.rst | 16 +++++ .../docs/clang-tidy/checks/cert-pos44-c.rst | 9 +++ .../docs/clang-tidy/checks/list.rst | 2 + .../bugprone-bad-signal-to-kill-thread.cpp | 38 ++++++++++ 10 files changed, 191 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/bugprone/BadSignalToKillThreadCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/bugprone/BadSignalToKillThreadCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/bugprone-bad-signal-to-kill-thread.rst create mode 100644 clang-tools-extra/docs/clang-tidy/checks/cert-pos44-c.rst create mode 100644 clang-tools-extra/test/clang-tidy/bugprone-bad-signal-to-kill-thread.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/BadSignalToKillThreadCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/BadSignalToKillThreadCheck.cpp new file mode 100644 index 000000000000..1f3dec497c07 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/BadSignalToKillThreadCheck.cpp @@ -0,0 +1,70 @@ +//===--- BadSignalToKillThreadCheck.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 "BadSignalToKillThreadCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace bugprone { + +void BadSignalToKillThreadCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + callExpr(allOf(callee(functionDecl(hasName("::pthread_kill"))), + argumentCountIs(2)), + hasArgument(1, integerLiteral().bind("integer-literal"))) + .bind("thread-kill"), + this); +} + +static Preprocessor *PP; + +void BadSignalToKillThreadCheck::check(const MatchFinder::MatchResult &Result) { + const auto IsSigterm = [](const auto &KeyValue) -> bool { + return KeyValue.first->getName() == "SIGTERM"; + }; + const auto TryExpandAsInteger = + [](Preprocessor::macro_iterator It) -> Optional { + if (It == PP->macro_end()) + return llvm::None; + const MacroInfo *MI = PP->getMacroInfo(It->first); + const Token &T = MI->tokens().back(); + StringRef ValueStr = StringRef(T.getLiteralData(), T.getLength()); + + llvm::APInt IntValue; + constexpr unsigned AutoSenseRadix = 0; + if (ValueStr.getAsInteger(AutoSenseRadix, IntValue)) + return llvm::None; + return IntValue.getZExtValue(); + }; + + const auto SigtermMacro = llvm::find_if(PP->macros(), IsSigterm); + + if (!SigtermValue && !(SigtermValue = TryExpandAsInteger(SigtermMacro))) + return; + + const auto *MatchedExpr = Result.Nodes.getNodeAs("thread-kill"); + const auto *MatchedIntLiteral = + Result.Nodes.getNodeAs("integer-literal"); + if (MatchedIntLiteral->getValue() == *SigtermValue) { + diag(MatchedExpr->getBeginLoc(), + "thread should not be terminated by raising the 'SIGTERM' signal"); + } +} + +void BadSignalToKillThreadCheck::registerPPCallbacks( + const SourceManager &SM, Preprocessor *pp, Preprocessor *ModuleExpanderPP) { + PP = pp; +} + +} // namespace bugprone +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/bugprone/BadSignalToKillThreadCheck.h b/clang-tools-extra/clang-tidy/bugprone/BadSignalToKillThreadCheck.h new file mode 100644 index 000000000000..d39fdec2e7de --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/BadSignalToKillThreadCheck.h @@ -0,0 +1,37 @@ +//===--- BadSignalToKillThreadCheck.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_BADSIGNALTOKILLTHREADCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_BADSIGNALTOKILLTHREADCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang { +namespace tidy { +namespace bugprone { + +/// Finds ``pthread_kill`` function calls when thread is terminated by +/// ``SIGTERM`` signal. +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-bad-signal-to-kill-thread.html +class BadSignalToKillThreadCheck : public ClangTidyCheck { +public: + BadSignalToKillThreadCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP, + Preprocessor *ModuleExpanderPP) override; + Optional SigtermValue; +}; + +} // namespace bugprone +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_BADSIGNALTOKILLTHREADCHECK_H diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp index 074aa7a8ba6b..99eff40ebf42 100644 --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -12,6 +12,7 @@ #include "../cppcoreguidelines/NarrowingConversionsCheck.h" #include "ArgumentCommentCheck.h" #include "AssertSideEffectCheck.h" +#include "BadSignalToKillThreadCheck.h" #include "BoolPointerImplicitConversionCheck.h" #include "BranchCloneCheck.h" #include "CopyConstructorInitCheck.h" @@ -68,6 +69,8 @@ public: "bugprone-argument-comment"); CheckFactories.registerCheck( "bugprone-assert-side-effect"); + CheckFactories.registerCheck( + "bugprone-bad-signal-to-kill-thread"); CheckFactories.registerCheck( "bugprone-bool-pointer-implicit-conversion"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt index ab3363f43733..4b499e68069c 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -3,6 +3,7 @@ set(LLVM_LINK_COMPONENTS support) add_clang_library(clangTidyBugproneModule ArgumentCommentCheck.cpp AssertSideEffectCheck.cpp + BadSignalToKillThreadCheck.cpp BoolPointerImplicitConversionCheck.cpp BranchCloneCheck.cpp BugproneTidyModule.cpp diff --git a/clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp b/clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp index 341968b6fa6b..23820446cd17 100644 --- a/clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp @@ -10,6 +10,7 @@ #include "../ClangTidyModule.h" #include "../ClangTidyModuleRegistry.h" #include "../bugprone/UnhandledSelfAssignmentCheck.h" +#include "../bugprone/BadSignalToKillThreadCheck.h" #include "../google/UnnamedNamespaceInHeaderCheck.h" #include "../misc/NewDeleteOverloadsCheck.h" #include "../misc/NonCopyableObjects.h" @@ -82,6 +83,9 @@ public: CheckFactories.registerCheck("cert-msc30-c"); CheckFactories.registerCheck( "cert-msc32-c"); + // POS + CheckFactories.registerCheck( + "cert-pos44-c"); } ClangTidyOptions getModuleOptions() override { diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 40b7aed41784..a94ee3defd1e 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -67,6 +67,12 @@ The improvements are... Improvements to clang-tidy -------------------------- +- New :doc:`bugprone-bad-signal-to-kill-thread + ` check. + + Finds ``pthread_kill`` function calls when a thread is terminated by + raising ``SIGTERM`` signal. + - New :doc:`bugprone-dynamic-static-initializers ` check. @@ -88,6 +94,11 @@ Improvements to clang-tidy Without the null terminator it can result in undefined behaviour when the string is read. +- New alias :doc:`cert-pos44-cpp + ` to + :doc:`bugprone-bad-signal-to-kill-thread + ` was added. + - New :doc:`cppcoreguidelines-init-variables ` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone-bad-signal-to-kill-thread.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone-bad-signal-to-kill-thread.rst new file mode 100644 index 000000000000..05f5c9363973 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone-bad-signal-to-kill-thread.rst @@ -0,0 +1,16 @@ +.. title:: clang-tidy - bugprone-bad-signal-to-kill-thread + +bugprone-bad-signal-to-kill-thread +================================== + +Finds ``pthread_kill`` function calls when a thread is terminated by +raising ``SIGTERM`` signal and the signal kills the entire process, not +just the individual thread. Use any signal except ``SIGTERM``. + +.. code-block: c++ + + pthread_kill(thread, SIGTERM); + +This check corresponds to the CERT C Coding Standard rule +`POS44-C. Do not use signals to terminate threads +`_. diff --git a/clang-tools-extra/docs/clang-tidy/checks/cert-pos44-c.rst b/clang-tools-extra/docs/clang-tidy/checks/cert-pos44-c.rst new file mode 100644 index 000000000000..5bc48a685afd --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/cert-pos44-c.rst @@ -0,0 +1,9 @@ +.. title:: clang-tidy - cert-pos44-c +.. meta:: + :http-equiv=refresh: 5;URL=bugprone-bad-signal-to-kill-thread.html + +cert-pos44-c +============ + +The cert-pos44-c check is an alias, please see +`bugprone-bad-signal-to-kill-thread `_ 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 index b499a84fdcfe..9e6e3472d064 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -39,6 +39,7 @@ Clang-Tidy Checks boost-use-to-string bugprone-argument-comment bugprone-assert-side-effect + bugprone-bad-signal-to-kill-thread bugprone-bool-pointer-implicit-conversion bugprone-branch-clone bugprone-copy-constructor-init @@ -105,6 +106,7 @@ Clang-Tidy Checks cert-msc51-cpp cert-oop11-cpp (redirects to performance-move-constructor-init) cert-oop54-cpp (redirects to bugprone-unhandled-self-assignment) + cert-pos44-c (redirects to bugprone-bad-signal-to-kill-thread) clang-analyzer-core.CallAndMessage (redirects to https://clang.llvm.org/docs/analyzer/checkers) clang-analyzer-core.DivideZero (redirects to https://clang.llvm.org/docs/analyzer/checkers) clang-analyzer-core.DynamicTypePropagation diff --git a/clang-tools-extra/test/clang-tidy/bugprone-bad-signal-to-kill-thread.cpp b/clang-tools-extra/test/clang-tidy/bugprone-bad-signal-to-kill-thread.cpp new file mode 100644 index 000000000000..5de2001e26b9 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/bugprone-bad-signal-to-kill-thread.cpp @@ -0,0 +1,38 @@ +// RUN: %check_clang_tidy %s bugprone-bad-signal-to-kill-thread %t + +#define SIGTERM 15 +#define SIGINT 2 +using pthread_t = int; +using pthread_attr_t = int; + +int pthread_create(pthread_t *thread, const pthread_attr_t *attr, + void *(*start_routine)(void *), void *arg); + +int pthread_kill(pthread_t thread, int sig); + +int pthread_cancel(pthread_t thread); + +void *test_func_return_a_pointer(void *foo); + +int main() { + int result; + pthread_t thread; + + if ((result = pthread_create(&thread, nullptr, test_func_return_a_pointer, 0)) != 0) { + } + if ((result = pthread_kill(thread, SIGTERM)) != 0) { + // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: thread should not be terminated by raising the 'SIGTERM' signal [bugprone-bad-signal-to-kill-thread] + } + + //compliant solution + if ((result = pthread_cancel(thread)) != 0) { + } + + if ((result = pthread_kill(thread, SIGINT)) != 0) { + } + if ((result = pthread_kill(thread, 0xF)) != 0) { + // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: thread should not be terminated by raising the 'SIGTERM' signal [bugprone-bad-signal-to-kill-thread] + } + + return 0; +}