Invalid read of size 1 in strstr() using valgrind

Question

I'm inspecting some piece of code with valgrind and I get this error:

==7001== Invalid read of size 1
==7001==    at 0x402E21B: strstr (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==7001==    by 0x8049742: replace_in_file (functions.c:191)
==7001==    by 0x8049C55: parse_dir (functions.c:295)
==7001==    by 0x8049059: main (main.c:214)
==7001==  Address 0x42018c3 is 0 bytes after a block of size 3 alloc'd
==7001==    at 0x402B018: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==7001==    by 0x80496EC: replace_in_file (functions.c:183)
==7001==    by 0x8049C55: parse_dir (functions.c:295)
==7001==    by 0x8049059: main (main.c:214)

The code snippet looks as follows:

long int replace_in_file(char **sets, FILE *file, char *path, const char *mode){
    long int n = 0, size = 0, len, remaining_len;

    char *buffer,
         *found,
         *before,
         *after;

    size = file_size(file);
    if(-1 != size){

        buffer = malloc(size * sizeof(char));

        rewind(file);
        fread(buffer, 1, size, file);

        int i=0;
        do{
            do{
                found = strstr(buffer, sets[i]); // LINE 191
                if(NULL != found && '\0' != sets[i][0]){

//rest of code...

I cannot realise why I get that error, because everything works as expected and in the debugger every variable seems fine.

What is wrong, how can I fix it?

Can you guarantee that `buffer` is Nul-terminated after the `fread()`? I notice you don't `bzero()` it or anything. — chrisaycock, May 03 '12 at 16:47
How many (char *) pointers are in the (sets) array? Is it possible that sets[i] has a value for i that puts it outside of the (sets) array? Also, how is it guaranteed that (buffer) will be NUL-terminated? fread() won't do that for you... — Jeremy Friesner, May 03 '12 at 16:49
Any chance file_size is returning 0? I would use if(size > 0) in your conditional. — Burton Samograd, May 03 '12 at 16:51
Solved, @chrisaycock, add your answer so I can accept it, that was the problem. Jeremy Friesner, I saw your answer after solving it. — Paul, May 03 '12 at 16:51
Alright, I posted it as an answer. You should also upvote [FatalError](http://stackoverflow.com/a/10435825/478288) since he found the same bug. — chrisaycock, May 03 '12 at 16:53
Yep, upvoted everyone that touched that subject of nul-termiantion. — Paul, May 03 '12 at 16:55

score 5 · Answer 1 · answered May 03 '12 at 16:48

5

fread() does not nul-terminate the data it reads, but strstr() expects a nul-terminated string. You should allocate size+1 bytes and explicitly add the nul-terminator to be safe.

answered May 03 '12 at 16:48

FatalError

52,695
14
99
116

score 4 · Accepted Answer · answered May 03 '12 at 16:52

4

Your buffer is not nul-terminated. Usually, you can just bzero() it before the fread() to ensure that it will work with strstr() and similar functions.

answered May 03 '12 at 16:52

chrisaycock

36,470
14
88
125

score 2 · Answer 3 · answered May 03 '12 at 17:02

from what I read in my manpage, fread() does not NULL-terminate its result according to C89 - and it does not on my system, either - so strstr() will simply read over the end of buffer unless you terminate it e.g. like so

buffer = malloc((size + 1) * sizeof(char));
memset(buffer, '\0', size + 1);

(which has the advantage of also terminating it in case fread fails to read the whole file)

a sidenote: since you already use sizeof(char) in your malloc() call, you might also consider replacing the 1 in the fread() for sake of consistency.

Invalid read of size 1 in strstr() using valgrind

3 Answers3