CODEWITHSUNDEEP
cpointersmalloc
Tango_Chaser
Tango_Chaser

Reputation: 331

Free a pointer from an external function

I wrote a program that uses a stack ADT.
The main creates a new stack while giving 3 functions to use from the user: 

Stack my_stack = sCreate (copy_int, free_int, print_int);

when I call to a 'peek' function:

printf ("Peek: |%d|\n\n", *(int*)sPeek(my_stack));

I have a memory leak.

the peek function looks like this:

Element sPeek (Stack stack){
if ((NULL == stack) || (0 >= stack->used_places))
    return NULL;

Element returnElement = stack->copy_function(stack->stack_array[stack->used_places-1]);
if (NULL == returnElement){
    free (returnElement);
    return NULL;
}   
return returnElement;

It's caused probably by the copy_function called there, which is the copy_int that was given by the user:

Element copy_int (Element element){
int *new_int = (int*) malloc(sizeof(int*));
*new_int = *(int*)element;
if (NULL != new_int)
    return new_int;
else
    return NULL;

How do I release the pointer (malloc) from copy_int?

Upvotes: 5

Views: 203

Answers (4)

user3386109
user3386109

Reputation: 34839

In the last code snippet, you're using *new_int before checking the return value from malloc. That's going to cause a segmentation fault if new_int is NULL. Furthermore, the if/else is completely worthless as written. Those four lines could be replaced with return new_int; with absolutely no change in behavior under any circumstances. Finally, don't cast the return value from malloc.

With all of those problems fixed, the last code snippet looks like this

Element copy_int (Element element)
{
    int *new_int = malloc(sizeof(int));
    if ( new_int )
        *new_int = *(int*)element;
    return new_int;
}

In the sPeek function, you have a similar worthless if statement. If the returnElement is NULL, there's nothing to free. So the sPeek function should be

Element sPeek (Stack stack)
{
    if ( stack && stack->used_places > 0 )
        return stack->copy_function(stack->stack_array[stack->used_places-1]);
    else
        return NULL;
}

Finally to your question, the memory returned by copy_int will be leaked unless you keep a copy of that pointer, and free it when you're done with it. Also, you're asking for another segmentation fault if you pass a NULL pointer to printf. So the printf line needs to be replaced with this code (assuming that Element is really void *)

int *value = sPeek(my_stack);
if (value)
    printf ("Peek: |%d|\n\n", *value);
free(value);

Upvotes: 1

kaylum
kaylum

Reputation: 14044

Element e = sPeek(my_stack);
if (e) {
    printf ("Peek: |%d|\n\n", *(int*)e);
}
free(e);

Seems a bit obvious so not sure if that's what you meant.

Upvotes: 2

Ulrich Eckhardt
Ulrich Eckhardt

Reputation: 17444

Any function that returns a resource that is not automatically released after use must have documentation how to release the resource. In the case of malloc(), it's documented as free(), for fopen() it's fclose() etc.

When you create a function yourself, you can e.g. refer to free(), if you return a pointer that you in turn received from malloc(). If you have a more complex setup, you might have to create your own function.

Looking at your function, you alloc memory using malloc(), then assign to the memory (or blow up if the allocation fails, which you verify too late), then return exactly the pointer received from malloc(). Therefore, you are returning a resource that can (and must!) be released with free().

BTW: Consider not copying things unnecessarily. Your call of copy_int() seems superfluous to me, just return a pointer to const int, referencing the existing element, and you should be fine.

Upvotes: 1

alk
alk

Reputation: 70981

How do I release the pointer (malloc) from copy_int?

Just call free() on it, if you do not need it any more.

Also here

int * new_int = (int*) malloc(sizeof(int*));
*new_int = *(int*)element;
if (NULL != new_int)
  return new_int;
else
  return NULL;

the test for NULL should be done before de-referencing the pointer element:

int *new_int = malloc(sizeof(int*));
if (NULL != new_int)
{
  *new_int = *(int*)element;
  return new_int;
}
else
  return NULL;

Note: Casting the result of malloc/calloc/realloc is not necessary in C nor is it recommended in any way.

Also^2 the call to free() here:

if (NULL == returnElement){
  free (returnElement);
  return NULL;
}

is use less, as there is nothing to free(), as returnElement point nowhere b carrying NULL. You want to remove it.

Upvotes: 1

Related Questions