[clang-tidy] new check for throw expressions in destructors

[JonasToth]

Summary

This check will catch throw expression in destructors. These are extremly unsafe, since
causing and exception while handling an exception will crash the program.
There is no further analysis if the throw can actually occur, but throws in destructors
should not exist (at least believing bjarne).

This check can be aliased into other modules, the cppcoreguidelines have a section as well,
i will do this when accepted.
Furthermore there is no analysis for noexcept specification, since there already exists
another check for that. Maybe those could be merged as well.

[JonasToth]

• [Doc] adjust docs

[JonasToth]

• [Fix] link to documentation was wrong

[JonasToth]

add more complicated problematic destructor to see if hasAncestor does what i think

[mgehre]

clang-tidy/safety/NoThrowInDestructorCheck.cpp

30

I would word the diag "throw expression in destructor" to be more inline with the other checks' diags.

docs/clang-tidy/checks/safety-no-throw-in-destructor.rst

7

I would put throw into `` to make it highlight like other code constructs.

8

"stack unwinding"
Would it be better to say "the program will call terminate()"?

[aaron.ballman]

This check is also somewhat similar to DCL57-CPP from the CERT C++ secure coding guidelines (https://www.securecoding.cert.org/confluence/display/cplusplus/DCL57-CPP.+Do+not+let+exceptions+escape+from+destructors+or+deallocation+functions). If you're interested in extending this check to cover deallocation functions, which have the same concerns as destructors, then it could also be added to the CERT module as well.

clang-tidy/safety/NoThrowInDestructorCheck.cpp

30

This check has false positives with otherwise reasonable code where the exceptions are also caught in the destructor. Since this only concerns an intraprocedural throw, you should be able to see that the exception is also caught by the destructor, which avoids the false positives without needing any path-sensitive checks. e.g.,

~D() {
  try {
    throw 12;
  } catch (int) {
  }
}

As it stands, I think the check's current form may be rather low-value. Have you run it over any large code bases that use exceptions to see how many true-positives it finds?

An issue that you see more frequently is when a call to a function that is marked noexcept(false) or has a throwing dynamic exception specification, but the call is not in a try/catch block. Perhaps you could couple that with this check to make the check a bit higher value?

[JonasToth]

• issues by mgehre fixed

[JonasToth]

@aaron.ballman I would like to make this more valuable and mor applicable to other rulesets as well.
Do you think it would be enough to do the same bad logic in deallocation, without deep analysis of the exceptions that can occur and if they are caught some where?

In my opinion this code-smell-check is ok, and a deeper analysis for the noexcept specification would be good.
I have one question. Is a function marked as noexcept allowed to handle exceptions, or must there be no exception at all.
An example would be your note in the code, could that destructor be marked as noexcept?

If there is a problem with this issue, the check can watch for functions that are noexcept but contain throw expressions as well.
I would rename it then :)

• marked issues by mghere as done

clang-tidy/safety/NoThrowInDestructorCheck.cpp

30

Yes the false positive exists. I thought of this check more in the direction of code smell.
So one could do that, but maybe its not a good idea, since that catch can change over time and a latent bug would be introduced possibly.

Iterating statments and checking for the noexcept correctness is more interesting, but maybe this is something for another check?
The logic to see if noexcept is acutally valid would apply to all functions or am I mistaken? Is there already a compiler diagnostic for it, and should there be one, since the compiler could see that as well?

The try/catch issue would be, that one must truly know which exceptions can occur in a nonnoexcept-call. that would go into static analyzer area and I don't have to skills. But i would be interested in learning. :)

[aaron.ballman]

In D30746#696361, @JonasToth wrote:

@aaron.ballman I would like to make this more valuable and mor applicable to other rulesets as well.
Do you think it would be enough to do the same bad logic in deallocation, without deep analysis of the exceptions that can occur and if they are caught some where?

I think the same logic should apply there, yes. However, that's insufficient to make it applicable to the CERT rules, though it has some overlap.

In my opinion this code-smell-check is ok, and a deeper analysis for the noexcept specification would be good.
I have one question. Is a function marked as noexcept allowed to handle exceptions, or must there be no exception at all.
An example would be your note in the code, could that destructor be marked as noexcept?

Yes, a function marked 'noexept' is allowed to handle and even throw exceptions (so long as they're caught by the function). The noexcept specifier is a contract with the caller that no unhandled exceptions escape the function when called. The code I posted can be explicitly marked noexcept (it has an implicit noexcept by virtue of it being a destructor).

If there is a problem with this issue, the check can watch for functions that are noexcept but contain throw expressions as well.
I would rename it then :)

I don't know that it would be reasonable to do so. I'm still somewhat on the fence about this being a code smell, but throwing in a destructor (or deallocation function) and then immediately catching the exception is sufficiently strange that it may be reasonable. However, I think this turns the check into a very low-value check. Outside of compiler test suites, does this check find any true positives when run on a large corpus of code that uses exceptions?

The reason I bring up the CERT rule wording is because that's covering the usual ways in which you run into problems. Most often, the throw statement is buried somewhere in a function that is called from the destructor, and the user fails to handle the exception. I think this check really should be a path-sensitive static analyzer check rather than a clang-tidy check. That would then cover this trivial case (which is unlikely to happen in the wild) as well as the more common cases.

clang-tidy/safety/NoThrowInDestructorCheck.cpp

30

Hmm, I suppose this is a somewhat reasonable code smell check. The presence of a throw statement that is properly caught in the destructor is sufficiently weird that it's not likely to happen often in well-designed code, even if it's not unsafe to do so. It's more likely to run into a throw statement that isn't caught, which is is unsafe to do.

[JonasToth]

I agree with the static analysis. Iam not capable so atm, since i have no clue how to write sensefull static analysis :) But i would like to learn it.

What would be a good codebase to check against it? I tried to get Chromium, but it failed on some point and i had no energy to find out what failed.
The libcxx from clang should use exceptions, is there a run-clang-tidy for it as well (like in llvm)? But on the other hand, that should not be a good codebase for that check.

The reason i wrote this was High Integrity has a section on this. I thought this stupid form would be a good start that can be enhanced. But its ok if this will fall out when there is a way better way of dealing with the issue of exceptions in general and in destructors particulary.

[aaron.ballman]

In D30746#698874, @JonasToth wrote:

I agree with the static analysis. Iam not capable so atm, since i have no clue how to write sensefull static analysis :) But i would like to learn it.

I don't have a good insight as to whether this would be an easy check to write for the static analyzer or not, but it may be a good way to learn it.

What would be a good codebase to check against it? I tried to get Chromium, but it failed on some point and i had no energy to find out what failed.
The libcxx from clang should use exceptions, is there a run-clang-tidy for it as well (like in llvm)? But on the other hand, that should not be a good codebase for that check.

I don't think libc++ would be a useful test, as I doubt any STL implementation throws in a destructor. Perhaps you could look for source bases on GitHub by searching for C++ repositories that use exception handling? I've used several code bases from there before (Qt, rethinkdb, cocos2d, opencv, etc).

The reason i wrote this was High Integrity has a section on this. I thought this stupid form would be a good start that can be enhanced. But its ok if this will fall out when there is a way better way of dealing with the issue of exceptions in general and in destructors particulary.

My reading of the HIC++ (http://www.codingstandard.com/section/15-2-constructors-and-destructors/) is that you should not allow exceptions to escape the destructor, not that you cannot throw from the destructor. For instance, this code would violate the HIC++ (but not be caught by your check):

struct S {
  void f() { throw 12; }
  ~S() {
    f();
  }
};

Your check is a reasonable start, but to actually conform to the HIC++ rule, it would need to be path sensitive and clang-tidy checks are not a good place for path-sensitive checks (the static analyzer is more appropriate). That means that you really can't extend this clang-tidy check much farther.

[JonasToth]

yeah you are right.
i will look for some codebases, maybe some interesting results there.

trying to save this one :D
keeping the throw check and see if every function call not in a try statement is noexcept?

[aaron.ballman]

In D30746#698887, @JonasToth wrote:

yeah you are right.
i will look for some codebases, maybe some interesting results there.

trying to save this one :D
keeping the throw check and see if every function call not in a try statement is noexcept?

That's somewhat better, but unlikely to give good results. A *lot* of functions do not throw exceptions but are not marked noexcept. I suspect this will generate a ton of false positives. It's also likely to have false negatives with code like:

struct S {
  void f() { throw "abc"; }
  ~S() {
    try {
      f();
    } catch (int) {
    }
  }
};

[JonasToth]

that is true as well.
lets see what the static analysis brings. maybe its easy to achieve something there.

is there a check for noexcept already?

double square(double v) {
    return v*v;
}

This one could be easily noexcept. Do we have something that would add the noexcept in this case and if not, do you think that should/could be done.
And is this the right place to ask? :) iam not so used to clang development, so feel free to point me somewhere else.

[aaron.ballman]

In D30746#698891, @JonasToth wrote:

that is true as well.
lets see what the static analysis brings. maybe its easy to achieve something there.

is there a check for noexcept already?

I don't think there's a check for noexcept in the static analyzer yet.

double square(double v) {
    return v*v;
}

This one could be easily noexcept. Do we have something that would add the noexcept in this case and if not, do you think that should/could be done.
And is this the right place to ask? :) iam not so used to clang development, so feel free to point me somewhere else.

I don't think we have anything that suggests noexcept like that currently, but it could make for an interesting feature. Alternatively, it might be handy to have a check that adds noexcept(false) if it notices that a function throws exceptions out of it.

As for whether this is the right place to ask -- sure, it can be. ;-) You can always ask general questions on cfe-dev or cfe-commits as well, if you want a wider audience to comment on it.

[JonasToth]

@aaron.ballman i asked on the mailing list and we had a little discussion for a general noexcept check. Most were the opinion that flow analysis should be enough.
@mboehme answered on the mailing list, i will just cite the part thats somewhat addressed to you:

       I will try to work on a prototype for the clear cases, so we will find out where problems are.
      The review with the remarks from aaron is here https://reviews.llvm.org/D30746

Thanks!
I see Aaron's comment there that the analysis would need to be path-sensitive, but unfortunately he doesn't explain why (or I missed the explanation). Maybe you want to ask him again?

The current state is, that i will try to implement a first prototype for the really clear cases from above. Maybe we can discuss about more complicated things later.

[mboehme]

In D30746#705279, @JonasToth wrote:

@aaron.ballman i asked on the mailing list and we had a little discussion for a general noexcept check. Most were the opinion that flow analysis should be enough.

I don't currently see why even flow-sensitive analysis is required -- the cases I can think of can be done on the AST, but I'm sure there's something I'm missing, and I'm curious what it is...

[aaron.ballman]

In D30746#705329, @mboehme wrote:

In D30746#705279, @JonasToth wrote:

@aaron.ballman i asked on the mailing list and we had a little discussion for a general noexcept check. Most were the opinion that flow analysis should be enough.

I don't currently see why even flow-sensitive analysis is required -- the cases I can think of can be done on the AST, but I'm sure there's something I'm missing, and I'm curious what it is...

This check is trying to warn the user when they're throwing in a destructor is going to cause problems. The original implementation of the check was just looking "does this destructor have a throw statement?" which is not particularly good granularity for the check. What the question really is (ideally): "does something in this destructor (even transitively) throw an exception that is not caught and thus escapes the destructor?" That's why I think it could be path or flow sensitive (I admit, I sometimes get these terms confused). If the call cannot throw because some condition is impossible to meet, the code isn't doing anything bad that the user needs to change.

As a case in point:

struct S {
  ~S() {
    if (!some_condition)
      something();
  }

  void something() {
    if (some_condition)
      throw 12;
  }
};

Computing an exception specification for something() will result in it being considered noexcept(false) because it can throw, but the destructor guards the call on the correct predicate to ensure that something() doesn't ever throw. This is not an unreasonable code pattern (and I'm pretty sure I've seen it in the wild a time or two), and it would be nice if we didn't diagnose ~S() as potentially throwing in this sort of case.

[mboehme]

(Apologies for the late reply)

In D30746#705636, @aaron.ballman wrote:

In D30746#705329, @mboehme wrote:

In D30746#705279, @JonasToth wrote:

@aaron.ballman i asked on the mailing list and we had a little discussion for a general noexcept check. Most were the opinion that flow analysis should be enough.

I don't currently see why even flow-sensitive analysis is required -- the cases I can think of can be done on the AST, but I'm sure there's something I'm missing, and I'm curious what it is...

This check is trying to warn the user when they're throwing in a destructor is going to cause problems. The original implementation of the check was just looking "does this destructor have a throw statement?" which is not particularly good granularity for the check. What the question really is (ideally): "does something in this destructor (even transitively) throw an exception that is not caught and thus escapes the destructor?" That's why I think it could be path or flow sensitive (I admit, I sometimes get these terms confused). If the call cannot throw because some condition is impossible to meet, the code isn't doing anything bad that the user needs to change.

As a case in point:

[snip]

Ah, I now see what you mean.

At the same time, this increases the cost of the check significantly from simply walking the AST to performing a path-sensitive analysis. It would be useful to know how common these patterns are in practice and how hard they are to fix so a path-sensitive analysis isn't necessary.

clang-tidy/safety/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidySafetyModule
   NoAssemblerCheck.cpp
+  NoThrowInDestructorCheck.cpp
   SafetyTidyModule.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   )

clang-tidy/safety/NoThrowInDestructorCheck.h

+//===--- no-throw-in-destructorCheck.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_SAFETY_NO_THROW_IN_DESTRUCTOR_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_SAFETY_NO_THROW_IN_DESTRUCTOR_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace safety {
+
+/// This checks warns for throw statements in destructors.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/safety-no-throw-in-destructor.html
+class NoThrowInDestructorCheck : public ClangTidyCheck {
+public:
+  NoThrowInDestructorCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace safety
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_SAFETY_NO_THROW_IN_DESTRUCTOR_H

clang-tidy/safety/NoThrowInDestructorCheck.cpp

+//===--- no-throw-in-destructorCheck.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 "NoThrowInDestructorCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace safety {
+
+void NoThrowInDestructorCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      cxxThrowExpr(hasAncestor(cxxDestructorDecl()))
+          .bind("throw"),
+      this);
+}
+
+void NoThrowInDestructorCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *MatchedThrow = Result.Nodes.getNodeAs<CXXThrowExpr>("throw");
+  diag(MatchedThrow->getLocStart(), "throw expression in destructor");
+}
+
+} // namespace safety
+} // namespace tidy
+} // namespace clang

clang-tidy/safety/SafetyTidyModule.cpp

 //===------- SafetyTidyModule.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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "NoAssemblerCheck.h"
+#include "NoThrowInDestructorCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace safety {
 
 class SafetyModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<NoAssemblerCheck>(
         "safety-no-assembler");
+    CheckFactories.registerCheck<NoThrowInDestructorCheck>(
+        "safety-no-throw-in-destructor");
   }
 };
 
 // Register the SafetyModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<SafetyModule>
     X("safety-module", "Adds safety-critical checks.");
 
 } // namespace safety
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the SafetyModule.
 volatile int SafetyModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

docs/clang-tidy/checks/list.rst

.. toctree::
    readability-redundant-function-ptr-dereference
    readability-redundant-member-init
    readability-redundant-smartptr-get
    readability-redundant-string-cstr
    readability-redundant-string-init
    readability-simplify-boolean-expr
    readability-static-definition-in-anonymous-namespace
    readability-uniqueptr-delete-release
+   safety-no-throw-in-destructor
    safety-no-assembler

docs/clang-tidy/checks/safety-no-throw-in-destructor.rst

+.. title:: clang-tidy - safety-no-throw-in-destructor
+
+safety-no-throw-in-destructor
+=============================
+
+This check will raise warnings for all `throw` expression occuring in destructors.
+When an exception is handled and during stack unwinding another exception occurs, the program will call `std::terminate()`. 
+This is the reason why destructors should be noexcept and never cause an exception.

test/clang-tidy/safety-no-throw-in-destructor.cpp

+// RUN: %check_clang_tidy %s safety-no-throw-in-destructor %t
+
+namespace std {
+struct runtime_error {};
+}
+
+struct GoodInlineDestructor {
+  ~GoodInlineDestructor() {
+    int i = 1; // Ok
+  }
+};
+
+struct GoodDestructor {
+  ~GoodDestructor();
+};
+
+GoodDestructor::~GoodDestructor() {
+  int i = 1;
+  float j = 0.5f;
+  // Ok
+}
+
+struct BadDestructor {
+  ~BadDestructor();
+};
+
+BadDestructor::~BadDestructor() {
+  throw std::runtime_error();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: throw expression in destructor
+}
+
+struct BadInlineDestructor {
+  ~BadInlineDestructor() { throw std::runtime_error(); }
+  // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: throw expression in destructor
+};
+
+struct ComplicatedInline {
+  ~ComplicatedInline() {
+    int i = 0;
+    float j = 0.5f;
+    char string[] = "Hallo welt";
+    // Ok
+  }
+};
+
+struct ComplicatedBad {
+  ~ComplicatedBad() {
+    int i = 15;
+    if (i == 15)
+      throw std::runtime_error();
+    // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: throw expression in destructor
+    else
+      throw std::runtime_error();
+    // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: throw expression in destructor
+  }
+};