Pointer being freed was not allocated, dynamic array C++

Question

I had a similar issue with C, but my problem now is actually more similar to this.

Unfortunately, I'm just learning C++, and I can't see how to apply the solution to my previous issue (if it indeed does apply), and the latter post was a specific problem with his code, that is more complex than my own.

Here is the relevant code:

double n1, n2; //temporary data for user entry
int pcount = 0; //size of my array
struct point{double x; double y;};
point *p = new point[1]; //my array
point *tmp;  //temporary array while resizing

while (points >> n1 >> n2){ //for each element the user enters, 
    pcount++; //increase the array size
    tmp = new point[pcount]; //allocate new memory for the array
    tmp = p; //copy the elements from the old to the temporary
    delete [] p; //delete the old array
    p = new point[pcount]; //allocate memory for the new array
    p = tmp; //copy the elements from the temporary to the new array
    delete [] tmp; //delete the temporary
    p[pcount-1].x = n1; //now push back the new element
    p[pcount-1].y = n2;
}

As you can see, p and tmp point to arrays that have initial sizes, and are freed within a few lines. Crucially, I cannot see how "a pointer being freed was not allocated" - p is allocated on declaration, tmp within the loop, then p is freed and reallocated, then tmp is freed, and so the loop continues...

I also tried implementing via two loops, but then the printed 'points' are (0, 0), whatever they actually are - I can't spot why?

while (points >> n1 >> n2){
    pcount++;
}
p = new point[pcount];
int i = 0;
while (points >> n1 >> n2){
    p[i].x = n1;
    p[i].y = n2;
    i++;
}

A) You have memory leaks, B) you're double deleting something, C) Save yourself the pain and use [`std::vector`](http://en.cppreference.com/w/cpp/container/vector) — Borgleader, Jan 30 '14 at 23:28
Why don't you do this in two passes? First count how many points you need and then just allocate one array of points? No need to juggle the two pointers and less memory fragmentation. — JHagdahl, Jan 30 '14 at 23:29
`tmp = new point[pcount]; tmp = p;` - what is the point of the first assignment, if you immediately assign to `tmp` something else? — Wojtek Surowka, Jan 30 '14 at 23:29
@Borgleader This is an non-assessed lab task, it notes existence of `std::vector`, but the objective is to use and understand dynamic arrays, `new`, `delete []`. — OJFord, Jan 30 '14 at 23:31
@JHagdahl I tried that initially, despite it not being the method laid out on the lab sheet - it seemed to me this was more efficient, as fewer memory accesses. However, I had this error/seg fault 11, so I decided to just follow the sheet. — OJFord, Jan 30 '14 at 23:33
@WojtekSurowka Perhaps I misunderstood, I thought the first gave it the space, and the second put the value there. Can I just do `tmp = new p`? — OJFord, Jan 30 '14 at 23:34
@OllieFord: Dynamic arrays cannot be copied, you have to manually copy the data one element at a time. — Mooing Duck, Jan 30 '14 at 23:40
@MooingDuck Ah. Then I'm definitely reverting to the two loop implementation. — OJFord, Jan 30 '14 at 23:45
@OllieFord: You also don't need that many dynamic allocations: http://coliru.stacked-crooked.com/a/43284d2852274fb5 — Mooing Duck, Jan 30 '14 at 23:48

score 8 · Accepted Answer · answered Jan 30 '14 at 23:30

8

There is an error in almost every line here:

point *p = new point[1]; // Allocation #1
tmp = new point[pcount]; // Allocation #2
tmp = p;                 // Allocation #2 lost (memory leak)
delete [] p;             // Now 'tmp' is "pointing to junk"
p = new point[pcount];   // Allocation #3
p = tmp;                 // Allocation #3 lost (memory leak), and 'p' is "pointing to junk"
delete [] tmp;           // Segmentation fault, since 'tmp' is "pointing to junk"
p[pcount-1].x = n1;      // Segmentation fault, since 'p' is "pointing to junk"
p[pcount-1].y = n2;      // Segmentation fault, since 'p' is "pointing to junk"

answered Jan 30 '14 at 23:30

barak manos

29,648
10
62
114

Well shit. Thanks. I screwed up my pointers. When I did `tmp = p; delete [] p;` I wanted `tmp` to have 'become' p. I addressed the two-time assignment of `tmp` in comments in OP. – OJFord Jan 30 '14 at 23:37
Oh wait, this was the issue I had before with two-loops: it returns them all as being 0..: `while (points >> n1 >> n2){pcount++}; p = new point[pcount]; int i = 0; while (points >> n1 >> n2){p[i].x = n1; p[i].y = n2; i++}` -- that's barely readable I'll edit into OP. – OJFord Jan 31 '14 at 00:30
Nice. Patronise me like that again and I'll never "click the V and make it green". – OJFord Jan 31 '14 at 17:12
No patronizing meant here; I apologize if you interpreted it that way, and no need to click the V unless you want to... – barak manos Jan 31 '14 at 17:21

score 1 · Answer 2 · answered Jan 30 '14 at 23:34

1

You seem to misunderstand how pointers work. tmp is assigned a new point[] in the second line of your loop, but in the third line, tmp is overwritten by another value, destroying any reference to the memory just allocated... This causes a memory leak. I know, this was not your question, but I think you should think the whole thing over...

answered Jan 30 '14 at 23:34

Roxxorfreak

380
2
10

Yes, I know, I see it now. I'm not certain how I make `tmp` 'become' `p` though, do I need to dereference `p` on that assignment? `tmp = new *p`? – OJFord Jan 30 '14 at 23:38
I am afraid I don't know, since I don't know what your code is supposed to do :-( – Roxxorfreak Jan 30 '14 at 23:41
@OllieFord: You have to use a `for` loop to copy the elements one at a time. – Mooing Duck Jan 30 '14 at 23:41
1

@Doberman: I figured out what he meant. I added comments to the OP to clarify his thought process. (Ollie, this is why comments are important) – Mooing Duck Jan 30 '14 at 23:43

Pointer being freed was not allocated, dynamic array C++

2 Answers2