Valgrind complains with "Invalid write of size 8"

Question

I'm working on a small hobby project (www.github.com/AzP/GLSL-Validate) where I've taken old code (too much c and to little c++ for my own taste, but hey, what can you do?) and I'm trying to get it up and running on Linux and Windows. I've had a couple of crashes (fixed now hopefully), but since I started running Valgrind to find the issues, I got stuck with wanting to fix the complaints I get.

I just can't see what's wrong with this code (except that it's quite hard to read with nice "magic numbers" spread over the place) with regards to the Valgrind complaints.

I'm running Valgrind with the following command valgrind --track-origins=yes ./Program

291 //
292 //   Malloc a string of sufficient size and read a string into it.
293 //
294 # define MAX_SOURCE_STRINGS 5
295 char** ReadFileData(char *fileName)
296 {
297     FILE *in = fopen(fileName, "r");
298     char *fdata;
299     int count = 0;
300     char**return_data=(char**)malloc(MAX_SOURCE_STRINGS+1);
301 
302     //return_data[MAX_SOURCE_STRINGS]=NULL;
303     if (!in) {
304         printf("Error: unable to open input file: %s\n", fileName);
305         return 0;
306     }
307 
308     // Count size of file by looping through it
309     while (fgetc(in) != EOF)
310         count++;
311 
312     fseek(in, 0, SEEK_SET);
313 
314 
315     if (!(fdata = (char *)malloc(count+2))) {
316             printf("Error allocating memory\n");
317             return 0;
318     }
319     if (fread(fdata, sizeof(char), count, in) != count) {
320             printf("Error reading input file: %s\n", fileName);
321             return 0;
322     }
323     fdata[count] = '\0';
324     fclose(in);
325     if(count==0){
326         return_data[0]=(char*)malloc(count+2);
327         return_data[0][0]='\0';
328         OutputMultipleStrings=0;
329         return return_data;
330     }
331 
332     int len = (int)(ceil)((float)count/(float)OutputMultipleStrings);
333     int ptr_len=0,i=0;
334     while(count>0){
335         return_data[i]=(char*)malloc(len+2);
336         memcpy(return_data[i],fdata+ptr_len,len);
337         return_data[i][len]='\0';
338         count-=(len);
339         ptr_len+=(len);
340         if(count<len){
341             if(count==0){
342                OutputMultipleStrings=(i+1);
343                break;
344             }
345            len = count;
346         }
347         ++i;
348     }
349     return return_data;
350 }

And here comes the Valgrind output. Does the is 0 bytes inside a block of size 6 alloc'd mean that I can disregard it? I mean '0 bytes' doesn't sound dangerous? But since I posted the question here, I guess you can see that I think that I should pay attention to it.

==10570== Invalid write of size 8
==10570==    at 0x401602: ReadFileData(char*) (StandAlone.cpp:335)
==10570==    by 0x4013D8: CompileFile(char*, void*, int, TBuiltInResource const*) (StandAlone.cpp:255)
==10570==    by 0x401016: main (StandAlone.cpp:152)
==10570==  Address 0x5f627a0 is 0 bytes inside a block of size 6 alloc'd
==10570==    at 0x4C2880D: malloc (vg_replace_malloc.c:236)
==10570==    by 0x401475: ReadFileData(char*) (StandAlone.cpp:300)
==10570==    by 0x4013D8: CompileFile(char*, void*, int, TBuiltInResource const*) (StandAlone.cpp:255)
==10570==    by 0x401016: main (StandAlone.cpp:152)
==10570== 
==10570== Invalid read of size 8
==10570==    at 0x401624: ReadFileData(char*) (StandAlone.cpp:336)
==10570==    by 0x4013D8: CompileFile(char*, void*, int, TBuiltInResource const*) (StandAlone.cpp:255)
==10570==    by 0x401016: main (StandAlone.cpp:152)
==10570==  Address 0x5f627a0 is 0 bytes inside a block of size 6 alloc'd
==10570==    at 0x4C2880D: malloc (vg_replace_malloc.c:236)
==10570==    by 0x401475: ReadFileData(char*) (StandAlone.cpp:300)
==10570==    by 0x4013D8: CompileFile(char*, void*, int, TBuiltInResource const*) (StandAlone.cpp:255)
==10570==    by 0x401016: main (StandAlone.cpp:152)
==10570== 
==10570== Invalid read of size 8
==10570==    at 0x40163F: ReadFileData(char*) (StandAlone.cpp:337)
==10570==    by 0x4013D8: CompileFile(char*, void*, int, TBuiltInResource const*) (StandAlone.cpp:255)
==10570==    by 0x401016: main (StandAlone.cpp:152)
==10570==  Address 0x5f627a0 is 0 bytes inside a block of size 6 alloc'd
==10570==    at 0x4C2880D: malloc (vg_replace_malloc.c:236)
==10570==    by 0x401475: ReadFileData(char*) (StandAlone.cpp:300)
==10570==    by 0x4013D8: CompileFile(char*, void*, int, TBuiltInResource const*) (StandAlone.cpp:255)
==10570==    by 0x401016: main (StandAlone.cpp:152)

EDIT: I need the code to compile in a c++ compiler, that's why I have to keep all the casts of malloc.

Solution

This looks wrong:

char**return_data=(char**)malloc(MAX_SOURCE_STRINGS+1);

Should probably be:

char **return_data = malloc ( (MAX_SOURCE_STRINGS+1) * sizeof *return_data );

(spaces added for convenience).

EDIT: Some additional explanation: When you say return_data[i]=... you are trying to write something into return_data[i]. Now, return_data is char**, so return_data[i] is char*. So you are writing a pointer into some location in memory.

It looks like your pointers are 8 bytes long (which is fine), but you've only allocated 6 bytes: MAX_SOURCE_STRING+1. So there is a problem.

The fact that you're trying to write it into offset 0 doesn't matter - you're still trying to write more data than the buffer can take, and that's what valgrind is complaining about.

To solve the problem, you should allocate enough space to hold an array of pointers. Each pointer takes sizeof(char*), which can also be written as sizeof(*return_data) or sizeof *return_data. So in total you should allocate n * sizeof *return_data bytes, where n is (in your case) the magic number 6.

Comments

[AzP]

Thanks for your answer! I interpreted it as return_data is an array of char*, which can also be thought of as an array of char arrays. I figured that the malloc would allocate an array of 6 char arrays. Then the code performs another malloc at line 335, where it allocates the sub-array at the position i (return_data[i]). So we could reach that array by doing for instance return_data[i][0] etc.

[AzP]

But just like you said, changing the MALLOC_SOURCE_STRINGS to 8 does actually get rid of the complaint. I have to re-think this until I understand what I'm doing wrong, and if it matches your information =)

[AzP]

Ah, I think I get it now. You are right regarding the pointers, I figured we needed 6 pointers, and that they took up 1 byte each, which of course isn't the case.

[Omri Barel]

Yes, you have to remember that malloc doesn't know anything about types, it only knows about bytes. So you have to calculate the number of bytes yourself (which, as you discovered, is not the same as number of items).

[alk]

Using calloc(number, size) instead of malloc(total_size) makes the need to think about the size much more obvious ... - besides the positive effect of doing an initialisation of the freshly requested memory.