From fd78cc88cf3990f4342c02ae63d148ab0db28972 Mon Sep 17 00:00:00 2001 From: Aaron Ballman Date: Fri, 9 Oct 2015 20:42:44 +0000 Subject: [PATCH] Add a new checker that tests whether a throw expression throws by value, and whether a catch statement catches by reference. Patch by Tobias Langner! llvm-svn: 249899 --- .../clang-tidy/misc/CMakeLists.txt | 1 + .../clang-tidy/misc/MiscTidyModule.cpp | 3 + .../ThrowByValueCatchByReferenceCheck.cpp | 159 ++++++++++++++++++ .../misc/ThrowByValueCatchByReferenceCheck.h | 49 ++++++ ...misc-throw-by-value-catch-by-reference.cpp | 156 +++++++++++++++++ 5 files changed, 368 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/misc/ThrowByValueCatchByReferenceCheck.h create mode 100644 clang-tools-extra/test/clang-tidy/misc-throw-by-value-catch-by-reference.cpp diff --git a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt index 462ff43718d8..cddf5e3a8adb 100644 --- a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt @@ -17,6 +17,7 @@ add_clang_library(clangTidyMiscModule SizeofContainerCheck.cpp StaticAssertCheck.cpp SwappedArgumentsCheck.cpp + ThrowByValueCatchByReferenceCheck.cpp UndelegatedConstructor.cpp UnusedAliasDeclsCheck.cpp UnusedParametersCheck.cpp diff --git a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp index 44ba505f0a85..f093b0f5c951 100644 --- a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp @@ -25,6 +25,7 @@ #include "SizeofContainerCheck.h" #include "StaticAssertCheck.h" #include "SwappedArgumentsCheck.h" +#include "ThrowByValueCatchByReferenceCheck.h" #include "UndelegatedConstructor.h" #include "UniqueptrResetReleaseCheck.h" #include "UnusedAliasDeclsCheck.h" @@ -66,6 +67,8 @@ public: "misc-static-assert"); CheckFactories.registerCheck( "misc-swapped-arguments"); + CheckFactories.registerCheck( + "misc-throw-by-value-catch-by-reference"); CheckFactories.registerCheck( "misc-undelegated-constructor"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp b/clang-tools-extra/clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp new file mode 100644 index 000000000000..3fbbe8ea860a --- /dev/null +++ b/clang-tools-extra/clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp @@ -0,0 +1,159 @@ +//===--- ThrowByValueCatchByReferenceCheck.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 "ThrowByValueCatchByReferenceCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/AST/OperationKinds.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { + +ThrowByValueCatchByReferenceCheck::ThrowByValueCatchByReferenceCheck( + StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + CheckAnonymousTemporaries(Options.get("CheckThrowTemporaries", true)) {} + +void ThrowByValueCatchByReferenceCheck::registerMatchers(MatchFinder *Finder) { + // This is a C++ only check thus we register the matchers only for C++ + if (!getLangOpts().CPlusPlus) + return; + + Finder->addMatcher(cxxThrowExpr().bind("throw"), this); + Finder->addMatcher(cxxCatchStmt().bind("catch"), this); +} + +void ThrowByValueCatchByReferenceCheck::storeOptions( + ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "CheckThrowTemporaries", true); +} + +void ThrowByValueCatchByReferenceCheck::check( + const MatchFinder::MatchResult &Result) { + diagnoseThrowLocations(Result.Nodes.getNodeAs("throw")); + diagnoseCatchLocations(Result.Nodes.getNodeAs("catch"), + *Result.Context); +} + +bool ThrowByValueCatchByReferenceCheck::isFunctionParameter( + const DeclRefExpr *declRefExpr) { + return isa(declRefExpr->getDecl()); +} + +bool ThrowByValueCatchByReferenceCheck::isCatchVariable( + const DeclRefExpr *declRefExpr) { + auto *valueDecl = declRefExpr->getDecl(); + if (auto *varDecl = dyn_cast(valueDecl)) + return varDecl->isExceptionVariable(); + return false; +} + +bool ThrowByValueCatchByReferenceCheck::isFunctionOrCatchVar( + const DeclRefExpr *declRefExpr) { + return isFunctionParameter(declRefExpr) || isCatchVariable(declRefExpr); +} + +void ThrowByValueCatchByReferenceCheck::diagnoseThrowLocations( + const CXXThrowExpr *throwExpr) { + if (!throwExpr) + return; + auto *subExpr = throwExpr->getSubExpr(); + if (!subExpr) + return; + auto qualType = subExpr->getType(); + if (qualType->isPointerType()) { + // The code is throwing a pointer. + // In case it is strng literal, it is safe and we return. + auto *inner = subExpr->IgnoreParenImpCasts(); + if (isa(inner)) + return; + // If it's a variable from a catch statement, we return as well. + auto *declRef = dyn_cast(inner); + if (declRef && isCatchVariable(declRef)) { + return; + } + diag(subExpr->getLocStart(), "throw expression throws a pointer; it should " + "throw a non-pointer value instead"); + } + // If the throw statement does not throw by pointer then it throws by value + // which is ok. + // There are addition checks that emit diagnosis messages if the thrown value + // is not an RValue. See: + // https://www.securecoding.cert.org/confluence/display/cplusplus/ERR09-CPP.+Throw+anonymous+temporaries + // This behavior can be influenced by an option. + + // If we encounter a CXXThrowExpr, we move through all casts until you either + // encounter a DeclRefExpr or a CXXConstructExpr. + // If it's a DeclRefExpr, we emit a message if the referenced variable is not + // a catch variable or function parameter. + // When encountering a CopyOrMoveConstructor: emit message if after casts, + // the expression is a LValue + if (CheckAnonymousTemporaries) { + bool emit = false; + auto *currentSubExpr = subExpr->IgnoreImpCasts(); + const DeclRefExpr *variableReference = + dyn_cast(currentSubExpr); + const CXXConstructExpr *constructorCall = + dyn_cast(currentSubExpr); + // If we have a DeclRefExpr, we flag for emitting a diagnosis message in + // case the referenced variable is neither a function parameter nor a + // variable declared in the catch statement. + if (variableReference) + emit = !isFunctionOrCatchVar(variableReference); + else if (constructorCall && + constructorCall->getConstructor()->isCopyOrMoveConstructor()) { + // If we have a copy / move construction, we emit a diagnosis message if + // the object that we copy construct from is neither a function parameter + // nor a variable declared in a catch statement + auto argIter = + constructorCall + ->arg_begin(); // there's only one for copy constructors + auto *currentSubExpr = (*argIter)->IgnoreImpCasts(); + if (currentSubExpr->isLValue()) { + if (auto *tmp = dyn_cast(currentSubExpr)) + emit = !isFunctionOrCatchVar(tmp); + else if (isa(currentSubExpr)) + emit = true; + } + } + if (emit) + diag(subExpr->getLocStart(), + "throw expression should throw anonymous temporary values instead"); + } +} + +void ThrowByValueCatchByReferenceCheck::diagnoseCatchLocations( + const CXXCatchStmt *catchStmt, ASTContext &context) { + const char *diagMsgCatchReference = "catch handler catches a pointer value; " + "should throw a non-pointer value and " + "catch by reference instead"; + if (!catchStmt) + return; + auto caughtType = catchStmt->getCaughtType(); + if (caughtType.isNull()) + return; + auto *varDecl = catchStmt->getExceptionDecl(); + if (const auto *PT = caughtType.getCanonicalType()->getAs()) { + // We do not diagnose when catching pointer to strings since we also allow + // throwing string literals. + if (!PT->getPointeeType()->isAnyCharacterType()) + diag(varDecl->getLocStart(), diagMsgCatchReference); + } else if (!caughtType->isReferenceType()) { + // If it's not a pointer and not a reference then it must be thrown "by + // value". In this case we should emit a diagnosis message unless the type + // is trivial. + if (!caughtType.isTrivialType(context)) + diag(varDecl->getLocStart(), diagMsgCatchReference); + } +} + +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/misc/ThrowByValueCatchByReferenceCheck.h b/clang-tools-extra/clang-tidy/misc/ThrowByValueCatchByReferenceCheck.h new file mode 100644 index 000000000000..703003183774 --- /dev/null +++ b/clang-tools-extra/clang-tidy/misc/ThrowByValueCatchByReferenceCheck.h @@ -0,0 +1,49 @@ +//===--- ThrowByValueCatchByReferenceCheck.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_MISC_THROW_BY_VALUE_CATCH_BY_REFERENCE_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_THROW_BY_VALUE_CATCH_BY_REFERENCE_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { + +///\brief checks for locations that do not throw by value +// or catch by reference. +// The check is C++ only. It checks that all throw locations +// throw by value and not by pointer. Additionally it +// contains an option ("CheckThrowTemporaries", default value "true") that +// checks that thrown objects are anonymous temporaries. It is also +// acceptable for this check to throw string literals. +// This test checks that exceptions are caught by reference +// and not by value or pointer. It will not warn when catching +// pointer to char, wchar_t, char16_t or char32_t. This is +// due to not warning on throwing string literals. +class ThrowByValueCatchByReferenceCheck : public ClangTidyCheck { +public: + ThrowByValueCatchByReferenceCheck(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; + +private: + void diagnoseThrowLocations(const CXXThrowExpr *throwExpr); + void diagnoseCatchLocations(const CXXCatchStmt *catchStmt, + ASTContext &context); + bool isFunctionParameter(const DeclRefExpr *declRefExpr); + bool isCatchVariable(const DeclRefExpr *declRefExpr); + bool isFunctionOrCatchVar(const DeclRefExpr *declRefExpr); + const bool CheckAnonymousTemporaries; +}; + +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_THROW_BY_VALUE_CATCH_BY_REFERENCE_H diff --git a/clang-tools-extra/test/clang-tidy/misc-throw-by-value-catch-by-reference.cpp b/clang-tools-extra/test/clang-tidy/misc-throw-by-value-catch-by-reference.cpp new file mode 100644 index 000000000000..87b30d70adf6 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/misc-throw-by-value-catch-by-reference.cpp @@ -0,0 +1,156 @@ +// RUN: %python %S/check_clang_tidy.py %s misc-throw-by-value-catch-by-reference %t + + +class logic_error { +public: + logic_error(const char *message) {} +}; + +typedef logic_error *logic_ptr; +typedef logic_ptr logic_double_typedef; + +int lastException; + +template struct remove_reference { typedef T type; }; +template struct remove_reference { typedef T type; }; +template struct remove_reference { typedef T type; }; + +template typename remove_reference::type &&move(T &&arg) { + return static_cast::type &&>(arg); +} + +logic_error CreateException() { return logic_error("created"); } + +void testThrowFunc() { + throw new logic_error("by pointer"); + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: throw expression throws a pointer; it should throw a non-pointer value instead [misc-throw-by-value-catch-by-reference] + logic_ptr tmp = new logic_error("by pointer"); + throw tmp; + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: throw expression should throw anonymous temporary values instead [misc-throw-by-value-catch-by-reference] + // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: throw expression throws a pointer; it should throw a non-pointer value instead [misc-throw-by-value-catch-by-reference] + throw logic_error("by value"); + auto *literal = "test"; + throw logic_error(literal); + throw "test string literal"; + throw L"throw wide string literal"; + const char *characters = 0; + throw characters; + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: throw expression should throw anonymous temporary values instead [misc-throw-by-value-catch-by-reference] + // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: throw expression throws a pointer; it should throw a non-pointer value instead [misc-throw-by-value-catch-by-reference] + logic_error lvalue("lvalue"); + throw lvalue; + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: throw expression should throw anonymous temporary values instead [misc-throw-by-value-catch-by-reference] + + throw move(lvalue); + int &ex = lastException; + throw ex; + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: throw expression should throw anonymous temporary values instead [misc-throw-by-value-catch-by-reference] + throw CreateException(); +} + +void throwReferenceFunc(logic_error &ref) { throw ref; } + +void catchByPointer() { + try { + testThrowFunc(); + } catch (logic_error *e) { + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: catch handler catches a pointer value; should throw a non-pointer value and catch by reference instead [misc-throw-by-value-catch-by-reference] + } +} + +void catchByValue() { + try { + testThrowFunc(); + } catch (logic_error e) { + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: catch handler catches a pointer value; should throw a non-pointer value and catch by reference instead [misc-throw-by-value-catch-by-reference] + } +} + +void catchByReference() { + try { + testThrowFunc(); + } catch (logic_error &e) { + } +} + +void catchByConstReference() { + try { + testThrowFunc(); + } catch (const logic_error &e) { + } +} + +void catchTypedef() { + try { + testThrowFunc(); + } catch (logic_ptr) { + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: catch handler catches a pointer value; should throw a non-pointer value and catch by reference instead [misc-throw-by-value-catch-by-reference] + } +} + +void catchAll() { + try { + testThrowFunc(); + } catch (...) { + } +} + +void catchLiteral() { + try { + testThrowFunc(); + } catch (const char *) { + } catch (const wchar_t *) { + // disabled for now until it is clear + // how to enable them in the test + //} catch (const char16_t*) { + //} catch (const char32_t*) { + } +} + +// catching fundamentals should not warn +void catchFundamental() { + try { + testThrowFunc(); + } catch (int) { + } catch (double) { + } catch (unsigned long) { + } +} + +struct TrivialType { + double x; + double y; +}; + +void catchTrivial() { + try { + testThrowFunc(); + } catch (TrivialType) { + } +} + +typedef logic_error &fine; +void additionalTests() { + try { + } catch (int i) { // ok + throw i; // ok + } catch (fine e) { // ok + throw e; // ok + } catch (logic_error *e) { + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: catch handler catches a pointer value; should throw a non-pointer value and catch by reference instead [misc-throw-by-value-catch-by-reference] + throw e; // ok, despite throwing a pointer + } catch (...) { // ok + throw; // ok + } +} + +struct S {}; + +S &returnByReference(); +S returnByValue(); + +void f() { + throw returnByReference(); // Should diagnose + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: throw expression should throw anonymous temporary values instead [misc-throw-by-value-catch-by-reference] + throw returnByValue(); // Should not diagnose +}