Delete object in handle class potentially causes undefined behavior

Question

I have the following piece of code (from Koening & Moo Accelerated C++ page 255) that defines a generic handle class Handle. Handle is used to manage the memory of objects:

#include <iostream>
#include <stdexcept>

///Handle
template <class T>
class Handle
{
  public:
    Handle() : p(0) {}
    Handle &operator=(const Handle &);
    T *operator->() const;
    ~Handle() { delete p; }
    Handle(T *t) : p(t) {}

  private:
    T *p;
};

template <class T>
Handle<T> &Handle<T>::operator=(const Handle &rhs)
{
    if (&rhs != this)
    {
        delete p;
        p = rhs.p ? rhs.p->clone() : 0;
    }
    return *this;
};

template <class T>
T *Handle<T>::operator->() const
{
    if (p)
        return p;
    throw std::runtime_error("error");
};

class test_classA
{
    friend class Handle<test_classA>;

  private:
    virtual test_classA *clone() const { return new test_classA; }

  public:
    virtual void run() { std::cout << "HiA"; }
};

class test_classB : public test_classA
{
  private:
    virtual test_classB *clone() const { return new test_classB; }

  public:
    virtual void run() { std::cout << "HiB"; }
};

int main()
{

    Handle<test_classA> h;
    h = new test_classA;
    h->run();

    return 0;
}

When I compile this using g++ -o main main.cpp -Wall I get the warning:

warning: deleting object of polymorphic class type ‘test_classA’ which has non-virtual destructor might cause undefined behaviour [-Wdelete-non-virtual-dtor]
     ~Handle() { delete p; }

I don't quite understand the warning. The handle class automatically deletes the pointer *p in the destructor regardless of its type, so where is the potential pitfall?

Answer 1

In C++, if you have a base class (here, test_classA) that has other classes derive from it (here, test_classB), you have to be careful about deleting pointers of type test_classA if those pointers might actually point at objects of type test_classB. Notice that you're doing precisely this in the code that you've written here.

If you do something like this, you need to give your base class (test_classA) a virtual destructor, like this:

class test_classA {
public:
    virtual ~test_classA() = default;
    // ...
};

This way, when C++ tries to delete a pointer of type test_classA, it knows that the pointer in question might not actually point at a test_classA object and will correctly call the right destructor.

This issue is completely independent of your wrapper type, by the way. You could get the same issue by writing

test_classA* ptr = new test_classB;
delete ptr; // <--- Warning! Not good unless you have a virtual dtor.

Answer 2

Your warning says it all. Your class A is polymorphic, but destructor is non-virtual. Deleting object of derived class through the pointer to base class when base class doesn't have virtual destructor is undefined behavior.

In your specific example there is no undefined behavior, because you do not have the object of derived class, but compiler probably can't make sure of it, so it warns you regardless.

Answer 3

The problem is that if the object handled is of a subclass of the template instanciation type wrong deletion will happen.

In your case it will happen if your Handle<test_classA> h; will handle object of type test_classB...

Answer 4

Handle h has type Handle<test_classA> so it will store a pointer to test_classA and call a destructor of test_classA. However you can store a pointer to test_classB in your handle and in this case test_classB destructor won't be called since test_classA destructor is not virtual:

h = static_cast< test_classA * >(new test_classB);

Also note that this Handle class has poorly chosen name and it is essentially a smart pointer kind of class.

Answer 5

The warning is due to the fact that your base class' destructor is not virtual. If you want to use your class polymorhically (For instance creating a vector with base class pointers where the pointed to object is a derived class) you gonna have undefined behavior.

Another thing to mention is that you declare the Handle class as a friend of class test_classA in order to gain access to the clone function. Pay attention that the friend keyword is not transitive so in your derived class the Handle class has no access to the clone function.

Finally your clone function is not so clear to me. Let's look at the main function:

Handle<test_classA> h;
h = new test_classA

1. You instantiate a class Handle with its default constructor which initialize p to be a nullpointer (by the way, if you use C++11 it is much better to initialize it with nullptr instead of 0 or NULL)
2. In the next line you call the operator= of the instance h. Here, your operator= waits for a Handle class. So the expression new test_classA will implicitly call your Handle class constructor Handle(T *t) : p(t) {}. Then this will be used in your operator= function where you check whether rhs.p is NULL or not. As it is not null you will call the clone function (what you wrote is equivalent to (rhs.p)->clone so its not the Handle class' operator-> which is called but the pointer p) which will create a memory again in the HEAP.

I think it was not what you wanted.