CODEWITHSUNDEEP
cpointersmemory-managementstructfree
Yiannis
Yiannis

Reputation: 950

C - What is wrong with my memory freeing function?

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

Upvotes: 1

Views: 850

Answers (4)

atk
atk

Reputation: 9314

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

Upvotes: 1

alain
alain

Reputation: 12047

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.

Upvotes: 5

DavidW
DavidW

Reputation: 30890

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.

Upvotes: 1

R Sahu
R Sahu

Reputation: 206567

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.

Upvotes: 4

Related Questions