[analyzer] Add C++ array delete checker

[Discookie]

Summary

This checker reports cases where an array of polymorphic objects are deleted as their base class.
Deleting an array where the array's static type is different from its dynamic type is undefined.

Since the checker is similar to DeleteWithNonVirtualDtorChecker, I refactored that checker to support more detection types.

This checker corresponds to the SEI Cert rule EXP51-CPP: Do not delete an array through a pointer of the incorrect type.

[donat.nagy]

This checker is a straightforward, clean implementation of the SEI-CERT rule EXP51-CPP, it'll be a good addition to Static Analyzer. I'm satisfied with the implementation, although there might be others who have a more refined taste in C++ coding style. The only issue that I found is the misleading name of the "common ancestor" hidden checker that I mentioned in the inline comment.

The test suite nicely covers all the basic situations, but I'd be happy to see some corner cases like virtual and non-virtual diamond inheritance or even a degenerate example like

class Base { /*...*/ };
class Derived: public Base {
  Base basesAsMember[10];
  /*...*/
};
void f() {
  Derived *d = new Derived(...);
  delete[] (d->basesAsMember);
}

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

757–760

The name and help text of this hidden dependency is very misleading: it's not particularly connected to arrays (the connection between the two child checkers is that they both handle the "delete" operator) and it's not a modeling checker (which would provide function evaluation, constraints and transitions) but an "empty shell" checker that does nothing by itself, but provides a singleton checker object where the registration of the child checkers can enable the two analysis modes.

Please rename this to CXXDeleteHelper or something similar.

clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp

89

This logic (which is inherited from the earlier checker) is a bit surprising because it strips all Element, Field, BaseObject and DerivedObject layers from the MemRegion object.

I think it works in practice because
(1) there is an `isDerivedFrom() check after it and
(2) it's rare to write (buggy) code like

struct Inner { /*...*/ };
struct Outer { Inner inner; /*...*/ };
void f() {
  Outer *o = new Outer(...);
  delete(o->inner);
}

Perhaps this could be explained by a comment if you feel that it's warranted; but I mostly wrote this as a note for myself and other reviewers.

[donat.nagy]

By the way, what would you think about putting the implementation of the two checkers (DeleteWithNonVirtualDtor, ArrayDelete) into two separate checker classes and placing the shared code into a common base class of these classes? I think this could be a significant simplification: we could avoid the manual management of the two checker names and bug types and register two regular checker objects instead of the one dummy prerequisite checker + two "fake" checker registrations that just toggle boolean flags.

[Discookie]

Indeed it would make more sense to separate checkers into their own classes, but using a shared base. This checker doesn't have quite as advanced logic as MallocChecker, so there's not that much of a need for a single class managing everything. I'll change the checker to that system.

[balazske]

I found that this concept of "fake" checkers (that change only options of a common checker instance) has more problems (checker dependencies, callback order, separation of "modeling" and bug report generation independently), it is better to avoid this. It is really the same as having options of a single checker (separate checkers are not the same as options of a checker), otherwise a common base class or common code module is a better option.

[steakhal]

I share the raised concerns. And I think we are aligned.

PS: sorry if any of my comments are dups, or outdated. I only had a little time last week, and things have progressed since then I noticed. I still decided to submit my possibly outdated and partial review comments. I hope you understand.
Its quite difficult to allocate time to do reviews from day to to day work. I unfortunately usually do this in my spare time, if I have.

clang/docs/analyzer/checkers.rst

1787–1804

I think you should probably mention EXP51-CPP CERT rule somehow here.

1789–1791

1793–1803

Make sure in the example the create is related (e.g. called/used).
Also, refrain from using sink in the docs. It's usually used in the context of taint analysis.

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

757–760

clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp

51–52

62

66–69

[steakhal]

One more thing to mention. Its usually illadvised to rename files or do changes that would seriously impact git blame, unless we have a really good reason doing so.
Aesthetics usually don't count one, especially if the name is not user-facing. However, maintainability is another axis, thus as it's always, not black and white.

[donat.nagy]

In D158156#4604242, @steakhal wrote:

One more thing to mention. Its usually illadvised to rename files or do changes that would seriously impact git blame, unless we have a really good reason doing so.
Aesthetics usually don't count one, especially if the name is not user-facing. However, maintainability is another axis, thus as it's always, not black and white.

Git (e.g. git blame) can be smart enough to recognize renamed files if there aren't too much changes, but this change may be too large for that (at least Phabricator doesn't recognize it as a move, which confused me at first when I looked at this review). I'd suggest keeping the old filename in this commit; if you wish (and the community accepts it) you could rename the file in a separate followup NFC commit that doesn't do anything else.

In D158156#4604221, @steakhal wrote:

PS: sorry if any of my comments are dups, or outdated. I only had a little time last week, and things have progressed since then I noticed. I still decided to submit my possibly outdated and partial review comments. I hope you understand.
Its quite difficult to allocate time to do reviews from day to to day work. I unfortunately usually do this in my spare time, if I have.

Thanks for reviewing our commits, it's very helpful for us.

[Discookie]

Refactored the checkers to use a shared base class, otherwise I kept the same functionality.

I'll push this diff in a way where the blame transfers between the files, but I don't think keeping the file name the same makes sense, since this is definitely not a DeleteWithNonVirtualDtorChecker.

[Discookie]

Fixed the formatting issues as well.

clang/docs/analyzer/checkers.rst

1787–1804

Added a link to the CERT rules, similar to how others do it, thanks!

1793–1803

Changed the example - should I change the DeleteWithNonVirtualDtor example as well? That has the same issues as you said here.

[steakhal]

Add a test where we have multiple imp derived to base casts but only one leads to a bug. Do it for both ways, once that the first is the offending, and once the second.

clang/docs/analyzer/checkers.rst

1793–1803

I have no opinion. You could.

clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp

126

Is this transitive?

BTW inheritance can only be expressed if the class is a definition, right?
Thus passing this should imply has definition.

143

I think addVisitor already takes a template param, and it will call make unique for you.

192

Aren't you actually interested in N->getLocation().getAs<StmtPoint>().getStmt()?

The diag stmt can be fuzzy, but the PP is exact.

196

If you dyncast to ImplicitCastExpr, couldn't you have done it here?

202

How do you know that that this castexpr corresponds to the region for which you report the bug? To mez this might be some unrelated castexpr.
I was expecting the offending memregion being passed to the visitor, that it can check against.

217

There are so many early returns going on. I feel, we could miss the program point where it should have been marked satisfied. After this point, the visitor will never or should never emit a note.

224

Use named argument passing.

[donat.nagy]

clang/docs/analyzer/checkers.rst

1793–1803

I think you should definitely update it, for the sake of consistency and just improving things whenever we can. This commit already touches the code of that checker, there is no point in leaving bad documentation behind us...

[Discookie]

Added a test for taking last upcast only for the note tag. For now the visitor matches all explicit casts, because there are too many edge cases to count for now wrt. explicit upcasting.

[Discookie]

clang/docs/analyzer/checkers.rst

1793–1803

Updated.

clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp

126

It's transitive indeed.

I thought there were some presumptions about hasDefinition() in isDerivedFrom(), but apparently not, and it didn't cause any crashes either. Removed the redundant checks.

192

As far as I can tell, getStmtForDiagnostics() does exactly that, but with a bit more edge case handling and a couple fallbacks.

196

I'm interested in all cast expressions, not just implicit ones. There can also be explicit upcasts as well.

202

We know it's the one we are looking for, because the checker marked it interesting earlier. Not sure how memregion equality check would differ from taking isInteresting(), but it can be changed to that if needed.

[steakhal]

I have concerns mostly about the cast visitor.

clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp

192

If they do the same, and it does not depend on the mentioned fallbacks, I think we should use the Stmt of the programpoint to be explicit.

194–201

Have you considered dyn_cast<CastExpr>() to make it work for both implicit and explicit casts?
BTW simular -> similar

220

We should assert that this visitor always adds a Note. In other words, that it must find the Stmt where the derived->base conversion happened. If ever that's not true, we have a bug.

clang/test/Analysis/ArrayDelete.cpp

86–89

Hmm, the static type of d3 doesn't tell us much about how it refers to an object of type DoubleDerived.
To me, it would make sense to have multiple Conversion from derived to base happened here, even telling us what static type it converted to what other static type in the message.
And it should add a new visitor of the same kind tracking the castee.

Derived *d3 = new DoubleDerived[10]; // note: `DoubleDerived` -> `Derived` here
Base *b3 = d3; // note: `Derived` -> `Base` here
delete[] b3; // warn: Deleting `Derived` objects as `Base` objects.

WDYT @donat.nagy ?

91–94

I think in such cases a static_cast should suffice; unless you intentionally wanted to test reinterpret_cast of course.

[donat.nagy]

clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp

47

I like that yo switched to a more descriptive class name :)

clang/test/Analysis/ArrayDelete.cpp

86–89

I agree that it would be good to be good to mention the class names in the message.

91–94

Note that the effects of reinterpret_cast and static_cast are different [1]: static_cast<Derived*>(BasePtr) adjusts the pointer value to ensure that it points to the derived object whose Base ancestor object would be located at BasePtr, while a reinterpret_cast keeps the raw pointer value (which is complete nonsense from an object-oriented point-of-view, but could be relevant in some low-level hacks).

I'd guess that this part is directly testing the strange behavior of reinterpret_cast; but it'd be good to add a comment that clarifies this.

[1]: https://stackoverflow.com/questions/9138730/why-is-it-important-to-use-static-cast-instead-of-reinterpret-cast-here

[steakhal]

clang/test/Analysis/ArrayDelete.cpp

86–89

Do you also agree that we should have all steps where such a conversion happened?
Notice the 2 note: markers in my example. @donat.nagy

[donat.nagy]

clang/test/Analysis/ArrayDelete.cpp

86–89

It would be a nice addition if it wouldn't seriously complicate the implementation.

If we want to report multiple/all conversions, then we would need to create messages for back-and-forth conversions (e.g. allocating Derived, converting it to Base, back to Derived, back to Base, then deleting it illegally).

[Discookie]

Updated the visitor to track all conversions, and have type names for clarity.

[Discookie]

clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp

192

I'd rather not, since this pattern isn't used anywhere else, while getStmtForDiagnostics is widely used across the codebase.

194–201

I did exactly that a little earlier :D but I reworked the logic to filter based on the availability of types instead of the CastType.

217

Since we're emitting notes for all casts, this isn't needed anymore.

clang/test/Analysis/ArrayDelete.cpp

86–89

Added, as requested! While casts of reference types are not supported, it already makes the flow of classes much clearer.

91–94

Sadly there isn't any deeper meaning to reinterpret_cast here, just a mistake on my part. Fixed and changed to static_cast.

[Discookie]

clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp

220

Can't do that just yet as casts in the form of Derived &d = ...; Base &b = d; is not supported.

[donat.nagy]

Looks good, thanks for the improvements!

One minor remark: your commit uses backticks (`) around the class names in the bug reports, which is commonly used to mark code fragments e.g. in commit messages; however, in the bug reports IIRC most other checkers use apostrophes (').

[Discookie]

@steakhal gentle ping for one last round of reviews

[steakhal]

The code looks good now.
Let's have one last round.

Do you have real-world reports?

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

758–759

I thin these strings are concatenated like in C, thus it's gonna have "aredestructed" joined.

clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp

175–178

Prefer single quotes over backticks.

200–202

What is the problem with this?
I thought getPointeeType() works for ReferenceTypes.

219–220

We use single apostrophes for quoting names in CSA.

[Discookie]

Replaced backticks with single quotes.

[Discookie]

Ran the checker on a couple larger projects, but no real-world reports found so far (same as DeleteWithNonVirtualDtor). Can't tell if it's an engine limitation or just the bug being uncommon in general.

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

758–759

Whoops, fixed.

clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp

200–202

Apparently not, because references aren't ReferenceTypes but qualified Types. I could add support for it in a future commit, but I'd think casting and deleting array-references wrongly is even less common than deleting array-pointers.

219–220

Got it, thanks!

[steakhal]

It's good enough already, given you apply my last suggestion.
References would be nice to support for notes, but not a blocker.

clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp

200–202

Nop, this is for tracking conversions. That might be common.
Such as in our codebase, we usually prefer references. However, in clang we have pointers all over the place, thus internally one might cast references, but delete or pass them back to a legacy API by taking the address of the reference.

clang/test/Analysis/ArrayDelete.cpp

103–110

clang/docs/analyzer/checkers.rst

  void test(int x) {
    var = 77 / x;
    if (x == 0) { } // warn
  }
 
 alpha.cplusplus
 ^^^^^^^^^^^^^^^
 
+.. _alpha-cplusplus-ArrayDelete:
+
+alpha.cplusplus.ArrayDelete (C++)
+"""""""""""""""""""""""""""""""""
+Reports destructions of arrays of polymorphic objects that are destructed as their base class.
+This checker corresponds to the CERT rule `EXP51-CPP: Do not delete an array through a pointer of the incorrect type <https://wiki.sei.cmu.edu/confluence/display/cplusplus/EXP51-CPP.+Do+not+delete+an+array+through+a+pointer+of+the+incorrect+type>`_.
+
+.. code-block:: cpp
+
+ class Base {
+   virtual ~Base() {}
+ };
+ class Derived : public Base {}
+
+ Base *create() {
+   Base *x = new Derived[10]; // note: conversion from derived to base
+                              //       happened here
+   return x;
+ }
+
+ void foo() {
+   Base *x = create();
+   delete[] x; // warn: Deleting an array of polymorphic objects is undefined
+ }
+
 .. _alpha-cplusplus-DeleteWithNonVirtualDtor:
 
 alpha.cplusplus.DeleteWithNonVirtualDtor (C++)
 """"""""""""""""""""""""""""""""""""""""""""""
 Reports destructions of polymorphic objects with a non-virtual destructor in their base class.
 
 .. code-block:: cpp
 
+ class NonVirtual {};
+ class NVDerived : public NonVirtual {};
+
  NonVirtual *create() {
    NonVirtual *x = new NVDerived(); // note: conversion from derived to base
                                     //       happened here
    return x;
  }
 
- void sink(NonVirtual *x) {
+ void foo() {
+   NonVirtual *x = create();
    delete x; // warn: destruction of a polymorphic object with no virtual
              //       destructor
  }
 
 .. _alpha-cplusplus-EnumCastOutOfRange:
 
 alpha.cplusplus.EnumCastOutOfRange (C++)
 """"""""""""""""""""""""""""""""""""""""

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

 
 let ParentPackage = CplusplusAlpha in {
 
 def ContainerModeling : Checker<"ContainerModeling">,
   HelpText<"Models C++ containers">,
   Documentation<NotDocumented>,
   Hidden;
 
+def CXXArrayDeleteChecker : Checker<"ArrayDelete">,
+  HelpText<"Reports destructions of arrays of polymorphic objects that are"
+           "destructed as their base class.">,
+  Documentation<HasDocumentation>;
+
 def DeleteWithNonVirtualDtorChecker : Checker<"DeleteWithNonVirtualDtor">,
   HelpText<"Reports destructions of polymorphic objects with a non-virtual "
            "destructor in their base class">,
   Documentation<HasDocumentation>;
 
 def EnumCastOutOfRangeChecker : Checker<"EnumCastOutOfRange">,
   HelpText<"Check integer to enumeration casts for out of range values">,
   Documentation<HasDocumentation>;

clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt

add_clang_library(clangStaticAnalyzerCheckers
   CheckPlacementNew.cpp
   CheckSecuritySyntaxOnly.cpp
   CheckSizeofPointer.cpp
   CheckerDocumentation.cpp
   ChrootChecker.cpp
   CloneChecker.cpp
   ContainerModeling.cpp
   ConversionChecker.cpp
+  CXXDeleteChecker.cpp
   CXXSelfAssignmentChecker.cpp
   DeadStoresChecker.cpp
   DebugCheckers.cpp
   DebugContainerModeling.cpp
   DebugIteratorModeling.cpp
-  DeleteWithNonVirtualDtorChecker.cpp
   DereferenceChecker.cpp
   DirectIvarAssignment.cpp
   DivZeroChecker.cpp
   DynamicTypePropagation.cpp
   DynamicTypeChecker.cpp
   EnumCastOutOfRangeChecker.cpp
   ErrnoChecker.cpp
   ErrnoModeling.cpp

clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp

+//=== CXXDeleteChecker.cpp -------------------------------------*- C++ -*--===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file defines two new checkers for C++ delete expressions:
+//
+//   * DeleteWithNonVirtualDtorChecker
+//       Defines a checker for the OOP52-CPP CERT rule: Do not delete a
+//       polymorphic object without a virtual destructor.
+//
+//       Diagnostic flags -Wnon-virtual-dtor and -Wdelete-non-virtual-dtor
+//       report if an object with a virtual function but a non-virtual
+//       destructor exists or is deleted, respectively.
+//
+//       This check exceeds them by comparing the dynamic and static types of
+//       the object at the point of destruction and only warns if it happens
+//       through a pointer to a base type without a virtual destructor. The
+//       check places a note at the last point where the conversion from
+//       derived to base happened.
+//
+//   * CXXArrayDeleteChecker
+//       Defines a checker for the EXP51-CPP CERT rule: Do not delete an array
+//       through a pointer of the incorrect type.
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicType.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+class CXXDeleteChecker : public Checker<check::PreStmt<CXXDeleteExpr>> {
+protected:
+  class PtrCastVisitor : public BugReporterVisitor {
+  public:
+    void Profile(llvm::FoldingSetNodeID &ID) const override {
+      static int X = 0;
+      ID.AddPointer(&X);
+    }
+    PathDiagnosticPieceRef VisitNode(const ExplodedNode *N,
+                                     BugReporterContext &BRC,
+                                     PathSensitiveBugReport &BR) override;
+
+  private:
+    bool Satisfied = false;
+  };
+
+  virtual void
+  checkTypedDeleteExpr(const CXXDeleteExpr *DE, CheckerContext &C,
+                       const TypedValueRegion *BaseClassRegion,
+                       const SymbolicRegion *DerivedClassRegion) const = 0;
+
+public:
+  void checkPreStmt(const CXXDeleteExpr *DE, CheckerContext &C) const;
+};
+
+class DeleteWithNonVirtualDtorChecker : public CXXDeleteChecker {
+  mutable std::unique_ptr<BugType> BT;
+
+  void
+  checkTypedDeleteExpr(const CXXDeleteExpr *DE, CheckerContext &C,
+                       const TypedValueRegion *BaseClassRegion,
+                       const SymbolicRegion *DerivedClassRegion) const override;
+};
+
+class CXXArrayDeleteChecker : public CXXDeleteChecker {
+  mutable std::unique_ptr<BugType> BT;
+
+  void
+  checkTypedDeleteExpr(const CXXDeleteExpr *DE, CheckerContext &C,
+                       const TypedValueRegion *BaseClassRegion,
+                       const SymbolicRegion *DerivedClassRegion) const override;
+};
+} // namespace
+
+void CXXDeleteChecker::checkPreStmt(const CXXDeleteExpr *DE,
+                                    CheckerContext &C) const {
+  const Expr *DeletedObj = DE->getArgument();
+  const MemRegion *MR = C.getSVal(DeletedObj).getAsRegion();
+  if (!MR)
+    return;
+
+  OverloadedOperatorKind DeleteKind =
+      DE->getOperatorDelete()->getOverloadedOperator();
+
+  if (DeleteKind != OO_Delete && DeleteKind != OO_Array_Delete)
+    return;
+
+  const auto *BaseClassRegion = MR->getAs<TypedValueRegion>();
+  const auto *DerivedClassRegion = MR->getBaseRegion()->getAs<SymbolicRegion>();
+  if (!BaseClassRegion || !DerivedClassRegion)
+    return;
+
+  checkTypedDeleteExpr(DE, C, BaseClassRegion, DerivedClassRegion);
+}
+
+void DeleteWithNonVirtualDtorChecker::checkTypedDeleteExpr(
+    const CXXDeleteExpr *DE, CheckerContext &C,
+    const TypedValueRegion *BaseClassRegion,
+    const SymbolicRegion *DerivedClassRegion) const {
+  const auto *BaseClass = BaseClassRegion->getValueType()->getAsCXXRecordDecl();
+  const auto *DerivedClass =
+      DerivedClassRegion->getSymbol()->getType()->getPointeeCXXRecordDecl();
+  if (!BaseClass || !DerivedClass)
+    return;
+
+  if (!BaseClass->hasDefinition() || !DerivedClass->hasDefinition())
+    return;
+
+  if (BaseClass->getDestructor()->isVirtual())
+    return;
+
+  if (!DerivedClass->isDerivedFrom(BaseClass))
+    return;
+
+  if (!BT)
+    BT.reset(new BugType(this,
+                         "Destruction of a polymorphic object with no "
+                         "virtual destructor",
+                         "Logic error"));
+
+  ExplodedNode *N = C.generateNonFatalErrorNode();
+  if (!N)
+    return;
+  auto R =
+      std::make_unique<PathSensitiveBugReport>(*BT, BT->getDescription(), N);
+
+  // Mark region of problematic base class for later use in the BugVisitor.
+  R->markInteresting(BaseClassRegion);
+  R->addVisitor(std::make_unique<PtrCastVisitor>());
+  C.emitReport(std::move(R));
+}
+
+void CXXArrayDeleteChecker::checkTypedDeleteExpr(
+    const CXXDeleteExpr *DE, CheckerContext &C,
+    const TypedValueRegion *BaseClassRegion,
+    const SymbolicRegion *DerivedClassRegion) const {
+  const auto *BaseClass = BaseClassRegion->getValueType()->getAsCXXRecordDecl();
+  const auto *DerivedClass =
+      DerivedClassRegion->getSymbol()->getType()->getPointeeCXXRecordDecl();
+  if (!BaseClass || !DerivedClass)
+    return;
+
+  if (!BaseClass->hasDefinition() || !DerivedClass->hasDefinition())
+    return;
+
+  if (DE->getOperatorDelete()->getOverloadedOperator() != OO_Array_Delete)
+    return;
+
+  if (!DerivedClass->isDerivedFrom(BaseClass))
+    return;
+
+  if (!BT)
+    BT.reset(new BugType(this,
+                         "Deleting an array of polymorphic objects "
+                         "is undefined",
+                         "Logic error"));
+
+  ExplodedNode *N = C.generateNonFatalErrorNode();
+  if (!N)
+    return;
+  auto R =
+      std::make_unique<PathSensitiveBugReport>(*BT, BT->getDescription(), N);
+
+  // Mark region of problematic base class for later use in the BugVisitor.
+  R->markInteresting(BaseClassRegion);
+  R->addVisitor(std::make_unique<PtrCastVisitor>());
+  C.emitReport(std::move(R));
+}
+
+PathDiagnosticPieceRef
+CXXDeleteChecker::PtrCastVisitor::VisitNode(const ExplodedNode *N,
+                                            BugReporterContext &BRC,
+                                            PathSensitiveBugReport &BR) {
+  // Stop traversal after the first conversion was found on a path.
+  if (Satisfied)
+    return nullptr;
+
+  const Stmt *S = N->getStmtForDiagnostics();
+  if (!S)
+    return nullptr;
+
+  const auto *CastE = dyn_cast<CastExpr>(S);
+  if (!CastE)
+    return nullptr;
+
+  // Only interested in DerivedToBase implicit casts.
+  // Explicit casts can have different CastKinds.
+  if (const auto *ImplCastE = dyn_cast<ImplicitCastExpr>(CastE)) {
+    if (ImplCastE->getCastKind() != CK_DerivedToBase)
+      return nullptr;
+  }
+
+  // Region associated with the current cast expression.
+  const MemRegion *M = N->getSVal(CastE).getAsRegion();
+  if (!M)
+    return nullptr;
+
+  // Check if target region was marked as problematic previously.
+  if (!BR.isInteresting(M))
+    return nullptr;
+
+  // Stop traversal on this path.
+  Satisfied = true;
+
+  SmallString<256> Buf;
+  llvm::raw_svector_ostream OS(Buf);
+  OS << "Conversion from derived to base happened here";
+  PathDiagnosticLocation Pos(S, BRC.getSourceManager(),
+                             N->getLocationContext());
+  return std::make_shared<PathDiagnosticEventPiece>(Pos, OS.str(), true);
+}
+
+void ento::registerCXXArrayDeleteChecker(CheckerManager &mgr) {
+  mgr.registerChecker<CXXArrayDeleteChecker>();
+}
+
+bool ento::shouldRegisterCXXArrayDeleteChecker(const CheckerManager &mgr) {
+  return true;
+}
+
+void ento::registerDeleteWithNonVirtualDtorChecker(CheckerManager &mgr) {
+  mgr.registerChecker<DeleteWithNonVirtualDtorChecker>();
+}
+
+bool ento::shouldRegisterDeleteWithNonVirtualDtorChecker(
+    const CheckerManager &mgr) {
+  return true;
+}

clang/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp

-//===-- DeleteWithNonVirtualDtorChecker.cpp -----------------------*- C++ -*--//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-//
-// Defines a checker for the OOP52-CPP CERT rule: Do not delete a polymorphic
-// object without a virtual destructor.
-//
-// Diagnostic flags -Wnon-virtual-dtor and -Wdelete-non-virtual-dtor report if
-// an object with a virtual function but a non-virtual destructor exists or is
-// deleted, respectively.
-//
-// This check exceeds them by comparing the dynamic and static types of the
-// object at the point of destruction and only warns if it happens through a
-// pointer to a base type without a virtual destructor. The check places a note
-// at the last point where the conversion from derived to base happened.
-//
-//===----------------------------------------------------------------------===//
-
-#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
-#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
-#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
-#include "clang/StaticAnalyzer/Core/Checker.h"
-#include "clang/StaticAnalyzer/Core/CheckerManager.h"
-#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
-#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
-#include "clang/StaticAnalyzer/Core/PathSensitive/DynamicType.h"
-#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
-
-using namespace clang;
-using namespace ento;
-
-namespace {
-class DeleteWithNonVirtualDtorChecker
-    : public Checker<check::PreStmt<CXXDeleteExpr>> {
-  mutable std::unique_ptr<BugType> BT;
-
-  class DeleteBugVisitor : public BugReporterVisitor {
-  public:
-    DeleteBugVisitor() : Satisfied(false) {}
-    void Profile(llvm::FoldingSetNodeID &ID) const override {
-      static int X = 0;
-      ID.AddPointer(&X);
-    }
-    PathDiagnosticPieceRef VisitNode(const ExplodedNode *N,
-                                     BugReporterContext &BRC,
-                                     PathSensitiveBugReport &BR) override;
-
-  private:
-    bool Satisfied;
-  };
-
-public:
-  void checkPreStmt(const CXXDeleteExpr *DE, CheckerContext &C) const;
-};
-} // end anonymous namespace
-
-void DeleteWithNonVirtualDtorChecker::checkPreStmt(const CXXDeleteExpr *DE,
-                                                   CheckerContext &C) const {
-  const Expr *DeletedObj = DE->getArgument();
-  const MemRegion *MR = C.getSVal(DeletedObj).getAsRegion();
-  if (!MR)
-    return;
-
-  const auto *BaseClassRegion = MR->getAs<TypedValueRegion>();
-  const auto *DerivedClassRegion = MR->getBaseRegion()->getAs<SymbolicRegion>();
-  if (!BaseClassRegion || !DerivedClassRegion)
-    return;
-
-  const auto *BaseClass = BaseClassRegion->getValueType()->getAsCXXRecordDecl();
-  const auto *DerivedClass =
-      DerivedClassRegion->getSymbol()->getType()->getPointeeCXXRecordDecl();
-  if (!BaseClass || !DerivedClass)
-    return;
-
-  if (!BaseClass->hasDefinition() || !DerivedClass->hasDefinition())
-    return;
-
-  if (BaseClass->getDestructor()->isVirtual())
-    return;
-
-  if (!DerivedClass->isDerivedFrom(BaseClass))
-    return;
-
-  if (!BT)
-    BT.reset(new BugType(this,
-                         "Destruction of a polymorphic object with no "
-                         "virtual destructor",
-                         "Logic error"));
-
-  ExplodedNode *N = C.generateNonFatalErrorNode();
-  if (!N)
-    return;
-  auto R = std::make_unique<PathSensitiveBugReport>(*BT, BT->getDescription(), N);
-
-  // Mark region of problematic base class for later use in the BugVisitor.
-  R->markInteresting(BaseClassRegion);
-  R->addVisitor(std::make_unique<DeleteBugVisitor>());
-  C.emitReport(std::move(R));
-}
-
-PathDiagnosticPieceRef
-DeleteWithNonVirtualDtorChecker::DeleteBugVisitor::VisitNode(
-    const ExplodedNode *N, BugReporterContext &BRC,
-    PathSensitiveBugReport &BR) {
-  // Stop traversal after the first conversion was found on a path.
-  if (Satisfied)
-    return nullptr;
-
-  const Stmt *S = N->getStmtForDiagnostics();
-  if (!S)
-    return nullptr;
-
-  const auto *CastE = dyn_cast<CastExpr>(S);
-  if (!CastE)
-    return nullptr;
-
-  // Only interested in DerivedToBase implicit casts.
-  // Explicit casts can have different CastKinds.
-  if (const auto *ImplCastE = dyn_cast<ImplicitCastExpr>(CastE)) {
-    if (ImplCastE->getCastKind() != CK_DerivedToBase)
-      return nullptr;
-  }
-
-  // Region associated with the current cast expression.
-  const MemRegion *M = N->getSVal(CastE).getAsRegion();
-  if (!M)
-    return nullptr;
-
-  // Check if target region was marked as problematic previously.
-  if (!BR.isInteresting(M))
-    return nullptr;
-
-  // Stop traversal on this path.
-  Satisfied = true;
-
-  SmallString<256> Buf;
-  llvm::raw_svector_ostream OS(Buf);
-  OS << "Conversion from derived to base happened here";
-  PathDiagnosticLocation Pos(S, BRC.getSourceManager(),
-                             N->getLocationContext());
-  return std::make_shared<PathDiagnosticEventPiece>(Pos, OS.str(), true);
-}
-
-void ento::registerDeleteWithNonVirtualDtorChecker(CheckerManager &mgr) {
-  mgr.registerChecker<DeleteWithNonVirtualDtorChecker>();
-}
-
-bool ento::shouldRegisterDeleteWithNonVirtualDtorChecker(
-                                                    const CheckerManager &mgr) {
-  return true;
-}

clang/test/Analysis/ArrayDelete.cpp

+// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.cplusplus.ArrayDelete -std=c++11 -verify -analyzer-output=text %s
+
+struct Base {
+    virtual ~Base() = default;
+};
+
+struct Derived : public Base {};
+
+struct DoubleDerived : public Derived {};
+
+Derived *get();
+
+Base *create() {
+    Base *b = new Derived[3]; // expected-note{{Conversion from derived to base happened here}}
+    return b;
+}
+
+void sink(Base *b) {
+    delete[] b; // expected-warning{{Deleting an array of polymorphic objects is undefined}}
+    // expected-note@-1{{Deleting an array of polymorphic objects is undefined}}
+}
+
+void sink_cast(Base *b) {
+    delete[] reinterpret_cast<Derived*>(b); // no-warning
+}
+
+void sink_derived(Derived *d) {
+    delete[] d; // no-warning
+}
+
+void same_function() {
+    Base *sd = new Derived[10]; // expected-note{{Conversion from derived to base happened here}}
+    delete[] sd; // expected-warning{{Deleting an array of polymorphic objects is undefined}}
+    // expected-note@-1{{Deleting an array of polymorphic objects is undefined}}
+    
+    Base *dd = new DoubleDerived[10]; // expected-note{{Conversion from derived to base happened here}}
+    delete[] dd; // expected-warning{{Deleting an array of polymorphic objects is undefined}}
+    // expected-note@-1{{Deleting an array of polymorphic objects is undefined}}
+}
+
+void different_function() {
+    Base *assigned = get(); // expected-note{{Conversion from derived to base happened here}}
+    delete[] assigned; // expected-warning{{Deleting an array of polymorphic objects is undefined}}
+    // expected-note@-1{{Deleting an array of polymorphic objects is undefined}}
+
+    Base *indirect;
+    indirect = get(); // expected-note{{Conversion from derived to base happened here}}
+    delete[] indirect; // expected-warning{{Deleting an array of polymorphic objects is undefined}}
+    // expected-note@-1{{Deleting an array of polymorphic objects is undefined}}
+
+    Base *created = create(); // expected-note{{Calling 'create'}}
+    // expected-note@-1{{Returning from 'create'}}
+    delete[] created; // expected-warning{{Deleting an array of polymorphic objects is undefined}}
+    // expected-note@-1{{Deleting an array of polymorphic objects is undefined}}
+
+    Base *sb = new Derived[10]; // expected-note{{Conversion from derived to base happened here}}
+    sink(sb); // expected-note{{Calling 'sink'}}
+}
+
+void safe_function() {
+    Derived *d = new Derived[10];
+    delete[] d; // no-warning
+
+    Base *b = new Derived[10];
+    delete[] reinterpret_cast<Derived*>(b); // no-warning
+
+    Base *sb = new Derived[10];
+    sink_cast(sb); // no-warning
+
+    Derived *sd = new Derived[10];
+    sink_derived(sd); // no-warning
+}

clang/www/analyzer/alpha_checks.html

 <!-- =========================== cplusplus alpha =========================== -->
 <h3 id="cplusplus_alpha_checkers">C++ Alpha Checkers</h3>
 <table class="checkers">
 <colgroup><col class="namedescr"><col class="example"></colgroup>
 <thead><tr><td>Name, Description</td><td>Example</td></tr></thead>
 <tbody>
 
 
+<tr><td><a id="alpha.cplusplus.ArrayDelete"><div class="namedescr expandable"><span class="name">
+alpha.cplusplus.ArrayDelete</span><span class="lang">
+(C++)</span><div class="descr">
+Reports destructions of arrays of polymorphic objects that are destructed as
+their base class
+</div></div></a></td>
+<td><div class="exampleContainer expandable">
+<div class="example"><pre>
+Base *create() {
+  Base *x = new Derived[10]; // note: conversion from derived to base
+                             //       happened here
+  return x;
+}
+
+void sink(Base *x) {
+  delete[] x; // warn: Deleting an array of polymorphic objects is undefined
+}
+
+</pre></div></div></td></tr>
+
+
 <tr><td><a id="alpha.cplusplus.DeleteWithNonVirtualDtor"><div class="namedescr expandable"><span class="name">
 alpha.cplusplus.DeleteWithNonVirtualDtor</span><span class="lang">
 (C++)</span><div class="descr">
 Reports destructions of polymorphic objects with a non-virtual destructor in
 their base class
 </div></div></a></td>
 <td><div class="exampleContainer expandable">
 <div class="example"><pre>