2

I am sharing a set of globals between different threads, so I allocate the memory as needed depending on how much is being placed into it. Kinda like a buffer. Whenever I try to free it from the same thread, but different function, glib goes crazy and segfaults.

I'm not a c programmer by far. I am trying to clear up memory leaks in the code that I hacked together.

char *keybuffer[], *valuesbuffer[], *returnbuffer[], *bufferaction;
int memcache_lock=1,*bufferelements, bufferowner=0;


void memcache_clear_buffers(){   
        free(&bufferaction); //I get the same reaction when I have the & and without the &
//      free(*keybuffer);    //I thought maybe I need to use the pointer. Then
//      free(*valuesbuffer); // I thought maybe I needed to reference the address in memory.
//      free(*returnbuffer); // I appreciate any help.
//      free(*bufferelements);
}

void memcache_allocate_buffers(int size){
        *keybuffer = (char *)malloc(size * sizeof(char *));
        *valuesbuffer = (char *)malloc(size * sizeof(char *)); 
        *returnbuffer = (char *)malloc(size  * sizeof(char *));
        bufferelements = malloc(sizeof(int));
        bufferaction = (char *)malloc(sizeof(char*));
}

Here is the output from valgrind.

 3500== Invalid free() / delete / delete[]
 ==3500==    at 0x4027C02: free (vg_replace_malloc.c:366)
 ==3500==    by 0x4191D4C: memcache_clear_buffers (dm_memcache.c:254)
 ==3500==    by 0x41921ED: memcache_set (dm_memcache.c:328)
 ==3500==    by 0x4192281: memcache_sid (dm_memcache.c:151) 
 ==3500==    by 0x418316D: db_user_exists (dm_db.c:3208)
 ==3500==    by 0x403F7F2: auth_user_exists (authsql.c:49)
 ==3500==    by 0x419ADA6: auth_user_exists (authmodule.c:130)
 ==3500==    by 0x4040D4E: auth_validate (authsql.c:333)
 ==3500==    by 0x419B106: auth_validate (authmodule.c:163)
 ==3500==    by 0x74656CFF: ???
 ==3500==  Address 0x41b55c0 is 0 bytes inside data symbol "bufferaction"

How is it an invalid free?

yoozer8
  • 7,361
  • 7
  • 58
  • 93
Craig Sparks
  • 23
  • 1
  • 2
  • 6
  • The ampersand in the free is definitely wrong. Show us every place where you use `bufferaction`. – cnicutar Apr 03 '12 at 08:17
  • Be also careful to mutex-lock these `free()` calls, since you could be accessing the memory from the other thread. – Gui13 Apr 03 '12 at 08:19
  • It could also be because of a buffer overflow. Are you sure you allocated enough space for bufferaction? – w00 Apr 03 '12 at 08:37

2 Answers2

4
  • malloc() and free() are not thread-safe functions. You need to protect the calls to those functions with a mutex.
  • You need to protect all shared variables with a mutex as well. You can use the same one as you use for malloc/free, one per variable.
  • You need to declare variables shared between several threads as volatile, to prevent dangerous optimizer bugs on some compilers. Note that this is no replacement for mutex guards.
  • Are the buffers arrays, or two-dimensional arrays (like arrays of C strings)? You have declared all buffers as potential two-dimensional arrays, but you never allocate the inner-most dimension.
  • Never typecast the result of malloc in C. Read this and this.
  • free(bufferaction), not free(&bufferaction).
  • Initialize all pointers to NULL explicitly. After free(), make sure to set the pointer to NULL. Before the memory is accessed by either thread, make sure to check the pointer against NULL.
Community
  • 1
  • 1
Lundin
  • 195,001
  • 40
  • 254
  • 396
  • 4
    `malloc()` and `free()` may well be thread safe depending on the C library implementation and the flags passed to the compiler. See this question for example http://stackoverflow.com/questions/855763/malloc-thread-safe – JeremyP Apr 03 '12 at 09:05
  • @JeremyP They are not thread-safe in a generic/portable manner. If you make assumptions about their implementation, your code is non-portable. – Lundin Apr 03 '12 at 10:50
  • 5
    As of C99, there were no generic portable thread libraries, so portability is not really a concern. As of C11 (which does have threads), malloc() has to be thread safe. – JeremyP Apr 03 '12 at 12:45
  • @JeremyP Where did you get that from? I can find no such statement in the C11 standard. I can however find this: (9899:2011 7.1.4/4) `"The functions in the standard library are not guaranteed to be reentrant and may modify objects with static or thread storage duration."` I know the difference between reentrant and thread-safe, but the standard does not mention thread-safe library functions anywhere. – Lundin Apr 03 '12 at 19:06
  • 3
    @JeremyP Actually, I found a specific section for the memory management functions 7.22.3/2: `For purposes of determining the existence of a data race, memory allocation functions behave as though they accessed only memory locations accessible through their arguments and not other static duration storage.`. So malloc is thread-safe indeed. Still, this assumes that you have a C11 compiler and that the code will never get ported to C99 or C90 compilers. – Lundin Apr 04 '12 at 11:23
  • 1
    I agree. I doubt if anybody has a C11 compiler yet. However, in prior versions of the standard there are no threads so we always come down to implementations and most implementations that support threads, support a thread safe malloc. – JeremyP Apr 04 '12 at 12:38
1

You're freeing &bufferaction when you should be freeing bufferaction (no ampersand)

I also suspect you may be corrupting memory when you use bufferaction. You see, you're only allocating 4 bytes (or 8 if you're compiling for a 64-bit platform). If you use any more than that, you'll end up corrupting memory after the allocated memory. This may only show itself when freeing up the memory, because you might have messed with some internal heap structure.

I really don't think this is a concurrency-related bug.

zmbq
  • 38,013
  • 14
  • 101
  • 171