1

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);
}
Improve this question
9
  • I believe you mean applying freeStruct recursively on e->next_element instead of free (but it should be tail recursion). Commented Mar 12, 2015 at 19:24
  • 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. Commented Mar 12, 2015 at 19:24
  • 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);} Commented Mar 12, 2015 at 19:25
  • 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. Commented Mar 12, 2015 at 19:25
  • 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 Commented Mar 12, 2015 at 19:28

4 Answers 4

Reset to default
5

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.

Improve this answer
Sign up to request clarification or add additional context in comments.

7 Comments

Indeed, you only set a value for next_element when you have another struct to link.
...because if you allocate memory right now for the next struct you have the problem of what do you do with next_element in that struct, and so on.
so i don't need to allocate memory for the pointer and hence dont need to free it after? which data types need further allocating when allocating memory for structs then? i remember when i had a struct with a string i had to allocate memory for the string aswell.. ints and pointers dont require, strings do require, what else?
Yes, you don't need to allocate memory for the pointer, and you don't need to free it. All things that are 'allocated statically', like something mything; don't need free, and all things that are 'allocated dynamically` like something *mything = malloc(... need free
In almost all cases, you can choose if you want to allocate statically or dynamically. int arr[20]; is a statically allocated array, int* arr = malloc(20*sizeof(int)); is a dynamically allocated array.
|
4

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.

Improve this answer

2 Comments

i only want to allocate memory for a pointer to a struct, not a whole struct
@Yiannis, the struct already has pointer. The pointer can be NULL or it needs to point to a struct. It does not make sense to allocate memory for a pointer only and assign that to e->next_element.
1

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.

Improve this answer

Comments

1

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);
}
Improve this answer

12 Comments

Calling free with a null pointer does nothing and is perfectly safe.
@alain: Yes, but after free, the pointer is not guaranteed to point to NULL. So while the if is unnecessary, the e->next_element = NULL; would still be a good idea, in my opinion.
i will only be freeing one q_element at a time, one for each pop
Yes, setting freed pointers to NULL is a good idea and makes the code safer.
What I meant is: In general it's a good idea, but here there is no point in setting e->next_element = NULL;, because e is freed too.
|

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.