Array Wrapper Corrupts Heap

Question

Sequel to: array wrapper corrupts stack

The project:

I am working on a std::vector replacement.

The error:

I get a heap corruption error whenever I attempt to delete a temporary array I am creating in order to store the copied elements of another array that I delete in order to reallocate it when my array gets resized. (It seems that if I try to assign one array to another, I actually end up assigning the pointer to the array to the other array instead of copying the elements. Truly crazy stuff).

I read online that this error may actually come from the part where I actually interact with the array, but the error only pops out when I attempt to delete it.

Where I interact with the array is the function _tempcpy. I pass a pointer to a source and destination array and copy each element from the source to the destination. I struggled a bit with making sure that the array would start from 0 and not 1 and in the process I messed a bit too much with the element_count member, so it may be that i am somehow writing outside of bounds and I have stared so much at the code I can't see the issue.

The code:

template<class T> class dyn_arr
{
public:

    dyn_arr(void)
    {
        this->array         = {};
        this->element_count = {};
    }

    ~dyn_arr(void)
    {
        this->dealloc();
    }

    bool alloc(unsigned int element_count)
    {
        if (0 == element_count)
        {
            element_count = 1;
        }

        if (this->array != nullptr)
        {
            T* temp = new T[this->element_count];

            if (false == this->_tempcpy(&this->array, &temp, this->element_count))
            {
                return false;
            }

            delete[] this->array;
            this->array = new T[this->element_count];

            if (false == this->_tempcpy(&temp, &this->array, this->element_count))
            {
                return false;
            }

            delete[] temp;

            if (nullptr != this->array)
            {
                return true;
            }
        }
        else
        {
            this->array = new T[this->element_count];

            if (nullptr != this->array)
            {
                return true;
            }
        }

        return false;
    }

    bool dealloc(void)
    {
        if (nullptr == this->array)
        {
            return false;
        }

        delete[] this->array;

        return true;
    }

    bool add(T Object)
    {
        if (0 == Object)
        {
            return false;
        }

        if (true == this->alloc(this->element_count))
        {
            this->array[this->element_count] = Object;

            ++this->element_count;

            return true;
        }

        return false;
    }

    T get(unsigned int index)
    {
        if (index > this->element_count)
        {
            return T{};
        }

        return this->array[index];
    }

    unsigned int get_count(void)
    {
        return this->element_count;
    }

private:

    bool _tempcpy(T** src, T** dest, unsigned int count)
    {
        if ((nullptr == src) || (nullptr == dest) || (0 == count))
        {
            return false;
        }

        for (unsigned int i = 0; i < count; ++i)
        {
            *dest[i] = *src[i];
        }

        return true;
    }

    T* array;
    unsigned int element_count;
};

int main()
{
    dyn_arr<int> pNr = {};
    pNr.add(1);
    pNr.add(2);
    pNr.add(3);

    for (int i = 0; i < pNr.get_count(); ++i)
    {
        printf("%d\n", pNr.get(i));
    }

    getchar();
    return 0;
}

Answer 1

In your alloc function there lie a few problems, such as:

if (0 == element_count)
{
        element_count = 1;
}

is unnecessary. Instead, you can just do a +1 where necessary which is almost everywhere except the temp dynamic array.

bool alloc(...)
{
    this->array = new T[this->element_count];
    //...
    else
    {
       this->array = new T[this->element_count];  
    }
}

should be

    this->array = new T[this->element_count + 1];
    //...
    else
    {
       this->array = new T[this->element_count + 1];  
    }

this will fix the problems with allocation. That leaves us with _tempcpy() which fails because instead of trying to get the next element of the underlying array, it tries to do that with the double ptr itself. Read about operator precedence rules. Fixed version:

    bool _tempcpy(T** src, T** dest, unsigned int count)
    {
       //....
        for (unsigned int i = 0; i < count; ++i)
        {
            (*dest)[i] = (*src)[i];
        }
        //....
    }

However, I am unsure as to why double ptr is needed in this function in the first place. Just use single pointers. Also dest should be const as it is not changing in this function. It gives the reader of the function a clear idea of which parameters will change inside the function and which will not:

bool _tempcpy(T* src, const T* dest, unsigned int count) {...}

same could be applied to other parts of the class.

Additionally, in C++

dyn_arr(void)

We don't do this. There's no need to explicitly write void. According to the C++ standard:

8.3.5 Functions [dcl.fct] ... A parameter list consisting of a single unnamed parameter of non-dependent type void is equivalent to an empty parameter list.