Index: clang/docs/analyzer/checkers.rst =================================================================== --- clang/docs/analyzer/checkers.rst +++ clang/docs/analyzer/checkers.rst @@ -1784,6 +1784,31 @@ 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 `_. + +.. 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++) @@ -1792,13 +1817,17 @@ .. 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 } Index: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td =================================================================== --- clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -754,6 +754,11 @@ Documentation, Hidden; +def CXXArrayDeleteChecker : Checker<"ArrayDelete">, + HelpText<"Reports destructions of arrays of polymorphic objects that are" + "destructed as their base class.">, + Documentation; + def DeleteWithNonVirtualDtorChecker : Checker<"DeleteWithNonVirtualDtor">, HelpText<"Reports destructions of polymorphic objects with a non-virtual " "destructor in their base class">, Index: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp @@ -1,4 +1,4 @@ -//===-- 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. @@ -6,17 +6,25 @@ // //===----------------------------------------------------------------------===// // -// Defines a checker for the OOP52-CPP CERT rule: Do not delete a polymorphic -// object without a virtual destructor. +// This file defines two 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,13 +42,10 @@ using namespace ento; namespace { -class DeleteWithNonVirtualDtorChecker - : public Checker> { - mutable std::unique_ptr BT; - - class DeleteBugVisitor : public BugReporterVisitor { +class CXXDeleteChecker : public Checker> { +protected: + class PtrCastVisitor : public BugReporterVisitor { public: - DeleteBugVisitor() : Satisfied(false) {} void Profile(llvm::FoldingSetNodeID &ID) const override { static int X = 0; ID.AddPointer(&X); @@ -50,35 +55,68 @@ PathSensitiveBugReport &BR) override; private: - bool Satisfied; + 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 BT; + + void + checkTypedDeleteExpr(const CXXDeleteExpr *DE, CheckerContext &C, + const TypedValueRegion *BaseClassRegion, + const SymbolicRegion *DerivedClassRegion) const override; +}; + +class CXXArrayDeleteChecker : public CXXDeleteChecker { + mutable std::unique_ptr 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(); const auto *DerivedClassRegion = MR->getBaseRegion()->getAs(); 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,18 +132,53 @@ ExplodedNode *N = C.generateNonFatalErrorNode(); if (!N) return; - auto R = std::make_unique(*BT, BT->getDescription(), N); + auto R = + std::make_unique(*BT, BT->getDescription(), N); // Mark region of problematic base class for later use in the BugVisitor. R->markInteresting(BaseClassRegion); - R->addVisitor(std::make_unique()); + R->addVisitor(); + 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 (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(*BT, BT->getDescription(), N); + + // Mark region of problematic base class for later use in the BugVisitor. + R->markInteresting(BaseClassRegion); + R->addVisitor(); C.emitReport(std::move(R)); } PathDiagnosticPieceRef -DeleteWithNonVirtualDtorChecker::DeleteBugVisitor::VisitNode( - const ExplodedNode *N, BugReporterContext &BRC, - PathSensitiveBugReport &BR) { +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; @@ -120,6 +193,8 @@ // Only interested in DerivedToBase implicit casts. // Explicit casts can have different CastKinds. + // FIXME: The checker currently matches all explicit casts, + // but only ones casting to a base class (or simular) should be matcherd. if (const auto *ImplCastE = dyn_cast(CastE)) { if (ImplCastE->getCastKind() != CK_DerivedToBase) return nullptr; @@ -142,7 +217,15 @@ OS << "Conversion from derived to base happened here"; PathDiagnosticLocation Pos(S, BRC.getSourceManager(), N->getLocationContext()); - return std::make_shared(Pos, OS.str(), true); + return std::make_shared(Pos, OS.str(), /*addPosRange=*/true); +} + +void ento::registerCXXArrayDeleteChecker(CheckerManager &mgr) { + mgr.registerChecker(); +} + +bool ento::shouldRegisterCXXArrayDeleteChecker(const CheckerManager &mgr) { + return true; } void ento::registerDeleteWithNonVirtualDtorChecker(CheckerManager &mgr) { @@ -150,6 +233,6 @@ } bool ento::shouldRegisterDeleteWithNonVirtualDtorChecker( - const CheckerManager &mgr) { + const CheckerManager &mgr) { return true; } Index: clang/test/Analysis/ArrayDelete.cpp =================================================================== --- /dev/null +++ clang/test/Analysis/ArrayDelete.cpp @@ -0,0 +1,100 @@ +// 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(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(b); // no-warning + + Base *sb = new Derived[10]; + sink_cast(sb); // no-warning + + Derived *sd = new Derived[10]; + sink_derived(sd); // no-warning +} + +void multiple_derived() { + Base *b = new DoubleDerived[10]; // expected-note{{Conversion from derived to base happened here}} + delete[] b; // expected-warning{{Deleting an array of polymorphic objects is undefined}} + // expected-note@-1{{Deleting an array of polymorphic objects is undefined}} + + // FIXME: Currently all explicit casts are reported as derived-to-base casts. + Base *b2 = new DoubleDerived[10]; + Derived *d2 = reinterpret_cast(b2); // expected-note{{Conversion from derived to base happened here}} + delete[] d2; // expected-warning{{Deleting an array of polymorphic objects is undefined}} + // expected-note@-1{{Deleting an array of polymorphic objects is undefined}} + + Derived *d3 = new DoubleDerived[10]; + Base *b3 = d3; // expected-note{{Conversion from derived to base happened here}} + delete[] b3; // expected-warning{{Deleting an array of polymorphic objects is undefined}} + // expected-note@-1{{Deleting an array of polymorphic objects is undefined}} + + Base *b4 = new DoubleDerived[10]; + Derived *d4 = reinterpret_cast(b4); + DoubleDerived *dd4 = reinterpret_cast(d4); + delete[] dd4; // no-warning + + Base *b5 = new DoubleDerived[10]; + DoubleDerived *dd5 = reinterpret_cast(b5); + Derived *d5 = dd5; // expected-note{{Conversion from derived to base happened here}} + delete[] d5; // expected-warning{{Deleting an array of polymorphic objects is undefined}} + // expected-note@-1{{Deleting an array of polymorphic objects is undefined}} +} Index: clang/www/analyzer/alpha_checks.html =================================================================== --- clang/www/analyzer/alpha_checks.html +++ clang/www/analyzer/alpha_checks.html @@ -330,6 +330,27 @@ + + + +