Why does my overloaded + operator create an "Invalid address specified to RtlValidateHeap" error on return?

Question

I am attempting to create an overloaded + operator for my TurtleProgram class which stores a dynamically allocated array of strings. However, when the method returns, the destructor is called, and the delete operation inside throws an exception with the error "Invalid address specified to RtlValidateHeap".

Below are the overloaded operator definition, the destructor, the default constructor, the copy constructor, and the resize method definition used in the overloaded operator method.

I've done research on the error, but I haven't been able to find any issue related to mine.

// overloaded + operator
// postconditions: returns a new TurtleProgram object that is the sum of two TurtleProgam instruction arrays
TurtleProgram TurtleProgram::operator+(const TurtleProgram &that)
{
    TurtleProgram returnTP;

    returnTP.resize(this->size_ + that.size_);

    for (int i = 0; i < this->size_ + that.size_; i++)
    {
        if (i < this->size_)
        {
            returnTP.instructions_[i] = this->instructions_[i];
        }
        else
        {
            returnTP.instructions_[i] = that.instructions_[i - this->size_];
        }
    }

    return(returnTP);
}

// resize: resized the array to be of a new size
// preconditions: the new size must be a positive integer
// postconditions: the array is resized, and size is updated
void TurtleProgram::resize(int newSize)
{
    string *temp = new string[newSize];

    // iterate through, transferring the contents from the old array to the resized array, then setting any empty spaces to ""
    for (int i = 0; i < newSize; i++)
    {
        if (i < size_)
        {
            temp[i] = instructions_[i];
        }
        else
        {
            temp[i] = "";
        }
    }

    if (size_ != 0)
    {
        delete instructions_;
    }

    instructions_ = temp;

    size_ = newSize;
}

// Default constructor: initializes as an empty array
// postconditions: a new TurtleProgram object is made with an empty array and a size of 0
TurtleProgram::TurtleProgram()
{
    instructions_ = new string[0];
    size_ = 0;
}

// Default destructor: deallocates instructions_ before destroying the object
// postconditions: the memory is released, and the object destroyed
TurtleProgram::~TurtleProgram()
{
    if (size_ != 0)
    {
        delete instructions_;
    }
}

TurtleProgram& TurtleProgram::operator=(const TurtleProgram &that)
{
    resize(that.getLength());

    for (int i = 0; i < size_; i++)
    {
        this->instructions_[i] = that.instructions_[i];
    }

    return(*this);
}

Can anyone spot what I am doing wrong / not understanding? Thanks in advance for any help, and sorry in advance for any posting errors. This is my first question!

Most likely, `return(returnTP)` calls the copy constructor, but you did not implement your own version. — j6t, Apr 04 '18 at 08:38
@j6t No, it won't. If return type *was* autmatically deduced via `auto`, it *would* create a reference (to local!), though. — Aconcagua, Apr 04 '18 at 08:57
See [rule of three](https://stackoverflow.com/q/4172722/1312382)/[rule of five](https://stackoverflow.com/q/4782757/1312382)... — Aconcagua, Apr 04 '18 at 08:58
Side note: you have a memory leak: `instructions_ = new string[0]; size_ = 0`; but: `if(size_ != 0) delete instructions_;`. You might just always delete (it is legal to delete a nullptr), or you use a smart pointer. *Best*: use `std::vector` instead of raw array. — Aconcagua, Apr 04 '18 at 09:01
If code provided is complete, you could even just alias std::vector: `using TurtleProgram = std::vector;`... — Aconcagua, Apr 04 '18 at 09:08
Oh my - have you noticed that you delete an array with `delete` instead of `delete[]`??? — Aconcagua, Apr 04 '18 at 09:20

Aconcagua · Accepted Answer · 2018-04-04T10:09:23.340

The main problem is the lacking copy constructor (see rule of three)!

TurtleProgram::TurtleProgram(TurtleProgram const& that)
    : TurtleProgram()
{
    *this = that;
}

Be aware that even in an expression like

TurtleProgram x, y;
TurtleProgram z = x + y;

the copy (or move, if provided) constructor will be called instead of assignment operator!

Without one, a default is provided just copying the pointer, which will result in double deletion as both the temporary within operator+ and target object hold the same pointer.

From what I see, you are just re-implementing all the functionality that std::vector already provides! You should really consider using it instead of trying (and failing) to reinvent the wheel...

By the way: If you use std::vector, from what you provided so far, you could just skip entirely both constructors, destructor and assignment operator. The default will fit well then, as std::vector's constructor/destructor/assignment operator will be called then, which do the job you need nicely.

Some further problems and points on efficiency (just for learning, if you do not switch to std::vector):

Any resource allocated with new must be deleted, any one allocated with new[] must be delete[]d! If you intermix (as you did), undefined behaviour!!!

You introduced a memory leak when creating an empty array:

instructions_ = new string[0];
size_ = 0;

but only conditionally deleting the member (fixed bad deletion already):

if (size_ != 0)
{
    delete[] instructions_;
}

The empty array exists and occupies memory, too, so you need to delete the member in any case:

//if (size_ != 0)
//{
    delete[] instructions_;
//}

Even better:

instructions_ = nullptr;
size_ = 0;

//if (size_ != 0)
//{
    delete[] instructions_;
//}

So you don't occupy any memory at all for empty turtle; still you do not need to check, it is absolutely legal to delete null pointers (with both delete and delete[])...

You absolutely should provide a move constructor/assignment operator (rule of five):

TurtleProgram::TurtleProgram(TurtleProgram&& that)
    : TurtleProgram()
{
    *this = std::move(that);
}
TurtleProgram& TurtleProgram::operator=(TurtleProgram&& that)
{
    using std::swap;
    swap(size_, that.size_);
    swap(instructions_, that.instructions_);
    return *this;
}

You spare quite a lot of memory allocation and initialisation work...

for (int i = 0; i < this->size_ + that.size_; i++)
{
    if (i < this->size_)
    {
        returnTP.instructions_[i] = this->instructions_[i];
    }
    else
    {
        returnTP.instructions_[i] = that.instructions_[i - this->size_];
    }
}

Make two separate loops of:

for (int i = 0; i < this->size_; i++)
{
    returnTP.instructions_[i] = this->instructions_[i];
}

for (int i = this->size_; i < this->size_ + that.size_; i++)
{
    returnTP.instructions_[i] = that.instructions_[i - this->size_];
}

Similar:

for (int i = 0; i < newSize; i++)
{
    if (i < size_)
    {
        temp[i] = instructions_[i];
    }
    else
    {
        temp[i] = "";
    }
}

It will get:

for (int i = 0; i < size_; i++)
{
    temp[i] = instructions_[i];
}

Huh, and the second part??? Unnecessary, operator new[] already calls the default constructor for any array element, which will create empty strings anyway... One point open, though: appropriate handling for newSize being smaller than size_. Leaving this as excercise for you.

Why does my overloaded + operator create an "Invalid address specified to RtlValidateHeap" error on return?

1 Answers1