[clang-tidy] implement concurrency-mt-unsafe

Checks for some thread-unsafe functions against a black list
of known-to-be-unsafe functions. Usually they access static variables
without synchronization (e.g. gmtime(3)) or utilize signals
in a racy way (e.g. sleep(3)).

The patch adds a check instead of auto-fix as thread-safe alternatives
usually have API with an additional argument
(e.g. gmtime(3) v.s. gmtime_r(3)) or have a different semantics
(e.g. exit(3) v.s. __exit(3)), so it is a rather tricky
or non-expected fix.

An option specifies which functions in libc should be considered
thread-safe, possible values are `posix`, `glibc`,
or `any` (the most strict check). It defaults to 'any' as it is
unknown what target libc type is - clang-tidy may be run
on linux but check sources compiled for other *NIX.

The check is used in Yandex Taxi backend and has caught
many unpleasant bugs. A similar patch for coroutine-unsafe API
is coming next.

Reviewed By: lebedev.ri

Differential Revision: https://reviews.llvm.org/D90944

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

@@ -4,6 +4,7 @@ set(LLVM_LINK_COMPONENTS
 
 add_clang_library(clangTidyConcurrencyModule
   ConcurrencyTidyModule.cpp
+  MtUnsafeCheck.cpp
 
   LINK_LIBS
   clangTidy

clang-tools-extra/clang-tidy/concurrency/ConcurrencyTidyModule.cpp

@@ -9,6 +9,7 @@
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "MtUnsafeCheck.h"
 
 namespace clang {
 namespace tidy {
@@ -16,7 +17,10 @@ namespace concurrency {
 
 class ConcurrencyModule : public ClangTidyModule {
 public:
-  void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {}
+  void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<concurrency::MtUnsafeCheck>(
+        "concurrency-mt-unsafe");
+  }
 };
 
 } // namespace concurrency

clang-tools-extra/clang-tidy/concurrency/MtUnsafeCheck.cpp

@@ -0,0 +1,316 @@
+//===--- MtUnsafeCheck.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 "MtUnsafeCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+// Initial list was extracted from gcc documentation
+static const clang::StringRef GlibcFunctions[] = {
+    "::argp_error",
+    "::argp_help",
+    "::argp_parse",
+    "::argp_state_help",
+    "::argp_usage",
+    "::asctime",
+    "::clearenv",
+    "::crypt",
+    "::ctime",
+    "::cuserid",
+    "::drand48",
+    "::ecvt",
+    "::encrypt",
+    "::endfsent",
+    "::endgrent",
+    "::endhostent",
+    "::endnetent",
+    "::endnetgrent",
+    "::endprotoent",
+    "::endpwent",
+    "::endservent",
+    "::endutent",
+    "::endutxent",
+    "::erand48",
+    "::error_at_line",
+    "::exit",
+    "::fcloseall",
+    "::fcvt",
+    "::fgetgrent",
+    "::fgetpwent",
+    "::gammal",
+    "::getchar_unlocked",
+    "::getdate",
+    "::getfsent",
+    "::getfsfile",
+    "::getfsspec",
+    "::getgrent",
+    "::getgrent_r",
+    "::getgrgid",
+    "::getgrnam",
+    "::gethostbyaddr",
+    "::gethostbyname",
+    "::gethostbyname2",
+    "::gethostent",
+    "::getlogin",
+    "::getmntent",
+    "::getnetbyaddr",
+    "::getnetbyname",
+    "::getnetent",
+    "::getnetgrent",
+    "::getnetgrent_r",
+    "::getopt",
+    "::getopt_long",
+    "::getopt_long_only",
+    "::getpass",
+    "::getprotobyname",
+    "::getprotobynumber",
+    "::getprotoent",
+    "::getpwent",
+    "::getpwent_r",
+    "::getpwnam",
+    "::getpwuid",
+    "::getservbyname",
+    "::getservbyport",
+    "::getservent",
+    "::getutent",
+    "::getutent_r",
+    "::getutid",
+    "::getutid_r",
+    "::getutline",
+    "::getutline_r",
+    "::getutxent",
+    "::getutxid",
+    "::getutxline",
+    "::getwchar_unlocked",
+    "::glob",
+    "::glob64",
+    "::gmtime",
+    "::hcreate",
+    "::hdestroy",
+    "::hsearch",
+    "::innetgr",
+    "::jrand48",
+    "::l64a",
+    "::lcong48",
+    "::lgammafNx",
+    "::localeconv",
+    "::localtime",
+    "::login",
+    "::login_tty",
+    "::logout",
+    "::logwtmp",
+    "::lrand48",
+    "::mallinfo",
+    "::mallopt",
+    "::mblen",
+    "::mbrlen",
+    "::mbrtowc",
+    "::mbsnrtowcs",
+    "::mbsrtowcs",
+    "::mbtowc",
+    "::mcheck",
+    "::mprobe",
+    "::mrand48",
+    "::mtrace",
+    "::muntrace",
+    "::nrand48",
+    "::__ppc_get_timebase_freq",
+    "::ptsname",
+    "::putchar_unlocked",
+    "::putenv",
+    "::pututline",
+    "::pututxline",
+    "::putwchar_unlocked",
+    "::qecvt",
+    "::qfcvt",
+    "::register_printf_function",
+    "::seed48",
+    "::setenv",
+    "::setfsent",
+    "::setgrent",
+    "::sethostent",
+    "::sethostid",
+    "::setkey",
+    "::setlocale",
+    "::setlogmask",
+    "::setnetent",
+    "::setnetgrent",
+    "::setprotoent",
+    "::setpwent",
+    "::setservent",
+    "::setutent",
+    "::setutxent",
+    "::siginterrupt",
+    "::sigpause",
+    "::sigprocmask",
+    "::sigsuspend",
+    "::sleep",
+    "::srand48",
+    "::strerror",
+    "::strsignal",
+    "::strtok",
+    "::tcflow",
+    "::tcsendbreak",
+    "::tmpnam",
+    "::ttyname",
+    "::unsetenv",
+    "::updwtmp",
+    "::utmpname",
+    "::utmpxname",
+    "::valloc",
+    "::vlimit",
+    "::wcrtomb",
+    "::wcsnrtombs",
+    "::wcsrtombs",
+    "::wctomb",
+    "::wordexp",
+};
+
+static const clang::StringRef PosixFunctions[] = {
+    "::asctime",
+    "::basename",
+    "::catgets",
+    "::crypt",
+    "::ctime",
+    "::dbm_clearerr",
+    "::dbm_close",
+    "::dbm_delete",
+    "::dbm_error",
+    "::dbm_fetch",
+    "::dbm_firstkey",
+    "::dbm_nextkey",
+    "::dbm_open",
+    "::dbm_store",
+    "::dirname",
+    "::dlerror",
+    "::drand48",
+    "::encrypt",
+    "::endgrent",
+    "::endpwent",
+    "::endutxent",
+    "::ftw",
+    "::getc_unlocked",
+    "::getchar_unlocked",
+    "::getdate",
+    "::getenv",
+    "::getgrent",
+    "::getgrgid",
+    "::getgrnam",
+    "::gethostent",
+    "::getlogin",
+    "::getnetbyaddr",
+    "::getnetbyname",
+    "::getnetent",
+    "::getopt",
+    "::getprotobyname",
+    "::getprotobynumber",
+    "::getprotoent",
+    "::getpwent",
+    "::getpwnam",
+    "::getpwuid",
+    "::getservbyname",
+    "::getservbyport",
+    "::getservent",
+    "::getutxent",
+    "::getutxid",
+    "::getutxline",
+    "::gmtime",
+    "::hcreate",
+    "::hdestroy",
+    "::hsearch",
+    "::inet_ntoa",
+    "::l64a",
+    "::lgamma",
+    "::lgammaf",
+    "::lgammal",
+    "::localeconv",
+    "::localtime",
+    "::lrand48",
+    "::mrand48",
+    "::nftw",
+    "::nl_langinfo",
+    "::ptsname",
+    "::putc_unlocked",
+    "::putchar_unlocked",
+    "::putenv",
+    "::pututxline",
+    "::rand",
+    "::readdir",
+    "::setenv",
+    "::setgrent",
+    "::setkey",
+    "::setpwent",
+    "::setutxent",
+    "::strerror",
+    "::strsignal",
+    "::strtok",
+    "::system",
+    "::ttyname",
+    "::unsetenv",
+    "::wcstombs",
+    "::wctomb",
+};
+
+namespace clang {
+namespace tidy {
+
+template <> struct OptionEnumMapping<concurrency::MtUnsafeCheck::FunctionSet> {
+  static llvm::ArrayRef<
+      std::pair<concurrency::MtUnsafeCheck::FunctionSet, StringRef>>
+  getEnumMapping() {
+    static constexpr std::pair<concurrency::MtUnsafeCheck::FunctionSet,
+                               StringRef>
+        Mapping[] = {{concurrency::MtUnsafeCheck::FunctionSet::Posix, "posix"},
+                     {concurrency::MtUnsafeCheck::FunctionSet::Glibc, "glibc"},
+                     {concurrency::MtUnsafeCheck::FunctionSet::Any, "any"}};
+    return makeArrayRef(Mapping);
+  }
+};
+
+namespace concurrency {
+
+static ast_matchers::internal::Matcher<clang::NamedDecl>
+hasAnyMtUnsafeNames(MtUnsafeCheck::FunctionSet libc) {
+  switch (libc) {
+  case MtUnsafeCheck::FunctionSet::Posix:
+    return hasAnyName(PosixFunctions);
+  case MtUnsafeCheck::FunctionSet::Glibc:
+    return hasAnyName(GlibcFunctions);
+  case MtUnsafeCheck::FunctionSet::Any:
+    return anyOf(hasAnyName(PosixFunctions), hasAnyName(GlibcFunctions));
+  }
+  llvm_unreachable("invalid FunctionSet");
+}
+
+MtUnsafeCheck::MtUnsafeCheck(StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      FuncSet(Options.get("FunctionSet", MtUnsafeCheck::FunctionSet::Any)) {}
+
+void MtUnsafeCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "FunctionSet", FuncSet);
+}
+
+void MtUnsafeCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      callExpr(callee(functionDecl(hasAnyMtUnsafeNames(FuncSet))))
+          .bind("mt-unsafe"),
+      this);
+}
+
+void MtUnsafeCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *Call = Result.Nodes.getNodeAs<CallExpr>("mt-unsafe");
+  assert(Call && "Unhandled binding in the Matcher");
+
+  diag(Call->getBeginLoc(), "function is not thread safe");
+}
+
+} // namespace concurrency
+} // namespace tidy
+} // namespace clang

clang-tools-extra/clang-tidy/concurrency/MtUnsafeCheck.h

@@ -0,0 +1,43 @@
+//===--- MtUnsafeCheck.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_CONCURRENCY_MTUNSAFECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CONCURRENCY_MTUNSAFECHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace concurrency {
+
+/// Checks that non-thread-safe functions are not used.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/threads-mt-unsafe.html
+class MtUnsafeCheck : public ClangTidyCheck {
+public:
+  MtUnsafeCheck(StringRef Name, ClangTidyContext *Context);
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+  enum class FunctionSet {
+    Posix,
+    Glibc,
+    Any,
+  };
+
+private:
+  const FunctionSet FuncSet;
+};
+
+} // namespace concurrency
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CONCURRENCY_MTUNSAFECHECK_H

clang-tools-extra/docs/ReleaseNotes.rst

@@ -117,6 +117,12 @@ New checks
   Finds condition variables in nested ``if`` statements that were also checked
   in the outer ``if`` statement and were not changed.
 
+- New :doc:`concurrency-mt-unsafe <clang-tidy/checks/concurrency-mt-unsafe>`
+  check.
+
+  Finds thread-unsafe functions usage. Currently knows about POSIX and
+  Glibc function sets.
+
 - New :doc:`bugprone-signal-handler
   <clang-tidy/checks/bugprone-signal-handler>` check.
 

clang-tools-extra/docs/clang-tidy/checks/concurrency-mt-unsafe.rst

@@ -0,0 +1,52 @@
+.. title:: clang-tidy - concurrency-mt-unsafe
+
+concurrency-mt-unsafe
+=====================
+
+Checks for some thread-unsafe functions against a black list of
+known-to-be-unsafe functions. Usually they access static variables without
+synchronization (e.g. gmtime(3)) or utilize signals in a racy way.
+The set of functions to check is specified with the `FunctionSet` option.
+
+Note that using some thread-unsafe functions may be still valid in
+concurrent programming if only a single thread is used (e.g. setenv(3)),
+however, some functions may track a state in global variables which
+would be clobbered by subsequent (non-parallel, but concurrent) calls to
+a related function. E.g. the following code suffers from unprotected
+accesses to a global state:
+
+.. code-block:: c++
+
+    // getnetent(3) maintains global state with DB connection, etc.
+    // If a concurrent green thread calls getnetent(3), the global state is corrupted.
+    netent = getnetent();
+    yield();
+    netent = getnetent();
+
+
+Examples:
+
+.. code-block:: c++
+
+    tm = gmtime(timep); // uses a global buffer
+
+    sleep(1); // implementation may use SIGALRM
+
+.. option:: FunctionSet
+
+  Specifies which functions in libc should be considered thread-safe,
+  possible values are `posix`, `glibc`, or `any`.
+
+  `posix` means POSIX defined thread-unsafe functions. POSIX.1-2001
+  in "2.9.1 Thread-Safety" defines that all functions specified in the
+  standard are thread-safe except a predefined list of thread-unsafe
+  functions.
+
+  Glibc defines some of them as thread-safe (e.g. dirname(3)), but adds
+  non-POSIX thread-unsafe ones (e.g. getopt_long(3)). Glibc's list is
+  compiled from GNU web documentation with a search for MT-Safe tag:
+  https://www.gnu.org/software/libc/manual/html_node/POSIX-Safety-Concepts.html
+
+  If you want to identify thread-unsafe API for at least one libc or
+  unsure which libc will be used, use `any` (default).
+

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

@@ -138,6 +138,7 @@ Clang-Tidy Checks
    `clang-analyzer-valist.CopyToSelf <clang-analyzer-valist.CopyToSelf.html>`_,
    `clang-analyzer-valist.Uninitialized <clang-analyzer-valist.Uninitialized.html>`_,
    `clang-analyzer-valist.Unterminated <clang-analyzer-valist.Unterminated.html>`_,
+   `concurrency-mt-unsafe <concurrency-mt-unsafe.html>`_,
    `cppcoreguidelines-avoid-goto <cppcoreguidelines-avoid-goto.html>`_,
    `cppcoreguidelines-avoid-non-const-global-variables <cppcoreguidelines-avoid-non-const-global-variables.html>`_,
    `cppcoreguidelines-init-variables <cppcoreguidelines-init-variables.html>`_, "Yes"

clang-tools-extra/test/clang-tidy/checkers/concurrency-mt-unsafe-any.cpp

@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s concurrency-mt-unsafe %t
+
+extern unsigned int sleep (unsigned int __seconds);
+extern int *gmtime (const int *__timer);
+extern int *gmtime_r (const int *__timer, char*);
+extern char *dirname (char *__path);
+
+void foo() {
+  sleep(2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function is not thread safe [concurrency-mt-unsafe]
+  ::sleep(2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function is not thread safe [concurrency-mt-unsafe]
+
+  auto tm = gmtime(nullptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: function is not thread safe [concurrency-mt-unsafe]
+  tm = ::gmtime(nullptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: function is not thread safe [concurrency-mt-unsafe]
+
+  tm = gmtime_r(nullptr, nullptr);
+  tm = ::gmtime_r(nullptr, nullptr);
+
+  dirname(nullptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function is not thread safe [concurrency-mt-unsafe]
+}

clang-tools-extra/test/clang-tidy/checkers/concurrency-mt-unsafe-glibc.cpp

@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s concurrency-mt-unsafe %t -- -config='{CheckOptions: [{key: "concurrency-mt-unsafe.FunctionSet", value: "glibc"}]}'
+
+extern unsigned int sleep (unsigned int __seconds);
+extern int *gmtime (const int *__timer);
+extern char *dirname (char *__path);
+
+void foo() {
+  sleep(2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function is not thread safe [concurrency-mt-unsafe]
+
+  ::sleep(2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function is not thread safe [concurrency-mt-unsafe]
+
+  dirname(nullptr);
+}

clang-tools-extra/test/clang-tidy/checkers/concurrency-mt-unsafe-posix.cpp

@@ -0,0 +1,22 @@
+// RUN: %check_clang_tidy %s concurrency-mt-unsafe %t -- -config='{CheckOptions: [{key: "concurrency-mt-unsafe.FunctionSet", value: "posix"}]}'
+
+extern unsigned int sleep (unsigned int __seconds);
+extern int *gmtime (const int *__timer);
+extern int *gmtime_r (const int *__timer, char*);
+extern char *dirname (char *__path);
+
+void foo() {
+  sleep(2);
+  ::sleep(2);
+
+  auto tm = gmtime(nullptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: function is not thread safe [concurrency-mt-unsafe]
+  tm = ::gmtime(nullptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: function is not thread safe [concurrency-mt-unsafe]
+
+  tm = gmtime_r(nullptr, nullptr);
+  tm = ::gmtime_r(nullptr, nullptr);
+
+  dirname(nullptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function is not thread safe [concurrency-mt-unsafe]
+}