Swapping an object within itself

Question

I'm trying to swap an object within itself. It works but when I add a destructor it gives me a double free error. Is there a way to prevent this? The method I'm talking about is void swap(SimpleArray &object).

(Sorry if you read this before I had the wrong info in my post...)

#include "TestType.h"
class SimpleArray {

    private: 
        TestType* pArray;
        //TestType* temp;
    public:
        SimpleArray(TestType *array)
        {
            this->pArray = array;
        }
        ~SimpleArray() { delete[] pArray; }
        SimpleArray() { pArray = 0;}
        SimpleArray(const SimpleArray& arg){ pArray = arg.pArray; }
        ~SimpleArray() { delete[] pArray; }
        TestType * get() const{ return pArray; }
        bool isNonNull() const { return pArray != 0; }
        //TestType* pArray;
        void reset(TestType*& p) {this->pArray = p; }
        void reset() { pArray = 0; }

        void swap(SimpleArray &object) { SimpleArray temp; temp = object; object = *this; *this = temp;}
        TestType * release() { pArray = 0; return pArray; }
        TestType& getReference(int a) { return *pArray; }


};

This works but once I add the destructor it gives me a "double free or corruption error". How do I solve this? Here's the function in main where it messes up.

bool testGetReleaseSwap() {
    SimpleArray array1;
    if (array1.get() != 0)
        return false;

    TestType* directArray1 = new TestType[100];
    array1.reset(directArray1);
    if (array1.get() != directArray1)
        return false;

    TestType* directArray2 = new TestType[50];
    SimpleArray array2(directArray2);

    array1.swap(array2);
    if (array1.get() != directArray2 || array2.get() != directArray1)
        return false;

    array2.swap(array1);
    if (array1.get() != directArray1 || array2.get() != directArray2)
        return false;

    array1.swap(array1);
    if (array1.get() != directArray1)
        return false;

    if (array1.release() != directArray1 || array2.release() != directArray2)
        return false;

    if (array1.get() != 0 || array2.get() != 0)
        return false;

    delete[] directArray1;
    delete[] directArray2;

    return true;
}

possible duplicate of [What is The Rule of Three?](http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three) You're missing the copy-assignment operator, and your copy-constructor does a shallow copy, leaving both objects owning the same array. — Mike Seymour, Aug 27 '12 at 00:47

Alexander Gessler · Accepted Answer · 2012-08-27T00:07:21.083

The trivial way out here is to invoke temp.release() at the end if your swap method to prevent double deletion.

The underlying issue is much deeper, though. In C++ it is crucial to always maintain strict semantics of who owns something, for example a memory region that requires deletion.

A frequent pattern is that the object that allocates something is also responsible for cleaning up and no one else. This fits nicely with SimpleArray, but the copy constructor breaks it because it multiplies the number of owners!

To implement shared data semantics you have to invest more work (reference counting etc.) or you have to forbid array copying and make the copy constructor private.

A clean way to fix up swap to work without copying the object would be:

 void swap(SimpleArray &object) { 
    TestType* temp = object.pArray;
    object.pArray = this->pArray;
    this->pArray = temp;
 }

(std::swap(object.pArray, pArray); works as well)

Because to swap the memory regions of the array fits nicely with a single-owner pattern, what went wrong here is only the use of the full object copy.

You should read up on resource management and ownership semantics in C++. Your code will always be error prone unless you absolutely know who owns what.

Sorry they work, but there is still one heap of memory still out there. Part of the assignment is too make sure there are no memory leaks. — avoliva, Aug 27 '12 at 00:48
Thanks. After looking at the rules of three, I made a operator= method in my class. before the swap wasn't working as desired, but now it's working perfectly. — avoliva, Aug 27 '12 at 02:38

score 2 · Answer 2 · answered Aug 27 '12 at 00:05

2

It seems to me that you are trying to implement a class that has shallow copy semantics (and possibly copy-on-write). To do that successfully you need to track how many other owners of the shared data are still around and need to destroy the owned object, when that count reaches zero. You can either use a std::shared_ptr for that or implement the reference counting yourself.

As for the real problem in that specific example, look at what you copy constructor is doing. It is not copying but simply taking another reference (a pointer to be specific) to the object that is already owned by its argument. That by itself already enough to get a double free and your swap testcase is simply exposing that issue.

answered Aug 27 '12 at 00:05

pmr

58,701
10
113
156

So how do I fix the copy constructor? – avoliva Aug 27 '12 at 00:11
@user1447974 If you don't have the intention to actually implement either shallow copy or copy-on-write the **copy ctor** needs to actually **copy** the array of it's argument. – pmr Aug 27 '12 at 00:17
So I want to do a shallow copy? I guess what I'm doing right now is actually a deep copy? – avoliva Aug 27 '12 at 00:38
@user1447974 No, you are doing a shallow copy. As I said: you are just setting the pointer of the copy to the same object of the one you are copying from. What you want to do is a deep copy. – pmr Aug 27 '12 at 08:11

Swapping an object within itself

2 Answers2