free of struct 1 with an array of struct 2, inside of struct 2 there is an array of int

Question

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

Just remove free(p);. It wasn't allocated on its own, so don't free it on its own. — Ian Abbott
– Ian Abbott, Commented May 18, 2024 at 13:43
There are no arrays in your structures. There are pointers, but these are not the same thing at all. — John Bollinger
– John Bollinger, Commented May 18, 2024 at 14:03

n. m. could be an AI · Accepted Answer · 2024-05-18 15:54:55Z

1

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.

Have an array of partitions 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).
Have an array of pointers to 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).

edited May 18, 2024 at 15:54

answered May 18, 2024 at 13:51

n. m. could be an AI

122k14 gold badges141 silver badges267 bronze badges

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

3 Comments

SKOP_ Over a year ago

thanks a lot for the clarification, with the revised code and valgrind with this test function: partitions* ps = partitions_factory(1); add_p_to_ps(ps, partition_factory(1)); free_ps(ps); i get: total heap usage: 4 allocs, 3 frees, 52 bytes allocated ,LEAK SUMMARY: definitely lost: 16 bytes in 1 blocks I really dont know where im leaking.

n. m. could be an AI Over a year ago

@SKOP_ Your partition_factory function allocates an individual partition, but the allocation is lost when you do ps->partitions[ps->last] = *p; and throw away p. Your data structure doesn't store individually allocaterd partitions, but a single array of partitions. Look at my invalid example with an array of int. Now inagine that a function that adds an element to the array does malloc(sizeof(int)) and also does realloc on the array itself. That would be wrong, rigt? That's exactly what your code does. You need to rethink your design.

n. m. could be an AI Over a year ago

(contd) You have two ways of rdoing this. (1) Have an array of partitions partition* partitions; that is responsible for managing the memory, and never malloc or free individual partition structures; (2) Have an arry of pointers to partitions partition** partitions. The array only manages pointers, and *you*manage individual partitions by calling malloc and free on them, You will have to put back your free(p).

Collectives™ on Stack Overflow

free of struct 1 with an array of struct 2, inside of struct 2 there is an array of int

1 Answer 1

3 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

3 Comments

Your Answer

Sign up or log in

Post as a guest

Related