Memory leak in assigning pointers

Question

temp = p_long; is showing memory leak. I am not sure how it is memory leak.

long *temp = NULL; 
for (int i = 1; i < 10; i++) {
  if (i < 3) {
     long *p_long = new long;

     if ( p_long ) {
        if ( 0 == *p_long ) {
           flag = true;
        } else if ( 1 == *p_long ) {
           temp = p_long;                    -----> showing memory leak here
           continue;
        }
     }
  }
}

if (temp)
delete temp;

Thanks.

but temp is already NULL, not pointing to anything. how come it can be memory leak ? — Anand, May 28 '13 at 09:30
Its a for loop; so the second time loop comes to temp=p_long; the first new assignment is leaked already — vpram86, May 28 '13 at 09:31
But the point is that you are not always deleting the dynamically allocated memory. — juanchopanza, May 28 '13 at 09:34
Delete already checks for NULL. You don't need to check for it explicitly. — Kaz Dragon, May 28 '13 at 09:34
@Anand Pointers to memory are somewhat like tickets to pick your coat when you exit the disco. If you lose your ticket, you cannot pick your coat. If you take your ticket and erase it so that it no longer shows the number of your coat, this does not mean that you're _not_ losing it. To not lose it you need to actually keep pointing to that number, the number which _can_ possibly be used to access your coat. This number is the value of the address where you allocated memory, and which was returned to you by new. — Daniel Daranas, May 28 '13 at 09:35
Inspired by @DanielDaranas If you overwrite the ticket with some other number, before using it to pick your coat, your coat wont vanish in thin air. Your ticket might be overwritten to point to possibly some other coat, but your previous coat is orphaned without any ticket to identify it. — Suvarna Pattayil, May 28 '13 at 09:46

Preet Kukreti · Answer 1 · 2013-05-28T09:55:07.260

You are not freeing up the heap allocation long *p_long = new long; Considering that it is a for loop, you will have orphaned some memory blocks (newed, but no pointers referencing them anymore for delete-ion.)

You have to free p_long in the appropriate part of the loop if it is not assigned to temp. Your continue statement is redundant (maybe you meant break?). Also, the modern version of NULL is nullptr.

Another point is that your if ( p_long ) check is basically only going to be NULL in the case that your current new heap allocator for that type (long) had problems allocating (e.g. out of memory), and that the following if ( 0 == *p_long ) is checking that the newly allocated long was automatically initialised with a value of 0. AFAIK this is not the case, since in C++, you only pay in performance for what you need. The value is undefined (in reality it will probably be the existing, untampered value at that memory address).

Anthony Palmer · Answer 2 · 2013-05-28T09:42:42.150

0

long *temp = NULL; 
for (int i = 1; i < 10; i++) {
  if (i < 3) {
     long *p_long = new long;

     if ( p_long ) {
        if ( 0 == *p_long ) {
           flag = true;
        } else if ( 1 == *p_long ) {
           if(temp)
              delete temp; -----> leaked memory freed here
           temp = p_long;                   
           continue;
        }
     }
  }
}

edited May 28 '13 at 09:42

answered May 28 '13 at 09:33

Anthony Palmer

944
10
15

@Anand 1) it still leaks, 2) you don't have to check against `NULL` before calling delete. – juanchopanza May 28 '13 at 09:39
3

You don't even have to do that. You can just call `delete temp;` without any checks. – juanchopanza May 28 '13 at 09:44
Thanks juanchopanza, one more thing, i am deleting at last also, will that cause any problem ? – Anand May 28 '13 at 09:59
I am deleting temp when "for" loop ends. – Anand May 28 '13 at 11:11

score 0 · Answer 3 · edited May 23 '17 at 10:25

0

First time when you enter the for loop temp is NULL but in later iterations, you allocate dynamic memory to the pointer.

long *p_long = new long;

And in some iterations you just overwrite the address contained in the pointer. You lose track of the memory allocated using new. This does not mean it will automatically get freed. The memory is still allocated, just that you lost the pointer to it.

temp = p_long;

You need something like,

else if ( 1 == *p_long ) 
{
     delete temp; -----> leaked memory freed here. 
     temp = p_long;                   
continue;
}

This will delete the allocated memory before you assign a new allocated memory address to temp.

Note: delete is safe to use on NULL too

edited May 23 '17 at 10:25

Community

1
1

answered May 28 '13 at 09:42

Suvarna Pattayil

5,136
5
32
59

I dont think `if ( 1 == *p_long )` is what he wants to do. Maybe you mean `if (p_long)`. Dereferencing an unchecked pointer can be dangerous (in this case it could be `nullptr`). non-null is not the same as `==1`. – Preet Kukreti May 28 '13 at 09:46
@PreetKukreti Actually I wanted to indicate use `delete temp` within the `else if block` . The OP is already making a check for `NULL` pointer in `if(p_long)` – Suvarna Pattayil May 28 '13 at 09:50

Memory leak in assigning pointers

3 Answers3