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?

Off topic: Keep an eye out for the [Rule of Three](https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three). `Handle` is missing a copy constructor. — user4581301, Sep 08 '17 at 20:36
@user4581301 Isn't `Handle(T *t) : p(t) {}` a copy constructor? — BillyJean, Sep 09 '17 at 13:46
`Handle(T *t) : p(t) {}` is a constructor that initializes `p` to a given value. A copy constructor creates a new instance based on an existing instance and looks something like: `Handle(const Handle &source)`. In this case you probably don't want any copying and should probably disallow it by deleting the copy constructor `Handle(const Handle &source)= delete;` the alternative would be something that created a new `T` for `p` to point at: `Handle(const Handle &source): p(new T(source.p) {}` However you do it, you do not want two `Handles` with the same `p`. — user4581301, Sep 09 '17 at 16:38

score 1 · Answer 1 · answered Sep 08 '17 at 20:34

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.

score 1 · Answer 2 · answered Sep 08 '17 at 20:34

1

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...

answered Sep 08 '17 at 20:34

Jean-Baptiste Yunès

34,548
4
48
69

user7860670 · Answer 3 · 2017-09-08T20:41:32.577

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.

score 1 · Accepted Answer · answered Sep 08 '17 at 20:37

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.

Note, however, this is **not** what OP is doing. In code provided, there is no undefined behavior really. — SergeyA, Sep 08 '17 at 21:52

score 0 · Answer 5 · answered Sep 10 '17 at 18:21

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

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)
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.

Delete object in handle class potentially causes undefined behavior

5 Answers5