[clang-tidy] Adding misc-signal-terminated-thread check

[abelkocsis]

Summary

According to
https://wiki.sei.cmu.edu/confluence/display/c/POS44-C.+Do+not+use+signals+to+terminate+threads
misc-signal-terminated-thread check is created. The check warn if
pthread_kill function is called with SIGTERM signal.

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/misc/BadSignalToKillThreadCheck.cpp

28 ↗

(On Diff #225641)

static is enough, no need for anonymous namespace.

33 ↗

(On Diff #225641)

Unnecessary empty line.

37 ↗

(On Diff #225641)

Please don't use auto unless type is spelled in same statement or iterator.

67 ↗

(On Diff #225641)

Please add new line.

clang-tools-extra/clang-tidy/misc/BadSignalToKillThreadCheck.h

40 ↗

(On Diff #225641)

Please add new line.

clang-tools-extra/docs/ReleaseNotes.rst

82

Please keep alphabetical order in new checks list.

85

Please synchronize with first sentence in documentation. Link belongs to documentation.

clang-tools-extra/docs/clang-tidy/checks/misc-bad-signal-to-kill-thread.rst

8 ↗

(On Diff #225641)

Please separate with empty line.

11 ↗

(On Diff #225641)

Please add new line.

clang-tools-extra/test/clang-tidy/misc-bad-signal-to-kill-thread.cpp

31 ↗

(On Diff #225641)

Please add new line.

[steakhal]

clang-tools-extra/clang-tidy/misc/BadSignalToKillThreadCheck.cpp

34–48 ↗

(On Diff #225641)

I think using while loop here makes this less readable compared to using an algorithm like llvm::find_if.

I think a more declarative approach would help here.

• Handles properly if the definition of SIGTERM uses other than decimal base.
• Checks if ValueStr.getAsInteger() succeeded.

Like.:

const auto IsSigterm = [](const auto &KeyValue) -> bool {
  return KeyValue.first->getName() == "SIGTERM";
};
const auto TryExpandAsInteger =
    [PP = PP](Preprocessor::macro_iterator It) -> Optional<unsigned> {
  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;

55 ↗

(On Diff #225641)

I think it would be valuable to mention the name of the signal.

clang-tools-extra/docs/ReleaseNotes.rst

82–83

Your checker's name is different from these. Could you check these two lines please?

clang-tools-extra/docs/clang-tidy/checks/misc-bad-signal-to-kill-thread.rst

7 ↗

(On Diff #225641)

It would be kind to offer a compliant solution for fixing the warning.

clang-tools-extra/test/clang-tidy/misc-bad-signal-to-kill-thread.cpp

26 ↗

(On Diff #225641)

Maybe worth to note here that this would be the compliant solution.

Also check if it doesn't warn for different signals, but warns for the pthread_kill(thread, 0xF))

[abelkocsis]

Thank you for the reviews!

[abelkocsis]

Auto types replaced on checker files. New lines added when necessary.
Reordered ReleaseNotes by alphabetical order. Checker name is
updated on ReleaseNotes, compliant solution is wrote to the
documentation. Another two test is written.

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/misc/BadSignalToKillThreadCheck.cpp

31 ↗

(On Diff #225769)

In this case auto is reasonable, because this is iterator.

clang-tools-extra/docs/ReleaseNotes.rst

127

Please synchronize with first statement in documentation.

129

Please move this statement at the end of documentation.

[abelkocsis]

Header documentation and Release Notes synchronised. Documentation of
checker updated. Type changed to auto type when necessary.

[Eugene.Zelenko]

clang-tools-extra/docs/clang-tidy/checks/misc-bad-signal-to-kill-thread.rst

9 ↗

(On Diff #225784)

Usually links to documentation are located at the end of documentation. See other checks documentation as examples.

[abelkocsis]

Test clang-formatted. Uncaught word removed from documentations.

[abelkocsis]

The documentation of the checker updated.

[abelkocsis]

Findig SIGTERM replaced on checker

[aaron.ballman]

Thank you for this! I don't think that the check should live in misc. It should have an alias in the CERT module, but maybe we want to have this live in a posix module? If not, this should probably live in bugprone.

clang-tools-extra/clang-tidy/misc/BadSignalToKillThreadCheck.cpp

31 ↗

(On Diff #225834)

You can drop the trailing return type on the lambda.

35 ↗

(On Diff #225834)

This lambda capture looks suspicious -- why do you need to initialize the capture?

59 ↗

(On Diff #225834)

Clang-tidy diagnostics are not supposed to be grammatically correct. I think a better way to phrase it might be something like: thread should not be terminated by raising the 'SIGTERM' signal

clang-tools-extra/docs/clang-tidy/checks/misc-bad-signal-to-kill-thread.rst

6 ↗

(On Diff #225834)

when thread is terminated by -> when a thread is terminated by raising the

8 ↗

(On Diff #225834)

Why does the check not look for SIGKILL as well as SIGTERM?

[steakhal]

clang-tools-extra/clang-tidy/misc/BadSignalToKillThreadCheck.cpp

28 ↗

(On Diff #225834)

Can't we place this PP variable to the checker class?
Like the modernize/UseTrailingReturnTypeCheck does?

35 ↗

(On Diff #225834)

You are right, I forgot that PP is a global variable when I offered this solution.
Global variables should not be captured, so empty capture is good enough.

[abelkocsis]

Checker moved to bugprone module, cert alias added. Small fixes in checker
file, warning message updated.

[abelkocsis]

clang-tools-extra/docs/clang-tidy/checks/misc-bad-signal-to-kill-thread.rst

8 ↗

(On Diff #225834)

In my opinion SIGKILL is used only in cases when the programmer wants the entire process to terminate. I updated the docs not mentioning SIGKILL.

[abelkocsis]

Small fixes

[Eugene.Zelenko]

clang-tools-extra/docs/clang-tidy/checks/bugprone-bad-signal-to-kill-thread.rst

4

Please make same length as title above.

[abelkocsis]

Small fix in documentation

[Eugene.Zelenko]

You should also add actual alias code to cert module.

[abelkocsis]

Alias added to cert module

[Eugene.Zelenko]

Will be good idea to mention alias in Release Notes too (in aliases section, in alphabetical order).

[aaron.ballman]

LGTM aside from the release notes alias request.

[Charusso]

Forgotten amend of commit message. Commited as:

https://github.com/llvm/llvm-project/commit/8d288a0668a574863d52784084ff565c89f7366e

clang-tools-extra/clang-tidy/bugprone/BadSignalToKillThreadCheck.h

+//===--- 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<unsigned> SigtermValue;
+};
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_BADSIGNALTOKILLTHREADCHECK_H

clang-tools-extra/clang-tidy/bugprone/BadSignalToKillThreadCheck.cpp

+//===--- 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<unsigned> {
+    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<Expr>("thread-kill");
+  const auto *MatchedIntLiteral =
+      Result.Nodes.getNodeAs<IntegerLiteral>("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

clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp

 //===--- BugproneTidyModule.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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "../cppcoreguidelines/NarrowingConversionsCheck.h"
 #include "ArgumentCommentCheck.h"
 #include "AssertSideEffectCheck.h"
+#include "BadSignalToKillThreadCheck.h"
 #include "BoolPointerImplicitConversionCheck.h"
 #include "BranchCloneCheck.h"
 #include "CopyConstructorInitCheck.h"
 #include "DanglingHandleCheck.h"
 #include "DynamicStaticInitializersCheck.h"
 #include "ExceptionEscapeCheck.h"
 #include "FoldInitTypeCheck.h"
 #include "ForwardDeclarationNamespaceCheck.h"

 
 class BugproneModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<ArgumentCommentCheck>(
         "bugprone-argument-comment");
     CheckFactories.registerCheck<AssertSideEffectCheck>(
         "bugprone-assert-side-effect");
+    CheckFactories.registerCheck<BadSignalToKillThreadCheck>(
+        "bugprone-bad-signal-to-kill-thread");
     CheckFactories.registerCheck<BoolPointerImplicitConversionCheck>(
         "bugprone-bool-pointer-implicit-conversion");
     CheckFactories.registerCheck<BranchCloneCheck>(
         "bugprone-branch-clone");
     CheckFactories.registerCheck<CopyConstructorInitCheck>(
         "bugprone-copy-constructor-init");
     CheckFactories.registerCheck<DanglingHandleCheck>(
         "bugprone-dangling-handle");

clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyBugproneModule
   ArgumentCommentCheck.cpp
   AssertSideEffectCheck.cpp
+  BadSignalToKillThreadCheck.cpp
   BoolPointerImplicitConversionCheck.cpp
   BranchCloneCheck.cpp
   BugproneTidyModule.cpp
   CopyConstructorInitCheck.cpp
   DanglingHandleCheck.cpp
   DynamicStaticInitializersCheck.cpp
   ExceptionEscapeCheck.cpp
   FoldInitTypeCheck.cpp

clang-tools-extra/docs/ReleaseNotes.rst

 Improvements to clang-rename
 ----------------------------
 
 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
+- New :doc:`bugprone-bad-signal-to-kill-thread
+  <clang-tidy/checks/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
   <clang-tidy/checks/bugprone-dynamic-static-initializers>` check.
 
   Finds instances where variables with static storage are initialized
   dynamically in header files.
 
 - New :doc:`bugprone-infinite-loop
   <clang-tidy/checks/bugprone-infinite-loop>` check.
 
   Finds obvious infinite loops (loops where the condition variable is not
   changed at all).
 
 - New :doc:`bugprone-not-null-terminated-result
   <clang-tidy/checks/bugprone-not-null-terminated-result>` check
 
   Finds function calls where it is possible to cause a not null-terminated
   result. Usually the proper length of a string is ``strlen(str) + 1`` or equal
   length of this expression, because the null terminator needs an extra space.
- New :doc:`llvm-prefer-register-over-unsigned
   <clang-tidy/checks/llvm-prefer-register-over-unsigned>` check.
 
   Finds historical use of ``unsigned`` to hold vregs and physregs and rewrites
   them to use ``Register``
 
 - New :doc:`objc-missing-hash
   <clang-tidy/checks/objc-missing-hash>` check.
 
   Finds Objective-C implementations that implement ``-isEqual:`` without also
   appropriately implementing ``-hash``.
 
 - Improved :doc:`bugprone-posix-return
   <clang-tidy/checks/bugprone-posix-return>` check.
 
   Now also checks if any calls to ``pthread_*`` functions expect negative return
   values.
 
 - The 'objc-avoid-spinlock' check was renamed to :doc:`darwin-avoid-spinlock
   <clang-tidy/checks/darwin-avoid-spinlock>`

clang-tools-extra/docs/clang-tidy/checks/bugprone-bad-signal-to-kill-thread.rst

+.. 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
+<https://wiki.sei.cmu.edu/confluence/display/c/POS44-C.+Do+not+use+signals+to+terminate+threads>`_.

clang-tools-extra/docs/clang-tidy/checks/cert-pos44-c.rst

+.. 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 <bugprone-bad-signal-to-kill-thread.html>`_ for more information.

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. toctree::
    android-cloexec-open
    android-cloexec-pipe
    android-cloexec-pipe2
    android-cloexec-socket
    android-comparison-in-temp-failure-retry
    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
    bugprone-dangling-handle
    bugprone-dynamic-static-initializers
    bugprone-exception-escape
    bugprone-fold-init-type
    bugprone-forward-declaration-namespace
.. toctree::
    cert-fio38-c (redirects to misc-non-copyable-objects) <cert-fio38-c>
    cert-flp30-c
    cert-msc30-c (redirects to cert-msc50-cpp) <cert-msc30-c>
    cert-msc32-c (redirects to cert-msc51-cpp) <cert-msc32-c>
    cert-msc50-cpp
    cert-msc51-cpp
    cert-oop11-cpp (redirects to performance-move-constructor-init) <cert-oop11-cpp>
    cert-oop54-cpp (redirects to bugprone-unhandled-self-assignment) <cert-oop54-cpp>
+   cert-pos44-c (redirects to bugprone-bad-signal-to-kill-thread) <cert-pos44-c>
    clang-analyzer-core.CallAndMessage
    clang-analyzer-core.DivideZero
    clang-analyzer-core.DynamicTypePropagation
    clang-analyzer-core.NonNullParamChecker
    clang-analyzer-core.NullDereference
    clang-analyzer-core.StackAddressEscape
    clang-analyzer-core.UndefinedBinaryOperatorResult
    clang-analyzer-core.VLASize

clang-tools-extra/test/clang-tidy/bugprone-bad-signal-to-kill-thread.cpp

+// 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;
+}