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.

Try to use Valgrind's Memcheck - it will show you where you fail. — qrdl, Sep 03 '09 at 06:44
Note: there's absolutely no point in naming your structs, if you're going to typedef them. It just clutters the code with a meaningless name, making anyone read the code go "hm ... I wonder what this name is used for?" and then feeling confused when the name isn't found. Yes, I'm projecting, but that doesn't mean I'm wrong. — unwind, Sep 03 '09 at 06:49
@unwind: You are wrong. If you need to use the struct definition in the body of the struct itself, then you need a name and a typedef. And that's exactly what he is doing with the _Frag/Frag struct. — Daniel Pryden, Sep 03 '09 at 06:53
@unwind - the first struct is a linked list, so he has to name it in order for it to have a reference to itself. The second one, not so much. — Chris Lutz, Sep 03 '09 at 06:54
So Valgrind found many memory errors in your code, telling you exact locations where problem occurred. I think you can close your question now and start fixing your code. Do not expect other people to fix it for you - it's your call. — qrdl, Sep 03 '09 at 07:23
I know, I should fix it for myself. But I have beening debuging for two days, and I have no idea. I don't what to do, I'm in trouble. I want to get some help from others. — Charlie Epps, Sep 03 '09 at 07:36

caf · Answer 1 · 2009-09-03T07:13:19.323

10

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.

edited Sep 03 '09 at 07:13

answered Sep 03 '09 at 06:52

caf

233,326
40
323
462

+1, this combines everybody else's answers into one. I believe it covers everything you need. – Daniel Pryden Sep 03 '09 at 06:56
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: http://stackoverflow.com/questions/482375/c-strdup-function – plinth Sep 03 '09 at 14:53

score 7 · Answer 2 · answered Sep 03 '09 at 06:44

7

You are attempting to free the middle of your array.

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

answered Sep 03 '09 at 06:44

Dipstick

9,854
2
30
30

1

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. – Chris Lutz Sep 03 '09 at 06:55
2

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.") – Paul Stephenson Sep 03 '09 at 07:23

score 7 · Answer 3 · answered Sep 03 '09 at 06:49

7

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

answered Sep 03 '09 at 06:49

epatel

45,805
17
110
144

2

It's not causing his problem (directly or indirectly), but it is dangerous indeed. – Michael Foukarakis Sep 03 '09 at 06:50
`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. – Chris Lutz Sep 03 '09 at 06:58

score 3 · Answer 4 · answered Sep 03 '09 at 06:45

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.

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. — Charlie Epps, Sep 03 '09 at 06:49

score 2 · Answer 5 · answered Sep 03 '09 at 06:43

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.

score 0 · Answer 6 · answered Sep 03 '09 at 06:43

0

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

answered Sep 03 '09 at 06:43

erelender

6,175
32
49

C memory management error?

6 Answers6