typedef struct partition_struct {
int* elements;
int last; //last element index
int dim; //n allocated int
} partition;
typedef struct partitions_struct {
partition* partitions;
int last; //last partition index
int dim; //n allocated partition
} partitions;
why the free function of partition("free_p") gives a "munmap_chunk(): invalid pointer" error on free(p) line when called the second time from "free_ps"?.
void free_p(partition* p){
if(p==NULL) return;
if(p->elements) free(p->elements);
free(p); //<--Gives error here
}
void free_ps(partitions* ps){
if(ps==NULL) return;
for(int i=0; i<=ps->last; i++){
free_p(&ps->partitions[i]);
}
if(ps->partitions) free(ps->partitions);
free(ps);
}
Is there any correct way to free struct partitions_struct ?
I've tried to remove the free(p) when freeing the partitions struct but im concerned about mem leaks this below is the code that is working:
void free_ps(partitions* ps){
if(ps==NULL) return;
for(int i=0; i<=ps->last; i++){
free(ps->partitions[i].elements);
}
if(ps->partitions) free(ps->partitions);
free(ps);
}
entra functions for reference:
partition* partition_factory(int size){
partition* new = (partition*) malloc(sizeof(partition) * 1);
new->dim = size;
new->elements = (int*) malloc(sizeof(int) * size);
new->last = -1;
return new;
}
partitions* partitions_factory(int size){
partitions* new = (partitions*) malloc(sizeof(partitions) * 1);
new->dim = size;
new->partitions = (partition*) malloc(sizeof(partition) * size);
new->last = -1;
return new;
}
void add_el_to_p(partition* p, int el){
if((p->dim)-1 == p->last){
int new_size = p->dim * 2;
int* new_elements = (int*)realloc(p->elements, new_size * sizeof(int));
if(new_elements==NULL) {
exit(1);
} else {
p->dim = new_size;
p->elements = new_elements;
}
}
p->last++;
p->elements[p->last] = el;
}
void add_p_to_ps(partitions* ps, partition* p){
if((ps->dim)-1 == ps->last){
int new_size = ps->dim * 2;
partition* new_partitions = (partition*)realloc(ps->partitions, new_size * sizeof(partition));
if(new_partitions==NULL) {
exit(1);
} else {
ps->dim = new_size;
ps->partitions = new_partitions;
}
}
ps->last++;
ps->partitions[ps->last] = *p;
}
Yes there is a correct way. Just remove the offending free(p)
. There will be no memory leak (check with the address sanitizer or valgrind or any other tool).
In order to see why is it so, just imagine these functions:
struct int_array {
int* is;
int size;
};
free_int(int* i) {
free(i);
}
free_int_array(int_array* ia) {
for (int i = 0; i < ia->size; ++i)
free_int(&ia->is[i]);
free(ia->is);
free(ia);
}
"Of course that's completely wrong" you would say, and you would be absolutely right. Yet this is exactly what you are doing in your code, only with int
replaced by partition_struct
.
Now the function free_p
doesn't actually free a partition_struct
(nor should it), so it's better to rename it to say cleanup_p
.
Edit No, there will be memory leaks. You allocate individual partitions, but then forget about them:
ps->partitions[ps->last] = *p;
The structure pointed by p
is copied to ps->partitions[ps->last]
, but the original one gets lost when p
goes out of scope. Continuing our array of integers analogy, imagine that you have
int* int_factory(int value) {
int* p = malloc(sizeof(int));
*p = value;
return p;
}
and then somewhere in the function that adds an element:
is->is[i] = *p;
which is again obviously wrong, but that's exactly what you aredoing.
You need to rethink your design.
You have two ways of doing this.
partition* partitions;
that is responsible for managing the memory of partitions. You never malloc
or free
individual partition
structures. Your partitions_factory
needs to return its result by value: partition* partition_factory(int size) {...}
(just one malloc
inside). Or perhaps have a function that initializes a partition passed to it instead: void init_partition(partition* p, int size)
.partitions partition** partitions;
. The array only manages pointers, and you manage individual partitions by calling malloc
and free
on them. You will need to put back free(p)
.