Valgrind errors, Invalid read of size & conditional jump or move depens on uninitialised value

Question

I've got those two errors and could need some help to find a solution after searching for a long time:

==4902== 1 errors in context 1 of 2:
==4902== Invalid read of size 1
==4902==    at 0x4010A0: getData (main.c:321)
==4902==    by 0x402527: main (main.c:783)
==4902==  Address 0x52007af is 1 bytes before a block of size 2,152 alloc'd
==4902==    at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==4902==    by 0x400FF1: getData (main.c:309)
==4902==    by 0x402527: main (main.c:783)
==4902== 
==4902== 
==4902== 1 errors in context 2 of 2:
==4902== Conditional jump or move depends on uninitialised value(s)
==4902==    at 0x4C2E0E9: strlen (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==4902==    by 0x40107A: getData (main.c:319)
==4902==    by 0x402527: main (main.c:783)
==4902==  Uninitialised value was created by a heap allocation
==4902==    at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==4902==    by 0x400FF1: getData (main.c:309)
==4902==    by 0x402527: main (main.c:783)

  char** buffer = malloc(file_size * sizeof(char**));
  if(buffer == NULL)
  {
    status = EXITCODE_4;
    return status;
  }

  int buffer_counter = 0;
  int buffer_length = 0;

 while(!feof(datafile))
  {
    buffer[buffer_counter] = malloc(file_size * sizeof(char*));
    if(buffer[buffer_counter] == NULL)
    {
      status = EXITCODE_4;
      free2D(buffer, buffer_counter);
      return status;
    }

    fgets(buffer[buffer_counter], file_size, datafile);

    buffer_length = strlen(buffer[buffer_counter]) - 1;

    if((buffer[buffer_counter][buffer_length]) == NEWLINE)
      buffer[buffer_counter][buffer_length] = 0;
    buffer_counter++;
  }

line 309 is where the second malloc happens, 321 the if and 319 the strlen

i'm not very experienced with valgrind, so i don't know how to fix that. Thx for any help i can get.

`while(!feof(datafile))` [is always wrong](http://stackoverflow.com/questions/5431941/while-feof-file-is-always-wrong). — Iharob Al Asimi, Feb 11 '15 at 01:35
this line: '' buffer[buffer_counter] = malloc(file_size * sizeof(char*)); should be: ' buffer[buffer_counter] = malloc(file_size);' because want an array of char, not an array of pointer to char — user3629249, Feb 11 '15 at 02:15
should clear the initial malloc'd array to all NULL to make it (relatively) easy when it comes time to free all the memory allocations — user3629249, Feb 11 '15 at 02:17
suggest either performing first line allocation before the while loop and control the while loop with fgets() --or-- use getline() and then no separate calls to malloc other than the initial list of pointers. — user3629249, Feb 11 '15 at 02:20
`char** buffer = malloc(file_size * sizeof(char**));` also mallocs the wrong data type. To avoid this error use `sizeof *buffer` instead of naming a type explicitly — M.M, Feb 11 '15 at 02:55
Not explicitly mentioned yet, but the reason you get the valgrind error comes from after the last line `fgets` fails, so `buffer[buffer_counter]` contains uninitialized values and then you call `strlen` on garbage. To avoid this sort of error always check return values of I/O functions. — M.M, Feb 11 '15 at 02:57

score 1 · Accepted Answer · edited May 23 '17 at 11:57

Change

while (!feof(datafile))

with

while (fgets(buffer[buffer_counter], file_size, datafile) != NULL)

because while (!feof(datafile)) will iterate once beyond the end of the file, read why while (!feof(datafile)) is always wrong.

The EOF marker is set after fgets() attempts to read past the end of file, so it requires one extra iteration for that to happen, but fgets() will return NULL at the end of file, so you will be safe from accessing uninitialized values if you test for it in the while loop condition.

Of course you will need to rethink the program flow, i suggest this

char** buffer = malloc(file_size * sizeof(char *));
if (buffer == NULL)
{
    status = EXITCODE_4;
    return status;
}

int  buffer_counter = 0;
int  buffer_length  = 0;
char line[file_size];

while (fgets(line, file_size, datafile) != NULL)
{
    size_t length;

    length = strlen(line);
    if (line[length - 1] == NEWLINE)
      line[--length] = '\0';
    buffer[buffer_counter] = malloc(1 + length);
    if (buffer[buffer_counter] == NULL)
    {
      status = EXITCODE_4;
      free2D(buffer, buffer_counter);
      return status;
    }
    strcpy(buffer[buffer_counter], line);
    buffer_counter++;
}

also, malloc(file_size * sizeof(char *)) is allocating more memory than you need, you need malloc(file_size * sizeof(char)), and sizeof(char) == 1 so just malloc(file_size), I fixed it anyway to allocate space for the exact string to fit.

@PinkieClownie by the way I am very happy to see that you check for `malloc()`'s return value. Keep that style, and you will be very happy, trust me. — Iharob Al Asimi, Feb 11 '15 at 01:51

score 0 · Answer 2 · answered Feb 11 '15 at 02:34

here is a suggested fix for the problems

char** buffer = malloc(file_size * sizeof(char**));
if(buffer == NULL)
{
    status = EXITCODE_4;
    return status;
}

// implied else, malloc successful

// clear list of pointer to NULLs
memset( buffer, 0x00, (file_size* sizeof(char**) ) );

int buffer_counter = 0;
int buffer_length = 0;

while(0 < (buffer_length = getline( buffer[buffer_counter], file_size, datafile)))
{

    if ( 0 >= buffer_length )
    {
        status = EXITCODE_4;
        free2D(buffer, buffer_counter);
        return status;
    }

    // implied else, getline successful

    if((buffer[buffer_counter][buffer_length-1]) == NEWLINE)
    {
        // trim newline 
        buffer[buffer_counter][buffer_length-1] = '\0';
        buffer_length--;
    }

    buffer_counter++;
} // end while

Valgrind errors, Invalid read of size & conditional jump or move depens on uninitialised value

2 Answers2