[PATCH] Add clang-tidy check for static or thread_local objects where construction may throw

[aaron.ballman]

Summary

Throwing an exception from the constructor of an object being used with static or thread_local storage duration is a dangerous operation. The exception thrown for an object with static storage duration cannot be caught, even by function-try-blocks in main, and the exception thrown for an object with thread_local storage duration cannot be caught by a function-try-block of the initial thread. This patch adds a checker to flag such constructs.

This check corresponds to the CERT secure coding rule: https://www.securecoding.cert.org/confluence/display/cplusplus/ERR58-CPP.+Constructors+of+objects+with+static+or+thread+storage+duration+must+not+throw+exceptions

[aaron.ballman]

Ensuring unresolved exception specifications are properly handled.

[aaron.ballman]

Btw, here are some statistics as to the rate at which this diagnostic is triggered:

LLVM: 21255 warnings (about 18000 warnings were due to two AST matcher constructors; adding noexcept to those brings the count to 2895 warnings, most of which are cl::opt-related and can be similarly silenced) (2.1MM LoC)
Qt: 0 warnings (4.4MM LoC)
rethinkdb: 44 warnings (5.7MM LoC)
cocos2d: 1339 warnings (984k LoC)
opencv: 203 warnings (657k LoC)

As best I can tell from random sampling, all of the diagnostics are true positives in that the compiler is unaware of whether the constructor will throw or not. However, many of these true positives come from user-provided constructors where the initializer list does all the work, and the constructor body is empty. In the samples I looked at, the constructors do not throw and can be marked noexcept to silence the warning. However, a few of the constructors can throw and this is a definite true positive instead of a practical false positive.

One possible heuristic to silence more of the warnings is to not warn when the constructor called is the default constructor accepting no arguments and the constructor body is trivial, with the belief that default initialization generally does not throw. However, I don't think this diagnostic is chatty enough to warrant that measure when the user can simply add noexcept to the constructor in the event it truly does not throw.

[alexfh]

Sorry for the delay, I was sick last week.

Looks mostly fine.

clang-tidy/cert/CERTTidyModule.cpp

19

Please sort includes.

clang-tidy/cert/StaticObjectExceptionCheck.cpp

19

ThrownExceptionTypeCheck.cpp defines a similar matcher. Looks like we need to move the isNothrow part to the ASTMatchers.h already.

clang-tidy/cert/StaticObjectExceptionCheck.h

19

Maybe address the FIXME right away? ;)

[aaron.ballman]

Updating patch based on review comments.

[aaron.ballman]

In D14824#296574, @alexfh wrote:

Sorry for the delay, I was sick last week.

No worries on the delay, I was sick the last two weeks as well. I hope you are feeling better!

clang-tidy/cert/StaticObjectExceptionCheck.cpp

19

Agreed; implemented.

clang-tidy/cert/StaticObjectExceptionCheck.h

19

LOL! Good catch. :-)

[alexfh]

LG

clang-tidy/utils/Matchers.h

26

Anything wrong with putting this to the astmatchers library?

[aaron.ballman]

Commit in r254415.

clang-tidy/utils/Matchers.h

26

I may move that there post-commit because it seems general-purpose enough for that.

clang-tidy/cert/CERTTidyModule.cpp

 #include "../misc/StaticAssertCheck.h"
 #include "../misc/ThrowByValueCatchByReferenceCheck.h"
 #include "SetLongJmpCheck.h"
+#include "StaticObjectExceptionCheck.h"
 #include "ThrownExceptionTypeCheck.h"
 #include "VariadicFunctionDefCheck.h"
 

     // ERR
     CheckFactories.registerCheck<SetLongJmpCheck>(
         "cert-err52-cpp");
+    CheckFactories.registerCheck<StaticObjectExceptionCheck>(
+        "cert-err58-cpp");
     CheckFactories.registerCheck<ThrownExceptionTypeCheck>(
         "cert-err60-cpp");
     CheckFactories.registerCheck<ThrowByValueCatchByReferenceCheck>(

clang-tidy/cert/CMakeLists.txt

 add_clang_library(clangTidyCERTModule
   CERTTidyModule.cpp
   SetLongJmpCheck.cpp
+  StaticObjectExceptionCheck.cpp
   ThrownExceptionTypeCheck.cpp
   VariadicFunctionDefCheck.cpp
 

   clangTidy
   clangTidyGoogleModule
   clangTidyMiscModule
+  clangTidyUtils
   )

clang-tidy/cert/StaticObjectExceptionCheck.h

+//===--- StaticObjectExceptionCheck.h - clang-tidy---------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_ERR58_CPP_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_ERR58_CPP_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+
+/// Checks whether the constructor for a static or thread_local object will
+/// throw.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cert-static-object-exception.html
+class StaticObjectExceptionCheck : public ClangTidyCheck {
+public:
+  StaticObjectExceptionCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_ERR58_CPP_H
+

clang-tidy/cert/StaticObjectExceptionCheck.cpp

+//===--- StaticObjectExceptionCheck.cpp - clang-tidy-----------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "StaticObjectExceptionCheck.h"
+#include "../utils/Matchers.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+
+void StaticObjectExceptionCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
+
+  // Match any static or thread_local variable declaration that is initialized
+  // with a constructor that can throw.
+  Finder->addMatcher(
+      varDecl(anyOf(hasThreadStorageDuration(), hasStaticStorageDuration()),
+              hasInitializer(cxxConstructExpr(hasDeclaration(
+                  cxxConstructorDecl(unless(matchers::isNoThrow()))
+                      .bind("ctor")))))
+          .bind("var"),
+      this);
+}
+
+void StaticObjectExceptionCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *VD = Result.Nodes.getNodeAs<VarDecl>("var");
+  const auto *Ctor = Result.Nodes.getNodeAs<CXXConstructorDecl>("ctor");
+
+  diag(VD->getLocation(),
+       "construction of %0 with %select{static|thread_local}1 storage "
+       "duration may throw an exception that cannot be caught")
+      << VD << (VD->getStorageDuration() == SD_Static ? 0 : 1);
+  diag(Ctor->getLocation(), "possibly throwing constructor declared here",
+       DiagnosticIDs::Note);
+}
+
+} // namespace tidy
+} // namespace clang
+

clang-tidy/cert/ThrownExceptionTypeCheck.cpp

 //===----------------------------------------------------------------------===//
 
 #include "ThrownExceptionTypeCheck.h"
+#include "../utils/Matchers.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 

 using namespace clang::ast_matchers;
 
 namespace clang {
-namespace {
-AST_MATCHER(CXXConstructorDecl, isNoThrowCopyConstructor) {
-  if (!Node.isCopyConstructor())
-    return false;
-
-  const auto *FnTy = Node.getType()->getAs<FunctionProtoType>();
-  // Assume the best for any unresolved exception specification.
-  if (isUnresolvedExceptionSpec(FnTy->getExceptionSpecType()))
-    return true;
-
-  return FnTy->isNothrow(Node.getASTContext());
-}
-} // end namespace
-
 namespace tidy {
 void ThrownExceptionTypeCheck::registerMatchers(MatchFinder *Finder) {
   if (!getLangOpts().CPlusPlus)

   Finder->addMatcher(
       cxxThrowExpr(
           has(cxxConstructExpr(hasDeclaration(cxxConstructorDecl(
-              isCopyConstructor(), unless(isNoThrowCopyConstructor()))))
+              isCopyConstructor(), unless(matchers::isNoThrow()))))
           .bind("expr"))),
       this);
 }

clang-tidy/utils/Matchers.h

   return IsExpensive && *IsExpensive;
 }
 
+AST_MATCHER(FunctionDecl, isNoThrow) {
+  const auto *FnTy = Node.getType()->getAs<FunctionProtoType>();
+  
+  // If the function does not have a prototype, then it is assumed to be a
+  // throwing function (as it would if the function did not have any exception
+  // specification).
+  if (!FnTy)
+    return false;
+
+  // Assume the best for any unresolved exception specification.
+  if (isUnresolvedExceptionSpec(FnTy->getExceptionSpecType()))
+    return true;
+
+  return FnTy->isNothrow(Node.getASTContext());
+}
+
 } // namespace matchers
 } // namespace tidy
 } // namespace clang

docs/clang-tidy/checks/cert-static-object-exception.rst

+cert-err58-cpp
+==============
+
+This check flags all static or thread_local variable declarations where the
+constructor for the object may throw an exception.
+
+This check corresponds to the CERT C++ Coding Standard rule
+`ERR58-CPP. Constructors of objects with static or thread storage duration must not throw exceptions
+<https://www.securecoding.cert.org/confluence/display/cplusplus/ERR58-CPP.+Constructors+of+objects+with+static+or+thread+storage+duration+must+not+throw+exceptions>`_.

docs/clang-tidy/checks/list.rst

 List of clang-tidy Checks
 =========================
 
-.. toctree::
+.. toctree::   
    cert-setlongjmp
+   cert-static-object-exception
    cert-thrown-exception-type
    cert-variadic-function-def
    cppcoreguidelines-pro-bounds-array-to-pointer-decay

test/clang-tidy/cert-static-object-exception.cpp

+// RUN: %check_clang_tidy %s cert-err58-cpp %t
+
+struct S {
+  S() noexcept(false);
+};
+
+struct T {
+  T() noexcept;
+};
+
+struct U {
+  U() {}
+};
+
+struct V {
+  explicit V(const char *) {} // Can throw
+};
+
+
+S s;
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: construction of 's' with static storage duration may throw an exception that cannot be caught [cert-err58-cpp]
+// CHECK-MESSAGES: 4:3: note: possibly throwing constructor declared here
+T t; // ok
+U u;
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: construction of 'u' with static storage duration may throw an exception that cannot be caught
+// CHECK-MESSAGES: 12:3: note: possibly throwing constructor declared here
+V v("v");
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: construction of 'v' with static storage duration may throw an exception that cannot be caught
+// CHECK-MESSAGES: 16:12: note: possibly throwing constructor declared here
+
+void f(S s1, T t1, U u1, V v1) { // ok, ok, ok, ok
+  S s2; // ok
+  T t2; // ok
+  U u2; // ok
+  V v2("v"); // ok
+
+  thread_local S s3;
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: construction of 's3' with thread_local storage duration may throw an exception that cannot be caught
+  thread_local T t3; // ok
+  thread_local U u3;
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: construction of 'u3' with thread_local storage duration may throw an exception that cannot be caught
+  thread_local V v3("v");
+  // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: construction of 'v3' with thread_local storage duration may throw an exception that cannot be caught
+
+  static S s4;
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: construction of 's4' with static storage duration may throw an exception that cannot be caught
+  static T t4; // ok
+  static U u4;
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: construction of 'u4' with static storage duration may throw an exception that cannot be caught
+  static V v4("v");
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: construction of 'v4' with static storage duration may throw an exception that cannot be caught
+}