Discookie created this revision.
Discookie added reviewers: NoQ, donat.nagy, balazske.
Discookie added projects: clang, All.
Herald added subscribers: steakhal, manas, ASDenysPetrov, martong, dkrupp,
Szelethus, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun.
Discookie requested review of this revision.
Herald added a subscriber: cfe-commits.
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
<https://wiki.sei.cmu.edu/confluence/display/cplusplus/EXP51-CPP.+Do+not+delete+an+array+through+a+pointer+of+the+incorrect+type>.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D158156
Files:
clang/docs/analyzer/checkers.rst
clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp
clang/lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.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,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<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
+}
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<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;
-}
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<check::PreStmt<CXXDeleteExpr>> {
+ mutable std::unique_ptr<BugType> ArrayBT;
+ mutable std::unique_ptr<BugType> 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<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 (!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<PathSensitiveBugReport>(
+ *ArrayBT, ArrayBT->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));
+ }
+
+ 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<PathSensitiveBugReport>(
+ *NonVirtualBT, NonVirtualBT->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
+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<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::registerCXXArrayModeling(CheckerManager &mgr) {
+ mgr.registerChecker<CXXDeleteChecker>();
+}
+
+bool ento::shouldRegisterCXXArrayModeling(const CheckerManager &mgr) {
+ return true;
+}
+
+void ento::registerCXXArrayDeleteChecker(CheckerManager &mgr) {
+ CXXDeleteChecker *checker = mgr.getChecker<CXXDeleteChecker>();
+ checker->ArrayDeleteEnabled = true;
+ checker->ArrayDeleteName = mgr.getCurrentCheckerName();
+}
+
+bool ento::shouldRegisterCXXArrayDeleteChecker(const CheckerManager &mgr) {
+ return true;
+}
+
+void ento::registerDeleteWithNonVirtualDtorChecker(CheckerManager &mgr) {
+ CXXDeleteChecker *checker = mgr.getChecker<CXXDeleteChecker>();
+ checker->NonVirtualDeleteEnabled = true;
+ checker->NonVirtualDeleteName = mgr.getCurrentCheckerName();
+}
+
+bool ento::shouldRegisterDeleteWithNonVirtualDtorChecker(
+ const CheckerManager &mgr) {
+ return true;
+}
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/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<NotDocumented>,
Hidden;
+def CXXArrayModeling: Checker<"CXXArrayModeling">,
+ HelpText<"The base of a few CXX array related checkers.">,
+ Documentation<NotDocumented>,
+ Hidden;
+
+def CXXArrayDeleteChecker : Checker<"ArrayDelete">,
+ HelpText<"Reports destructions of arrays of polymorphic objects that are"
+ "destructed as their base class.">,
+ Dependencies<[CXXArrayModeling]>,
+ Documentation<HasDocumentation>;
+
def DeleteWithNonVirtualDtorChecker : Checker<"DeleteWithNonVirtualDtor">,
HelpText<"Reports destructions of polymorphic objects with a non-virtual "
"destructor in their base class">,
+ Dependencies<[CXXArrayModeling]>,
Documentation<HasDocumentation>;
def EnumCastOutOfRangeChecker : Checker<"EnumCastOutOfRange">,
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++)
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
