Memcpy for nested structures containing pointers

Question

The following code gives a crash stating double free. How to fix this with leaving no memory leaks? Trying to copy contents of tmp to list. Removing free from get_copy gives no crash. But that will result in a memory leak. How to solve this

We are allocating memory thrice here(ignoring loops), so it's being freed three times. memcpy will copy memory and not the contents, I understand. But then, how do I release memory that was allocated previously? Seems the info provided should be sufficient to understand the problem here. But Stackoverflow is not letting me post without adding more text. So, I am writing nothing useful. Please ignore the last paragraph and help to solve the query. This is a crash while freeing memory after memcpy in the case of nested pointers.

#include <stdio.h>
#include <stdlib.h>

typedef struct s1_ {
    int x,y,z;
} tmp_type;

typedef struct s2_ {
    tmp_type* p1;
    char a;
} tmp_data;

typedef struct s3_ {
    tmp_data* v1;
    int num;
} tmp_list;

void get_copy(tmp_list *list) {
    tmp_data tmp[10];
    int i;
    for(i=0;i<10;i++){
        tmp[i].p1= calloc(1,sizeof(tmp_type));
        tmp[i].p1->x = tmp[i].p1->y = tmp[i].p1->z = 6;
        tmp[i].a = 'a'+i;
    }

    list->num = 5;
    list->v1 = calloc(5,sizeof(tmp_data));

    for(i=0;i<5;i++) {
        list->v1[i].p1 = calloc(1,sizeof(tmp_type));
    }
    memcpy(list->v1,tmp,sizeof(list->v1));
    
    for(i=0;i<5;i++) {
        memcpy((list->v1[i].p1), (tmp[i].p1), sizeof(tmp_type));
        free(tmp[i].p1);
    }

}

void main()
{
    tmp_list l1;
    int i;
    get_copy(&l1);

    for (i=0;i<l1.num;i++) {
        printf("list: %c %d %d %d\n",
        l1.v1[i].a,
        l1.v1[i].p1->x,
        l1.v1[i].p1->y,
        l1.v1[i].p1->z);
    }

    for(i=0;i<l1.num;i++){
        free(l1.v1[i].p1);
    }
    free(l1.v1);

}

Answer 1

First, you should have clarified your expected output rather than cribbing about SO's text limit in question. Your current code (after removing free) gives this output.

list:  6 6 6
list:  6 6 6
list:  6 6 6
list:  6 6 6
list:  6 6 6

But if I am right, you are expecting this output.

list: a 6 6 6
list: b 6 6 6
list: c 6 6 6
list: d 6 6 6
list: e 6 6 6

memcpy(list->v1,tmp,sizeof(list->v1)); isn't really doing what are you thinking. It's just copying the first few bytes of tmp as sizeof (list->v1) is the size of tmp_data pointer. To fix this, you can either use one of the below.

• replace memcpy(list->v1,tmp,sizeof(list->v1)); with memcpy(list->v1,tmp,5*sizeof(tmp_data)); and remove the next loop.
• you can also remove memcpy(list->v1,tmp,sizeof(list->v1)); from your code and add this list->v1[i].a=tmp[i].a; in your loop after memcpy.

Now, coming back to your memory management question. If you go with the first one, you don't need a free() to free the first five entries of tmp as p1's pointer will be copied to list->v1, and you are freeing them eventually in main. However, you need to free the last five tmp->p1.

If you choose the second one, you must free all tmp and list entries.

You can find corrected codes here: first second