Conditional jump or move depends on uninitialised value(s) after using fread()

user_17329635 0
Viewed 442

For some reason, this super simple code gives me a valgrind error. Code excerpt:

char* readFile(char* filename){
    FILE * f = fopen (filename, "rb");
    if (f){
        printf("sucessfully opened file\n");
        fseek (f, 0, SEEK_END);
        int length = ftell (f);
        fseek (f, 0, SEEK_SET);
        char* buffer = malloc (length*2);  <- line 109
        if (buffer){
            printf("in here");
            fread (buffer, 1, length, f);
        }
        fclose (f);
        buffer[length+1]='\0';
        printf("buffer: %s",buffer);         <- line 116

Valgrind:

==102== Memcheck, a memory error detector
==102== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==102== Using Valgrind-3.15.0 and LibVEX; rerun with -h for copyright info
==102== Command: ./run
==102==
==102== error calling PR_SET_PTRACER, vgdb might block
inputting file:
sucessfully opened file
==102== Conditional jump or move depends on uninitialised value(s)
==102==    at 0x483EF58: strlen (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==102==    by 0x48CEE94: __vfprintf_internal (vfprintf-internal.c:1688)
==102==    by 0x48B7EBE: printf (printf.c:33)
==102==    by 0x40172C: readFile (functions.c:116)
==102==    by 0x403108: main (functions.c:649)
==102==  Uninitialised value was created by a heap allocation
==102==    at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==102==    by 0x4016BB: readFile (functions.c:109)
==102==    by 0x403108: main (functions.c:649)
==102==

Part of a larger program of mine is this function that reads a file and stores its contents in a buffer. This is the first thing the program does, so errors I have elswhere shouldn't really affect the behaviour here. Help would be greatly appreciated.

valgrind c
1 Answers

You have an off-by-one bug. You read length bytes, into the elements buffer[0]...buffer[length-1] (zero based arrays, remember) and so the terminating null character should go in buffer[length]. But instead you put it in buffer[length+1], and so printf on line 116 is attempting to print the garbage character at buffer[length] that was never overwritten.

Another issue is that fread could fail, or read fewer than length bytes. If that should happen, the remaining bytes of buffer are not overwritten, and then the printf will be trying to print uninitialized data. You need to check the return value of fread to see how many bytes were successfully read, and respond accordingly if it is less than length.

Finally, if malloc should fail, it will set buffer to NULL. Then buffer[length+1] = '\0' will dereference a null-related pointer, causing undefined behavior. You need to alter your logic to properly handle malloc returning NULL, and not dereference or print buffer in that case.

user_6349190
Related