free() errors (debugging with valgrind)?

Question

I have these structs:

typedef struct _Frag{
  struct _Frag *next;
  char *seq;
  int x1; 
  int length;  
}Frag;

typedef struct _Fragment{ 
  int type; 
  Frag *frag_list;   
}Fragment;

And then I created a array

Fragment *fragments=malloc(1,sizeof(Fragment)); // or more
fragments->frag_list=malloc(1,sizeof(Frag)); // or more
Frag *frag=malloc(10,sizeof(Frag));
frag->seq="test str\n";
...
frag->next=malloc(1,sizeof(Frag));
frag->next->seq="test str\n";

At the end of program, I want to free the memory, the function is:

static void free_frags(){
  int i;
  Fragment *fragment;
  Frag *current,*next;
  for(i=0;i<1;i++){
    fragment=&snp_frags[i];
    current=fragment->frag_list;
    next=current->next;

    while(next!=NULL){
      free(current->seq);
      //free(current->next);
      free(current);
      current=next;
      next=current->next;
    }
    free(current->seq);
    //free(current->next);
    free(current);
    //free(fragment->frag_list);
    free(&snp_frags[i]);
  }
  free(snp_frags);
}

If I use valgrind to debug it, valgrind says that:

=============================================
==3810== Invalid read of size 4
==3810==    at 0x80490FD: free_snp (hap.c:16)
==3810==    by 0x80493AF: main (hap.c:73)
==3810==  Address 0x41b139c is 12 bytes inside a block of size 296 free'd
==3810==    at 0x4023EBA: free (in /usr/lib/valgrind/x86-linux/vgpreload_memcheck.so)
==3810==    by 0x8049167: free_snp (hap.c:30)
==3810==    by 0x80493AF: main (hap.c:73)
==3810== 
==3810== Invalid free() / delete / delete[]
==3810==    at 0x4023EBA: free (in /usr/lib/valgrind/x86-linux/vgpreload_memcheck.so)
==3810==    by 0x8049167: free_snp (hap.c:30)
==3810==    by 0x80493AF: main (hap.c:73)
==3810==  Address 0x41b1398 is 8 bytes inside a block of size 296 free'd
==3810==    at 0x4023EBA: free (in /usr/lib/valgrind/x86-linux/vgpreload_memcheck.so)
==3810==    by 0x8049167: free_snp (hap.c:30)
==3810==    by 0x80493AF: main (hap.c:73)

And please help me to fix these errors, thanx.

Answer 1

In

frag->seq="test str\n";

you haven't malloc'ed the memory block - the string is allocated in static storage - and later you try to free() that memory block. You can only free() blocks allocated with malloc(), otherwise you risk running into undefined behaviour.

You could either only put pointers to statically allocated strings into Frag::seq fields and never free() them or you could malloc() memory for these strings and copy the strings into malloc'ed blocks.

Answer 2

You seemed to be saying that you are freeing this memory as the last thing the program does.

Why bother? Why not just exit? Then your deallocation will be perfect, and faster.

Update: I never said this was always a perfect practice. In specialized environments like certain primitive embedded systems it might be important, certainly not on Windows or Unix/Mac, where doing this right before exit is considered bad practice. And it obviously doesn't hurt for the one developer to perfect his code. That's obvious.

For those of you commenting who really seem to believe this can leak memory, I guess you should never ^C your program. Reboot after every ^C, segfault or other debugging termination. Never kill anything with a task manager, or kill(1).

Everything commenters mentioned is in fact cleaned up. I do know one thing that isn't always cleaned up, which no one mentioned: temporary files.

Answer 3

1. You're frequently calling molloc() instead of malloc(). Check your vowels.
2. You're calling malloc() with the wrong number of arguments - it only takes one.
3. You can't assign strings - that performs pointer assignment, which is not what you want. You have to use strcpy() or strncpy() or memcpy(), depending on your religious perspective on the whole *cpy() mess, to copy the contents of one string into another.

Answer 4

remove the code line "free(fragment)". It will work well.