Heap Corruption error with Copy constructor and overloaded assignment operator

Question

I am a student so I apologize up front for not using the correct forum protocols. I have searched for hours on this problem, none of my classmates can help. My assignment is to create a copy constructor, overloaded assignment operator(=) and a destructor (the 'big three') in C++ to manage an array on the heap. What I wrote below in VS13 produces the correct output but I get a debug error: HEAP CORRUPTION DETECTED:c++ crt detected that the application wrote to memory after end of heap buffer Can anyone give me some guidance on this, I don't even know where to look. Thanks!!

//copy constructor

myList::myList(const myList& source){
cout << "Invoking copy constructor." << endl;
array_capacity = source.array_capacity; //shallow copy
elements = source.elements; //shallow copy
delete[] arrayPointer;
arrayPointer = new double(source.array_capacity); //deep copy

for (int i = 0; i < array_capacity; i++) //copy array contents
    {
        arrayPointer[i] = source.arrayPointer[i];
    }
}

//overloaded assignment operator
myList& myList::operator=(const myList& source){

cout << "Invoking overloaded assignment." << endl;

if (this != &source){

array_capacity = source.array_capacity; //shallow copy

elements = source.elements; //shallow copy

delete[] arrayPointer; //delete original array from heap

arrayPointer = new double(array_capacity); //deep copy

for (int i = 0; i < source.array_capacity; i++) {//copy array contents
    arrayPointer[i] = source.arrayPointer[i];
        }
    }
return *this;
}

//destructor
myList::~myList(){ 

cout << "Destructor invoked."<< endl;

delete[] arrayPointer;  // When done, free memory pointed to by myPointer.

arrayPointer = NULL;    // Clear myPointer to prevent using invalid memory reference.
}

Captain Obvlious · Accepted Answer · 2014-12-08T02:02:45.830

There are a couple of problems with your code. First you are invoking delete on arrayPointer but it hasn't been initialized to anything. This could in fact end up deleting memory you have already allocated or result in an excecption or asset in the implementation of delete. Second when you do initialize it you are allocating a single double initialized to the value of source.array_capacity. Notice the parenthesis used in the line below.

arrayPointer = new double(source.array_capacity);

This will certainly result in undefined behavior during the copy as you end up accessing elements outside the bounds of the array. The above line is present in both your copy constructor and copy-assignment operator and should be using square brackets instead like so:

arrayPointer = new double[source.array_capacity];

You also never check to see if there are any elements stored in the source instance of myList. In this case you should likely be assigning nullptr (or NULL in C++03) to arrayPointer.

As a side note you do not really need to assign NULL to arrayPointer in your destructor. Once the object is destroyed it's gone and any attempt to access it after the fact will result in undefined behavior anyway.

Your suggestions worked! You're a life saver, thank you! Should I marked this post as 'answered'? how do I do that? — dana_muise, Dec 08 '14 at 02:37
Yes, if an answer solves your problem then you should mark it as accepted by clicking the checkbox on the upper left hand side of the answer. If more than one answer is helpful it's up to you to decide which one is worthy of being accepted. You can also upvote answers as well if you feel like giving the contributor a little bonus. — Captain Obvlious, Dec 08 '14 at 02:43

score 1 · Answer 2 · edited May 23 '17 at 11:50

Captain Obvlious pointed out the problem in your copy-constructor already.

You will have noticed that the copy-constructor and the assignment-operator contain a lot of the same code but with a subtle difference. In fact this is probably how you ended up with the error (the assignment operator needs to delete[] the old value, but the copy-constructor doesn't.

Code duplication is bad because it leads to subtle errors like this creeping in. A good pattern you can use here is what's called the copy and swap idiom.

The gist is that you define the copy-constructor, and you also define swap. Then you get assignment for free. This works because swap is easier to implement correctly than the assignment operator, and another major benefit is that nothing can go wrong (you don't have to worry about out-of-memory errors and so on).

In your code; keeping your fixed copy-constructor, you could add inside the class definition:

friend void swap( myList &a, myList &b )
{
    std::swap( a.array_capacity, b.array_capacity );
    std::swap( a.arrayPointer, b.arrayPointer );
    std::swap( a.elements, b.elements );
}

and now the assignment operator is very simple:

myList& myList::operator=(const myList &source)
{
    myList temp(source);
    swap(*this, temp);
    return *this;
}

The old resources from the current object are deleted by the destructor of temp. This version doesn't even need to check for self-assignment because std::swap(x, x) is well-defined.

This can even be optimized further by taking source by value instead of by reference, as explained on the linked page.

Heap Corruption error with Copy constructor and overloaded assignment operator

2 Answers2