From de34985caae79ce7d125cd55fa634e74aa04427d Mon Sep 17 00:00:00 2001 From: Aaron Ballman Date: Tue, 29 Sep 2015 13:12:21 +0000 Subject: [PATCH] Adding a checker (misc-new-delete-overloads) that detects mismatched overloads of operator new and operator delete. Corresponds to the CERT C++ secure coding rule: https://www.securecoding.cert.org/confluence/display/cplusplus/DCL54-CPP.+Overload+allocation+and+deallocation+functions+as+a+pair+in+the+same+scope llvm-svn: 248791 --- .../clang-tidy/misc/CMakeLists.txt | 1 + .../clang-tidy/misc/MiscTidyModule.cpp | 3 + .../misc/NewDeleteOverloadsCheck.cpp | 215 ++++++++++++++++++ .../clang-tidy/misc/NewDeleteOverloadsCheck.h | 37 +++ .../docs/clang-tidy/checks/list.rst | 1 + .../checks/misc-new-delete-overloads.rst | 14 ++ ...isc-new-delete-overloads-sized-dealloc.cpp | 18 ++ .../clang-tidy/misc-new-delete-overloads.cpp | 75 ++++++ 8 files changed, 364 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/misc/NewDeleteOverloadsCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/misc/NewDeleteOverloadsCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/misc-new-delete-overloads.rst create mode 100644 clang-tools-extra/test/clang-tidy/misc-new-delete-overloads-sized-dealloc.cpp create mode 100644 clang-tools-extra/test/clang-tidy/misc-new-delete-overloads.cpp diff --git a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt index 6cdd621f81e5..ce4f82bf21e6 100644 --- a/clang-tools-extra/clang-tidy/misc/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/misc/CMakeLists.txt @@ -11,6 +11,7 @@ add_clang_library(clangTidyMiscModule MacroRepeatedSideEffectsCheck.cpp MiscTidyModule.cpp MoveConstructorInitCheck.cpp + NewDeleteOverloadsCheck.cpp NoexceptMoveConstructorCheck.cpp SizeofContainerCheck.cpp StaticAssertCheck.cpp diff --git a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp index dc9a4eb21a65..636deb282288 100644 --- a/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp @@ -19,6 +19,7 @@ #include "MacroParenthesesCheck.h" #include "MacroRepeatedSideEffectsCheck.h" #include "MoveConstructorInitCheck.h" +#include "NewDeleteOverloadsCheck.h" #include "NoexceptMoveConstructorCheck.h" #include "SizeofContainerCheck.h" #include "StaticAssertCheck.h" @@ -53,6 +54,8 @@ public: "misc-macro-repeated-side-effects"); CheckFactories.registerCheck( "misc-move-constructor-init"); + CheckFactories.registerCheck( + "misc-new-delete-overloads"); CheckFactories.registerCheck( "misc-noexcept-move-constructor"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/clang-tidy/misc/NewDeleteOverloadsCheck.cpp b/clang-tools-extra/clang-tidy/misc/NewDeleteOverloadsCheck.cpp new file mode 100644 index 000000000000..c62ba86f6bc3 --- /dev/null +++ b/clang-tools-extra/clang-tidy/misc/NewDeleteOverloadsCheck.cpp @@ -0,0 +1,215 @@ +//===--- NewDeleteOverloadsCheck.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 "NewDeleteOverloadsCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace { +AST_MATCHER(FunctionDecl, isPlacementOverload) { + bool New; + switch (Node.getOverloadedOperator()) { + default: + return false; + case OO_New: + case OO_Array_New: + New = true; + break; + case OO_Delete: + case OO_Array_Delete: + New = false; + break; + } + + // Variadic functions are always placement functions. + if (Node.isVariadic()) + return true; + + // Placement new is easy: it always has more than one parameter (the first + // parameter is always the size). If it's an overload of delete or delete[] + // that has only one parameter, it's never a placement delete. + if (New) + return Node.getNumParams() > 1; + if (Node.getNumParams() == 1) + return false; + + // Placement delete is a little more challenging. They always have more than + // one parameter with the first parameter being a pointer. However, the + // second parameter can be a size_t for sized deallocation, and that is never + // a placement delete operator. + if (Node.getNumParams() <= 1 || Node.getNumParams() > 2) + return true; + + const auto *FPT = Node.getType()->castAs(); + ASTContext &Ctx = Node.getASTContext(); + if (Ctx.getLangOpts().SizedDeallocation && + Ctx.hasSameType(FPT->getParamType(1), Ctx.getSizeType())) + return false; + + return true; +} +} // namespace + +namespace tidy { +namespace misc { + +namespace { +OverloadedOperatorKind getCorrespondingOverload(const FunctionDecl *FD) { + switch (FD->getOverloadedOperator()) { + default: break; + case OO_New: + return OO_Delete; + case OO_Delete: + return OO_New; + case OO_Array_New: + return OO_Array_Delete; + case OO_Array_Delete: + return OO_Array_New; + } + llvm_unreachable("Not an overloaded allocation operator"); +} + +const char *getOperatorName(OverloadedOperatorKind K) { + switch (K) { + default: break; + case OO_New: + return "operator new"; + case OO_Delete: + return "operator delete"; + case OO_Array_New: + return "operator new[]"; + case OO_Array_Delete: + return "operator delete[]"; + } + llvm_unreachable("Not an overloaded allocation operator"); +} + +bool areCorrespondingOverloads(const FunctionDecl *LHS, + const FunctionDecl *RHS) { + return RHS->getOverloadedOperator() == getCorrespondingOverload(LHS); +} + +bool hasCorrespondingOverloadInOneClass(const CXXRecordDecl *RD, + const CXXMethodDecl *MD) { + // Check the methods in the given class and accessible to derived classes. + for (const auto *BMD : RD->methods()) + if (BMD->isOverloadedOperator() && BMD->getAccess() != AS_private && + areCorrespondingOverloads(MD, BMD)) + return true; + + // Check base classes. + for (const auto &BS : RD->bases()) + if (hasCorrespondingOverloadInOneClass(BS.getType()->getAsCXXRecordDecl(), + MD)) + return true; + + return false; +} +bool hasCorrespondingOverloadInBaseClass(const CXXMethodDecl *MD) { + // Get the parent class of the method; we do not need to care about checking + // the methods in this class as the caller has already done that by looking + // at the declaration contexts. + const CXXRecordDecl *RD = MD->getParent(); + + for (const auto &BS : RD->bases()) + if (hasCorrespondingOverloadInOneClass(BS.getType()->getAsCXXRecordDecl(), + MD)) + return true; + + return false; +} +} // anonymous namespace + +void NewDeleteOverloadsCheck::registerMatchers(MatchFinder *Finder) { + if (!getLangOpts().CPlusPlus) + return; + + // Match all operator new and operator delete overloads (including the array + // forms). Do not match implicit operators, placement operators, or + // deleted/private operators. + // + // Technically, trivially-defined operator delete seems like a reasonable + // thing to also skip. e.g., void operator delete(void *) {} + // However, I think it's more reasonable to warn in this case as the user + // should really be writing that as a deleted function. + Finder->addMatcher( + functionDecl( + unless(anyOf(isImplicit(), isPlacementOverload(), isDeleted(), + cxxMethodDecl(isPrivate()))), + anyOf(hasOverloadedOperatorName("new"), + hasOverloadedOperatorName("new[]"), + hasOverloadedOperatorName("delete"), + hasOverloadedOperatorName("delete[]"))) + .bind("func"), + this); +} + +void NewDeleteOverloadsCheck::check(const MatchFinder::MatchResult &Result) { + // Add any matches we locate to the list of things to be checked at the + // end of the translation unit. + const auto *FD = Result.Nodes.getNodeAs("func"); + const CXXRecordDecl *RD = nullptr; + if (const auto *MD = dyn_cast(FD)) + RD = MD->getParent(); + Overloads[RD].push_back(FD); +} + +void NewDeleteOverloadsCheck::onEndOfTranslationUnit() { + // Walk over the list of declarations we've found to see if there is a + // corresponding overload at the same declaration context or within a base + // class. If there is not, add the element to the list of declarations to + // diagnose. + SmallVector Diagnose; + for (const auto &RP : Overloads) { + // We don't care about the CXXRecordDecl key in the map; we use it as a way + // to shard the overloads by declaration context to reduce the algorithmic + // complexity when searching for corresponding free store functions. + for (const auto *Overload : RP.second) { + const auto *Match = std::find_if( + RP.second.begin(), RP.second.end(), [&](const FunctionDecl *FD) { + if (FD == Overload) + return false; + // If the declaration contexts don't match, we don't + // need to check + // any further. + if (FD->getDeclContext() != Overload->getDeclContext()) + return false; + + // Since the declaration contexts match, see whether + // the current + // element is the corresponding operator. + if (!areCorrespondingOverloads(Overload, FD)) + return false; + + return true; + }); + + if (Match == RP.second.end()) { + // Check to see if there is a corresponding overload in a base class + // context. If there isn't, or if the overload is not a class member + // function, then we should diagnose. + const auto *MD = dyn_cast(Overload); + if (!MD || !hasCorrespondingOverloadInBaseClass(MD)) + Diagnose.push_back(Overload); + } + } + } + + for (const auto *FD : Diagnose) + diag(FD->getLocation(), "declaration of %0 has no matching declaration " + "of '%1' at the same scope") + << FD << getOperatorName(getCorrespondingOverload(FD)); +} + +} // namespace misc +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/misc/NewDeleteOverloadsCheck.h b/clang-tools-extra/clang-tidy/misc/NewDeleteOverloadsCheck.h new file mode 100644 index 000000000000..582e659d4ef3 --- /dev/null +++ b/clang-tools-extra/clang-tidy/misc/NewDeleteOverloadsCheck.h @@ -0,0 +1,37 @@ +//===--- NewDeleteOverloadsCheck.h - clang-tidy----------------------------===// +// +// 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_NEWDELETEOVERLOADS_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_NEWDELETEOVERLOADS_H + +#include "../ClangTidy.h" +#include "llvm/ADT/SmallVector.h" +#include + +namespace clang { +namespace tidy { +namespace misc { + +class NewDeleteOverloadsCheck : public ClangTidyCheck { + std::map> Overloads; + +public: + NewDeleteOverloadsCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + void onEndOfTranslationUnit() override; +}; + +} // namespace misc +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_NEWDELETEOVERLOADS_H diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index 600b09d92731..331edc479dea 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -30,6 +30,7 @@ List of clang-tidy Checks misc-macro-parentheses misc-macro-repeated-side-effects misc-move-constructor-init + misc-new-delete-overloads misc-noexcept-move-constructor misc-sizeof-container misc-static-assert diff --git a/clang-tools-extra/docs/clang-tidy/checks/misc-new-delete-overloads.rst b/clang-tools-extra/docs/clang-tidy/checks/misc-new-delete-overloads.rst new file mode 100644 index 000000000000..381371881eac --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/misc-new-delete-overloads.rst @@ -0,0 +1,14 @@ +misc-new-delete-overloads +========================= + +The check flags overloaded operator new() and operator delete() functions that +do not have a corresponding free store function defined within the same scope. +For instance, the check will flag a class implementation of a non-placement +operator new() when the class does not also define a non-placement operator +delete() function as well. + +The check does not flag implicitly-defined operators, deleted or private +operators, or placement operators. + +This check corresponds to CERT C++ Coding Standard rule `DCL54-CPP. Overload allocation and deallocation functions as a pair in the same scope +`_. diff --git a/clang-tools-extra/test/clang-tidy/misc-new-delete-overloads-sized-dealloc.cpp b/clang-tools-extra/test/clang-tidy/misc-new-delete-overloads-sized-dealloc.cpp new file mode 100644 index 000000000000..992c2fdf9ed7 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/misc-new-delete-overloads-sized-dealloc.cpp @@ -0,0 +1,18 @@ +// RUN: %python %S/check_clang_tidy.py %s misc-new-delete-overloads %t -- -std=c++14 -fsized-deallocation + +struct S { + // CHECK-MESSAGES: :[[@LINE+1]]:8: warning: declaration of 'operator delete' has no matching declaration of 'operator new' at the same scope [misc-new-delete-overloads] + void operator delete(void *ptr, size_t) noexcept; // not a placement delete +}; + +struct T { + // Because we have enabled sized deallocations explicitly, this new/delete + // pair matches. + void *operator new(size_t size) noexcept; + void operator delete(void *ptr, size_t) noexcept; // ok because sized deallocation is enabled +}; + +// While we're here, check that global operator delete with no operator new +// is also matched. +// CHECK-MESSAGES: :[[@LINE+1]]:6: warning: declaration of 'operator delete' has no matching declaration of 'operator new' at the same scope +void operator delete(void *ptr) noexcept; diff --git a/clang-tools-extra/test/clang-tidy/misc-new-delete-overloads.cpp b/clang-tools-extra/test/clang-tidy/misc-new-delete-overloads.cpp new file mode 100644 index 000000000000..6bb73c7c433c --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/misc-new-delete-overloads.cpp @@ -0,0 +1,75 @@ +// RUN: %python %S/check_clang_tidy.py %s misc-new-delete-overloads %t -- -std=c++14 + +struct S { + // CHECK-MESSAGES: :[[@LINE+1]]:9: warning: declaration of 'operator new' has no matching declaration of 'operator delete' at the same scope [misc-new-delete-overloads] + void *operator new(size_t size) noexcept; + // CHECK-MESSAGES: :[[@LINE+1]]:9: warning: declaration of 'operator new[]' has no matching declaration of 'operator delete[]' at the same scope + void *operator new[](size_t size) noexcept; +}; + +// CHECK-MESSAGES: :[[@LINE+1]]:7: warning: declaration of 'operator new' has no matching declaration of 'operator delete' at the same scope +void *operator new(size_t size) noexcept; + +struct T { + // Sized deallocations are not enabled by default, and so this new/delete pair + // does not match. However, we expect only one warning, for the new, because + // the operator delete is a placement delete and we do not warn on mismatching + // placement operations. + // CHECK-MESSAGES: :[[@LINE+1]]:9: warning: declaration of 'operator new' has no matching declaration of 'operator delete' at the same scope + void *operator new(size_t size) noexcept; + void operator delete(void *ptr, size_t) noexcept; // ok only if sized deallocation is enabled +}; + +struct U { + void *operator new(size_t size) noexcept; + void operator delete(void *ptr) noexcept; + + void *operator new[](size_t) noexcept; + void operator delete[](void *) noexcept; +}; + +struct Z { + // CHECK-MESSAGES: :[[@LINE+1]]:8: warning: declaration of 'operator delete' has no matching declaration of 'operator new' at the same scope + void operator delete(void *ptr) noexcept; + // CHECK-MESSAGES: :[[@LINE+1]]:8: warning: declaration of 'operator delete[]' has no matching declaration of 'operator new[]' at the same scope + void operator delete[](void *ptr) noexcept; +}; + +struct A { + void *operator new(size_t size, Z) noexcept; // ok, placement new +}; + +struct B { + void operator delete(void *ptr, A) noexcept; // ok, placement delete +}; + +// It is okay to have a class with an inaccessible free store operator. +struct C { + void *operator new(size_t, A) noexcept; // ok, placement new +private: + void operator delete(void *) noexcept; +}; + +// It is also okay to have a class with a delete free store operator. +struct D { + void *operator new(size_t, A) noexcept; // ok, placement new + void operator delete(void *) noexcept = delete; +}; + +struct E : U { + void *operator new(size_t) noexcept; // okay, we inherit operator delete from U +}; + +struct F : S { + // CHECK-MESSAGES: :[[@LINE+1]]:9: warning: declaration of 'operator new' has no matching declaration of 'operator delete' at the same scope + void *operator new(size_t) noexcept; +}; + +class G { + void operator delete(void *) noexcept; +}; + +struct H : G { + // CHECK-MESSAGES: :[[@LINE+1]]:9: warning: declaration of 'operator new' has no matching declaration of 'operator delete' at the same scope + void *operator new(size_t) noexcept; // base class operator is inaccessible +};