Free a pointer from an external function

Question

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?

Not directly related to your problem, but do you really think that allocating space for one int pointer (`malloc(sizeof(int*))`) is a good idea ? — Jabberwocky, Aug 21 '15 at 05:39
Are you suggesting to do the same thing on the stack? I mean without malloc? — Tango_Chaser, Aug 21 '15 at 06:01
Whenever possible you should try not to use using `malloc`, in this case, you don't need `malloc` at all, you could simply return the `int` without using malloc : `return *(int*)element;` — ex0ns, Aug 21 '15 at 06:08
Your peek primitive shouldn't copy anything. Peek means look. You don't copy things you want to look at. Push should copy, pop should free. Alternatively, if you do want to make a copy to look at, it becomes the user's responsibility to clean it up after use. — n. m. could be an AI, Aug 21 '15 at 06:12
Also tell your user that their `copy_int` is wrong. Count your stars: `Type name = malloc(nelems * sizeof(Type ))`. Easy! And don't cast the rwturn value of malloc. — n. m. could be an AI, Aug 21 '15 at 06:24

kaylum · Answer 1 · 2015-08-21T06:30:45.270

2

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.

edited Aug 21 '15 at 06:30

answered Aug 21 '15 at 06:13

kaylum

13,833
2
22
31

alk · Answer 2 · 2015-08-21T06:15:27.850

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.

user3386109 · Accepted Answer · 2015-08-21T06:36:43.833

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);

Your answer "fixed" my problem. I used the 2 fixes and it works great with less code. — Tango_Chaser, Aug 21 '15 at 14:43

score 1 · Answer 4 · answered Aug 21 '15 at 06:23

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.

Free a pointer from an external function

4 Answers4