Deleting array only fails occasionally

Question

I am learning pointers in C++ and have encountered a very strange problem. Our task is to write our own push_back function for our own class which holds an array of integers.

I have the following simple function:

void IntRow::push_back(int value){
    int *elements_temp = new int[size+1];
    for (int i = 0; i <= size; i++){
        elements_temp[i] = elements[i];
    }
    elements_temp[size + 1] = value;
    elements = new int[size+1];
    for (int i = 0; i <= size+1; i++){
        elements[i] = elements_temp[i];
    }
    delete [] elements_temp;
    size++;
}

This however fails, BUT ONLY SOMETIMES on the line: delete [] elements_temp;

With the following error message:

incorrect checksum for freed object - object was probably modified after being freed.

Why is this happening, and why does it only happen sometimes?

P.S.: Do I actually have to free elements_temp or is it freed anyways after the function exists?

`elements = new int[size+1];` you lost me here. Why are you allocating **another** buffer? — StoryTeller - Unslander Monica, Oct 19 '17 at 10:08
Because I want to copy everything from the temporary array to the new elements array to actually hold the old values + the new value which we are just pushing. — Balázs Vincze, Oct 19 '17 at 10:09
`elements_temp` is a pointer, to a buffer. You can reassign pointers... — StoryTeller - Unslander Monica, Oct 19 '17 at 10:11
Is Elements_temp[size +1]=value undefined behavior? Oh I see, it is writing beyond the end of the array. Which may be overwriting part of the checksum. — Robert Baron, Oct 19 '17 at 11:32

Bathsheba · Answer 1 · 2017-10-19T10:11:47.427

5

The behaviour of elements_temp[size + 1] = value; is undefined as you are accessing an element outside the array bounds.

This undefined behaviour manifests itself as an occasional program crash.

And yes, you need to call delete[] explicitly yourself on the pointer you get back from new[]. (Out of interest, who is calling delete[] on elements?)

Alternatively, use std::vector<int>.

edited Oct 19 '17 at 10:11

answered Oct 19 '17 at 10:08

Bathsheba

231,907
34
361
483

1

Your last sentence is a slight understatement. – Konrad Rudolph Oct 19 '17 at 10:11
But I start with declaring: `int *elements_temp = new int[size+1];` How is `[size+1]` out of bounds? – Balázs Vincze Oct 19 '17 at 10:12
2

Cuz the elements are indexed from 0, innit. C++ ain't Fortran you know. – Bathsheba Oct 19 '17 at 10:12
1

@BalázsVincze arrays are 0 based. So if i declare an array of (size+1)=3, i can access arr[0], arr[1] and arr[2]. arr[3] is out of bounds – Blue Granny Oct 19 '17 at 10:13
What a stupid mistake! Thank you for pointing this out. How is it that it's failing on the `delete []` line instead of this? – Balázs Vincze Oct 19 '17 at 10:14
1

Because undefined behaviour is undefined. – underscore_d Oct 19 '17 at 10:15
1

@BalázsVincze Because writing into random memory next to a block allocated to you has little reason to fail. However, then trying to *read* whatever was overwritten by that write (such as some management data structure of the memory manager), that's a different story... – Angew is no longer proud of SO Oct 19 '17 at 10:16

Mr.C64 · Answer 2 · 2017-10-19T10:45:05.917

int *elements_temp = new int[size+1];
for (int i = 0; i <= size; i++){
    elements_temp[i] = elements[i];
}
elements_temp[size + 1] = value;

You allocated elements_temp to store size+1 integers. Valid indexes inside this array go from 0 to size (not size+1), as indexes are 0-based in C++. So, you are writing outside the array bounds in the last line above. This is undefined behavior, and it's likely the cause of your problem.

What you probably want is allocating room for size+1 integers, and copying the previous integers to the new array, then add the last new element:

// Allocate room for size+1 integers:
const int newSize = size + 1; 
int* elements_temp = new int[newSize];

// Copy the old array data to the new array.
// Note: i < size, *not* <= !
for (int i = 0; i < size; i++) {
    elements_temp[i] = elements[i];
}

// Write the new item at the end of the array.
// Note: The index is "size", *not* "size+1"
elements_temp[size] = value;

// Update the size data member in your array class to store the new size
size = newSize;

Moreover, you should delete the old elements array before assigning the newly created one, e.g.:

// Get rid of old array memory
delete[] elements;

// If you don't call delete[] above, you will *leak* the old elements array

// Take ownership of the new array
elements = elements_temp;

A Better Allocation Policy

I'd like to add that, allocating a new array each time you add an item to it is a very inefficient policy. A better approach (used e.g. by the standard std::vector container) is to allocate some memory in advance, creating a kind of available capacity, and then allocate a new array and copying the old data to the new array, only when the old vector has no more room (in other words, only when there's no more "capacity" available).

In fact, dynamic memory allocations with new[] are expensive, and it's better to minimize them. (Although this capacity optimization may or may not be what your teacher wants from you in this learning process.)

Thanks, it's fairly clear now! Why are you assigning `const` for the new size? Is it better than just using `size+1`? — Balázs Vincze, Oct 19 '17 at 10:32
You're welcome. I'm glad I was of some help. I think that defining a read-only variable (i.e. a variable that you cannot modify) with `const` and storing in it the new size value, makes the code clearer and less bug-prone than repeating `size+1` in several places and potentially missing the `+1` somewhere... Anyway, feel free to choose what you find more readable. — Mr.C64, Oct 19 '17 at 10:35
Oh, so it is not a question of performance gains right? They perform just the same way? — Balázs Vincze, Oct 19 '17 at 10:37
Yes, it's something I did for the programmer and the code maintainers, more than the C++ compiler :) I think in this case there's no performance difference. — Mr.C64, Oct 19 '17 at 10:38

Deleting array only fails occasionally

2 Answers2