Is it safe to make destructor not virtual, and delete base pointer in special circumstances?

Question

Let's say we have a class BST_Node :

struct BST_Node {
  BST_Node* left;
  BST_Node* right;
}

And a class AVL_Node :

struct AVL_Node : BST_Node {
  int height;
}

and in some function

void destroyTree() {
  BST_Node *mynode = new AVL_Node;
  delete mynode; //  Is it ok ?
}

Question #1

When the destructor is non virtual but there are only primitives types in derived, is it safe to call delete on base class ? (will there be no memory leaks ?)

Question #2

What is the rule when declaring a destructor virtual in derived class only ? As I understood, all of the destructors are the same function, we can call it destructor() and then when we delete a base pointer, the destructor is called only for the base class, but when deleting a derived class, the destructor will also being dispatched into sub-derived classes.

Answer 1

When the destructor is non virtual but there are only primitives types in derived, is it safe to call delete on base class ? (will there be no memory leaks ?)

You might not be aware of it, but these are two different questions.

The latter answer is: no, there won't be any memory leaks for this specific example, but there could be for other examples.

And the reason why is the answer to the former question: no, it is not safe to do this. This constitutes undefined behavior, even if the behavior is well-understood for nearly all compilers—and 'understood' is not synecticy for "is safe to do", just to be clear.

When you write code like delete mynode;, the compiler has to figure out which destructor to call. If the destructor for mynode is not virtual, then it will always use the base destructor, doing whatever the base destructor needs to do, but not whatever the derived destructor needs to do.

In this case, that's not such a big deal: the only thing AVL_Node adds is a locally allocated int variable, which will be cleaned up as part of the same process that cleans up the whole pointer.

But if your code were like this instead:

struct AVL_Node : public BST_Node {
    std::unique_ptr<int> height = std::make_unique<int>();
};

Then this code would definitely cause memory leaks, even though we've expressly used a smart pointer in the construction of the derived object! The smart pointer doesn't save us from the tribulations of deleteing a base pointer with a non-virtual destructor.

And in general, your code could cause any kind of leak, including but not limited to resource leaks, file handle leaks, and so on, if AVL_Node were responsible for other objects. Consider, for example, if AVL_Node had something like this, which is extremely common in certain kinds of graphics code:

struct AVL_Node : public BST_Node {
    int handle;
    AVL_Node() {
        glGenArrays(1, &handle);
    }
    /*
     * Pretend we implemented the copy/move constructors/assignment operators as needed
     */
    ~AVLNode() {
        glDeleteArrays(1, &handle);
    }
};

Your code wouldn't leak memory (in your own code), but it would leak an OpenGL object (and any memory allocated by that object).

What is the rule when declaring a destructor virtual in derived class only ?

If you never plan to store a pointer to the base class, then this is fine.

It's also unnecessary unless you plan to also create further derived instances of the derived class.

So here's the example we'll use for the sake of clarity:

struct A {
    std::unique_ptr<int> int_ptr = std::make_unique<int>();
};

struct B : A {
    std::unique_ptr<int> int_ptr_2 = std::make_unique<int>();
    virtual ~B() = default;
};

struct C : B {
    std::unique_ptr<int> int_ptr_3 = std::make_unique<int>();
    //virtual ~C() = default; // Unnecessary; implied by B having a virtual destructor
};

Now here's all the the code that's safe and unsafe to use with these three classes:

auto a1 = std::make_unique<A>(); //Safe; a1 knows its own type
std::unique_ptr<A> a2 = std::make_unique<A>(); //Safe; exactly the same as a1
auto b1 = std::make_unique<B>(); //Safe; b1 knows its own type
std::unique_ptr<B> b2 = std::make_unique<B>(); //Safe; exactly the same as b1
std::unique_ptr<A> b3 = std::make_unique<B>(); //UNSAFE: A does not have a virtual destructor!
auto c1 = std::make_unique<C>(); //Safe; c1 knows its own type
std::unique_ptr<C> c2 = std::make_unique<C>(); //Safe; exactly the same as c1
std::unique_ptr<B> c3 = std::make_unique<C>(); //Safe; B has a virtual destructor
std::unique_ptr<A> c4 = std::make_unique<C>(); //UNSAFE: A does not have a virtual destructor!

So if B (a class with a virtual destructor) inherits from A (a class without a virtual destructor), but as a programmer, you promise you will never refer to an instance of B with an A pointer, then you have nothing to worry about. So in that case, like my example tries to show, there may be valid reasons to declare the destructor of a derived class virtual while leaving the super class' destructor non-virtual.

Answer 2

Run away

Deleting a derived object through a pointer to base when there is no virtual destructor is undefined behavior. This is true regardless of how simple the derived type is.

Now, at runtime, every compiler turns delete foo into "find the destructor code, run it, then clean up the memory". But you cannot base your understanding of what C++ code means based on the runtime code emitted by a compiler.

So you naively can think "I don't care if we run the wrong destruction code; the only thing I added was an int. And the memory cleanup code handles over-allocation. So we are good!"

You even go and test it, and you look at the assembly produced, and everything works! And you conclude there is no problem here.

You are wrong.

Compilers do two things. First, the emit runtime code. Second, they use the structure of your program to reason about it.

That second part is a powerful feature, but it also makes doing undefined behavior extremely dangerous.

What your C++ program means in the "abstract machine" the C++ standard specifies matters. It is in that abstract machine that optimizations and code transformations occur. Knowing how an isolated snippet of code gets emitted on your physical machien does not tell you what that snippet of code does.

Here is a concrete example:

struct Foo {};
struct Bar:Foo{};

Foo* do_something( bool cond1, bool cond2 ) {
  Foo* foo = nullptr;
  if (cond1)
    foo = new Bar;
  else
    foo = new Foo;

  if (cond2 && !cond1)
    inline_code_to_delete_user_folder();

  if (cond2) {
    delete foo;
    foo = nullptr;
  }
  return foo;
}

here is a toy with some toy types.

In it, we create a pointer to either a Bar or a Foo based on cond1.

Then we possibly do something dangerous.

Finally, if cond2 is true, we cleanup the Foo* foo.

The thing is, if we call delete foo and foo is not a Foo, it is undefined behavior. The compiler can legitimately reason "ok, so we are calling delete foo, thus *foo is an object of type Foo".

But if foo is a pointer to an actual Foo, then obviously cond1 must be false, because only when it is false is foo pointing at an actual Foo.

Thus, logically, cond2 is true implies cond1 is true. Always. Everywhere. Retroactively.

So the compiler actually knows this is a legitimate transformation of your program:

Foo* do_something( bool cond1, bool cond2 ) {
  if (cond2) {
    Foo* foo = new Foo;
    inline_code_to_delete_user_folder();
    delete foo;
    return nullptr;
  }       
  Foo* foo = nullptr;
  if (cond1)
    foo = new Bar;
  else
    foo = new Foo;

  return foo;
}

which is pretty dangerous, isn't it? We just elided checking cond1 and deleted the user folder whenever you passed true to cond2.

I don't know if any current or future compilers use the detection of UB in deleting the wrong type to do logical back-propogation of UB branches, but compilers do do something similar with other kinds of UB, even things as seemingly innocuous as signed integer overflow.

And to ensure this cannot happen, you need to audit every optimization in every compiler from every compiler that will ever compile your code.

Run away