Using delete[] in a destructor: Invalid address specified to RtlValidateHeap

Question

I am writing an array class template but have trouble with the destructor.

#ifndef ARRAY_CPP
#define ARRAY_CPP
using namespace std;
#include<iostream>
#include<sstream>
#include "Array.h"


//Define default constructor, initialize an array of size 10 with all elements being (0,0)
template<typename T> //Define type parameter
Array<T>::Array() {
    size = 10;
    m_data = new T[size];
}


template<typename T> //Define type parameter
Array<T>::Array(int si) {
    size = si;
    m_data = new T[size];
}

template<typename T> //Define type parameter
Array<T>::Array(const Array<T>& source) {
    size = source.size; //Set the size equal to the size of the source
    m_data = new T[size]; //Create a new source on the heap
    //Loop through and copy each member of the source array to the new array
    for (int i = 0; i < size; i++) {
        m_data [i] = source.m_data [i];
    }
}

//Define default destructor
template<typename T> //Define type parameter
Array<T>::~Array() {
        delete[] m_data;
    }

    //Define operator =
template<typename T> //Define type parameter
Array<T>& Array<T>::operator = (const Array<T>& source) {
    //Check self-assignment
    if (this != &source) {
        //Delete the old array
        delete[] m_data;
        size = source.size; //Set the size equal to the size of the source
        m_data = source.m_data; //Set the dynamic C array equal to that of the source
        //Loop through each element and copy each member of the source array to the current array
        for (int i = 0; i < size; i++) {
            m_data[i] = source.m_data [i];

        }
    }
    //Return current array
    return *this;

}
//Define operator[]
template<typename T> //Define type parameter
T& Array<T>::operator[](int index) {
    if ((index < 0) || (index>= size))
        throw -1;
    return m_data[index];
}

//Define constant operator[]
template<typename T> //Define type parameter
const T& Array<T>::operator [] (int index) const {
    if ((index < 0) || (index >= size))
        throw -1;
    return m_data[index];
}

using namespace std;
#include<iostream>
#include<sstream>
#include "Array.cpp"


void main() {

    Array<double> test(10);
    Array<double> test2(10);
    test2 = test;
}

Whenever the destructor of array is called in main, it gives me error: Invalid address specified to RtlValidateHeap. I understand that this is because I am trying to delete memory on the stack. However, in my constructor, I initialize the array using new and this should create memory on the heap...The code worked well before I implemented template. Many thanks in advance !

Can you give a full example that demonstrates the problem? Add the simplest `main` you need to add to demonstrate the issue, and show the full error. Your class definition is also incomplete. Try to instead make your class complete and give a main, and remove any methods that are not needed to demonstrate the example (e.g. you probably don't need both constructors). — Nir Friedman, Apr 10 '17 at 20:06
1) There is no need to check for NULL when calling `delete[]` in the destructor. 2) Your assignment operator is flawed in a big way. 3) Where is your `main` function? 4) Why are you including `Point.h` in your array header? — PaulMcKenzie, Apr 10 '17 at 20:07
`#ifndef ARRAY_CPP`, Have you look at [why-can-templates-only-be-implemented-in-the-header-file](http://stackoverflow.com/questions/495021/why-can-templates-only-be-implemented-in-the-header-file) ? — Jarod42, Apr 10 '17 at 20:11
I noticed you are splitting template code in a header and source file. [You might want to rethink that](http://stackoverflow.com/questions/495021/why-can-templates-only-be-implemented-in-the-header-file) — NathanOliver, Apr 10 '17 at 20:11
Also, there is nothing wrong with the destructor. All the destructor is doing is revealing that you made mistakes in memory management before calling the destructor. — PaulMcKenzie, Apr 10 '17 at 20:12
`m_data = source.m_data;` should be `m_data = new T[size];` (as for constructor). — Jarod42, Apr 10 '17 at 20:13
Please [use copy/swap in the assignment operator](http://coliru.stacked-crooked.com/a/242bef26ad32861f). Then you don't make the errors pointed out previously, and you don't get into trouble if `new []` throws an exception. — PaulMcKenzie, Apr 10 '17 at 20:21
I am new to C++ and thank you all for being patient ! I have added a simple main that calls the error. — haozzzzz, Apr 10 '17 at 20:32
I'd recommend looking at the [rule of five](http://stackoverflow.com/questions/4782757/rule-of-three-becomes-rule-of-five-with-c11?rq=1) as well. — Pandatyr, Apr 10 '17 at 20:33
Possible duplicate of [What is The Rule of Three?](http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three) — Raymond Chen, Apr 10 '17 at 20:38
`using namespace` in headers is bad. `using namespace` before `#include` is bad, too. — Werner Henze, Apr 11 '17 at 07:25

Martin York · Answer 1 · 2017-04-11T05:26:50.937

I can see one error and one potential error:

Error: In the assignment operator. You make a copy of the pointer value.

template<typename T>
Array<T>& Array<T>::operator = (const Array<T>& source) {
        // STUFF

        // This is a problem
        m_data = source.m_data;

        // MORE STUFF
    }
    return *this;

}

You now have two objects that are pointing at the same dynamically allocated piece of memory. This is a problem as when they go out of scope the destructor in both objects will try and call delete on the same pointer value.

Potential Error: I don't see a copy constructor. If you don't define one (or don't explicitly delete it) then the compiler will generate it for you. The default one does not work with owned resources (ie dynamically allocated memory) as it does a shallow copy of the resource.

The compiler generated one looks like this:

template<typename T>
Array<T>& Array<T>::Array(const Array<T>& source)
    : size(source.size)
    , m_data(source.m_data)
{}

You can see this has the same problem as your assignment operator. It simply copies the pointer value. Leading to the same issue of two objects pointing at the same dynamically allocated memory.

@haozzzzz: Yes. But there is more ways to improve. You should get a code review at https://codereview.stackexchange.com — Martin York, Apr 11 '17 at 05:28

Using delete[] in a destructor: Invalid address specified to RtlValidateHeap

1 Answers1