[PATCH] New checker for mismatched operator new/operator delete definitions

[aaron.ballman]

Summary

When defining a free store function, it is almost invariably a bug to fail to declare the corresponding free store function. For instance, implementing operator new() but failing to implement operator delete(). This new checker catches instances where the free store functions are mismatched, while still allowing for common code patterns involving deleted functions or placement forms of functions. Specifically, this will not warn on implicit free store functions, placement forms, deleted functions (such as defining a placement new and a deleted operator delete(void*)), and private functions (for code that predates C++11).

This patch corresponds to: https://www.securecoding.cert.org/confluence/display/cplusplus/DCL54-CPP.+Overload+allocation+and+deallocation+functions+as+a+pair+in+the+same+scope

I ran this checker over Clang and LLVM and it did find two issues, one of which I have fixed. YAMLParser.h had a protected operator delete() with a trivial definition that I converted to be a deleted function in r248320. This was a borderline false positive -- the code was correct because the body of operator delete() was trivial. However, defining the function as deleted is cleaner since accidental deletion from within Node or one of its subclasses will now be diagnosed instead of silently accepted and doing nothing. The other is a true positive as best I can tell. The MDNode class in Metadata.h defines a placement new operator, a free store operator delete(), and two placement delete functions. The free store operator delete() has no matching free store operator new() (including when looking in super classes), but the implementation does not appear to trigger undefined behavior -- it just seems like a really confused design (which is mitigated by the fact that the free store delete is protected). Also, one of the placement delete functions is not required by std, as the comments suggest, and should be removed.

~Aaron

[alexfh]

Please add a documentation file.

clang-tidy/misc/NewDeleteOverloadsCheck.cpp

43

Please no else after return.

[aaron.ballman]

Added documentation, fixed else after return.

~Aaron

[alexfh]

clang-tidy/misc/NewDeleteOverloadsCheck.cpp

67

http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

Function names [...] should be camel case, and start with a lower case letter (e.g. openFile() or isFoo()).

169

Please don't use "O", "l", "I" as variable names.

171

I just noticed that this will be an O(N^2) from all new/delete overloads in all classes in a TU. This should probably be not much usually, but I can imagine a corner-case, where this is going to be slooow. How about sharding these by the enclosing record declaration?

test/clang-tidy/misc-new-delete-overloads-sized-dealloc.cpp

7

nit: Let's include the check name in brackets to the check pattern once.

[aaron.ballman]

Thank you for the review!

clang-tidy/misc/NewDeleteOverloadsCheck.cpp

67

Ugh, I always get this backwards because of other parts of the code base that didn't follow the convention. I'll fix.

169

I thought this was the correct style for identifiers that do not require descriptive names (we use it *everywhere* in Clang)? I'm not opposed, but I am wondering if clang-tidy has different style guides?

171

Yes, the O(N^2) is unfortunate, sorry for not calling that out explicitly. I figured that N should be incredibly minimal, however (especially since we only care about *written* overloads that are not placement overloads). So realistically, the maximum that N can be here is 6: operator new(), operator new[](), operator delete(), operator delete[](), and sized operator delete()/operator delete[](). I figured that this wasn't worth complicating the code over since N is bounded.

But I suppose the worry is if you have these operators defined in a a lot of classes in the same TU? In that case, I suppose I could replace SmallVector<FunctionDecl *> Overloads with MapVector<CXXRecordDecl *, FunctionDecl *> Overloads?

test/clang-tidy/misc-new-delete-overloads-sized-dealloc.cpp

7

Can do!

[alexfh]

clang-tidy/misc/NewDeleteOverloadsCheck.cpp

169

I'm not opposed to single-character identifiers, as long as they don't use characters that are indistinguishable from some other characters in some fonts. E.g. I don't want ever to be confused about map[O] vs map[0] (same for "l", "I", and sometimes even "1").

171

But I suppose the worry is if you have these operators defined in a a lot of
classes in the same TU? In that case, I suppose I could replace
SmallVector<FunctionDecl *> Overloads with MapVector<CXXRecordDecl
*, FunctionDecl *> Overloads?

Yes, this is what I meant. Though I didn't know about MapVector<> before you told me ;)

[aaron.ballman]

clang-tidy/misc/NewDeleteOverloadsCheck.cpp

169

Ah, this makes perfect sense. Can do. :-)

171

Er, not MapVector, but perhaps a std::map<CXXRecordDecl *, std::vector<FunctionDecl *>>, I guess.

[alexfh]

clang-tidy/misc/NewDeleteOverloadsCheck.cpp

171

Might be a DenseMap<CXXRecordDecl *, SmallVector<FunctionDecl *, 6>> as well. Or std::map<..., SmallVector<,>>.

[aaron.ballman]

Addressing review comments.

[alexfh]

Looks good with a few comments.

Thanks for the new check! Do you think whether this could be a compiler warning some day?

clang-tidy/misc/NewDeleteOverloadsCheck.cpp

52

nit: const auto *FPT would be better here to avoid duplication of the type name.

clang-tidy/misc/NewDeleteOverloadsCheck.h

22

I wonder whether "class" here and below is actually needed. Did you try without it?

test/clang-tidy/misc-new-delete-overloads.cpp

3

I think, "unsigned long" should be used instead (and we might have to define it differently for different platforms).

[aaron.ballman]

In D13071#255446, @alexfh wrote:

Looks good with a few comments.

Thanks for the new check! Do you think whether this could be a compiler warning some day?

I thought about implementing it as a frontend warning, but it seemed somewhat expensive for there (since it would have to be triggered on every TU completion). However, it might be reasonable to move it in there at some point.

Thank you for the reviews! I've commit in r248791.

~Aaron

test/clang-tidy/misc-new-delete-overloads.cpp

3

I get a type redefinition error then, so I removed the typedef declaration entirely and will watch the bots to make sure this isn't an artifact of being on an MSVC build.

clang-tidy/misc/CMakeLists.txt

   MacroRepeatedSideEffectsCheck.cpp
   MiscTidyModule.cpp
   MoveConstructorInitCheck.cpp
+  NewDeleteOverloadsCheck.cpp
   NoexceptMoveConstructorCheck.cpp
   SizeofContainerCheck.cpp
   StaticAssertCheck.cpp

clang-tidy/misc/MiscTidyModule.cpp

 #include "MacroParenthesesCheck.h"
 #include "MacroRepeatedSideEffectsCheck.h"
 #include "MoveConstructorInitCheck.h"
+#include "NewDeleteOverloadsCheck.h"
 #include "NoexceptMoveConstructorCheck.h"
 #include "SizeofContainerCheck.h"
 #include "StaticAssertCheck.h"

         "misc-macro-repeated-side-effects");
     CheckFactories.registerCheck<MoveConstructorInitCheck>(
         "misc-move-constructor-init");
+    CheckFactories.registerCheck<NewDeleteOverloadsCheck>(
+        "misc-new-delete-overloads");
     CheckFactories.registerCheck<NoexceptMoveConstructorCheck>(
         "misc-noexcept-move-constructor");
     CheckFactories.registerCheck<SizeofContainerCheck>(

clang-tidy/misc/NewDeleteOverloadsCheck.h

+//===--- 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 <map>
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+class NewDeleteOverloadsCheck : public ClangTidyCheck {
+  std::map<const class clang::CXXRecordDecl *,
+           llvm::SmallVector<const class clang::FunctionDecl *, 4>> 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

clang-tidy/misc/NewDeleteOverloadsCheck.cpp

+//===--- 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 FunctionProtoType *FPT = Node.getType()->castAs<FunctionProtoType>();
+  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<FunctionDecl>("func");
+  const CXXRecordDecl *RD = nullptr;
+  if (const auto *MD = dyn_cast<CXXMethodDecl>(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<const FunctionDecl *, 4> 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<CXXMethodDecl>(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

docs/clang-tidy/checks/list.rst

    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

docs/clang-tidy/checks/misc-new-delete-overloads.rst

+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
+<https://www.securecoding.cert.org/confluence/display/cplusplus/DCL54-CPP.+Overload+allocation+and+deallocation+functions+as+a+pair+in+the+same+scope>`_.

test/clang-tidy/misc-new-delete-overloads-sized-dealloc.cpp

+// RUN: %python %S/check_clang_tidy.py %s misc-new-delete-overloads %t -- -std=c++14 -fsized-deallocation
+
+typedef unsigned int size_t;
+
+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;

test/clang-tidy/misc-new-delete-overloads.cpp

+// RUN: %python %S/check_clang_tidy.py %s misc-new-delete-overloads %t -- -std=c++14
+
+typedef unsigned int size_t;
+
+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
+};