cpointersmemory-leaksfreedynamic-arrays

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


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

Solution

  • 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.

    1. 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).
    2. 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).