C++: Fork AV Rule 78 into NonVirtualDestructorInBaseClass

AV Rule 78 has proved too noisy for use on lgtm.com. However, if we make the rule less noisy by, say, allowing a protected destructor to be non-virtual, we're no longer actually enforcing AV Rule 78. Instead, I've copied AV Rule 78 into NonVirtualDestructorInBaseClass.ql, given the new query the `@id` that AV Rule 78 had, and given AV Rule 78 a new JSF-specific `@id`. The new rule allows non-public non-virtual destructors, which is the problem originally reported by an lgtm.com user.

cpp/ql/src/Likely Bugs/OO/NonVirtualDestructorInBaseClass.cpp

@@ -0,0 +1,34 @@
+class Base {
+public:
+	Resource *p;
+	Base() {
+		p = createResource();
+	}
+	virtual void f() { //has virtual function
+		//...
+	}
+	//...
+	~Base() { //wrong: is non-virtual
+		freeResource(p);
+	}
+};
+
+class Derived: public Base {
+public:
+	Resource *dp;
+	Derived() {
+		dp = createResource2();
+	}
+	~Derived() {
+		freeResource2(dp);
+	}
+};
+
+int f() {
+	Base *b = new Derived(); //creates resources for both Base::p and Derived::dp
+	//...
+
+	//will only call Base::~Base(), leaking the resource dp.
+	//Change both destructors to virtual to ensure they are both called.
+	delete b;
+}

cpp/ql/src/Likely Bugs/OO/NonVirtualDestructorInBaseClass.qhelp

@@ -0,0 +1,38 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+
+<overview>
+<p>
+This rule finds classes with virtual functions but no virtual destructor. Deleting a class without a virtual destructor will 
+only call the destructor of the type of the pointer being deleted. This can cause a defect if the pointer type is a base 
+type while the object instance is a derived type.
+</p>
+
+
+</overview>
+<recommendation>
+<p>
+Make sure that all classes with virtual functions also have a virtual destructor, especially if other classes derive from them.
+</p>
+
+</recommendation>
+<example><sample src="NonVirtualDestructorInBaseClass.cpp" />
+
+
+
+</example>
+<references>
+
+<li>
+  S. Meyers. <em>Effective C++ 3d ed.</em> pp 40-44. Addison-Wesley Professional, 2005.
+</li>
+<li>
+  <a href="http://blogs.msdn.com/b/oldnewthing/archive/2004/05/07/127826.aspx">When should your destructor be virtual?</a>
+</li>
+
+
+</references>
+</qhelp>

cpp/ql/src/Likely Bugs/OO/NonVirtualDestructorInBaseClass.ql

@@ -0,0 +1,27 @@
+/**
+ * @name Non-virtual destructor in base class
+ * @description All base classes with a virtual function should define a virtual destructor. If an application attempts to delete a derived class object through a base class pointer, the result is undefined if the base class destructor is non-virtual.
+ * @kind problem
+ * @problem.severity warning
+ * @precision high
+ * @id cpp/virtual-destructor
+ * @tags reliability
+ *       readability
+ *       language-features
+ */
+import cpp
+
+// find classes with virtual functions that have a destructor that is not virtual and for which there exists a derived class
+// when calling the destructor of a derived class the destructor in the base class may not be called
+
+from Class c
+where exists(VirtualFunction f | f.getDeclaringType() = c)
+  and exists(Destructor d | d.getDeclaringType() = c and
+                            // Ignore non-public destructors, which prevent an object of the declaring class from being deleted
+                            // directly (except from within the class itself). This is a common pattern in real-world code.
+                            d.hasSpecifier("public") and
+                            not d.isVirtual() and
+                            not d.isDeleted() and
+                            not d.isCompilerGenerated())
+  and exists(ClassDerivation d | d.getBaseClass() = c)
+select c, "A base class with a virtual function should define a virtual destructor."

cpp/ql/src/jsf/4.10 Classes/AV Rule 78.ql

@@ -4,7 +4,7 @@
  * @kind problem
  * @problem.severity warning
  * @precision high
- * @id cpp/virtual-destructor
+ * @id cpp/jsf/av-rule-78
  * @tags reliability
  *       readability
  *       language-features

cpp/ql/test/query-tests/Likely Bugs/OO/NonVirtualDestructorInBaseClass/NonVirtualDestructorInBaseClass.cpp

@@ -0,0 +1,121 @@
+struct HasDtor
+{
+    ~HasDtor();
+};
+
+struct Base_NonVirtual_NoDtor
+{
+    void NonVirtualFunction();
+};
+
+struct Base_NonVirtual_VirtualDtor
+{
+    virtual ~Base_NonVirtual_VirtualDtor();
+    void NonVirtualFunction();
+};
+
+struct Base_NonVirtual_NonVirtualDtor
+{
+    ~Base_NonVirtual_NonVirtualDtor();
+    void NonVirtualFunction();
+};
+
+struct Base_NonVirtual_ImplicitDtor
+{
+    HasDtor m_hasDtor;
+    void NonVirtualFunction();
+};
+
+struct Derived_NonVirtual_NoDtor : public Base_NonVirtual_NoDtor
+{
+};
+
+struct Derived_NonVirtual_VirtualDtor : public Base_NonVirtual_VirtualDtor
+{
+};
+
+struct Derived_NonVirtual_NonVirtualDtor : public Base_NonVirtual_NonVirtualDtor
+{
+};
+
+struct Derived_NonVirtual_ImplicitDtor : public Base_NonVirtual_ImplicitDtor
+{
+};
+
+struct Base_Virtual_NoDtor
+{
+    virtual void VirtualFunction();
+};
+
+struct Base_Virtual_VirtualDtor
+{
+    virtual ~Base_Virtual_VirtualDtor();
+    virtual void VirtualFunction();
+};
+
+struct Base_Virtual_NonVirtualDtor
+{
+    ~Base_Virtual_NonVirtualDtor();
+    virtual void VirtualFunction();
+};
+
+struct Base_Virtual_ImplicitDtor
+{
+    HasDtor m_hasDtor;
+    virtual void VirtualFunction();
+};
+
+struct Base_Virtual_NonVirtualDtorWithDefinition
+{
+    ~Base_Virtual_NonVirtualDtorWithDefinition();
+    virtual void VirtualFunction();
+};
+
+Base_Virtual_NonVirtualDtorWithDefinition::~Base_Virtual_NonVirtualDtorWithDefinition()
+{
+}
+
+struct Base_Virtual_NonVirtualDtorWithInlineDefinition
+{
+    ~Base_Virtual_NonVirtualDtorWithInlineDefinition()
+    {
+    }
+    virtual void VirtualFunction();
+};
+
+struct Base_Virtual_ProtectedNonVirtualDtor
+{
+protected:
+    ~Base_Virtual_ProtectedNonVirtualDtor();
+
+public:
+    virtual void VirtualFunction();
+};
+
+struct Derived_Virtual_NoDtor : public Base_Virtual_NoDtor
+{
+};
+
+struct Derived_Virtual_VirtualDtor : public Base_Virtual_VirtualDtor
+{
+};
+
+struct Derived_Virtual_NonVirtualDtor : public Base_Virtual_NonVirtualDtor
+{
+};
+
+struct Derived_Virtual_ImplicitDtor : public Base_Virtual_ImplicitDtor
+{
+};
+
+struct Derived_Virtual_NonVirtualDtorWithDefinition: public Base_Virtual_NonVirtualDtorWithDefinition
+{
+};
+
+struct Derived_Virtual_NonVirtualDtorWithInlineDefinition: public Base_Virtual_NonVirtualDtorWithInlineDefinition
+{
+};
+
+struct Derived_Virtual_ProtectedNonVirtualDtor : public Base_Virtual_ProtectedNonVirtualDtor
+{
+};

cpp/ql/test/query-tests/Likely Bugs/OO/NonVirtualDestructorInBaseClass/NonVirtualDestructorInBaseClass.expected

@@ -0,0 +1,3 @@
+| NonVirtualDestructorInBaseClass.cpp:56:8:56:34 | Base_Virtual_NonVirtualDtor | Base classes with a virtual function must define a virtual destructor. |
+| NonVirtualDestructorInBaseClass.cpp:68:8:68:48 | Base_Virtual_NonVirtualDtorWithDefinition | Base classes with a virtual function must define a virtual destructor. |
+| NonVirtualDestructorInBaseClass.cpp:78:8:78:54 | Base_Virtual_NonVirtualDtorWithInlineDefinition | Base classes with a virtual function must define a virtual destructor. |

cpp/ql/test/query-tests/Likely Bugs/OO/NonVirtualDestructorInBaseClass/NonVirtualDestructorInBaseClass.qlref

@@ -0,0 +1 @@
+Likely Bugs/OO/NonVirtualDestructorInBaseClass.ql