Index: clang/docs/analyzer/checkers.rst =================================================================== --- clang/docs/analyzer/checkers.rst +++ clang/docs/analyzer/checkers.rst @@ -1784,6 +1784,24 @@ alpha.cplusplus ^^^^^^^^^^^^^^^ +.. _alpha-cplusplus-ArrayDelete: + +alpha.cplusplus.ArrayDelete (C++) +"""""""""""""""""""""""""""""""""""""""""""""" +Reports destructions of arrays of polymorphic objects that are destructed as their base class. + +.. code-block:: cpp + + 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 + } + .. _alpha-cplusplus-DeleteWithNonVirtualDtor: alpha.cplusplus.DeleteWithNonVirtualDtor (C++) Index: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td =================================================================== --- clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -754,9 +754,21 @@ Documentation, Hidden; +def CXXArrayModeling: Checker<"CXXArrayModeling">, + HelpText<"The base of a few CXX array related checkers.">, + Documentation, + Hidden; + +def CXXArrayDeleteChecker : Checker<"ArrayDelete">, + HelpText<"Reports destructions of arrays of polymorphic objects that are" + "destructed as their base class.">, + Dependencies<[CXXArrayModeling]>, + Documentation; + def DeleteWithNonVirtualDtorChecker : Checker<"DeleteWithNonVirtualDtor">, HelpText<"Reports destructions of polymorphic objects with a non-virtual " "destructor in their base class">, + Dependencies<[CXXArrayModeling]>, Documentation; def EnumCastOutOfRangeChecker : Checker<"EnumCastOutOfRange">, Index: clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt =================================================================== --- clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -29,12 +29,12 @@ 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 Index: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp =================================================================== --- /dev/null +++ clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp @@ -0,0 +1,217 @@ +//=== 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> { + mutable std::unique_ptr ArrayBT; + mutable std::unique_ptr NonVirtualBT; + + 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: + bool ArrayDeleteEnabled = false; + CheckerNameRef ArrayDeleteName; + bool NonVirtualDeleteEnabled = false; + CheckerNameRef NonVirtualDeleteName; + + void checkPreStmt(const CXXDeleteExpr *DE, CheckerContext &C) const; +}; +} // end anonymous 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; + + const auto *BaseClass = BaseClassRegion->getValueType()->getAsCXXRecordDecl(); + const auto *DerivedClass = + DerivedClassRegion->getSymbol()->getType()->getPointeeCXXRecordDecl(); + if (!BaseClass || !DerivedClass) + return; + + if (!BaseClass->hasDefinition() || !DerivedClass->hasDefinition()) + return; + + if (!DerivedClass->isDerivedFrom(BaseClass)) + return; + + if (DeleteKind != OO_Array_Delete && DeleteKind != OO_Delete) + return; + + if (ArrayDeleteEnabled && DeleteKind == OO_Array_Delete) { + if (!ArrayBT) + ArrayBT.reset( + new BugType(ArrayDeleteName, + "Deleting an array of polymorphic objects is undefined", + "Logic error")); + + ExplodedNode *N = C.generateNonFatalErrorNode(); + if (!N) + return; + auto R = std::make_unique( + *ArrayBT, ArrayBT->getDescription(), N); + + // Mark region of problematic base class for later use in the BugVisitor. + R->markInteresting(BaseClassRegion); + R->addVisitor(std::make_unique()); + C.emitReport(std::move(R)); + } + + if (NonVirtualDeleteEnabled && !BaseClass->getDestructor()->isVirtual()) { + if (!NonVirtualBT) + NonVirtualBT.reset( + new BugType(NonVirtualDeleteName, + "Destruction of a polymorphic object with no " + "virtual destructor", + "Logic error")); + + ExplodedNode *N = C.generateNonFatalErrorNode(); + if (!N) + return; + auto R = std::make_unique( + *NonVirtualBT, NonVirtualBT->getDescription(), N); + + // Mark region of problematic base class for later use in the BugVisitor. + R->markInteresting(BaseClassRegion); + R->addVisitor(std::make_unique()); + C.emitReport(std::move(R)); + } +} + +PathDiagnosticPieceRef +CXXDeleteChecker::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(S); + if (!CastE) + return nullptr; + + // Only interested in DerivedToBase implicit casts. + // Explicit casts can have different CastKinds. + if (const auto *ImplCastE = dyn_cast(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(Pos, OS.str(), true); +} + +void ento::registerCXXArrayModeling(CheckerManager &mgr) { + mgr.registerChecker(); +} + +bool ento::shouldRegisterCXXArrayModeling(const CheckerManager &mgr) { + return true; +} + +void ento::registerCXXArrayDeleteChecker(CheckerManager &mgr) { + CXXDeleteChecker *checker = mgr.getChecker(); + checker->ArrayDeleteEnabled = true; + checker->ArrayDeleteName = mgr.getCurrentCheckerName(); +} + +bool ento::shouldRegisterCXXArrayDeleteChecker(const CheckerManager &mgr) { + return true; +} + +void ento::registerDeleteWithNonVirtualDtorChecker(CheckerManager &mgr) { + CXXDeleteChecker *checker = mgr.getChecker(); + checker->NonVirtualDeleteEnabled = true; + checker->NonVirtualDeleteName = mgr.getCurrentCheckerName(); +} + +bool ento::shouldRegisterDeleteWithNonVirtualDtorChecker( + const CheckerManager &mgr) { + return true; +} Index: clang/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp +++ /dev/null @@ -1,155 +0,0 @@ -//===-- 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> { - mutable std::unique_ptr 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(); - const auto *DerivedClassRegion = MR->getBaseRegion()->getAs(); - 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(*BT, BT->getDescription(), N); - - // Mark region of problematic base class for later use in the BugVisitor. - R->markInteresting(BaseClassRegion); - R->addVisitor(std::make_unique()); - 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(S); - if (!CastE) - return nullptr; - - // Only interested in DerivedToBase implicit casts. - // Explicit casts can have different CastKinds. - if (const auto *ImplCastE = dyn_cast(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(Pos, OS.str(), true); -} - -void ento::registerDeleteWithNonVirtualDtorChecker(CheckerManager &mgr) { - mgr.registerChecker(); -} - -bool ento::shouldRegisterDeleteWithNonVirtualDtorChecker( - const CheckerManager &mgr) { - return true; -} Index: clang/test/Analysis/ArrayDelete.cpp =================================================================== --- /dev/null +++ clang/test/Analysis/ArrayDelete.cpp @@ -0,0 +1,72 @@ +// 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 +} Index: clang/www/analyzer/alpha_checks.html =================================================================== --- clang/www/analyzer/alpha_checks.html +++ clang/www/analyzer/alpha_checks.html @@ -330,6 +330,27 @@ + + + +