[PATCH] clang-tidy checker for nothrow copy constructible exception objects

[aaron.ballman]

Summary

This patch adds a new clang-tidy checker that flags throw expressions whose thrown type is not nothrow copy constructible. While the compiler is free to elide copy constructor calls in some cases, it is under no obligation to do so, which makes the code a portability concern as well as a security concern.

This checker corresponds to the CERT secure coding rule: https://www.securecoding.cert.org/confluence/display/cplusplus/ERR60-CPP.+Exception+objects+must+be+nothrow+copy+constructible

[bcraig]

I would love to see a test case with dynamic allocation. Something along the following...

struct Allocates {

int *x;
Allocates() : x(new int(0)) {}
Allocates(const Allocates &other) : x(new int(*other.x)) {}

};

... and then the flip side of that ...

struct OptionallyAllocates {

int *x;
OptionallyAllocates () : x(new int(0)) {}
OptionallyAllocates (const Allocates &other) {
  // try / catch(...) would be fine here if std::nothrow is unappealing
  x = new(std::nothrow) int(*other.x));
}

};

test/clang-tidy/cert-throw-exception-type.cpp

2

Doesn't this need an "| FileCheck %s"

[aaron.ballman]

Added new test cases

[aaron.ballman]

test/clang-tidy/cert-throw-exception-type.cpp

3

Doesn't this need an "| FileCheck %s"

No, check_clang_tidy handles that for you.

[alexfh]

clang-tidy/cert/ThrownExceptionTypeCheck.cpp

37

I'd slightly prefer this check to be expressed as a matcher:

cxxThrowExpr(has(cxxConstructExpr(hasDeclaration(<insert your isNothrow() matcher here>))))

Which is also a more correct thing to do and doesn't seem to be "a lot harder" ;)

[bcraig]

Looks good to me.

[aaron.ballman]

Addressing review comments.

[aaron.ballman]

clang-tidy/cert/ThrownExceptionTypeCheck.cpp

38

Your slight preference is my firmest desire! :-P Seriously, I think the code is more clear after implementing your suggestion, so thank you!

[alexfh]

Much better now. A few more comments.

clang-tidy/cert/ThrownExceptionTypeCheck.cpp

19

nit: I suggest changing isNoThrowCopyConstructible to isNoThrowCopyConstructor, because "constructible" is a trait of a class, not its constructor.

23

Out of curiosity: are we short-circuiting here for performance reasons or would the code below return false in this case?

35

Is this return reachable? If yes, when exactly does this happen and is there a test for this case?

[aaron.ballman]

clang-tidy/cert/ThrownExceptionTypeCheck.cpp

19

Good catch; that was a holdover from the previous version where it was operating on the CXXRecordDecl instead.

23

I pulled the logic from the unary type trait evaluation logic, but stripped out the bits that were already handled by the AST matcher logic itself. I don't believe this is actually necessary for this checker, however.

35

No, it is not reachable. There's no way, that I am aware of, to get a copy constructor with no function prototype.

[aaron.ballman]

Updated to address review comments.

[alexfh]

Looks good with one nit. Thank you!

clang-tidy/cert/ThrownExceptionTypeCheck.cpp

23

The comment is not relevant now.

[aaron.ballman]

Commit in r253246

clang-tidy/cert/CERTTidyModule.cpp

 #include "../misc/StaticAssertCheck.h"
 #include "../misc/ThrowByValueCatchByReferenceCheck.h"
 #include "SetLongJmpCheck.h"
+#include "ThrownExceptionTypeCheck.h"
 #include "VariadicFunctionDefCheck.h"
 
 namespace clang {

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

clang-tidy/cert/CMakeLists.txt

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

clang-tidy/cert/ThrownExceptionTypeCheck.h

+//===--- ThrownExceptionTypeCheck.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_THROWNEXCEPTIONTYPECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_THROWNEXCEPTIONTYPECHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+
+/// Checks whether a thrown object is nothrow copy constructible.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cert-thrown-exception-type.html
+class ThrownExceptionTypeCheck : public ClangTidyCheck {
+public:
+  ThrownExceptionTypeCheck(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_THROWNEXCEPTIONTYPECHECK_H
+

clang-tidy/cert/ThrownExceptionTypeCheck.cpp

+//===--- ThrownExceptionTypeCheck.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 "ThrownExceptionTypeCheck.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;
+
+  if (const auto *FnTy = Node.getType()->getAs<FunctionProtoType>()) {
+    // If any of the copy constructors is throwing, we will assume that it
+    // is plausible that the constructor may be selected by the throw and
+    // diagnose. TODO: we could work a lot harder to only consider the
+    // copy constructor selected by the throw expression and diagnose only
+    // if that one is nonthrowing. However, the FP rate from this is likely
+    // to be quite low.
+    return FnTy->isNothrow(Node.getASTContext());
+  }
+  llvm_unreachable("Copy constructor with no prototype");
+}
+} // end namespace
+
+namespace tidy {
+void ThrownExceptionTypeCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
+
+  Finder->addMatcher(
+      cxxThrowExpr(
+          has(cxxConstructExpr(hasDeclaration(cxxConstructorDecl(
+              isCopyConstructor(), unless(isNoThrowCopyConstructor()))))
+          .bind("expr"))),
+      this);
+
+}
+
+void ThrownExceptionTypeCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *E = Result.Nodes.getNodeAs<Expr>("expr");
+  diag(E->getExprLoc(),
+       "thrown exception type is not nothrow copy constructible");
+}
+
+} // namespace tidy
+} // namespace clang
+

docs/clang-tidy/checks/cert-thrown-exception-type.rst

+cert-err60-cpp
+==============
+
+This check flags all throw expressions where the exception object is not nothrow
+copy constructible.
+
+This check corresponds to the CERT C++ Coding Standard rule
+`ERR60-CPP. Exception objects must be nothrow copy constructible
+<https://www.securecoding.cert.org/confluence/display/cplusplus/ERR60-CPP.+Exception+objects+must+be+nothrow+copy+constructible>`_.

docs/clang-tidy/checks/list.rst

 
 .. toctree::
    cert-setlongjmp
+   cert-thrown-exception-type
    cert-variadic-function-def
    cppcoreguidelines-pro-bounds-array-to-pointer-decay
    cppcoreguidelines-pro-bounds-pointer-arithmetic

test/clang-tidy/cert-throw-exception-type.cpp

+// RUN: %check_clang_tidy %s cert-err60-cpp %t -- -- -std=c++11 -fcxx-exceptions
+
+struct S {};
+struct T : S {};
+struct U {
+  U() = default;
+  U(const U&) = default;
+};
+
+struct V {
+  V() = default;
+  V(const V&) noexcept;
+};
+
+struct W {
+  W() = default;
+  W(const W&) noexcept(false);
+};
+
+struct X {
+  X() = default;
+  X(const X&) {}
+};
+
+struct Y {
+  Y() = default;
+  Y(const Y&) throw();
+};
+
+struct Z {
+  Z() = default;
+  Z(const Z&) throw(int);
+};
+
+void g() noexcept(false);
+
+struct A {
+  A() = default;
+  A(const A&) noexcept(noexcept(g()));
+};
+
+struct B {
+  B() = default;
+  B(const B&) = default;
+  B(const A&) noexcept(false);
+};
+
+class C {
+  W M; // W is not no-throw copy constructible
+public:
+  C() = default;
+  C(const C&) = default;
+};
+
+struct D {
+  D() = default;
+  D(const D&) noexcept(false);
+  D(D&) noexcept(true);
+};
+
+struct E {
+  E() = default;
+  E(E&) noexcept(true);
+  E(const E&) noexcept(false);
+};
+
+struct Allocates {
+  int *x;
+  Allocates() : x(new int(0)) {}
+  Allocates(const Allocates &other) : x(new int(*other.x)) {}
+};
+
+struct OptionallyAllocates {
+  int *x;
+  OptionallyAllocates() : x(new int(0)) {}
+  OptionallyAllocates(const Allocates &other) noexcept(true) {
+    try {
+      x = new int(*other.x);
+    } catch (...) {
+      x = nullptr;
+    }
+  }
+};
+
+void f() {
+  throw 12; // ok
+  throw "test"; // ok
+  throw S(); // ok
+  throw T(); // ok
+  throw U(); // ok
+  throw V(); // ok
+  throw W(); // match, noexcept(false)
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: thrown exception type is not nothrow copy constructible [cert-err60-cpp]
+  throw X(); // match, no noexcept clause, nontrivial
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: thrown exception type is not nothrow copy constructible
+  throw Y(); // ok
+  throw Z(); // match, throw(int)
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: thrown exception type is not nothrow copy constructible
+  throw A(); // match, noexcept(false)
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: thrown exception type is not nothrow copy constructible
+  throw B(); // ok
+  throw C(); // match, C has a member variable that makes it throwing on copy
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: thrown exception type is not nothrow copy constructible
+  throw D(); // match, has throwing copy constructor
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: thrown exception type is not nothrow copy constructible
+  throw E(); // match, has throwing copy constructor
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: thrown exception type is not nothrow copy constructible
+  throw Allocates(); // match, copy constructor throws
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: thrown exception type is not nothrow copy constructible
+  throw OptionallyAllocates(); // ok
+
+}