Discookie updated this revision to Diff 556402.
Discookie marked 8 inline comments as done.
Discookie added a comment.
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.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D158156/new/
https://reviews.llvm.org/D158156
Files:
clang/docs/analyzer/checkers.rst
clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp
clang/test/Analysis/ArrayDelete.cpp
clang/www/analyzer/alpha_checks.html
Index: clang/www/analyzer/alpha_checks.html
===================================================================
--- clang/www/analyzer/alpha_checks.html
+++ clang/www/analyzer/alpha_checks.html
@@ -330,6 +330,27 @@
<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">
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<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
+}
+
+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<Derived*>(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<Derived*>(b4);
+ DoubleDerived *dd4 = reinterpret_cast<DoubleDerived*>(d4);
+ delete[] dd4; // no-warning
+
+ Base *b5 = new DoubleDerived[10];
+ DoubleDerived *dd5 = reinterpret_cast<DoubleDerived*>(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/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<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() : 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<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,18 +132,53 @@
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));
+}
+
+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<PathSensitiveBugReport>(*BT, BT->getDescription(), 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
-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<ImplicitCastExpr>(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<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) {
@@ -150,6 +233,6 @@
}
bool ento::shouldRegisterDeleteWithNonVirtualDtorChecker(
- const CheckerManager &mgr) {
+ const CheckerManager &mgr) {
return true;
}
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<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">,
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 <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++)
@@ -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
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
