C memory management error?

Question

This is my C program:

#include <stdlib.h>
#include <stdio.h>
#include <string.h>
#include <time.h>
#include <ctype.h>
#define FALSE 0
#define TRUE 1

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

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

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

      while (current != NULL)
    {
      free (current->seq);
      fragment->frag_list = current->next;
      free (current);
      current = fragment->frag_list;
    }

      /* to do : free fragment */
      free (fragment);
      fragment = NULL;
    }
  free (frags);
}

int
main ()
{
  Fragment *frags = (Fragment *) malloc (10 * sizeof (Fragment));
  int i, j;
  for (i = 0; i < 10; i++)
    {
      Fragment *fragment = &frags[i];
      fragment->frag_list = (Frag *) malloc (1 * sizeof (Frag));
      Frag *frag = fragment->frag_list;
      frag->seq = malloc (6 * sizeof (char));
      strcpy (frag->seq, "55555");
      frag->next = (Frag *) malloc (1 * sizeof (Frag));
      frag = frag->next;
      frag->seq = malloc (6 * sizeof (char));
      strcpy (frag->seq, "55555");
      frag->next=NULL;
    }
  free_frags (frags, 10);
  return 0;
}

when I debug it with gdb, the error message is :

(gdb) run a.out 
..........................
..........................
09574000-09595000 rwxp 00000000 00:00 0          [heap]
b7e00000-b7e21000 rwxp 00000000 00:00 0 
b7e21000-b7f00000 ---p 00000000 00:00 0 
b7f2e000-b7f4b000 r-xp 00000000 08:08 298454     /usr/lib/libgcc_s.so.1
b7f4b000-b7f4c000 rwxp 0001c000 08:08 298454     /usr/lib/libgcc_s.so.1
b7f4c000-b7f4d000 rwxp 00000000 00:00 0 
b7f4d000-b808d000 r-xp 00000000 08:08 67152259   /lib/libc-2.10.1.so
b808d000-b808f000 r-xp 0013f000 08:08 67152259   /lib/libc-2.10.1.so
b808f000-b8090000 rwxp 00141000 08:08 67152259   /lib/libc-2.10.1.so
b8090000-b8094000 rwxp 00000000 00:00 0 
b80ae000-b80af000 r-xp 00000000 00:00 0          [vdso]
b80af000-b80cb000 r-xp 00000000 08:08 67152744   /lib/ld-2.10.1.so
b80cb000-b80cc000 r-xp 0001b000 08:08 67152744   /lib/ld-2.10.1.so
b80cc000-b80cd000 rwxp 0001c000 08:08 67152744   /lib/ld-2.10.1.so
bfc0f000-bfc24000 rw-p 00000000 00:00 0          [stack]

Program received signal SIGABRT, Aborted.
0xb80ae424 in __kernel_vsyscall ()
(gdb) where
#0  0xb80ae424 in __kernel_vsyscall ()
#1  0xb7f77411 in raise () from /lib/libc.so.6
#2  0xb7f78c12 in abort () from /lib/libc.so.6
#3  0xb7fb271d in __libc_message () from /lib/libc.so.6
#4  0xb7fb8581 in malloc_printerr () from /lib/libc.so.6
#5  0xb7fb9c82 in _int_free () from /lib/libc.so.6
#6  0xb7fbcd4d in free () from /lib/libc.so.6
#7  0x08048488 in free_frags (frags=0x9574008, len=10) at main.c:41
#8  0x080485b3 in main () at main.c:65
(gdb) 

The valgrind message is as following:

==2832== Memcheck, a memory error detector.
==2832== Copyright (C) 2002-2008, and GNU GPL'd, by Julian Seward et al.
==2832== Using LibVEX rev 1884, a library for dynamic binary translation.
==2832== Copyright (C) 2004-2008, and GNU GPL'd, by OpenWorks LLP.
==2832== Using valgrind-3.4.1, a dynamic binary instrumentation framework.
==2832== Copyright (C) 2000-2008, and GNU GPL'd, by Julian Seward et al.
==2832== For more details, rerun with: -v
==2832== 
==2832== Invalid read of size 4
==2832==    at 0x8048442: free_frags (main.c:31)
==2832==    by 0x80485B2: main (main.c:66)
==2832==  Address 0x418b034 is 12 bytes inside a block of size 80 free'd
==2832==    at 0x4023EBA: free (in /usr/lib/valgrind/x86-linux/vgpreload_memcheck.so)
==2832==    by 0x8048487: free_frags (main.c:42)
==2832==    by 0x80485B2: main (main.c:66)
==2832== 
==2832== Invalid write of size 4
==2832==    at 0x8048460: free_frags (main.c:36)
==2832==    by 0x80485B2: main (main.c:66)
==2832==  Address 0x418b034 is 12 bytes inside a block of size 80 free'd
==2832==    at 0x4023EBA: free (in /usr/lib/valgrind/x86-linux/vgpreload_memcheck.so)
==2832==    by 0x8048487: free_frags (main.c:42)
==2832==    by 0x80485B2: main (main.c:66)
==2832== 
==2832== Invalid read of size 4
==2832==    at 0x8048471: free_frags (main.c:38)
==2832==    by 0x80485B2: main (main.c:66)
==2832==  Address 0x418b034 is 12 bytes inside a block of size 80 free'd
==2832==    at 0x4023EBA: free (in /usr/lib/valgrind/x86-linux/vgpreload_memcheck.so)
==2832==    by 0x8048487: free_frags (main.c:42)
==2832==    by 0x80485B2: main (main.c:66)
==2832== 
==2832== Invalid free() / delete / delete[]
==2832==    at 0x4023EBA: free (in /usr/lib/valgrind/x86-linux/vgpreload_memcheck.so)
==2832==    by 0x8048487: free_frags (main.c:42)
==2832==    by 0x80485B2: main (main.c:66)
==2832==  Address 0x418b030 is 8 bytes inside a block of size 80 free'd
==2832==    at 0x4023EBA: free (in /usr/lib/valgrind/x86-linux/vgpreload_memcheck.so)
==2832==    by 0x8048487: free_frags (main.c:42)
==2832==    by 0x80485B2: main (main.c:66)
==2832== 
==2832== Invalid free() / delete / delete[]
==2832==    at 0x4023EBA: free (in /usr/lib/valgrind/x86-linux/vgpreload_memcheck.so)
==2832==    by 0x80484A5: free_frags (main.c:45)
==2832==    by 0x80485B2: main (main.c:66)
==2832==  Address 0x418b028 is 0 bytes inside a block of size 80 free'd
==2832==    at 0x4023EBA: free (in /usr/lib/valgrind/x86-linux/vgpreload_memcheck.so)
==2832==    by 0x8048487: free_frags (main.c:42)
==2832==    by 0x80485B2: main (main.c:66)
==2832== 
==2832== ERROR SUMMARY: 55 errors from 5 contexts (suppressed: 13 from 1)
==2832== malloc/free: in use at exit: 0 bytes in 0 blocks.
==2832== malloc/free: 41 allocs, 51 frees, 520 bytes allocated.
==2832== For counts of detected errors, rerun with: -v
==2832== All heap blocks were freed -- no leaks are possible.

Please help me to fix them, thanks.

Solution 1

You have multiple problems.

Here, you allocate space for 5 chars but copy 6 in (the nul-terminator at the end of the string needs a space, too):

  frag->seq = malloc (5 * sizeof (char));
  strcpy (frag->seq, "55555");

At the same point, you never set frag->next in the second frag you allocate. You need to set it to NULL, so that the while loop in the free_frag routine doesn't run off into the weeds.

The third problem is here:

  /* to do : free fragment */
  free (fragment);

You free fragment, but it's not a whole block recieved from malloc - it's just one of the single block of 10 fragments you allocated in one go. The later free(frags) frees that block properly, so you just need to remove that buggy line.

Solution 2

You are attempting to free the middle of your array.

Fragment *fragment = &frags[i];
...
...
/* to do : free fragment */
free (fragment);
fragment = NULL;

Solution 3

These lines seem to have a Bufferoverflow

  frag->seq = malloc (5 * sizeof (char));
  strcpy (frag->seq, "55555");

Because the string 55555 will also include a terminating zero character that is also written into memory outside the allocated 5 bytes.

Instead you could use strdup() which allocates and copies a string

  frag->seq = strdup("55555");

Solution 4

You're treating frag_list as a linked list of Frag pointers, but you aren't putting in a terminator when you create the list.

Try this:

int
main ()
{
  Fragment *frags = (Fragment *) malloc (10 * sizeof (Fragment));
  int i, j;
  for (i = 0; i < 10; i++)
    {
      Fragment *fragment = &frags[i];
      fragment->frag_list = (Frag *) malloc (1 * sizeof (Frag));
      Frag *frag = fragment->frag_list;
      frag->seq = malloc (5 * sizeof (char));
      strcpy (frag->seq, "55555");
      frag->next = (Frag *) malloc (1 * sizeof (Frag));
      frag = frag->next;
      frag->seq = malloc (5 * sizeof (char));
      strcpy (frag->seq, "55555");
      frag->next = NULL; // <--------------------- This is what you need to do
    }
  free_frags (frags, 10);
  return 0;
}

The issue is that when you malloc() a new block of memory, the compiler and/or the OS might null it out for you, but more likely it will just give you garbage. When you try to free() that garbage, you get a crash.

Solution 5

If you're going to use the Frag.next pointer as a sentinel (in free_frags()) then you need to set it to NULL somewhere in your code.

Also, be careful - you're using malloc() to allocate 5 characters for Frag.seq and you're copying a non-NULL-terminated string in that space.

View more solutions

Comments

[Charlie Epps]

I changed as you said, But there are still some errors. But your tips is right, and I forget that. thank you all the same.

[Michael Foukarakis]

It's not causing his problem (directly or indirectly), but it is dangerous indeed.

[visual_learner]

As a random note, why take an address of a pointer dereference? Why not just use pointer arithmetic? I think frags + i is a little clearer than &frags[i] but that's just my perspective.

[Daniel Pryden]

+1, this combines everybody else's answers into one. I believe it covers everything you need.

[visual_learner]

strdup() is non-standard (unless POSIX is good enough for you), although it's fairly easy to write your own if your OS doesn't provide one.

[Paul Stephenson]

Well personally I think &frags[i] is clearer than frags + i, since the first is obviously a pointer to an element of an array just by looking at it. Without checking other lines of code, frags + i could just be an arithmetic expression. The less context needed to understand an expression the better, in my opinion. (Think of the questioner's co-worker who has just been told, "Go fix that bug by this afternoon.")

[plinth]

And for love of all that is holy, please use strdup() for making dynamically allocated copies of strings - then your first bug won't ever happen again. See here for more info: stackoverflow.com/questions/482375/c-strdup-function