[analyzer] Delete with non-virtual destructor check

[rnkovacs]

Summary

This check warns if a derived type object is deleted through a base pointer with a non-virtual destructor in its base class. It also places a note at the last point where the conversion from derived to base happened.

Corresponding CERT rule: OOP52-CPP: Do not delete a polymorphic object without a virtual destructor.

[aaron.ballman]

How does this check differ from the -Wdelete-non-virtual-dtor warning class that comes out of the frontend?

[NoQ]

It seems that this check is more powerful because it works by knowing the dynamic type of the object. However, i still suspect that -Wnon-virtual-dtor (the other one, without delete-, that simply asks to make the destructor of polymorphic classes virtual) covers most practical cases. The only thing i see not covered by -Wnon-virtual-dtor but covered by this checker is the situation when the destructor is private. Reka, would you confirm our understanding?

[rnkovacs]

In D35796#819965, @NoQ wrote:

It seems that this check is more powerful because it works by knowing the dynamic type of the object. However, i still suspect that -Wnon-virtual-dtor (the other one, without delete-, that simply asks to make the destructor of polymorphic classes virtual) covers most practical cases. The only thing i see not covered by -Wnon-virtual-dtor but covered by this checker is the situation when the destructor is private. Reka, would you confirm our understanding?

You're right, the flag covers many cases, but there are some common exceptions. E.g. deleting a polymorphic object without a virtual destructor might not be a problem if it's referenced by its precise type. The checker is aware of this. It looks for object deletions, checks their dynamic and static types, and only warns if an object is deleted through a base pointer (with no virtual destructor).

I added a slightly more sophisticated test case for the private destructor situation, I'm wondering whether you meant something like that.

[dcoughlin]

This looks like a useful checker! Have you run this on large codebases yet? Does it find bugs? What kind of false positives do you see? Do you have a sense of what additional work would it take to bring this out of alpha and have it turned on by default?

Other than some super tiny comments in-line, this looks good to me.

include/clang/StaticAnalyzer/Checkers/Checkers.td

296

I think users would find it helpful if this had a more specific name. There are a lot of ways to misuse polymorphic objects, and this checker won't check all of them.

What do you think about "DeleteWithNonVirtualDestructor"?

lib/StaticAnalyzer/Checkers/MisusedPolymorphicObjectChecker.cpp

16 ↗

(On Diff #108226)

I think it would be helpful to future maintainers to provide a high-level description of how this differs from -Wnon-virtual-dtor and -Wdelete-non-virtual-dtor.

137 ↗

(On Diff #108226)

A minor style suggestion to avoid the stacked noun phrase: "Conversion from derived to base happened here".

[rnkovacs]

include/clang/StaticAnalyzer/Checkers/Checkers.td

296

I settled with the slightly abbreviated "DeleteWithNonVirtualDtor" version to shorten it a little.

[rnkovacs]

Sorry for the late reply. I did run it on a few open-source projects as well as LLVM/Clang and honestly it didn't find anything. As the test cases seem to work fine it might already be in a state ready to bring out of alpha.

[rnkovacs]

• Accidentally left-in comment removed.
• Checker file clang-formatted.

[dcoughlin]

This looks good to me! Do you have commit access, or do you need someone to commit it for you?

[rnkovacs]

In D35796#878200, @dcoughlin wrote:

This looks good to me! Do you have commit access, or do you need someone to commit it for you?

Thanks! I don't, so it would be nice if someone committed it for me.

include/clang/StaticAnalyzer/Checkers/Checkers.td

 def VirtualCallChecker : Checker<"VirtualCall">,
   HelpText<"Check virtual function calls during construction or destruction">,
   DescFile<"VirtualCallChecker.cpp">;
 
 } // end: "optin.cplusplus"
 
 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">;
 
 def MisusedMovedObjectChecker: Checker<"MisusedMovedObject">,
      HelpText<"Method calls on a moved-from object and copying a moved-from "
               "object will be reported">,
      DescFile<"MisusedMovedObjectChecker.cpp">;
 
 } // end: "alpha.cplusplus"
 
 
 //===----------------------------------------------------------------------===//

lib/StaticAnalyzer/Checkers/CMakeLists.txt

add_clang_library(clangStaticAnalyzerCheckers
   CheckerDocumentation.cpp
   ChrootChecker.cpp
   ClangCheckers.cpp
   CloneChecker.cpp
   ConversionChecker.cpp
   CXXSelfAssignmentChecker.cpp
   DeadStoresChecker.cpp
   DebugCheckers.cpp
+  DeleteWithNonVirtualDtorChecker.cpp
   DereferenceChecker.cpp
   DirectIvarAssignment.cpp
   DivZeroChecker.cpp
   DynamicTypePropagation.cpp
   DynamicTypeChecker.cpp
   ExprInspectionChecker.cpp
   FixedAddressChecker.cpp
   GenericTaintChecker.cpp

lib/StaticAnalyzer/Checkers/DeleteWithNonVirtualDtorChecker.cpp

+//===-- 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 (DerivedClass->getNumBases() != 1)
+      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

+// 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
+}