[analyzer] Add new delete with non-virtual destructor check

Patch by: Reka Nikolett Kovacs

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


git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@313973 91177308-0d34-0410-b5e6-96231b3b80d8

include/clang/StaticAnalyzer/Checkers/Checkers.td

@@ -284,6 +284,11 @@ def VirtualCallChecker : Checker<"VirtualCall">,
 
 let ParentPackage = CplusplusAlpha in {
 
+def DeleteWithNonVirtualDtorChecker : Checker<"DeleteWithNonVirtualDtor">,
+  HelpText<"Reports destructions of polymorphic objects with a non-virtual "
+           "destructor in their base class">,
+  DescFile<"DeleteWithNonVirtualDtorChecker.cpp">;
+
 def IteratorRangeChecker : Checker<"IteratorRange">,
   HelpText<"Check for iterators used outside their valid ranges">,
   DescFile<"IteratorChecker.cpp">;

lib/StaticAnalyzer/Checkers/CMakeLists.txt

@@ -29,6 +29,7 @@ add_clang_library(clangStaticAnalyzerCheckers
   CXXSelfAssignmentChecker.cpp
   DeadStoresChecker.cpp
   DebugCheckers.cpp
+  DeleteWithNonVirtualDtorChecker.cpp
   DereferenceChecker.cpp
   DirectIvarAssignment.cpp
   DivZeroChecker.cpp

lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp

@@ -0,0 +1,153 @@
+//===-- DeleteWithNonVirtualDtorChecker.cpp -----------------------*- C++ -*--//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// 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 "ClangSACheckers.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/DynamicTypeMap.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 BugReporterVisitorImpl<DeleteBugVisitor> {
+  public:
+    DeleteBugVisitor() : Satisfied(false) {}
+    void Profile(llvm::FoldingSetNodeID &ID) const override {
+      static int X = 0;
+      ID.AddPointer(&X);
+    }
+    std::shared_ptr<PathDiagnosticPiece> VisitNode(const ExplodedNode *N,
+                                                   const ExplodedNode *PrevN,
+                                                   BugReporterContext &BRC,
+                                                   BugReport &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();
+  auto R = llvm::make_unique<BugReport>(*BT, BT->getName(), N);
+
+  // Mark region of problematic base class for later use in the BugVisitor.
+  R->markInteresting(BaseClassRegion);
+  R->addVisitor(llvm::make_unique<DeleteBugVisitor>());
+  C.emitReport(std::move(R));
+}
+
+std::shared_ptr<PathDiagnosticPiece>
+DeleteWithNonVirtualDtorChecker::DeleteBugVisitor::VisitNode(
+    const ExplodedNode *N, const ExplodedNode *PrevN, BugReporterContext &BRC,
+    BugReport &BR) {
+  // Stop traversal after the first conversion was found on a path.
+  if (Satisfied)
+    return nullptr;
+
+  ProgramStateRef State = N->getState();
+  const LocationContext *LC = N->getLocationContext();
+  const Stmt *S = PathDiagnosticLocation::getStmt(N);
+  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 = State->getSVal(CastE, LC).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,
+                                                    nullptr);
+}
+
+void ento::registerDeleteWithNonVirtualDtorChecker(CheckerManager &mgr) {
+  mgr.registerChecker<DeleteWithNonVirtualDtorChecker>();
+}

test/Analysis/DeleteWithNonVirtualDtor.cpp

@@ -0,0 +1,187 @@
+// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.cplusplus.DeleteWithNonVirtualDtor -std=c++11 -verify -analyzer-output=text %s
+
+struct Virtual {
+  virtual ~Virtual() {}
+};
+
+struct VDerived : public Virtual {};
+
+struct NonVirtual {
+  ~NonVirtual() {}
+};
+
+struct NVDerived : public NonVirtual {};
+struct NVDoubleDerived : public NVDerived {};
+
+struct Base {
+  virtual void destroy() = 0;
+};
+
+class PrivateDtor final : public Base {
+public:
+  void destroy() { delete this; }
+private:
+  ~PrivateDtor() {}
+};
+
+struct ImplicitNV {
+  virtual void f();
+};
+
+struct ImplicitNVDerived : public ImplicitNV {};
+
+NVDerived *get();
+
+NonVirtual *create() {
+  NonVirtual *x = new NVDerived(); // expected-note{{Conversion from derived to base happened here}}
+  return x;
+}
+
+void sink(NonVirtual *x) {
+  delete x; // expected-warning{{Destruction of a polymorphic object with no virtual destructor}}
+  // expected-note@-1{{Destruction of a polymorphic object with no virtual destructor}}
+}
+
+void sinkCast(NonVirtual *y) {
+  delete reinterpret_cast<NVDerived*>(y);
+}
+
+void sinkParamCast(NVDerived *z) {
+  delete z;
+}
+
+void singleDerived() {
+  NonVirtual *sd;
+  sd = new NVDerived(); // expected-note{{Conversion from derived to base happened here}}
+  delete sd; // expected-warning{{Destruction of a polymorphic object with no virtual destructor}}
+  // expected-note@-1{{Destruction of a polymorphic object with no virtual destructor}}
+}
+
+void singleDerivedArr() {
+  NonVirtual *sda = new NVDerived[5]; // expected-note{{Conversion from derived to base happened here}}
+  delete[] sda; // expected-warning{{Destruction of a polymorphic object with no virtual destructor}}
+  // expected-note@-1{{Destruction of a polymorphic object with no virtual destructor}}
+}
+
+void doubleDerived() {
+  NonVirtual *dd = new NVDoubleDerived(); // expected-note{{Conversion from derived to base happened here}}
+  delete (dd); // expected-warning{{Destruction of a polymorphic object with no virtual destructor}}
+  // expected-note@-1{{Destruction of a polymorphic object with no virtual destructor}}
+}
+
+void assignThroughFunction() {
+  NonVirtual *atf = get(); // expected-note{{Conversion from derived to base happened here}}
+  delete atf; // expected-warning{{Destruction of a polymorphic object with no virtual destructor}}
+  // expected-note@-1{{Destruction of a polymorphic object with no virtual destructor}}
+}
+
+void assignThroughFunction2() {
+  NonVirtual *atf2;
+  atf2 = get(); // expected-note{{Conversion from derived to base happened here}}
+  delete atf2; // expected-warning{{Destruction of a polymorphic object with no virtual destructor}}
+  // expected-note@-1{{Destruction of a polymorphic object with no virtual destructor}}
+}
+
+void createThroughFunction() {
+  NonVirtual *ctf = create(); // expected-note{{Calling 'create'}}
+  // expected-note@-1{{Returning from 'create'}}
+  delete ctf; // expected-warning {{Destruction of a polymorphic object with no virtual destructor}}
+  // expected-note@-1{{Destruction of a polymorphic object with no virtual destructor}}
+}
+
+void deleteThroughFunction() {
+  NonVirtual *dtf = new NVDerived(); // expected-note{{Conversion from derived to base happened here}}
+  sink(dtf); // expected-note{{Calling 'sink'}}
+}
+
+void singleCastCStyle() {
+  NVDerived *sccs = new NVDerived();
+  NonVirtual *sccs2 = (NonVirtual*)sccs; // expected-note{{Conversion from derived to base happened here}}
+  delete sccs2; // expected-warning{{Destruction of a polymorphic object with no virtual destructor}}
+  // expected-note@-1{{Destruction of a polymorphic object with no virtual destructor}}
+}
+
+void doubleCastCStyle() {
+  NonVirtual *dccs = new NVDerived();
+  NVDerived *dccs2 = (NVDerived*)dccs;
+  dccs = (NonVirtual*)dccs2; // expected-note{{Conversion from derived to base happened here}}
+  delete dccs; // expected-warning{{Destruction of a polymorphic object with no virtual destructor}}
+  // expected-note@-1{{Destruction of a polymorphic object with no virtual destructor}}
+}
+
+void singleCast() {
+  NVDerived *sc = new NVDerived();
+  NonVirtual *sc2 = reinterpret_cast<NonVirtual*>(sc); // expected-note{{Conversion from derived to base happened here}}
+  delete sc2; // expected-warning{{Destruction of a polymorphic object with no virtual destructor}}
+  // expected-note@-1{{Destruction of a polymorphic object with no virtual destructor}}
+}
+
+void doubleCast() {
+  NonVirtual *dd = new NVDerived();
+  NVDerived *dd2 = reinterpret_cast<NVDerived*>(dd);
+  dd = reinterpret_cast<NonVirtual*>(dd2); // expected-note {{Conversion from derived to base happened here}}
+  delete dd; // expected-warning {{Destruction of a polymorphic object with no virtual destructor}}
+  // expected-note@-1{{Destruction of a polymorphic object with no virtual destructor}}
+}
+
+void implicitNV() {
+  ImplicitNV *invd = new ImplicitNVDerived(); // expected-note{{Conversion from derived to base happened here}}
+  delete invd; // expected-warning{{Destruction of a polymorphic object with no virtual destructor}}
+  // expected-note@-1{{Destruction of a polymorphic object with no virtual destructor}}
+}
+
+void doubleDecl() {
+  ImplicitNV *dd1, *dd2;
+  dd1 = new ImplicitNVDerived(); // expected-note{{Conversion from derived to base happened here}}
+  delete dd1; // expected-warning{{Destruction of a polymorphic object with no virtual destructor}}
+  // expected-note@-1{{Destruction of a polymorphic object with no virtual destructor}}
+}
+
+void virtualBase() {
+  Virtual *vb = new VDerived();
+  delete vb; // no-warning
+}
+
+void notDerived() {
+  NonVirtual *nd = new NonVirtual();
+  delete nd; // no-warning
+}
+
+void notDerivedArr() {
+  NonVirtual *nda = new NonVirtual[3];
+  delete[] nda; // no-warning
+}
+
+void cast() {
+  NonVirtual *c = new NVDerived();
+  delete reinterpret_cast<NVDerived*>(c); // no-warning
+}
+
+void deleteThroughFunction2() {
+  NonVirtual *dtf2 = new NVDerived();
+  sinkCast(dtf2); // no-warning
+}
+
+void deleteThroughFunction3() {
+  NVDerived *dtf3;
+  dtf3 = new NVDerived();
+  sinkParamCast(dtf3); // no-warning
+}
+
+void stackVar() {
+  NonVirtual sv2;
+  delete &sv2; // no-warning
+}
+
+// Deleting a polymorphic object with a non-virtual dtor
+// is not a problem if it is referenced by its precise type.
+
+void preciseType() {
+  NVDerived *pt = new NVDerived();
+  delete pt; // no-warning
+}
+
+void privateDtor() {
+  Base *pd = new PrivateDtor();
+  pd->destroy(); // no-warning
+}