Why does a double-free occur when using delete in the destructor of a shallow-copied type?

Question

I can't figure out why I get error for the code below.

The instances of object A will be pushed into a vector (vectorA.push_back(A a)) continuously. So sometimes, vectorA needs to be reallocated; the destructor will be called, which is where the destructor of A gets called, then the error message appears.

class A
{
    long filePos;

    union {
        Recording* recording;
        UINT64 timeStamp;
    };

public:
    inline A(long fpos, UINT64 ts) : filePos(fpos), timeStamp(ts) {}
    ~A()
    {
        if (getDetailedType() == RECORDING_TYPE)
            if (recording)
                delete recording; // error: scalar deleting destructor ???
    }

    inline short getDetailedType() const { return (short)(timeStamp % 5); }
    A(const A& edi)
    {
        filePos = edi.filePos;

        if (getDetailedType() == RECORDING_INFO)
            recording = edi.recording;
        else
            timeStamp = edi.timeStamp;
    }

}

class Recording : protected RECORDINGS
{
    UINT64 timestamp;
    float scalar;

public:
    ~Recording() // with or without this dtor, got same error
    {
    }

    inline Recording()
    {
        timestamp = 0;
        scalar = 2.0;
        time = 0;
        rate = 30;
        type = 1;
        side = 1;
    }
}

typedef struct
{
    UINT32 time;
    float rate;
    int type;
    int side;

} RECORDINGS;

Affinal

Answer 1

Your copy constructor does a shallow copy. So, now you have two objects that both have the same recording pointer.

You should either do a deep copy, or ensure the ownership is properly transferred (by using something like std::unique_ptr<Recording> if C++11 is available.

See This question on the difference between deep and shallow copies.

Let's look at some examples:

class ABadCopyingClass
{
public:
   ABadCopyingClass()
   {
       a_ = new int(5);
   }

   ~ABadCopyingClass()
   {
       delete a_;
   }

private:
    int* a_;   
};

The above class is bad because the default copy constructor and assignment operator will perform a shallow copy, and lead to two objects both thinking that they own the underlying a_ object. When one of them goes out of scope, the a_ will be deleted, and the other one will be left with a dangling pointer that will eventually lead to a crash.

class ABetterCopyingClass
{
public:
    ABetterCopyingClass()
       a_(new int(5))
    {
    }

    ABetterCopyingClass(const ABetterCopyingClass& r)
    {
        a_ = new int(*r.a_);
    }

    ABetterCopyingClass& operator=(const ABetterCopyingClass& r)
    {
        // in the case of reassignment...
        delete a_;

        a_ = new int(*r.a_);
        return *this;
    }

    ~ABetterCopyingClass()
    {
        delete a_;
    }

private:
    int* a_;    

};

This class improved our situation a little (note, that the normal error checking is left out in this simple example). Now the copy constructor and assignment operator properly perform the necessary deep copying. The drawback here is the amount of boilerplate code we had to add -- it's easy to get that wrong.

class ACannonicalCopyingClass
{
public:
   ACannonicalCopyingClass()
      : a_(new int(5))
   {
   }

   ACannonicalCopyingClass(ACannonicalCopyingClass&& moved_from)
   {
       a_ = std::move(moved_from.a_);
   }
private:
   std::unique_ptr<int> a_;
};

This example (C++11 only) is even better. We've removed a significant amount of boilerplate code, however the semantics here are a bit different. Instead of deep copying, we get in this case transfer of ownership of the underlying a_ object.

The easiest version (C++11 only) to implement is the version that provides shared ownership of the underlying a_ object. This is the version that is most similar to your provided example, with the added bonus that it does not cause a crash.

class ASharedCopyingClass
{
public:
   ASharedCopyingClass()
      : a_(std::make_shared<int>(5))
   {
   }

private:
    std::shared_ptr<int> a_;
};

This version can be copied at will, and the underlying a_ object will happily be reference counted. The last copy to go out of scope will set the reference count to 0, which will trigger the memory deallocation.

Siderosis

Answer 2

My psychic debugging skills tell me that you forgot to implement a copy constructor for A which then results in a double deletion of a Recording when the copy is destroyed.

The growth of the vector would trigger the copy-destroy pairs.

Lauter

Please check the update: I add copy ctor for class A. But it still crash, given the same error info. What do I do wrong? thanks. – Affinal

@Affinal You need to understand why a copy constructor is needed, and what it is supposed to do. Yours does exactly the wrong thing. – Emmeram

Your psychic debugging skills are spot on. – Carroty