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.

Answer 1

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.

Answer 2

You are referencing a memory address that does not belong to you. This is happening at free_frags() function, specifically at line 41, free (fragment); .

Answer 3

You are attempting to free the middle of your array.

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

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

Answer 5

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");

Answer 6

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.

Answer 7

For future refernce, look at the backtrace. See the line near the bottom:

#7  0x08048488 in free_frags (frags=0x9574008, len=10) at main.c:41
#8  0x080485b3 in main () at main.c:65

That tells you where you were when you had the trouble...