C - What is wrong with my memory freeing function?

1
votes

I have a struct which contains 2 integers and a pointer to another struct. I allocate memory for struct first and then for the pointer. When I free the memory I free up the pointer first and then I free up the struct.

When I run my program and call the function that frees memory it crashes when the call is made. When I don't call the function that frees memory it works fine, but then I'm not freeing up the memory.

I tried removing the line that frees the memory allocated to the pointer and the program doesn't crash, but I don't think thats right since a "free" is needed for every "malloc/calloc" right? Anyone see anything wrong with the freeing function?

//Define a struct data type
struct q_element
{
    //Declaration of struct members
    int element;
    int priority;
    struct q_element *next_element;
};

//Method to allocate memory
struct q_element* allocateStruct()
{
    //Declaration of a variable
    struct q_element *e;

    //Allocate memory for one queue element
    e = malloc(sizeof(struct q_element));

    //Allocate memory for one pointer to a queue element
    e->next_element = calloc(1,sizeof(struct q_element*));

    //Initialize integer members of queue element
    e->element = 0;
    e->priority = 0;

    return e;
}

//Method to free memory allocated
void freeStruct(struct q_element* e)
{
    //Free up pointer member
    free(e->next_element);

    //Free up struct
    free(e);
}
4
cpointersmemory-managementstructfree
I believe you mean applying freeStruct recursively on e->next_element instead of free (but it should be tail recursion). - Eugene Sh.
Are you sure that you do not use freed memory after the deallocation? But your structure is suspiciously similar to some linked list(queue?) item where such allocation-deallocation may become a source of problems. - Eugene Podskal
e->next_element = calloc(1,sizeof(struct q_element*)); --> e->next_element = NULL; , free(e->next_element);free(e); --> if(e){freeStruct(e->next_element);free(e);} - BLUEPIXY
Nothin wrong with the function itself if you only free one structure. But my guess is that these are used to form a list, and your code to free the whole list is wrong, but since it isn't here, we can't see it. - Lee Daniel Crocker
yes it is for a priority q queue and whenever i pop the highest priority element in the queue I call the freeStruct function in order to free the memory the popped element occupied.. i only free one at a time, not the whole list in one for loop @LeeDanielCrocker - Yiannis

4 Answers

5
votes

You don't need to allocate memory for the next_element pointer. The pointer is already there, just like int element for example.

So if you want to allocate just one element, you can set the next_element pointer to NULL and everything is fine.

4
votes

You are not allocating enough memory for e->next_element in the line:

e->next_element = calloc(1,sizeof(struct q_element*));
                                             //  ^^^ remove the *

That should be:

e->next_element = calloc(1,sizeof(struct q_element));

If you used e->next_element as though it were a valid pointer, you most likely ended up accessing memory that you did not allocate. That clobbered some of the bookkeeping information created by calloc, which lead to problems when you called free.

1
votes

In

//Allocate memory for one pointer to a queue element
e->next_element = calloc(1,sizeof(struct q_element*));

you allocate space for a pointer to a q_element structure, rather than a q_element structure. Do you attempt to write to this structure, because if so, that's probably where it goes wrong.

As a side note you might be better off just doing

e->next_element = 0

inside allocate_struct and then doing e->next_element = allocate_struct() outside the function later.

1
votes

In addition to what everyone else is mentioning about allocation, you also need a sentinel to check if the next_element was already freed. You may be attempting a double free.

Try the following code:

void freeStruct(struct q_element* e)
{
    //Free up pointer member
    if(e->next_element != 0){
        free(e->next_element);
        e->next_element = 0;
    }

    //Free up struct
    free(e);
}