[clang][analyzer] Add C++ array delete checker

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.

Differential Revision: https://reviews.llvm.org/D158156

Author: Discookie

clang/docs/analyzer/checkers.rst

@@ -1834,6 +1834,30 @@ Either the comparison is useless or there is division by zero.
 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: Casting from 'Derived' to 'Base' here
+   return x;
+ }
+
+ void foo() {
+   Base *x = create();
+   delete[] x; // warn: Deleting an array of 'Derived' objects as their base class 'Base' is undefined
+ }
+
 .. _alpha-cplusplus-DeleteWithNonVirtualDtor:
 
 alpha.cplusplus.DeleteWithNonVirtualDtor (C++)
@@ -1842,13 +1866,17 @@ Reports destructions of polymorphic objects with a non-virtual destructor in the
 
 .. code-block:: cpp
 
+ class NonVirtual {};
+ class NVDerived : public NonVirtual {};
+
  NonVirtual *create() {
-   NonVirtual *x = new NVDerived(); // note: conversion from derived to base
-                                    //       happened here
+   NonVirtual *x = new NVDerived(); // note: Casting from 'NVDerived' to
+                                    //       'NonVirtual' here
    return x;
  }
 
- void sink(NonVirtual *x) {
+ void foo() {
+   NonVirtual *x = create();
    delete x; // warn: destruction of a polymorphic object with no virtual
              //       destructor
  }

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

@@ -758,6 +758,11 @@ def ContainerModeling : Checker<"ContainerModeling">,
   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">,

clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp

@@ -1,22 +1,30 @@
-//===-- DeleteWithNonVirtualDtorChecker.cpp -----------------------*- C++ -*--//
+//=== 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
 //
 //===----------------------------------------------------------------------===//
 //
-// Defines a checker for the OOP52-CPP CERT rule: Do not delete a polymorphic
-// object without a virtual destructor.
+// This file defines the following new checkers for C++ delete expressions:
 //
-// 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.
+//   * DeleteWithNonVirtualDtorChecker
+//       Defines a checker for the OOP52-CPP CERT rule: Do not delete a
+//       polymorphic object without a virtual destructor.
 //
-// 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.
+//       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.
 //
 //===----------------------------------------------------------------------===//
 
@@ -34,51 +42,78 @@ using namespace clang;
 using namespace ento;
 
 namespace {
-class DeleteWithNonVirtualDtorChecker
-    : public Checker<check::PreStmt<CXXDeleteExpr>> {
-  mutable std::unique_ptr<BugType> BT;
-
-  class DeleteBugVisitor : public BugReporterVisitor {
+class CXXDeleteChecker : public Checker<check::PreStmt<CXXDeleteExpr>> {
+protected:
+  class PtrCastVisitor : public BugReporterVisitor {
   public:
-    DeleteBugVisitor() = default;
     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;
 };
-} // end anonymous namespace
 
-void DeleteWithNonVirtualDtorChecker::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;
 
@@ -94,22 +129,65 @@ void DeleteWithNonVirtualDtorChecker::checkPreStmt(const CXXDeleteExpr *DE,
   ExplodedNode *N = C.generateNonFatalErrorNode();
   if (!N)
     return;
-  auto R = std::make_unique<PathSensitiveBugReport>(*BT, BT->getDescription(), N);
+  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>());
+  R->addVisitor<PtrCastVisitor>();
   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;
+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 (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;
+
+  SmallString<256> Buf;
+  llvm::raw_svector_ostream OS(Buf);
+
+  QualType SourceType = BaseClassRegion->getValueType();
+  QualType TargetType =
+      DerivedClassRegion->getSymbol()->getType()->getPointeeType();
+
+  OS << "Deleting an array of '" << TargetType.getAsString()
+     << "' objects as their base class '"
+     << SourceType.getAsString(C.getASTContext().getPrintingPolicy())
+     << "' is undefined";
+
+  auto R = std::make_unique<PathSensitiveBugReport>(*BT, OS.str(), N);
+
+  // Mark region of problematic base class for later use in the BugVisitor.
+  R->markInteresting(BaseClassRegion);
+  R->addVisitor<PtrCastVisitor>();
+  C.emitReport(std::move(R));
+}
+
+PathDiagnosticPieceRef
+CXXDeleteChecker::PtrCastVisitor::VisitNode(const ExplodedNode *N,
+                                            BugReporterContext &BRC,
+                                            PathSensitiveBugReport &BR) {
   const Stmt *S = N->getStmtForDiagnostics();
   if (!S)
     return nullptr;
@@ -118,12 +196,12 @@ DeleteWithNonVirtualDtorChecker::DeleteBugVisitor::VisitNode(
   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;
-  }
+  // FIXME: This way of getting base types does not support reference types.
+  QualType SourceType = CastE->getSubExpr()->getType()->getPointeeType();
+  QualType TargetType = CastE->getType()->getPointeeType();
+
+  if (SourceType.isNull() || TargetType.isNull() || SourceType == TargetType)
+    return nullptr;
 
   // Region associated with the current cast expression.
   const MemRegion *M = N->getSVal(CastE).getAsRegion();
@@ -134,22 +212,31 @@ DeleteWithNonVirtualDtorChecker::DeleteBugVisitor::VisitNode(
   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";
+
+  OS << "Casting from '" << SourceType.getAsString() << "' to '"
+     << TargetType.getAsString() << "' here";
+
   PathDiagnosticLocation Pos(S, BRC.getSourceManager(),
                              N->getLocationContext());
-  return std::make_shared<PathDiagnosticEventPiece>(Pos, OS.str(), true);
+  return std::make_shared<PathDiagnosticEventPiece>(Pos, OS.str(),
+                                                    /*addPosRange=*/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) {
+    const CheckerManager &mgr) {
   return true;
 }